Re: [PATCH v6] net/af_xdp: fix shared UMEM refcount corruption

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.