[PATCH net v3] xsk: Do not free ZC Rx buffer when the Rx ring is full

[PATCH net v3] xsk: Do not free ZC Rx buffer when the Rx ring is full

[Marcin Knap via B4 Relay]

From: Marcin Knap <marcin.knap@vecima.com>

When an XDP program redirects a zero-copy buffer to an XSK and the
socket's Rx ring is full, xsk_rcv_zc() frees the buffer and returns
-ENOBUFS to xdp_do_redirect().

A failed xdp_do_redirect() leaves the buffer owned by the caller, so
the driver releases it as well. That is harmless when the driver frees
it right away, as i40e, ice, igc, stmmac, idpf and most others do:
xp_free() ignores a buffer that is already on the pool free list. It
breaks when the driver releases or uses the buffer later, after it may
have been reallocated:

- mlx5e treats the failure as XDP_DROP and releases the buffer when
  the Rx WQE is refilled. By then the buffer can be in use for another
  Rx descriptor, so an in-use buffer goes back on the free list and the
  same UMEM frame is later handed to user space twice. With striding
  RQ this also ended in a NULL pointer dereference in
  mlx5e_post_rx_wqes().
- ixgbe and igb keep the buffer in the ring on their need_wakeup
  -ENOBUFS (XDP_EXIT) path and run XDP on it again on the next poll.
- dpaa2 hands the buffer back to its hardware pool while it is also on
  the pool free list.
- octeontx2 returns false on a failed redirect, so the caller goes on
  to build an skb from the freed buffer.

Leave the buffer with the caller on both the single-buffer and the
multi-buffer -ENOBUFS paths, as xsk_rcv() and xsk_rcv_check() already
do. Make the drivers that relied on the core freeing the buffer free
it themselves:

- i40e and ice consume the descriptor on XDP_EXIT without freeing it.
- gve DQO counts the redirect error without freeing the buffer.
- octeontx2 now frees the buffer and reports it as consumed.

Seen on Mellanox ConnectX-6 DX MT2892 (FW 22.47.1026) with AF_XDP
zero-copy on 7.3.0-rc6 when the user-space consumer could not keep up
with the Rx ring. With this change no duplicate frames were seen with
striding RQ on or off.

Fixes: 269009893146 ("xsk: recycle buffer in case Rx queue was full")
Reviewed-by: Maciej Fijalkowski <maciej.fijalkowski@intel.com>
Reviewed-by: Björn Töpel <bjorn@kernel.org>
Reviewed-by: Jason Xing <kerneljasonxing@gmail.com>
Reviewed-by: Nitin Shetty J <nshettyj@marvell.com>
Signed-off-by: Marcin Knap <marcin.knap@vecima.com>
---
Tested on mlx5 (ConnectX-6 DX) in zero-copy single-buffer mode.
The i40e and ice changes and the multi-buffer path are build-tested
only, as I don't have that hardware available.
---
Changes in v3:
- Target net instead of bpf (Maciej).
- Reword the changelog only: drivers that free the buffer right away
  are not affected; name the ones with a deferred release or reuse
  after free (AI review). No code changes.
- Collect Reviewed-by tags.
- Link to v2: https://patch.msgid.link/20261030-xsk-zc-rx-double-free-v2-1-cf01d4a9f3f7@vecima.com

Changes in v2:
- Also free the buffer on zero-copy redirect failure in gve DQO and
  octeontx2, which relied on the core freeing it (AI review).
- Audited the remaining zero-copy drivers against Björn's table.
  ixgbe and igb no longer run XDP on a freed buffer after
  XDP_EXIT; making them drop and consume the descriptor (R2) is
  left for a follow-up unless you prefer it in this patch.
- Rebased on current bpf.
- Link to v1: https://patch.msgid.link/20261019-xsk-zc-rx-double-free-v1-1-2aa19f6628fb@vecima.com

