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

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

[Nicolas Escande]

From: Maxime Bizon <mbizon@freebox.fr>

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). 

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

Fixes: d5c65159f289 ("ath11k: driver for Qualcomm IEEE 802.11ax devices")
Signed-off-by: Maxime Bizon <mbizon@freebox.fr>
Signed-off-by: Nicolas Escande <nico.escande@gmail.com>
---
 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);
-- 
2.55.0

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

[Baochen Qiang]

On 7/29/2026 4:57 PM, Nicolas Escande wrote:
> From: Maxime Bizon <mbizon@freebox.fr>
> 
> 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 <mbizon@freebox.fr>
> Signed-off-by: Nicolas Escande <nico.escande@gmail.com>
> ---
>  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.

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

[Nicolas Escande]

On Fri Jul 31, 2026 at 8:49 AM CEST, Baochen Qiang wrote:
>
>
> On 7/29/2026 4:57 PM, Nicolas Escande wrote:
>> From: Maxime Bizon <mbizon@freebox.fr>
>> 
>> 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/
>
will do
[...]
>> 
>> Tested-on: QCN9074 PCI WLAN.HK.2.9.0.1-01977-QCAHKSWPL_SILICONZ-1
>
> hardware version missing betweeen target and bus type
>
will change
> QCN9074 hw1.0 PCI
>
>> 
>> Fixes: d5c65159f289 ("ath11k: driver for Qualcomm IEEE 802.11ax devices")
>> Signed-off-by: Maxime Bizon <mbizon@freebox.fr>
>> Signed-off-by: Nicolas Escande <nico.escande@gmail.com>
[...]
>
> 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.

Yes I remember that we fixed it this way with Maxime to minimize code change.
But doing it your way seems to be the better thing to do. So I agree I'll
rework this wit the single iteration under lock + cleanup pass.
But it will take me some time to do so, I do not have much time right now. 
So if Jeff prefers taking this one as is just to have a quick fix no objection
either, I'll rework it later for next. You guys tell me what you prefer.