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

[PATCH bpf] 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, and
drivers rely on that: mlx5e treats the failure as XDP_DROP and releases
the buffer when the Rx WQE is refilled, and i40e and ice free it on
XDP_CONSUMED. The buffer is therefore freed twice. It gets reused for
another Rx descriptor, the second free puts an in-use buffer back on
the pool free list, and the same UMEM frame is later handed to user
space twice. On mlx5 with striding RQ this also ended in a NULL pointer
dereference in mlx5e_post_rx_wqes().

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. i40e and ice relied on the core freeing the buffer on their
need_wakeup -ENOBUFS (XDP_EXIT) path, so make them free it there.

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")
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.
---
 drivers/net/ethernet/intel/i40e/i40e_xsk.c |  1 +
 drivers/net/ethernet/intel/ice/ice_xsk.c   |  1 +
 net/xdp/xsk.c                              | 15 +++------------
 3 files changed, 5 insertions(+), 12 deletions(-)

diff --git a/drivers/net/ethernet/intel/i40e/i40e_xsk.c b/drivers/net/ethernet/intel/i40e/i40e_xsk.c
index 9f47388eaba5..d32c7255f0b3 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_xsk.c
+++ b/drivers/net/ethernet/intel/i40e/i40e_xsk.c
@@ -358,6 +358,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/net/xdp/xsk.c b/net/xdp/xsk.c
index 33475b180ea6..0edc160c660f 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: ff47652a4b66c067c765a7ad464d930b5a9367cc
change-id: 20261019-xsk-zc-rx-double-free-1a3b059cd41f

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

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

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] xsk: missing xsk_buff_free() on redirect error in gve and octeontx2
--

commit 2859b3e778dcb29dabcd4c998977aac223d207b7
Author: Marcin Knap <marcin.knap@vecima.com>

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

This commit stops xsk_rcv_zc() from calling xsk_buff_free() when the XSK Rx
ring is full (-ENOBUFS) so that callers of xdp_do_redirect() do not
double-free the zero-copy buffer. It also updates i40e and ice to free the
buffer on their XDP_EXIT paths.

> diff --git a/net/xdp/xsk.c b/net/xdp/xsk.c
> index 33475b180ea6f..0edc160c660fd 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;
>  }

[Severity: High]
Does removing xsk_buff_free(xdp) here leak xdp_buffs in gve and octeontx2
when xdp_do_redirect() fails with -ENOBUFS?

In drivers/net/ethernet/google/gve/gve_rx_dqo.c, the call chain
gve_rx_poll_dqo() -> gve_rx_dqo() -> gve_rx_xsk_dqo() -> gve_xsk_done_dqo()
handles XDP_REDIRECT on zero-copy buffers without freeing xdp on error:

	case XDP_REDIRECT:
		if (unlikely(xdp_do_redirect(priv->dev, xdp, xprog)))
			goto err;
		break;
	}
	...
err:
	u64_stats_update_begin(&rx->statss);
	if (xdp_act == XDP_TX)
		rx->xdp_tx_errors++;
	if (xdp_act == XDP_REDIRECT)
		rx->xdp_redirect_errors++;
	u64_stats_update_end(&rx->statss);

Its caller gve_rx_xsk_dqo() then releases buf_state via
gve_free_buf_state(rx, buf_state) without freeing xdp, leaking it from
rx->xsk_pool.

Similarly, in drivers/net/ethernet/marvell/octeontx2/nic/otx2_txrx.c, the
call chain otx2_napi_handler() -> otx2_rx_napi_handler() ->
otx2_rcv_pkt_handler() -> otx2_xdp_rcv_pkt_handler() pops xsk_buff from
pool->xdp[--cq->rbpool->xdp_top] and does not free it if xdp_do_redirect()
fails:

	case XDP_REDIRECT:
		cq->pool_ptrs++;
		if (xsk_buff) {
			err = xdp_do_redirect(pfvf->netdev, xsk_buff, prog);
			if (!err) {
				*need_xdp_flush = true;
				return true;
			}
			return false;
		}

