On Fri, 14 Aug 2026 17:51: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, so 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
> unconditionally 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.
>
> Also 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]>
> ---
Looks good, a couple of other minor things from AI review should be addressed.
Yes, this is getting to the "AI bike shedding" stage. So optional
Review of [PATCH v2] net/af_xdp: fix shared UMEM refcount corruption
Re-verified against main (26.11.0-rc0): applies cleanly, net/af_xdp
builds with -Dwerror=true at debugoptimized and minsize, no new lines
over 100 columns. The v1 findings are all addressed.
Warning:
1. drivers/net/af_xdp/rte_eth_af_xdp.c, get_shared_umem()
The NULL guard is placed ahead of ctx_exists(), so a queue whose
setup failed no longer participates in duplicate netdev,qid
detection. That is a behaviour change beyond what the commit
message describes, and the guard only needs to protect the refcnt
load. Move it down:
if (mb_pool == internals->rx_queues[i].mb_pool) {
if (ctx_exists(rxq, ifname, list_rxq,
internals->if_name)) {
ret = -1;
goto out;
}
/* failed setup leaves mb_pool set with no umem */
if (internals->rx_queues[i].umem == NULL)
continue;
if (rte_atomic_load_explicit(...
Info:
2. The capacity log message is three concatenated literals and reads
long. Splitting is not needed here -- checkpatches.sh ignores
LONG_LINE_STRING, and the rest of this file keeps log strings on one
line -- and the mempool sizing advice is now in af_xdp.rst, so it
does not have to be repeated at every failure. Something like:
AF_XDP_LOG_LINE(ERR, "%s,qid%i: UMEM %s already at max %u
sockets",
internals->if_name, rxq->xsk_queue_idx,
umem->mb_pool->name, umem->max_xsks);
cnt is not worth printing: it can only equal or exceed max_xsks at
this point. The three-line comment above the check restates the
commit message and can go to one line or be dropped entirely once
the message says "already at max".
3. The two new comments use different styles. The one in
get_shared_umem() matches the file (/* on its own line); the one in
xdp_umem_configure() starts text on the opening line.
4. xsk_configure() assigns txq->umem = rxq->umem before the failure
points, so clearing only rxq->umem leaves rxq->pair->umem pointing
at a freed or no-longer-referenced UMEM. Nothing reaches it unless
an application ignores the queue setup error and starts the port,
but clearing both together is cheap:
rxq->umem = NULL;
txq->umem = NULL;
