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

["Nicolas Escande" <nico.escande@gmail.com>]

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.