Re: [PATCH ath-current] wifi: ath11k: fix locking problem in ath11k_dp_rx_tid_del_func()

On 7/29/2026 4:57 PM, Nicolas Escande wrote:
> From: Maxime Bizon <[email protected]>
> 
> In this function, we iterate over dp->reo_cmd_cache_flush_list using
> list_for_each_entry_safe(), and for each expired entries we call
> ath11k_dp_reo_cache_flush() then free the entry. As this can be called
> from multiple CPU we protect for concurrent access using dp->reo_cmd_lock.
> 
> The lock is dropped to call ath11k_dp_reo_cache_flush() and taken again
> before keeping iterating over the loop. That is broken.
> 
> The list_for_each_entry_safe() protects over deleting the entry being 
> iterated over but does not protect for concurrent access. So another
> thread might have taken the lock in between and modified the list, leading
> to a crash (see bellow). 

nit: s/bellow/below/

> 
> So fix it by restarting iterating over the list from the start once the
> lock is retaken.
> 
> 	BUG: Unable to handle kernel paging request at virtual address 00000010ddbeef8c
> 	Mem abort info:
> 	ESR = 0x0000000096000045
> 	EC = 0x25: DABT (current EL), IL = 32 bits
> 	SET = 0, FnV = 0
> 	EA = 0, S1PTW = 0
> 	FSC = 0x05: level 1 translation fault
> 	Data abort info:
> 	ISV = 0, ISS = 0x00000045
> 	CM = 0, WnR = 1
> 	user pgtable: 4k pages, 39-bit VAs, pgdp=000000000593f000
> 	[00000010ddbeef8c] pgd=0000000000000000, p4d=0000000000000000, pud=0000000000000000
> 	Internal error: Oops: 0000000096000045 [#1] SMP
> 	Modules linked in: ath11k_pci XXX
> 	CPU: 1 PID: 1775 Comm: napi/-29 Not tainted XXXX
> 	Hardware name: XXXX
> 	pstate: 80000005 (Nzcv daif -PAN -UAO -TCO -DIT -SSBS BTYPE=--)
> 	pc : ath11k_dp_rx_tid_del_func+0x164/0x3c8
> 	lr : ath11k_dp_rx_tid_del_func+0x2a0/0x3c8
> 	sp : ffffff8020c2fb90
> 	x29: ffffff8020c2fb90 x28: ffffff80207ae910 x27: 0000000000000080
> 	x26: ffffffc00861ebe4 x25: ffffffc008cefb88 x24: ffffff800d701b2c
> 	x23: 00000010ddbeef84 x22: ffffff800d701448 x21: ffffff8030e22d00
> 	x20: ffffff800d700000 x19: 0000000000000080 x18: 0000000000000000
> 	x17: 0000000000000000 x16: 0000000000000000 x15: 0000000000000000
> 	x14: 0000000000000026 x13: ffffff801958a100 x12: 0000000000000000
> 	x11: 0000000000000001 x10: 0000000000000001 x9 : fffffffe0081eba0
> 	x8 : ffffff801cc49300 x7 : ffffffc008f87130 x6 : ffffff80207ae900
> 	x5 : 0000000000210d00 x4 : 0000000000000000 x3 : 0000000000000000
> 	x2 : 000000010024519a x1 : 00000010ddbeef84 x0 : ffffff800d701b08
> 	Call trace:
> 	ath11k_dp_rx_tid_del_func+0x164/0x3c8
> 	ath11k_dp_process_reo_status+0x1d4/0x2fc
> 	ath11k_dp_service_srng+0x334/0x338
> 	ath11k_pcic_ext_grp_napi_poll+0x30/0xc0
> 	__napi_poll+0x34/0x184
> 	napi_threaded_poll+0xb4/0x1d8
> 	kthread+0xdc/0xe0
> 	ret_from_fork+0x10/0x20
> 	Code: b946e2c0 7101001f 54fffe29 a94002a1 (f9000420)
> 
> Tested-on: QCN9074 PCI WLAN.HK.2.9.0.1-01977-QCAHKSWPL_SILICONZ-1

hardware version missing betweeen target and bus type

QCN9074 hw1.0 PCI

> 
> Fixes: d5c65159f289 ("ath11k: driver for Qualcomm IEEE 802.11ax devices")
> Signed-off-by: Maxime Bizon <[email protected]>
> Signed-off-by: Nicolas Escande <[email protected]>
> ---
>  drivers/net/wireless/ath/ath11k/dp_rx.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/net/wireless/ath/ath11k/dp_rx.c b/drivers/net/wireless/ath/ath11k/dp_rx.c
> index 8e2abc7b8383..b8f939f7c04b 100644
> --- a/drivers/net/wireless/ath/ath11k/dp_rx.c
> +++ b/drivers/net/wireless/ath/ath11k/dp_rx.c
> @@ -784,6 +784,7 @@ static void ath11k_dp_rx_tid_del_func(struct ath11k_dp *dp, void *ctx,
>  	dp->reo_cmd_cache_flush_count++;
>  
>  	/* Flush and invalidate aged REO desc from HW cache */
> +retry:
>  	list_for_each_entry_safe(elem, tmp, &dp->reo_cmd_cache_flush_list,
>  				 list) {
>  		if (dp->reo_cmd_cache_flush_count > DP_REO_DESC_FREE_THRESHOLD ||
> @@ -796,6 +797,7 @@ static void ath11k_dp_rx_tid_del_func(struct ath11k_dp *dp, void *ctx,
>  			ath11k_dp_reo_cache_flush(ab, &elem->data);
>  			kfree(elem);
>  			spin_lock_bh(&dp->reo_cmd_lock);
> +			goto retry;
>  		}
>  	}
>  	spin_unlock_bh(&dp->reo_cmd_lock);

while this seems the minimal fix which stable team might prefer, how about refactoring as
firstly detaching all the expired entries onto a local list under the lock and flushing
that list afterwards:

  LIST_HEAD(flush_list);

  spin_lock_bh(&dp->reo_cmd_lock);
  list_for_each_entry_safe(elem, tmp, &dp->reo_cmd_cache_flush_list, list) {
        if () {
                list_move_tail(&elem->list, &flush_list);
        }
  }
  spin_unlock_bh(&dp->reo_cmd_lock);

  list_for_each_entry_safe(elem, tmp, &flush_list, list) {
        list_del(&elem->list);
        ath11k_dp_reo_cache_flush(ab, &elem->data);
        kfree(elem);
  }

This fully decouples the concurrency from the flushing: the detach phase runs entirely
under the lock, and the flush phase walks a thread-private list where no concurrency
exists. It's a single O(N) pass instead of the O(N^2) worst case of re-scanning from the
head, and it avoids bouncing reo_cmd_lock once per freed entry.

Also this seems simpler and more direct — its correctness is obvious by construction;
while the fix of this patch is less self-evident: a reader may not easily grasp why the
code has to rescan from the head after re-taking the lock.

Non-blocking though — either form fixes the crash. If you'd rather keep the minimal goto
retry for the stable backport and do the detach-list version as a follow-up cleanup, that
works for me too.