On 2/13/24 22:08, Korenblit, Miriam Rachel wrote:
: iwlwifi RFC related to iwl_mvm_tx_reclaimHello, 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@xxxxxxxxxxxxxxx> Candela Technologies Inc http://www.candelatech.com
