Re:Re:Re: [PATCH 4/5] wifi: ath12k: avoid IDR mutation during pending mgmt TX cleanup

[jiale yao]

At 2026-09-28 16:26:28, "jiale yao" <19888972804@xxxxxxx> wrote:
>At 2026-09-28 13:49:54, "Rameshkumar Sundaram" <rameshkumar.sundaram@xxxxxxxxxxxxxxxx> wrote:
>>On 9/26/2026 9:18 PM, Jiale Yao wrote:
>>> ath12k_mac_tx_mgmt_pending_free() is passed as an idr_for_each()
>>> callback and removes the current entry from txmgmt_idr. This can
>>> invalidate the radix-tree iterator retained by idr_for_each().
>>> 
>>> Both callers destroy the IDR immediately after the walk, so removing
>>> each entry in the callback is unnecessary. Split skb release from IDR
>>> removal and let the callback release the supplied skb without updating
>>> the IDR. The following idr_destroy() tears down the IDR itself.
>>> 
>>> Fixes: d889913205cf ("wifi: ath12k: driver for Qualcomm Wi-Fi 7 devices")
>>> Signed-off-by: Jiale Yao <yaojiale02@xxxxxxx>
>>> ---
>>>   drivers/net/wireless/ath/ath12k/mac.c | 25 +++++++++++++++----------
>>>   1 file changed, 15 insertions(+), 10 deletions(-)
>>> 
>>> diff --git a/drivers/net/wireless/ath/ath12k/mac.c b/drivers/net/wireless/ath/ath12k/mac.c
>>> index 99bf5cf79d10..7695b21149d1 100644
>>> --- a/drivers/net/wireless/ath/ath12k/mac.c
>>> +++ b/drivers/net/wireless/ath/ath12k/mac.c
>>> @@ -9166,18 +9166,11 @@ static void ath12k_mgmt_over_wmi_tx_drop(struct ath12k *ar, struct sk_buff *skb)
>>>   		wake_up(&ar->txmgmt_empty_waitq);
>>>   }
>>>   
>>> -static void ath12k_mac_tx_mgmt_free(struct ath12k *ar, int buf_id)
>>> +static void ath12k_mac_tx_mgmt_free_skb(struct ath12k *ar,
>>> +					struct sk_buff *msdu)
>>>   {
>>> -	struct sk_buff *msdu;
>>>   	struct ieee80211_tx_info *info;
>>>   
>>> -	spin_lock_bh(&ar->txmgmt_idr_lock);
>>> -	msdu = idr_remove(&ar->txmgmt_idr, buf_id);
>>> -	spin_unlock_bh(&ar->txmgmt_idr_lock);
>>> -
>>> -	if (!msdu)
>>> -		return;
>>> -
>>>   	dma_unmap_single(ar->ab->dev, ATH12K_SKB_CB(msdu)->paddr, msdu->len,
>>>   			 DMA_TO_DEVICE);
>>>   
>>> @@ -9187,11 +9180,23 @@ static void ath12k_mac_tx_mgmt_free(struct ath12k *ar, int buf_id)
>>>   	ath12k_mgmt_over_wmi_tx_drop(ar, msdu);
>>>   }
>>>   
>>> +static void ath12k_mac_tx_mgmt_free(struct ath12k *ar, int buf_id)
>>> +{
>>> +	struct sk_buff *msdu;
>>> +
>>> +	spin_lock_bh(&ar->txmgmt_idr_lock);
>>> +	msdu = idr_remove(&ar->txmgmt_idr, buf_id);
>>> +	spin_unlock_bh(&ar->txmgmt_idr_lock);
>>> +
>>> +	if (msdu)
>>> +		ath12k_mac_tx_mgmt_free_skb(ar, msdu);
>>> +}
>>> +
>>>   int ath12k_mac_tx_mgmt_pending_free(int buf_id, void *skb, void *ctx)
>>>   {
>>>   	struct ath12k *ar = ctx;
>>>   
>>> -	ath12k_mac_tx_mgmt_free(ar, buf_id);
>>> +	ath12k_mac_tx_mgmt_free_skb(ar, skb);
>>
>>this now keeps the freed skbs in the idr for the whole time 
>>idr_for_each() runs as opposed to previous implementation which removed 
>>the idr entry as well which looked safe.
>
>Thanks for the review.
>
>The original issue is the same as the one fixed by commit c38b1e19485a
>("firmware: arm_scmi: Avoid IDR updates while cleaning channels").
>ath12k_mac_tx_mgmt_pending_free() is called from idr_for_each(), and
>ath12k_mac_tx_mgmt_free() removes the current entry from that same IDR
>before the iterator advances. This can invalidate the iterator state.
>
>You are right that my patch leaves freed skb pointers in the IDR until
>idr_destroy(), which is undesirable.
>
>I will send a v2 using idr_for_each_entry(), which performs a fresh
>idr_get_next() lookup for each iteration. The entry will then be removed
>from the IDR before its skb is freed, avoiding both the invalid iterator
>state and stale skb pointers.
v2 in https://lore.kernel.org/all/20261003092104.520135-1-yaojiale02@xxxxxxx/
>
>Thanks,
>Jiale
>
>
>>
>>
>>>   
>>>   	return 0;
>>>   }
>>
>>
>>-- 
>>Ramesh