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

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 <[email protected]>
>>=20
>> 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_loc=
k.
>>=20
>> The lock is dropped to call ath11k_dp_reo_cache_flush() and taken again
>> before keeping iterating over the loop. That is broken.
>>=20
>> The list_for_each_entry_safe() protects over deleting the entry being=20
>> iterated over but does not protect for concurrent access. So another
>> thread might have taken the lock in between and modified the list, leadi=
ng
>> to a crash (see bellow).=20
>
> nit: s/bellow/below/
>
will do
[...]
>>=20
>> 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
>
>>=20
>> Fixes: d5c65159f289 ("ath11k: driver for Qualcomm IEEE 802.11ax devices"=
)
>> Signed-off-by: Maxime Bizon <[email protected]>
>> Signed-off-by: Nicolas Escande <[email protected]>
[...]
>
> while this seems the minimal fix which stable team might prefer, how abou=
t refactoring as
> firstly detaching all the expired entries onto a local list under the loc=
k 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-sc=
anning from the
> head, and it avoids bouncing reo_cmd_lock once per freed entry.
>
> Also this seems simpler and more direct =E2=80=94 its correctness is obvi=
ous 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 =E2=80=94 either form fixes the crash. If you'd rathe=
r 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 chang=
e.
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.=
=20
So if Jeff prefers taking this one as is just to have a quick fix no object=
ion
either, I'll rework it later for next. You guys tell me what you prefer.