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

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

[Stephen Hemminger]

On Wed, 12 Aug 2026 18:08:43 -0400
sandeep.penigalapati@intel.com wrote:

> From: Sandeep Penigalapati <sandeep.penigalapati@intel.com>
> 
> 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 enforces the per-mempool socket
> limit that shared UMEM was always intended to respect. Also document the
> shared mempool sizing requirement (4096 mbufs per socket).
> 
> Fixes: 74b46340e2d4 ("net/af_xdp: support shared UMEM")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Sandeep Penigalapati <sandeep.penigalapati@intel.com>
> ---

Detailed AI review found some issues:

Review of [PATCH v1] net/af_xdp: fix shared UMEM refcount corruption

Verified against DPDK main (26.11.0-rc0).  Applies cleanly; the driver
builds with -Dwerror=true at debugoptimized and minsize.  Fixes tag
74b46340e2d4 checks out and that commit does introduce the flaw.

Error:

1. drivers/net/af_xdp/rte_eth_af_xdp.c

   The new "return NULL" leaves rxq->mb_pool set while rxq->umem stays
   NULL: eth_rx_queue_setup() assigns rxq->mb_pool before calling
   xsk_configure(), and nothing clears it on the error path.

   get_shared_umem() then walks that stale entry and dereferences the
   NULL umem:

	if (mb_pool == internals->rx_queues[i].mb_pool) {
		if (ctx_exists(...))
			...
		if (rte_atomic_load_explicit(&internals->rx_queues[i].umem->refcnt,

   Any later queue setup using the same mempool crashes as soon as the
   failed rxq is the first match in the scan -- for example after the
   port that owns the UMEM is closed and removed from internal_list.

   The same scan can also reach a freed or over-shared pointer through
   the existing out_umem path in xsk_configure(), which calls
   xdp_umem_destroy(rxq->umem) without clearing rxq->umem, and leaves
   rxq->umem pointing at a still-live UMEM this rxq no longer holds a
   reference to.

   Both are latent today, but this patch makes reaching that state a
   routine outcome rather than an unusual one, so it should be closed
   here:

	/* in get_shared_umem() */
	if (internals->rx_queues[i].umem == NULL)
		continue;

   and clear rxq->umem unconditionally on the xsk_configure() error
   path, not only when the refcount reaches zero.

Warning:

2. doc/guides/nics/af_xdp.rst

   The new paragraph runs three sentences together across wrapped
   lines.  doc/guides/contributing/documentation.rst asks for one
   sentence per line, wrapped at punctuation points.

Info:

3. This is a behaviour change on a stable branch: setups that appeared
   to work (until close, or until the fill-queue crash) now fail at
   queue setup with -ENOMEM.  That is the right trade, but it is worth
   stating explicitly in the commit message for the stable maintainers.

4. The reflow of the rte_atomic_fetch_add_explicit() call is unrelated
   churn.  While the line is being touched: rte_memory_order_acquire on
   a refcount increment orders nothing useful; relaxed is sufficient
   there, and the matching decrement in eth_dev_close() wants release
   plus an acquire fence before xdp_umem_destroy().  Pre-existing, so
   only worth folding in if you are already rewriting the line.

5. The load and the increment are still not atomic with respect to each
   other -- get_shared_umem() releases internal_list_lock before
   returning, so two ports configured concurrently on the same mempool
   can both observe cnt < max_xsks and both increment past the cap.
   Control path, so the exposure is small, but a compare-exchange loop
   (or doing the check while holding internal_list_lock) is what
   actually enforces the limit the commit message describes.

6. refcnt is uint8_t while max_xsks is uint32_t.  A mempool of
   256 * 4096 mbufs or more produces max_xsks > 255, and the refcount
   wraps before the cap is ever reached.  Pre-existing.

7. "Port initialisation fails if the mempool is too small" -- it is the
   Rx queue setup that fails; "Queue setup fails" would be more precise
   and matches the -ENOMEM the caller returns.

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

[sandeep.penigalapati]

From: Sandeep Penigalapati <sandeep.penigalapati@intel.com>

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 enforces the per-mempool socket
limit that shared UMEM was always intended to respect. Also document the
shared mempool sizing requirement (4096 mbufs per socket).

Fixes: 74b46340e2d4 ("net/af_xdp: support shared UMEM")
Cc: stable@dpdk.org

Signed-off-by: Sandeep Penigalapati <sandeep.penigalapati@intel.com>
---
 doc/guides/nics/af_xdp.rst          |  5 +++++
 drivers/net/af_xdp/rte_eth_af_xdp.c | 24 ++++++++++++++++++++----
 2 files changed, 25 insertions(+), 4 deletions(-)

diff --git a/doc/guides/nics/af_xdp.rst b/doc/guides/nics/af_xdp.rst
index c455b4c066..00ba89dc97 100644
--- a/doc/guides/nics/af_xdp.rst
+++ b/doc/guides/nics/af_xdp.rst
@@ -99,6 +99,11 @@ configured like so:
     --vdev net_af_xdp0,iface=ens786f1,shared_umem=1 \
     --vdev net_af_xdp1,iface=ens786f2,shared_umem=1
 
+The shared mempool must be large enough for every socket sharing the UMEM. Each
+socket requires 4096 mbufs, so a UMEM shared by ``N`` sockets needs at least
+``4096 * N`` mbufs. Port initialisation fails if the mempool is too small to
+add another socket to the UMEM.
+
 xdp_prog
 ~~~~~~~~
 
diff --git a/drivers/net/af_xdp/rte_eth_af_xdp.c b/drivers/net/af_xdp/rte_eth_af_xdp.c
index 2cdb533276..6ef76bd60a 100644
--- a/drivers/net/af_xdp/rte_eth_af_xdp.c
+++ b/drivers/net/af_xdp/rte_eth_af_xdp.c
@@ -1188,12 +1188,28 @@ xsk_umem_info *xdp_umem_configure(struct pmd_internals *internals,
 		if (get_shared_umem(rxq, internals->if_name, &umem) < 0)
 			return NULL;
 
-		if (umem != NULL &&
-			rte_atomic_load_explicit(&umem->refcnt, rte_memory_order_acquire) <
-					umem->max_xsks) {
+		if (umem != NULL) {
+			uint32_t cnt = rte_atomic_load_explicit(&umem->refcnt,
+					rte_memory_order_acquire);
+
+			/* Reject sharing once the UMEM is at capacity: sharing without
+			 * taking a reference corrupts the refcount and crashes later.
+			 */
+			if (cnt >= umem->max_xsks) {
+				AF_XDP_LOG_LINE(ERR,
+					"UMEM %s is shared by %u socket(s), max %u: "
+					"cannot share with %s,qid%i. "
+					"Increase the mempool size (%d mbufs per socket required).",
+					umem->mb_pool->name, cnt, umem->max_xsks,
+					internals->if_name, rxq->xsk_queue_idx,
+					ETH_AF_XDP_NUM_BUFFERS);
+				return NULL;
+			}
+
 			AF_XDP_LOG_LINE(INFO, "%s,qid%i sharing UMEM",
 					internals->if_name, rxq->xsk_queue_idx);
-			rte_atomic_fetch_add_explicit(&umem->refcnt, 1, rte_memory_order_acquire);
+			rte_atomic_fetch_add_explicit(&umem->refcnt, 1,
+					rte_memory_order_acquire);
 		}
 	}
 
-- 
2.27.0

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

[Stephen Hemminger]

On Fri, 14 Aug 2026 17:51:07 -0400
sandeep.penigalapati@intel.com wrote:

> From: Sandeep Penigalapati <sandeep.penigalapati@intel.com>
> 
> 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: stable@dpdk.org
> 
> Signed-off-by: Sandeep Penigalapati <sandeep.penigalapati@intel.com>
> ---

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;

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

[sandeep.penigalapati]

From: Sandeep Penigalapati <sandeep.penigalapati@intel.com>

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: stable@dpdk.org

Signed-off-by: Sandeep Penigalapati <sandeep.penigalapati@intel.com>
---
v2:
- Guard get_shared_umem() against a NULL umem and clear rxq->umem on the
  xsk_configure() error path (review).
- doc: "Rx queue setup fails", one sentence per line.
- Drop unrelated reflow of the refcount increment.

 doc/guides/nics/af_xdp.rst          |  6 ++++++
 drivers/net/af_xdp/rte_eth_af_xdp.c | 30 ++++++++++++++++++++++++++---
 2 files changed, 33 insertions(+), 3 deletions(-)

diff --git a/doc/guides/nics/af_xdp.rst b/doc/guides/nics/af_xdp.rst
index c455b4c066..261689e4e9 100644
--- a/doc/guides/nics/af_xdp.rst
+++ b/doc/guides/nics/af_xdp.rst
@@ -99,6 +99,12 @@ configured like so:
     --vdev net_af_xdp0,iface=ens786f1,shared_umem=1 \
     --vdev net_af_xdp1,iface=ens786f2,shared_umem=1
 
+The shared mempool must be large enough for every socket sharing the UMEM.
+Each socket requires 4096 mbufs, so a UMEM shared by ``N`` sockets needs at
+least ``4096 * N`` mbufs.
+Rx queue setup fails if the mempool is too small to add another socket to the
+UMEM.
+
 xdp_prog
 ~~~~~~~~
 
diff --git a/drivers/net/af_xdp/rte_eth_af_xdp.c b/drivers/net/af_xdp/rte_eth_af_xdp.c
index 2cdb533276..a9e488d13c 100644
--- a/drivers/net/af_xdp/rte_eth_af_xdp.c
+++ b/drivers/net/af_xdp/rte_eth_af_xdp.c
@@ -1149,6 +1149,13 @@ get_shared_umem(struct pkt_rx_queue *rxq, const char *ifname,
 			if (rxq == list_rxq)
 				continue;
 			if (mb_pool == internals->rx_queues[i].mb_pool) {
+				/*
+				 * A failed queue setup can leave mb_pool set
+				 * with no umem; skip it to avoid a NULL
+				 * dereference below.
+				 */
+				if (internals->rx_queues[i].umem == NULL)
+					continue;
 				if (ctx_exists(rxq, ifname, list_rxq,
 						internals->if_name)) {
 					ret = -1;
@@ -1188,9 +1195,24 @@ xsk_umem_info *xdp_umem_configure(struct pmd_internals *internals,
 		if (get_shared_umem(rxq, internals->if_name, &umem) < 0)
 			return NULL;
 
-		if (umem != NULL &&
-			rte_atomic_load_explicit(&umem->refcnt, rte_memory_order_acquire) <
-					umem->max_xsks) {
+		if (umem != NULL) {
+			uint32_t cnt = rte_atomic_load_explicit(&umem->refcnt,
+					rte_memory_order_acquire);
+
+			/* Reject sharing once the UMEM is at capacity: sharing without
+			 * taking a reference corrupts the refcount and crashes later.
+			 */
+			if (cnt >= umem->max_xsks) {
+				AF_XDP_LOG_LINE(ERR,
+					"UMEM %s is shared by %u socket(s), max %u: "
+					"cannot share with %s,qid%i. "
+					"Increase the mempool size (%d mbufs per socket required).",
+					umem->mb_pool->name, cnt, umem->max_xsks,
+					internals->if_name, rxq->xsk_queue_idx,
+					ETH_AF_XDP_NUM_BUFFERS);
+				return NULL;
+			}
+
 			AF_XDP_LOG_LINE(INFO, "%s,qid%i sharing UMEM",
 					internals->if_name, rxq->xsk_queue_idx);
 			rte_atomic_fetch_add_explicit(&umem->refcnt, 1, rte_memory_order_acquire);
@@ -1818,6 +1840,8 @@ xsk_configure(struct pmd_internals *internals, struct pkt_rx_queue *rxq,
 out_umem:
 	if (rte_atomic_fetch_sub_explicit(&rxq->umem->refcnt, 1, rte_memory_order_acquire) - 1 == 0)
 		xdp_umem_destroy(rxq->umem);
+	/* Drop the dangling pointer so a later shared-UMEM scan skips it. */
+	rxq->umem = NULL;
 
 	return ret;
 }
-- 
2.27.0