Received-SPF: pass (google.com: domain of linux-wireless+bounces-3595-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Message-ID: <35f63154-eb24-c279-9ae3-096f3aec75bc@candelatech.com>
Date: Wed, 14 Feb 2024 10:08:58 -0800
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.15.1
Subject: Re: iwlwifi RFC related to iwl_mvm_tx_reclaim
Content-Language: en-US
To: "Korenblit, Miriam Rachel" <miriam.rachel.korenblit@intel.com>,
 linux-wireless <linux-wireless@vger.kernel.org>
References: <c9b0c01e-acac-9f15-730f-a0ba991a68dc@candelatech.com>
 <MW5PR11MB581035988083537C57286DADA34E2@MW5PR11MB5810.namprd11.prod.outlook.com>
From: Ben Greear <greearb@candelatech.com>
Organization: Candela Technologies
In-Reply-To: <MW5PR11MB581035988083537C57286DADA34E2@MW5PR11MB5810.namprd11.prod.outlook.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit

On 2/13/24 22:08, Korenblit, Miriam Rachel wrote:
> : iwlwifi RFC related to iwl_mvm_tx_reclaim
>>
>> Hello,
>>
>> I'm seeing a crash due to accessing an 'sta' object in this method that is '-2' as
>> integer.
>>
>> It fails the initial check for null STA, but I'm thinking it might should check for
>> IS_ERR(sta) as well.
>>
>> (I have my own patch that references sta before the IS_ERR check later in the
>> code, and this causes the crash I'm seeing.  I guess upstream will not crash in this
>> situation.).
>>
>> My question:  Is the patch below a preferred approach, or should I add special
>> checks to where I access sta and only exit the method lower where it already has
>> the IS_ERR(sta) check?
>>
>> diff --git a/drivers/net/wireless/intel/iwlwifi/mvm/tx.c
>> b/drivers/net/wireless/intel/iwlwifi/mvm/tx.c
>> index 0567f4eefebc..bd3d2fe424cd 100644
>> --- a/drivers/net/wireless/intel/iwlwifi/mvm/tx.c
>> +++ b/drivers/net/wireless/intel/iwlwifi/mvm/tx.c
>> @@ -2337,7 +2337,7 @@ static void iwl_mvm_tx_reclaim(struct iwl_mvm
>> *mvm, int sta_id, int tid,
>>           sta = rcu_dereference(mvm->fw_id_to_mac_id[sta_id]);
>>
>>           /* Reclaiming frames for a station that has been deleted ? */
>> -       if (WARN_ON_ONCE(!sta)) {
>> +       if (IS_ERR(sta) || !sta) {
>>                   rcu_read_unlock();
>>                   return;
>>           }
>>
> 
> Hi,
> 
> Did you see this: 2b3eb122342c?
> 
> This can explain why the code is how it is.
> And no, you should not access the sta pointer before checking if IS_ERR.

Thanks for the pointer.  I think if I ever see the WARN_ON_ONCE(!sta) hit then
I'd want to just try to remove that and let the reclaim code work, and check for null in the IS_ERR() logic
added by the patch you referenced.

But I have not seen sta be NULL, and Johannes is fine with current code, so for now
I think it is fine as is.

Thanks,
Ben

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