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

[PATCH v3] 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 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.

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>
---
v3:
- Move the NULL umem check after ctx_exists() so a failed queue is still
  checked for a duplicate context, as before.
- Shorten the "at capacity" log to one line and drop the count (always
  the max here).
- Also clear txq->umem, not just rxq->umem, when setup fails.
- Clarify "AF_XDP socket" in the doc.

 doc/guides/nics/af_xdp.rst          |  7 +++++++
 drivers/net/af_xdp/rte_eth_af_xdp.c | 19 ++++++++++++++++---
 2 files changed, 23 insertions(+), 3 deletions(-)

diff --git a/doc/guides/nics/af_xdp.rst b/doc/guides/nics/af_xdp.rst
index c455b4c066..cf4eeb63d0 100644
--- a/doc/guides/nics/af_xdp.rst
+++ b/doc/guides/nics/af_xdp.rst
@@ -99,6 +99,13 @@ 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 AF_XDP 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..3838d52d5d 100644
--- a/drivers/net/af_xdp/rte_eth_af_xdp.c
+++ b/drivers/net/af_xdp/rte_eth_af_xdp.c
@@ -1154,6 +1154,9 @@ get_shared_umem(struct pkt_rx_queue *rxq, const char *ifname,
 					ret = -1;
 					goto out;
 				}
+				/* A failed setup leaves mb_pool set with no umem. */
+				if (internals->rx_queues[i].umem == NULL)
+					continue;
 				if (rte_atomic_load_explicit(&internals->rx_queues[i].umem->refcnt,
 						    rte_memory_order_acquire)) {
 					*umem = internals->rx_queues[i].umem;
@@ -1188,9 +1191,16 @@ 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) {
+			/* Reject sharing once the UMEM is at capacity. */
+			if (rte_atomic_load_explicit(&umem->refcnt,
+					rte_memory_order_acquire) >= umem->max_xsks) {
+				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);
+				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 +1828,9 @@ 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 dangling pointers so a later shared-UMEM scan skips this queue. */
+	rxq->umem = NULL;
+	txq->umem = NULL;
 
 	return ret;
 }
-- 
2.27.0

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

[Stephen Hemminger]

On Mon, 17 Aug 2026 12:13:01 -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 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.
> 
> 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>
> ---

AI still spots errors on this patch. It can be wrong, but it
does seem to track error paths well.

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

The core fix is right: returning NULL once refcnt >= max_xsks removes
both the unaccounted reference and the bogus reserve_before decision
that followed from it.  Fixes: tag resolves to 74b46340e2d4
("net/af_xdp: support shared UMEM"), so Cc: stable is appropriate.

Findings below are against the tree with the patch applied.


Error
-----

1. drivers/net/af_xdp/rte_eth_af_xdp.c, xsk_configure()

   The fq_bufs allocated before socket creation are leaked on the
   error paths this patch is hardening.

   In the shared case reserve_before is false, so the 2048 mbufs
   obtained by rte_pktmbuf_alloc_bulk() are not handed to
   reserve_fill_queue() until after the socket exists:

	ret = rte_pktmbuf_alloc_bulk(rxq->umem->mb_pool, fq_bufs,
				     reserve_size);
	...
	if (reserve_before) { ... }          /* skipped when sharing */
	...
	ret = load_custom_xdp_prog(...);
	if (ret)
		goto out_umem;               /* fq_bufs leaked */
	...
	ret = create_shared_socket(...);
	if (ret)
		goto out_umem;               /* fq_bufs leaked */

	if (!reserve_before)
		ret = reserve_fill_queue(...);

   reserve_fill_queue_zc() frees the array itself when it fails, and
   the reserve_before path therefore cleans up, but out_umem does not.
   Every sharing socket that fails to bind leaks a full burst of
   mbufs back out of the shared mempool, which is the same mempool
   whose size now decides max_xsks.

   Suggest freeing them at out_umem, guarded so the buffers are not
   freed twice:

	out_xsk:
		xsk_socket__delete(rxq->xsk);
	out_umem:
		if (!reserve_before)
			rte_pktmbuf_free_bulk(fq_bufs, reserve_size);

   (or a bool tracking whether reserve_fill_queue() has consumed
   them, if the out_xsk path is folded in later).

   This predates the patch, but it is on the exact failure path the
   commit message says it is hardening, and the new -ENOMEM rejection
   makes it easier to reach.


Warning
-------

2. drivers/net/af_xdp/rte_eth_af_xdp.c, eth_dev_close()

   The patch makes "mb_pool set, umem NULL" a deliberate marker for a
   queue whose setup failed, and get_shared_umem() correctly skips
   such queues with continue.  eth_dev_close() still treats the same
   state as end-of-list:

	for (i = 0; i < internals->queue_cnt; i++) {
		rxq = &internals->rx_queues[i];
		if (rxq->umem == NULL)
			break;
		xsk_socket__delete(rxq->xsk);
		...
	}

   If a middle queue fails setup and a later queue succeeds (easy with
   per-queue mempools: queue 0 on pool A, queue 1 on pool A rejected
   at capacity, queue 2 on pool B), close stops at queue 1 and leaks
   queue 2's xsk socket and its UMEM reference, so that UMEM is never
   destroyed.

   break should be continue.  Skipping is safe: a failed queue now has
   umem == NULL and its socket was already deleted or never created.

3. Commit message errno does not match the code.

   The message states twice that queue setup "fails cleanly with
   -ENOMEM".  xsk_configure() does return -ENOMEM, but
   eth_rx_queue_setup() discards it:

	if (xsk_configure(internals, rxq, nb_rx_desc)) {
		AF_XDP_LOG_LINE(ERR, "Failed to configure xdp socket");
		ret = -EINVAL;
		goto err;
	}

   The application sees -EINVAL.  Either propagate the return value
   from xsk_configure() or reword the commit message; the behaviour
   note for the stable branches should say what the application will
   actually observe.


Info
----

4. xsk_configure(), early return leaves txq->umem stale.

	rxq->umem = xdp_umem_configure(internals, rxq);
	if (rxq->umem == NULL)
		return -ENOMEM;
	txq->umem = rxq->umem;

   The out_umem path now clears both rxq->umem and txq->umem, but this
   return clears only rxq->umem.  Harmless on a first setup because the
   queue arrays are rte_zmalloc'd, but stale after a re-setup of a
   queue that previously succeeded.  Clearing txq->umem here too would
   make the two exits consistent.

5. The capacity check and the increment are not atomic.

   get_shared_umem() drops internal_list_lock before returning, so the
   load of refcnt 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.  Control-path setup is
   normally single threaded so this is not urgent, but a
   compare-exchange loop would actually enforce the limit the patch is
   adding.

6. Log text when max_xsks is zero.

   max_xsks is only assigned when the creating port has shared_umem
   set, and it is populated_size / 4096, so it is 0 for a pool smaller
   than 4096 mbufs or for a UMEM created by a non-shared port.  In
   those cases the new message reads "already at max 0 sockets", which
   points away from the real cause.  Worth special-casing, e.g. report
   the mempool as too small for shared UMEM when max_xsks == 0.

7. get_shared_umem(), the new check can use the existing alias.

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

   list_rxq already points at &internals->rx_queues[i] a few lines
   above; list_rxq->umem would be shorter and match the ctx_exists()
   call just above it.


Notes
-----

Documentation change is accurate: max_xsks is populated_size divided
by ETH_AF_XDP_NUM_BUFFERS (4096), so 4096 * N is the correct figure
for N sockets.

Not verified: build.  libxdp/libbpf were not available here, so the
af_xdp PMD was not compiled.