On Thu, 20 Aug 2026 22:16:07 -0400
[email protected] wrote:
> From: Sandeep Penigalapati <[email protected]>
>
> Shared UMEM is meant to be shared by a limited number of sockets,
> governed by the mempool size (max_xsks). When the UMEM was already at
> capacity (refcnt >= max_xsks), xdp_umem_configure() returned the UMEM
> without incrementing its refcount, so the extra socket used it
> unaccounted for.
>
> This missing reference has two consequences. During queue setup the
> fill-queue reservation is chosen from the refcount, so the sharing
> socket reserves into its own uninitialised fill queue and crashes. At
> close, the under-counted refcount reaches zero while the UMEM is still
> in use, freeing it early and causing a use-after-free.
>
> Reject sharing once the UMEM is at capacity by returning NULL. The
> error is propagated from xsk_configure(), so Rx queue setup fails
> cleanly with -ENOMEM. This applies the per-mempool socket limit that
> shared UMEM was always intended to respect.
>
> Harden the failure path this makes reachable:
> - clear rxq->umem and its paired txq->umem when xsk_configure() fails,
> and skip queues whose UMEM is not yet set in get_shared_umem(), so a
> later scan over the same mempool cannot dereference a NULL or
> dangling UMEM;
> - free the fill-queue mbufs that were allocated but not yet handed to
> the fill queue when a sharing socket fails to bind, so it no longer
> leaks a burst of mbufs back out of the shared mempool;
> - propagate the map-insert failures in xsk_configure() instead of
> returning success, so a failed xsks_map update no longer leaves the
> caller using a deleted socket;
> - continue past, rather than stop at, a failed queue in eth_dev_close()
> so later successful queues and their UMEM references are still freed;
> - clamp max_xsks to UINT8_MAX so the cap stays within the uint8_t
> refcount.
>
> Also correct the UMEM refcount memory ordering: release on the shared
> increment and acquire-release on the final decrement, so the thread that
> drops the last reference observes all prior users' writes before it
> frees the UMEM.
>
> Document the shared mempool sizing requirement (4096 mbufs per socket).
>
> Note: on stable branches this is a behaviour change. Shared-UMEM setups
> that previously appeared to start, until the fill-queue crash or the
> use-after-free at close, now fail cleanly at Rx queue setup with
> -ENOMEM.
>
> Fixes: 74b46340e2d4 ("net/af_xdp: support shared UMEM")
> Cc: [email protected]
>
> Signed-off-by: Sandeep Penigalapati <[email protected]>
I am ok with it as is but AI still has some Info level comments.
Will take it as is, or you can revise (your choice).
Trimmed away the noise..
Review of [PATCH v6] net/af_xdp: fix shared UMEM refcount corruption
1. The refcount increment does not need release ordering.
rte_atomic_fetch_add_explicit(&umem->refcnt, 1,
rte_memory_order_release);
rte_memory_order_relaxed is the correct weakest choice here. The
incrementing thread has no prior writes to publish; the UMEM was
built by whoever created it, and that publication is already covered
by the release store of refcnt = 1 at the end of
xdp_umem_configure().
The commit message attributes the guarantee to the wrong operation:
"release on the shared increment ... so the thread that drops the
last reference observes all prior users' writes" is what the acq_rel
on the fetch_sub provides, not the increment. Worth correcting in
the message even if the ordering is left as is; it is harmless but
the rationale will outlive the patch in git history.
2. The capacity check and the increment are still not atomic, and
rxq->umem is mutated outside internal_list_lock.
get_shared_umem() releases internal_list_lock before returning, so
the load of refcnt in xdp_umem_configure() and the fetch_add that
follows are separate steps; two threads configuring queues on the
same mempool can both observe refcnt < max_xsks and both increment.
Separately, xsk_configure() and eth_dev_close() write rxq->umem
without the lock that get_shared_umem() holds when reading it, so a
concurrent failure could in principle free a UMEM between the NULL
check and the dereference.
Both are pre-existing and control-path setup is single threaded in
practice, so this is a note rather than a request. A
compare-exchange loop on refcnt would make the cap the patch adds
actually enforceable if that ever changes.
3. Mbufs already submitted to the fill queue are still lost at
out_xsk.
Once reserve_fill_queue() succeeds, the 2048 mbufs live in rxq->fq.
A later failure (map insert, busy-poll config) deletes the socket
and takes out_umem, and nothing drains the fill ring, so those mbufs
never return to the mempool. free_fq_bufs is correctly false at
that point, so this is not a regression from the patch, and
recovering them would mean unwinding the fill ring. Noting it as a
remaining gap rather than something to fix here.
