Received-SPF: pass (google.com: domain of linux-wireless+bounces-3490-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Message-ID: <ad6ae6b9-2ce1-e354-a151-98c3b33fd139@candelatech.com>
Date: Mon, 12 Feb 2024 15:27:12 -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
From: Ben Greear <greearb@candelatech.com>
To: linux-wireless <linux-wireless@vger.kernel.org>
References: <c9b0c01e-acac-9f15-730f-a0ba991a68dc@candelatech.com>
Organization: Candela Technologies
In-Reply-To: <c9b0c01e-acac-9f15-730f-a0ba991a68dc@candelatech.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit

On 2/12/24 15:22, Ben Greear wrote:
> 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;
>          }

Or another idea came to mind:  Should this check above go away entirely, and check for null
down where it currently checks IS_ERR()?  From the comment about the IS_ERR check, I am thinking
that might be a better idea...

Thanks,
Ben

> 
> Thanks,
> Ben
>