To: Joshua Washington <joshwash@google.com>
To: Harshitha Ramamurthy <hramamurthy@google.com>
To: Andrew Lunn <andrew+netdev@lunn.ch>
To: "David S. Miller" <davem@davemloft.net>
To: Eric Dumazet <edumazet@kernel.org>
To: Jakub Kicinski <kuba@kernel.org>
To: Paolo Abeni <pabeni@redhat.com>
To: Tony Nguyen <anthony.l.nguyen@intel.com>
To: Przemek Kitszel <przemyslaw.kitszel@intel.com>
To: Sunil Goutham <sgoutham@marvell.com>
To: Geetha sowjanya <gakula@marvell.com>
To: Ratheesh Kannoth <rkannoth@marvell.com>
To: Subbaraya Sundeep <sbhatta@marvell.com>
To: Bharat Bhushan <bbhushan2@marvell.com>
To: Magnus Karlsson <magnus.karlsson@intel.com>
To: Maciej Fijalkowski <maciej.fijalkowski@intel.com>
To: Stanislav Fomichev <sdf@fomichev.me>
To: Simon Horman <horms@kernel.org>
To: Alexei Starovoitov <ast@kernel.org>
To: Daniel Borkmann <daniel@iogearbox.net>
To: Jesper Dangaard Brouer <hawk@kernel.org>
To: John Fastabend <john.fastabend@gmail.com>
Cc: netdev@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Cc: intel-wired-lan@lists.osuosl.org
Cc: bpf@vger.kernel.org
---
 drivers/net/ethernet/google/gve/gve_rx_dqo.c           |  4 +++-
 drivers/net/ethernet/intel/i40e/i40e_xsk.c             |  1 +
 drivers/net/ethernet/intel/ice/ice_xsk.c               |  1 +
 drivers/net/ethernet/marvell/octeontx2/nic/otx2_txrx.c |  3 ++-
 net/xdp/xsk.c                                          | 15 +++------------
 5 files changed, 10 insertions(+), 14 deletions(-)

diff --git a/drivers/net/ethernet/google/gve/gve_rx_dqo.c b/drivers/net/ethernet/google/gve/gve_rx_dqo.c
index 82ed5e48b00a..cc544420df11 100644
--- a/drivers/net/ethernet/google/gve/gve_rx_dqo.c
+++ b/drivers/net/ethernet/google/gve/gve_rx_dqo.c
@@ -686,8 +686,10 @@ static void gve_xsk_done_dqo(struct gve_priv *priv, struct gve_rx_ring *rx,
 			goto err;
 		break;
 	case XDP_REDIRECT:
-		if (unlikely(xdp_do_redirect(priv->dev, xdp, xprog)))
+		if (unlikely(xdp_do_redirect(priv->dev, xdp, xprog))) {
+			xsk_buff_free(xdp);
 			goto err;
+		}
 		break;
 	}
 
diff --git a/drivers/net/ethernet/intel/i40e/i40e_xsk.c b/drivers/net/ethernet/intel/i40e/i40e_xsk.c
index 51a05ce4c7ce..a11ee1f3fa38 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_xsk.c
+++ b/drivers/net/ethernet/intel/i40e/i40e_xsk.c
@@ -360,6 +360,7 @@ static void i40e_handle_xdp_result_zc(struct i40e_ring *rx_ring,
 		return;
 
 	if (xdp_res == I40E_XDP_EXIT) {
+		xsk_buff_free(xdp_buff);
 		*failure = true;
 		return;
 	}
diff --git a/drivers/net/ethernet/intel/ice/ice_xsk.c b/drivers/net/ethernet/intel/ice/ice_xsk.c
index 0643017541c3..7c1dd4c87a71 100644
--- a/drivers/net/ethernet/intel/ice/ice_xsk.c
+++ b/drivers/net/ethernet/intel/ice/ice_xsk.c
@@ -636,6 +636,7 @@ int ice_clean_rx_irq_zc(struct ice_rx_ring *rx_ring,
 		if (likely(xdp_res & (ICE_XDP_TX | ICE_XDP_REDIR))) {
 			xdp_xmit |= xdp_res;
 		} else if (xdp_res == ICE_XDP_EXIT) {
+			xsk_buff_free(first);
 			failure = true;
 			first = NULL;
 			break;
diff --git a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_txrx.c b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_txrx.c
index 90af9b9044aa..6d10c132f61c 100644
--- a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_txrx.c
+++ b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_txrx.c
@@ -1555,7 +1555,8 @@ static bool otx2_xdp_rcv_pkt_handler(struct otx2_nic *pfvf,
 				*need_xdp_flush = true;
 				return true;
 			}
-			return false;
+			xsk_buff_free(xsk_buff);
+			return true;
 		}
 
 		err = xdp_do_redirect(pfvf->netdev, &xdp, prog);
diff --git a/net/xdp/xsk.c b/net/xdp/xsk.c
index 781ba46eb152..89bac2feeff2 100644
--- a/net/xdp/xsk.c
+++ b/net/xdp/xsk.c
@@ -224,21 +224,15 @@ static int xsk_rcv_zc(struct xdp_sock *xs, struct xdp_buff *xdp, u32 len)
 	struct list_head *xskb_list;
 	u32 contd = 0;
 	u32 num_desc;
-	int err;
 
-	if (likely(!frags)) {
-		err = __xsk_rcv_zc(xs, xskb, len, contd);
-		if (err)
-			goto err;
-		return 0;
-	}
+	if (likely(!frags))
+		return __xsk_rcv_zc(xs, xskb, len, contd);
 
 	contd = XDP_PKT_CONTD;
 	num_desc = xdp_get_shared_info_from_buff(xdp)->nr_frags + 1;
 	if (xskq_prod_nb_free(xs->rx, num_desc) < num_desc) {
 		xs->rx_queue_full++;
-		err = -ENOBUFS;
-		goto err;
+		return -ENOBUFS;
 	}
 
 	__xsk_rcv_zc_safe(xs, xskb, len, contd);
@@ -252,9 +246,6 @@ static int xsk_rcv_zc(struct xdp_sock *xs, struct xdp_buff *xdp, u32 len)
 	}
 
 	return 0;
-err:
-	xsk_buff_free(xdp);
-	return err;
 }
 
 static void *xsk_copy_xdp_start(struct xdp_buff *from)

