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

[Stephen Hemminger <stephen@networkplumber.org>]

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.