Received-SPF: pass (google.com: domain of linux-wireless-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Subject: Re: [PATCH] wifi: iwlwifi: fix double free on tx path.
To:     "Greenman, Gregory" <gregory.greenman@intel.com>,
        "linux-wireless@vger.kernel.org" <linux-wireless@vger.kernel.org>
Cc:     "amol.jawale@candelatech.com" <amol.jawale@candelatech.com>
References: <20220928193057.16132-1-greearb@candelatech.com>
 <5fc4f28a5fc079d0368f6047db0d2c5fa1ed3ad4.camel@intel.com>
From:   Ben Greear <greearb@candelatech.com>
Organization: Candela Technologies
Message-ID: <22d76af9-79e3-4d7c-12e0-71d57770fc49@candelatech.com>
Date:   Thu, 13 Oct 2022 09:39:26 -0700
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101
 Thunderbird/68.2.2
MIME-Version: 1.0
In-Reply-To: <5fc4f28a5fc079d0368f6047db0d2c5fa1ed3ad4.camel@intel.com>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 8bit
Precedence: bulk

On 10/12/22 7:17 AM, Greenman, Gregory wrote:

>>          if (WARN_ON_ONCE(!mvmsta))
>>                  return -1;
>> @@ -1238,8 +1239,17 @@ int iwl_mvm_tx_skb_sta(struct iwl_mvm *mvm, struct sk_buff *skb,
>>   
>>                  ret = iwl_mvm_tx_mpdu(mvm, skb, &info, sta);
>>                  if (ret) {
> 
> Maybe while on it, add here "if (unlikely(ret)) {"?

I don't think it is worth respinning the patch for that, but could add a follow-on patch.

Thanks,
Ben

> 
>> +                       /* Free skbs created as part of TSO logic that have not yet been dequeued */
>>                          __skb_queue_purge(&mpdus_skbs);
>> -                       return ret;
>> +                       /* skb here is not necessarily same as skb that entered this method,
>> +                        * so free it explicitly.
>> +                        */
>> +                       if (skb == orig_skb)
>> +                               ieee80211_free_txskb(mvm->hw, skb);
>> +                       else
>> +                               kfree_skb(skb);
>> +                       /* there was error, but we consumed skb one way or another, so return 0 */
>> +                       return 0;
>>                  }
>>          }
>>   
> 
> Thanks for the fix!
> Gregory
> 


-- 
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com