---
base-commit: fb7bd3a2aad70a76232a7168a505386d0a1396af
change-id: 20261019-xsk-zc-rx-double-free-1a3b059cd41f

Best regards,
--  
Marcin Knap <marcin.knap@vecima.com>

Re: [PATCH net v3] xsk: Do not free ZC Rx buffer when the Rx ring is full

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider.

Critical: 0 · High: 0 (3) · Medium: 1 · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] After this patch, ixgbe's need_wakeup -ENOBUFS path keeps a
  live buffer and runs XDP on it again, but with stale data pointers.

Pre-existing issues:
- [High] The rule that the caller owns the buffer when redirect fails
  still breaks when a zero-copy buffer is redirected to anything other…
- [High] In otx2_xdp_rcv_pkt_handler()
  (drivers/net/ethernet/marvell/octeontx2/nic/otx2_txrx.c), the two…
- [High] In otx2_xdp_rcv_pkt_handler()
  (drivers/net/ethernet/marvell/octeontx2/nic/otx2_txrx.c), the AF_XDP…

--

xsk: Do not free ZC Rx buffer when the Rx ring is full

This commit stops xsk_rcv_zc() from freeing the zero-copy buffer when the
XSK Rx ring is full. A failed xdp_do_redirect() now leaves the buffer with
the caller. The drivers that relied on the core freeing it (i40e, ice, gve
DQO, octeontx2) now call xsk_buff_free() themselves on that path.

> diff --git a/drivers/net/ethernet/google/gve/gve_rx_dqo.c b/drivers/net/ethernet/google/gve/gve_rx_dqo.c
> index 82ed5e48b00ab..cc544420df112 100644
> --- a/drivers/net/ethernet/google/gve/gve_rx_dqo.c
> +++ b/drivers/net/ethernet/google/gve/gve_rx_dqo.c
> @@ -686,8 +686,10 @@ static void gve_xsk_done_dqo(struct gve_priv *priv, struct gve_rx_ring *rx,
>  			goto err;
>  		break;
>  	case XDP_REDIRECT:
> -		if (unlikely(xdp_do_redirect(priv->dev, xdp, xprog)))
> +		if (unlikely(xdp_do_redirect(priv->dev, xdp, xprog))) {
> +			xsk_buff_free(xdp);
>  			goto err;
> +		}
>  		break;
>  	}
>  

[Severity: High]
This is a pre-existing issue, but does the rule in the commit message, "A
failed xdp_do_redirect() leaves the buffer owned by the caller", hold
when a zero-copy buffer is redirected to something other than an XSKMAP?

For non-XSKMAP targets, xdp_do_redirect() calls
xdp_convert_buff_to_frame(). For MEM_TYPE_XSK_BUFF_POOL that ends up in
xdp_convert_zc_to_xdp_frame(), which copies the packet into a new page
and always frees the ZC buffer:

net/core/xdp.c:xdp_convert_zc_to_xdp_frame() {
    ...
	page = dev_alloc_page();
    ...
	memcpy(addr, data_to_copy, totsize);
    ...
	xsk_buff_free(xdp);
	return xdpf;
}

