Re: [PATCH v6] net/mlx5: fix counter TAILQ race between free and query callback

[Raslan Darawsheh <rasland@nvidia.com>]

Hi,


On 18/06/2026 12:14 PM, Linhu Li wrote:
> flow_dv_counter_free() inserts counters into
> pool->counters[pool->query_gen] under pool->csl. Meanwhile,
> mlx5_flow_async_pool_query_handle() moves counters from
> pool->counters[query_gen ^ 1] to the global free list via
> TAILQ_CONCAT while holding only cmng->csl, not pool->csl.
> 
> The comment in flow_dv_counter_free() claims the lock is not needed
> because the query callback and the release function operate on
> different lists. That holds only if the free path always observes
> the up-to-date query_gen. It can be violated:
> 
> 1. A counter free thread (non-PMD, e.g. OVS offload thread) reads
>     pool->query_gen == 0 and is about to insert into counters[0].
> 2. The free thread is preempted by the OS scheduler; it is a regular
>     pthread, not pinned to a core.
> 3. The eal-intr-thread alarm fires: query_gen++ (now 1) and the async
>     query is sent.
> 4. Hardware completes the query and the callback runs TAILQ_CONCAT on
>     counters[0] (= query_gen ^ 1).
> 5. The free thread resumes and runs TAILQ_INSERT_TAIL on counters[0]
>     concurrently with step 4 on another core.
> 
> Because the two paths take different locks, TAILQ_INSERT_TAIL and
> TAILQ_CONCAT run concurrently on the same list with no synchronization
> and corrupt it: the pool-local list ends up with a NULL head but a
> dangling tqh_last, and the global free list tail no longer points to
> the real tail. The just-freed counter and every counter inserted
> afterwards become unreachable and are leaked.
> 
> Non-PMD threads can be preempted for hundreds of microseconds under
> CPU pressure, which is well within the async query round-trip time,
> so the window is reachable in practice.
> 
> Fix it by taking pool->csl in the query completion callback before
> operating on pool->counters[query_gen], serializing the CONCAT with
> any concurrent INSERT. The lock is taken once per pool per query
> completion in the eal-intr-thread context, not on the datapath, so
> the cost is negligible. Lock order is pool->csl then cmng->csl,
> matching all other sites.
> 
> Also handle the error path: previously the counters accumulated in
> pool->counters[query_gen] were abandoned when a query failed. Move
> them back to the global free list to avoid a leak on persistent
> query failures.
> 
> Additionally, fix a second independent race in flow_dv_counter_free():
> TAILQ_INSERT_TAIL is passed &pool->counters[pool->query_gen] directly,
> but the macro evaluates its head argument multiple times. Since
> pool->query_gen is a volatile bit-field, if mlx5_flow_query_alarm()
> increments query_gen between two evaluations of the macro, the same
> insertion can operate on two different lists: the earlier steps update
> counters[0] while the later steps update counters[1], leaving both
> lists with inconsistent metadata and leaking the counter. Fix by
> caching pool->query_gen into a local variable before calling the macro.
> 
> Fixes: ac79183dc6f7 ("net/mlx5: optimize free counter lookup")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Linhu Li <lilinhu618@gmail.com>
> Acked-by: Dariusz Sosnowski <dsosnowski@nvidia.com>

Patch applied to next-net-mlx,

Kindest regards
Raslan Darawsheh