Should gve_xsk_done_dqo() and otx2_xdp_rcv_pkt_handler() also be updated
to free the zero-copy buffer when xdp_do_redirect() fails?

>  static void *xsk_copy_xdp_start(struct xdp_buff *from)

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261019-xsk-zc-rx-double-free-v1-1-2aa19f6628fb@vecima.com?part=1

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

[bot+bpf-ci]

[-- Attachment #1: Type: text/plain, Size: 3233 bytes --]

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

TL;DR: Since xsk_rcv_zc() no longer frees the buffer on a full Rx ring, gve
DQO zero-copy Rx may leak one UMEM frame per dropped packet and eventually
stall AF_XDP Rx; it likely needs xsk_buff_free().

> diff --git a/net/xdp/xsk.c b/net/xdp/xsk.c
> index 33475b180ea6..0edc160c660f 100644
> --- a/net/xdp/xsk.c
> +++ b/net/xdp/xsk.c

[ ... ]

> @@ -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)

Can this leak the UMEM frame in the gve DQO zero-copy Rx path?

With this change xsk_rcv_zc() no longer frees the buffer when the Rx ring
is full and it returns -ENOBUFS, so every zero-copy driver has to free or
keep the buffer itself when xdp_do_redirect() fails.  i40e and ice are
updated here, but gve DQO also relied on the core free:

drivers/net/ethernet/google/gve/gve_rx_dqo.c:gve_xsk_done_dqo() {
    ...
	case XDP_REDIRECT:
		if (unlikely(xdp_do_redirect(priv->dev, xdp, xprog)))
			goto err;
		break;
    ...
err:
	u64_stats_update_begin(&rx->statss);
	...
	if (xdp_act == XDP_REDIRECT)
		rx->xdp_redirect_errors++;
	u64_stats_update_end(&rx->statss);
}

The err: path only updates stats and never calls xsk_buff_free(xdp).

The caller, gve_rx_xsk_dqo(), then calls gve_free_buf_state(rx, buf_state),
which puts the buf_state back on the free list.  gve_alloc_buf_state() later
sets buf_state->xsk_buff = NULL without freeing it, and the ring-stop cleanup
only frees xsk buffers attached to allocated buf_states, so nothing calls
xp_free() on this buffer.

The path that gets there is:

gve_rx_dqo()
  gve_rx_xsk_dqo()
    gve_xsk_done_dqo()
      xdp_do_redirect()
        __xdp_do_redirect_xsk()
          __xsk_map_redirect()
            xsk_rcv()
              xsk_rcv_zc()
                __xsk_rcv_zc()
                  xskq_prod_reserve_desc() returns -ENOBUFS

Before this patch the err: label in xsk_rcv_zc() freed the buffer in exactly
this case, which is the scenario the commit targets (user space not keeping
up with the Rx ring).  After this patch gve DQO leaks one UMEM frame for
each packet dropped because the ring is full.  The frame never returns to
the pool free list or to user space, so under sustained overload
xsk_buff_alloc() starts failing in gve_alloc_buffer() and AF_XDP Rx on that
queue stalls until the socket is torn down.

The commit message says "A failed xdp_do_redirect() leaves the buffer owned
by the caller, and drivers rely on that" and names only i40e and ice as
relying on the core free.  Does gve_xsk_done_dqo() need an xsk_buff_free(xdp)
on the XDP_REDIRECT failure path as part of this change?

The other zero-copy drivers that were checked (ixgbe, igb, igc, libeth/idpf,
gve GQI, mlx5, tsnep, fec, dpaa2, nfp, stmmac, icssg and virtio_net) already
free, recycle or keep the buffer on redirect failure.


---
AI reviewed your patch. Please fix the bug or email reply why it's not a bug.
See: https://github.com/kernel-patches/vmtest/blob/master/ci/claude/README.md

CI run summary: https://github.com/kernel-patches/bpf/actions/runs/39223384647