__xdp_do_redirect_frame() can still fail after that. Examples:

  -EBADRQC for a bare XDP_REDIRECT
  -EINVAL for a missing ifindex
  -ENOENT during broadcast map teardown
  -EOPNOTSUPP, -ENETDOWN or -EMSGSIZE from __xdp_enqueue()

The err: label only traces and returns:

err:
	_trace_xdp_redirect_map_err(dev, xdp_prog, fwd, map_type, map_id, ri->tgt_index, err);
	return err;

On this error, mlx5e_xdp_handle() counts a drop and does not set
MLX5E_RQ_FLAG_XDP_XMIT. The XSK Rx paths in mlx5e then skip setting
MLX5E_WQE_FRAG_SKIP_RELEASE, so the buffer is released again at WQE
refill.

Wouldn't that produce the same duplicate UMEM frame, and the same
possible NULL dereference in mlx5e_post_rx_wqes(), that the commit message
describes?

dpaa2_xsk_run_xdp() has the same problem: on a redirect error it calls
dpaa2_eth_recycle_buf() on a buffer that is already on the xsk free list.

The copied xdp_frame page also seems to leak on every such failure.
Neither __xdp_do_redirect_frame() nor __xdp_enqueue() frees xdpf on error.

For gve DQO and octeontx2, the new xsk_buff_free() is a second free on
this path. It is harmless because xp_free() returns early for a buffer
already on the free list, but the page leak remains.

Neither the commit message nor the changelog mentions this path. Could
they say that the mlx5e duplicate frame problem is only fixed for XSKMAP
redirects?

[ ... ]

> diff --git a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_txrx.c b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_txrx.c
> index 90af9b9044aa0..6d10c132f61cb 100644
> --- a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_txrx.c
> +++ b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_txrx.c
> @@ -1555,7 +1555,8 @@ static bool otx2_xdp_rcv_pkt_handler(struct otx2_nic *pfvf,
>  				*need_xdp_flush = true;
>  				return true;
>  			}
> -			return false;
> +			xsk_buff_free(xsk_buff);
> +			return true;
>  		}
>  
>  		err = xdp_do_redirect(pfvf->netdev, &xdp, prog);

[Severity: High]
This isn't a bug introduced by this patch, but the non-ZC branch just
below has the same problem the commit message describes for the ZC
branch. On redirect failure it does:

		otx2_dma_unmap_page(pfvf, iova, pfvf->rbsize,
				    DMA_FROM_DEVICE);
		xdpf = xdp_convert_buff_to_frame(&xdp);
		xdp_return_frame(xdpf);
		break;

It then falls out of the switch to return false.

otx2_rcv_pkt_handler() treats false as "not consumed" and continues with:

  napi_get_frags()
  otx2_skb_add_frag() for each cqe->sg segment, with a second
    cq->pool_ptrs++
  skb_mark_for_recycle()
  napi_gro_frags()

xdp_return_frame() has already given the page back to the page pool. Is
the skb then built on a page the pool can refill to hardware? And is that
page returned to the pool a second time when the skb is freed?

Should this branch also return true after freeing, as the XDP_DROP case
does?

[Severity: High]
This is a pre-existing issue, but on the zero-copy path the on-stack
xdp_buff in otx2_xdp_rcv_pkt_handler() is never initialized:

	struct xdp_buff xdp, *xsk_buff = NULL;
	...
	if (pool->xsk_pool) {
		xsk_buff = pool->xdp[--cq->rbpool->xdp_top];
		...
		act = bpf_prog_run_xdp(prog, xsk_buff);
		goto handle_xdp_verdict;
	}

This jumps past xdp_init_buff() and xdp_prepare_buff(). Only the
XDP_REDIRECT and drop cases check xsk_buff, so in ZC mode these cases run
on the uninitialized struct:

	case XDP_PASS:
		*metasize = xdp.data - xdp.data_meta;
		break;
	case XDP_TX:
		...
		xdpf = xdp_convert_buff_to_frame(&xdp);

Does XDP_TX here dereference an uninitialized xdp.rxq inside
xdp_convert_buff_to_frame()? With CONFIG_INIT_STACK_ALL_ZERO that is a
NULL dereference in NAPI context; without it, it is a wild pointer.

XDP_PASS also seems to compute metasize from uninitialized fields, and the
caller then passes that value to skb_metadata_set().

