>-----Original Message-----
>From: Stephen Hemminger <[email protected]>
>Sent: Friday, August 21, 2026 11:41 PM
>To: Penigalapati, Sandeep <[email protected]>
>Cc: [email protected]; [email protected]; Loftus, Ciara <[email protected]>
>Subject: 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).
Thanks, Stephen - I'm good with v6 going in as-is.
>
>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.