> diff --git a/net/xdp/xsk.c b/net/xdp/xsk.c
> index 781ba46eb1525..89bac2feeff26 100644
> --- a/net/xdp/xsk.c
> +++ b/net/xdp/xsk.c
> @@ -224,21 +224,15 @@ static int xsk_rcv_zc(struct xdp_sock *xs, struct xdp_buff *xdp, u32 len)
>  	struct list_head *xskb_list;
>  	u32 contd = 0;
>  	u32 num_desc;
> -	int err;
>  
> -	if (likely(!frags)) {
> -		err = __xsk_rcv_zc(xs, xskb, len, contd);
> -		if (err)
> -			goto err;
> -		return 0;
> -	}
> +	if (likely(!frags))
> +		return __xsk_rcv_zc(xs, xskb, len, contd);

[Severity: Medium]
Now that the buffer is no longer freed on -ENOBUFS, ixgbe's need_wakeup
retry runs XDP again on a live buffer. Is the buffer state still correct
for that rerun?

When need_wakeup is enabled, ixgbe_run_xdp_zc() returns IXGBE_XDP_EXIT on
-ENOBUFS. ixgbe_clean_rx_irq_zc() then breaks out without clearing
bi->xdp or advancing next_to_clean:

		} else if (xdp_res == IXGBE_XDP_EXIT) {
			failure = true;
			break;

On the next poll the same buffer is processed again with:

		bi->xdp->data_end = bi->xdp->data + size;
		xsk_buff_dma_sync_for_cpu(bi->xdp);
		xdp_res = ixgbe_run_xdp_zc(adapter, rx_ring, bi->xdp);

xdp->data and xdp->data_meta are not reset first; igb uses
xsk_buff_set_size() for this. Any bpf_xdp_adjust_head(),
bpf_xdp_adjust_meta() or header rewrite from the first run therefore
carries over.

After adjust_head(+N), wouldn't the rerun window extend N bytes past the
received frame, possibly past the end of the UMEM chunk? If the second
redirect succeeds, user space could get a shifted, truncated or over-long
descriptor.

In this patch, i40e and ice free and consume the descriptor on EXIT
instead. The v2 notes leave that change for ixgbe and igb to a follow-up.
Should ixgbe drop and consume the descriptor here as well, or reset the
buffer before the rerun?

On igb, the commit message says:

  - ixgbe and igb keep the buffer in the ring on their need_wakeup
    -ENOBUFS (XDP_EXIT) path and run XDP on it again on the next poll.

In igb_run_xdp_zc(), out_failure falls through to XDP_DROP, which
overwrites IGB_XDP_EXIT with IGB_XDP_CONSUMED. Does igb actually ever take
an EXIT path? If not, the igb part of this description looks inaccurate.

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261104-xsk-zc-rx-double-free-v3-1-11a0c7ef1973%40vecima.com

Re: [PATCH net v3] xsk: Do not free ZC Rx buffer when the Rx ring is full

[Marcin Knap]

> - [Medium] After this patch, ixgbe's need_wakeup -ENOBUFS path keeps a
>   live buffer and runs XDP on it again, but with stale data pointers.

This is pre-existing. ixgbe already ran XDP again on the same buffer
before this patch, and data and data_meta were not reset then either.
The only difference was that the buffer had already been freed by
xsk_rcv_zc() and could be on the pool free list. The stale pointers are
not introduced here. As discussed for v2, ixgbe is left for a follow-up.

The review also points out that igb never returns IGB_XDP_EXIT
(out_failure falls through to XDP_DROP, which sets IGB_XDP_CONSUMED),
so v4 removes igb from that part of the commit message.

> Pre-existing issues:
> - [High] The rule that the caller owns the buffer when redirect fails
>   still breaks when a zero-copy buffer is redirected to anything other…

Agreed, this is pre-existing. xdp_convert_zc_to_xdp_frame() frees the
ZC buffer before a non-XSKMAP redirect can still fail. It is separate
from the XSKMAP -ENOBUFS path fixed here; v4 says in the commit message
that the fix covers XSKMAP redirects.

> - [High] In otx2_xdp_rcv_pkt_handler()
>   (drivers/net/ethernet/marvell/octeontx2/nic/otx2_txrx.c), the two…
> - [High] In otx2_xdp_rcv_pkt_handler()
>   (drivers/net/ethernet/marvell/octeontx2/nic/otx2_txrx.c), the AF_XDP…

Both are pre-existing otx2 issues that are not related to the ZC Rx
ring full path, so they are not addressed by this patch.

pw-bot: cr