[PATCH net 0/9] xsk: ZC mbuf fixes

[PATCH net 0/9] xsk: ZC mbuf fixes

[Maciej Fijalkowski]

Welcome,

Nagato Shiratana reported that duplicate fill ring addresses in aligned
mode let a driver pass the same buffer to xsk_buff_add_frag() twice.
This corrupts pool->xskb_list and can lead to a NULL pointer
dereference in xp_free(). Patch 1 makes xsk_buff_add_frag() reject
such buffers.

Inspecting the rest of the zero-copy multi-buffer code found more
issues:

- i40e retries a freed frame, or stalls the queue on a duplicate, when
  xsk_buff_add_frag() fails (patch 2)
- ice leaks the buffer when a frag is empty or cannot be added (patch 3)
- ice and libeth free the frags of a frame still being received when
  a multi-buffer XDP_TX frame completes (patches 4-5)
- handing out a duplicate of a frame's head clears its frags flag, so
  its frags stay on pool->xskb_list and end up in the next frame
  (patch 6)
- ice and idpf leak a partially received frame on ring cleanup, and
  its frags end up in the next frame (patches 8-9)

Patch 7 is Chenguang Zhao's fix for i40e building broken skbs from
multi-buffer frames. It was posted earlier in an IWL pull request [0]
and is included here unchanged.

Patches 1 and 6 were tested with KUnit tests based on the reporter's
reproducer, plus I verified to-be-upstreamed xsk.py test suite passes
on ice driver (back-to-back setup based on netns) with this set applied.

Thanks,
MF

[0] https://lore.kernel.org/netdev/20260918212458.550425-9-anthony.l.nguyen@intel.com/


Chenguang Zhao (1):
  i40e: xsk: fix multi-buffer XDP_PASS skb construction

Maciej Fijalkowski (8):
  xsk: fix list corruption in xsk_buff_add_frag()
  i40e: xsk: fix xsk_buff_add_frag() error handling in Rx
  ice: xsk: fix leaking Rx buffers in multi-buffer path
  ice: xsk: do not free in-flight frags on XDP_TX completion
  libeth: xsk: do not free in-flight frags on XDP_TX completion
  xsk: do not clear frags flag when acquiring a buffer
  ice: xsk: free stashed Rx frame when cleaning the ring
  idpf: free stashed XSk Rx frame when releasing the queue

 drivers/net/ethernet/intel/i40e/i40e_xsk.c | 35 ++++++++++++----------
 drivers/net/ethernet/intel/ice/ice_xsk.c   | 19 ++++++++++--
 drivers/net/ethernet/intel/idpf/xsk.c      |  5 ++++
 include/net/libeth/xsk.h                   | 10 +++++--
 include/net/xdp_sock_drv.h                 | 11 +++++--
 net/xdp/xsk.c                              |  1 +
 net/xdp/xsk_buff_pool.c                    |  2 +-
 7 files changed, 60 insertions(+), 23 deletions(-)

-- 
2.43.0

[PATCH net 1/9] xsk: fix list corruption in xsk_buff_add_frag()

[Maciej Fijalkowski]

In aligned mode, a fill ring address maps straight to its pool->heads[]
entry. If user space posts the same address twice, a driver can pass
xsk_buff_add_frag() a buffer that is the head of the frame or is
already on pool->xskb_list or pool->free_list.

Re-adding a linked node corrupts the list. With buffers A, B, C, B,
xsk_rcv_zc() emits two descriptors instead of four and leaves the list
head pointing at a self-linked B. When the Rx ring later has no room
for a frame, the xsk_buff_free() cleanup follows B into pool->free_list
and passes the list head to xp_free():

  BUG: kernel NULL pointer dereference, address: 00000000000000c0
  RIP: 0010:xp_free+0x1a/0x50
  Call Trace:
   xsk_rcv_zc+0xca/0x200

The head is not on any list and needs a separate check. Adding it as
its own frag lets bpf_xdp_adjust_tail() free the in-use head and wrap
sinfo->nr_frags to 255.

Reject such buffers before adding them to the frags array, as commit
e37b2abca804 ("xsk: fix NULL pointer dereference in __xsk_rcv()") does
in the copy path. Callers already drop the frame on failure.

Unaligned mode is not affected, as each fill ring entry gets its own
xskb from pool->free_heads[].

Reproduced with a KUnit test feeding synthetic buffers to xsk_rcv_zc(),
by Nagato Shiratana.

Fixes: 24ea50127ecf ("xsk: support mbuf on ZC RX")
Reported-by: Nagato Shiratana <zuomao338@gmail.com>
Signed-off-by: Maciej Fijalkowski <maciej.fijalkowski@intel.com>
---
 include/net/xdp_sock_drv.h | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/include/net/xdp_sock_drv.h b/include/net/xdp_sock_drv.h
index d94aeb506379..29965e369598 100644
--- a/include/net/xdp_sock_drv.h
+++ b/include/net/xdp_sock_drv.h
@@ -160,15 +160,18 @@ static inline void xsk_buff_free(struct xdp_buff *xdp)
 static inline bool xsk_buff_add_frag(struct xdp_buff *head,
 				     struct xdp_buff *xdp)
 {
+	struct xdp_buff_xsk *frag = container_of(xdp, struct xdp_buff_xsk, xdp);
 	const void *data = xdp->data;
-	struct xdp_buff_xsk *frag;
+
+	/* Duplicate fill ring entries map to the same xskb in aligned mode */
+	if (unlikely(xdp == head || !list_empty(&frag->list_node)))
+		return false;
 
 	if (!__xdp_buff_add_frag(head, virt_to_netmem(data),
 				 offset_in_page(data), xdp->data_end - data,
 				 xdp->frame_sz, false))
 		return false;
 
-	frag = container_of(xdp, struct xdp_buff_xsk, xdp);
 	list_add_tail(&frag->list_node, &frag->pool->xskb_list);
 
 	return true;
-- 
2.43.0

[PATCH net 2/9] i40e: xsk: fix xsk_buff_add_frag() error handling in Rx

[Maciej Fijalkowski]

When xsk_buff_add_frag() fails, i40e_clean_rx_irq_zc() frees the frame
but stays on the current descriptor, so the next poll restarts from the
freed head. If the frags array was full, the retry passes on a frame
missing the freed frags, with its head already on the free list. If the
buffer is a duplicate, every poll fails the same way and the queue
stalls.

Free the current buffer along with the frame and move on to the next
descriptor, as ice does. The rest of the dropped frame is received as
a new frame. xp_free() skips a duplicate already freed with the frame.

Fixes: 1c9ba9c14658 ("i40e: xsk: add RX multi-buffer support")
Signed-off-by: Maciej Fijalkowski <maciej.fijalkowski@intel.com>
---
 drivers/net/ethernet/intel/i40e/i40e_xsk.c | 20 ++++++++++++++------
 1 file changed, 14 insertions(+), 6 deletions(-)

diff --git a/drivers/net/ethernet/intel/i40e/i40e_xsk.c b/drivers/net/ethernet/intel/i40e/i40e_xsk.c
index 51a05ce4c7ce..e6ae4c7a5cb3 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_xsk.c
+++ b/drivers/net/ethernet/intel/i40e/i40e_xsk.c
@@ -469,16 +469,24 @@ int i40e_clean_rx_irq_zc(struct i40e_ring *rx_ring, int budget)
 		xsk_buff_set_size(bi, size);
 		xsk_buff_dma_sync_for_cpu(bi);
 
-		if (!first)
+		if (++next_to_process == count)
+			next_to_process = 0;
+
+		if (!first) {
 			first = bi;
-		else if (!xsk_buff_add_frag(first, bi)) {
+		} else if (!xsk_buff_add_frag(first, bi)) {
+			/* Drop what has been gathered so far. xp_free() skips
+			 * @bi if it duplicates a buffer that is already on a
+			 * pool list. Remaining buffers of this packet, if any,
+			 * will be processed as a new packet.
+			 */
 			xsk_buff_free(first);
-			break;
+			xsk_buff_free(bi);
+			next_to_clean = next_to_process;
+			first = NULL;
+			continue;
 		}
 
-		if (++next_to_process == count)
-			next_to_process = 0;
-
 		if (i40e_is_non_eop(rx_ring, rx_desc))
 			continue;
 
-- 
2.43.0

[PATCH net 3/9] ice: xsk: fix leaking Rx buffers in multi-buffer path

[Maciej Fijalkowski]

ice_clean_rx_irq_zc() consumes a descriptor without freeing its buffer
when a frag has zero length or when xsk_buff_add_frag() fails, which
happens if the frags array is full or the fill ring has duplicates.
Such buffers are lost to user space until the socket is closed.

Free the buffer in both cases. xp_free() skips a duplicate already
freed with the frame.

Fixes: 1bbc04de607b ("ice: xsk: add RX multi-buffer support")
Fixes: 93f53db9f9dc ("ice: switch to Page Pool")
Signed-off-by: Maciej Fijalkowski <maciej.fijalkowski@intel.com>
---
 drivers/net/ethernet/intel/ice/ice_xsk.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/intel/ice/ice_xsk.c b/drivers/net/ethernet/intel/ice/ice_xsk.c
index 0643017541c3..ea8ac0fcdf6e 100644
--- a/drivers/net/ethernet/intel/ice/ice_xsk.c
+++ b/drivers/net/ethernet/intel/ice/ice_xsk.c
@@ -618,8 +618,11 @@ int ice_clean_rx_irq_zc(struct ice_rx_ring *rx_ring,
 
 		if (!first) {
 			first = xdp;
-		} else if (likely(size) && !xsk_buff_add_frag(first, xdp)) {
+		} else if (unlikely(!size)) {
+			xsk_buff_free(xdp);
+		} else if (!xsk_buff_add_frag(first, xdp)) {
 			xsk_buff_free(first);
+			xsk_buff_free(xdp);
 			first = NULL;
 		}
 
-- 
2.43.0

[PATCH net 4/9] ice: xsk: do not free in-flight frags on XDP_TX completion

[Maciej Fijalkowski]

ice_xmit_xdp_tx_zc() takes the frags of a multi-buffer frame off
pool->xskb_list but leaves XDP_FLAGS_HAS_FRAGS set on the head. When
the head completes, xsk_buff_free() frees whatever is on the list at
that point, which are the frags of a frame still being received.

If ice_xmit_xdp_tx_zc() completes such a head while cleaning the ring
before sending another frame, xsk_buff_get_frag() returns NULL and
xsk_buff_xdp_get_dma() dereferences it.

Clear the flag on the head once its frags are on the ring. As
xdp_get_buff_len() then covers only the head, count Rx bytes before
running the XDP program, like libeth does.

Fixes: eeb2b5381038 ("ice: xsk: Tx multi-buffer support")
Signed-off-by: Maciej Fijalkowski <maciej.fijalkowski@intel.com>
---
 drivers/net/ethernet/intel/ice/ice_xsk.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/intel/ice/ice_xsk.c b/drivers/net/ethernet/intel/ice/ice_xsk.c
index ea8ac0fcdf6e..bf14ed8f87a3 100644
--- a/drivers/net/ethernet/intel/ice/ice_xsk.c
+++ b/drivers/net/ethernet/intel/ice/ice_xsk.c
@@ -493,6 +493,11 @@ static int ice_xmit_xdp_tx_zc(struct xdp_buff *xdp,
 	tx_desc->cmd_type_offset_bsz |=
 		cpu_to_le64(ICE_TX_DESC_CMD_EOP << ICE_TXD_QW1_CMD_S);
 
+	/* Each frag is now freed via its own Tx buffer. Completing the head
+	 * must not release what is on the pool's frag list at that time.
+	 */
+	xdp_buff_clear_frags_flag(head);
+
 	return ICE_XDP_TX;
 
 busy:
@@ -592,6 +597,7 @@ int ice_clean_rx_irq_zc(struct ice_rx_ring *rx_ring,
 		struct sk_buff *skb;
 		u16 stat_err_bits;
 		u16 vlan_tci;
+		u32 xdp_len;
 
 		rx_desc = ICE_RX_DESC(rx_ring, ntc);
 
@@ -634,6 +640,7 @@ int ice_clean_rx_irq_zc(struct ice_rx_ring *rx_ring,
 
 		((struct libeth_xdp_buff *)first)->desc = rx_desc;
 
+		xdp_len = xdp_get_buff_len(first);
 		xdp_res = ice_run_xdp_zc(rx_ring, first, xdp_prog, xdp_ring,
 					 xsk_pool);
 		if (likely(xdp_res & (ICE_XDP_TX | ICE_XDP_REDIR))) {
@@ -648,7 +655,7 @@ int ice_clean_rx_irq_zc(struct ice_rx_ring *rx_ring,
 			goto construct_skb;
 		}
 
-		total_rx_bytes += xdp_get_buff_len(first);
+		total_rx_bytes += xdp_len;
 		total_rx_packets++;
 
 		first = NULL;
-- 
2.43.0

[PATCH net 5/9] libeth: xsk: do not free in-flight frags on XDP_TX completion

[Maciej Fijalkowski]

libeth_xsk_tx_queue_bulk() takes the frags of a multi-buffer frame off
pool->xskb_list but leaves XDP_FLAGS_HAS_FRAGS set on the head. When
the head completes, libeth_xsk_buff_free_slow() frees whatever is on
the list at that point.

The XDPSQ can be completed by a bulk flush while the frags of a frame
are being queued, or by its cleanup timer while a partial Rx frame is
stashed. Those frags are then freed while in use, and the frame being
queued is sent truncated.

Clear the flag on the head once all of its frags are queued.

Fixes: b3ad8450b4dc ("libeth: xsk: add XSk XDP_TX sending helpers")
Signed-off-by: Maciej Fijalkowski <maciej.fijalkowski@intel.com>
---
 include/net/libeth/xsk.h | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/include/net/libeth/xsk.h b/include/net/libeth/xsk.h
index a452b7828ce4..249b1c740788 100644
--- a/include/net/libeth/xsk.h
+++ b/include/net/libeth/xsk.h
@@ -77,11 +77,17 @@ libeth_xsk_tx_queue_bulk(struct libeth_xdp_tx_bulk *bq,
 	if (!libeth_xsk_tx_queue_head(bq, xdp))
 		goto out;
 
-	for (const struct libeth_xdp_buff *head = xdp; ; ) {
+	for (struct libeth_xdp_buff *head = xdp; ; ) {
 		xdp = container_of(xsk_buff_get_frag(&head->base),
 				   typeof(*xdp), base);
-		if (!xdp)
+		if (!xdp) {
+			/* Each frag is now freed via its own SQE. Freeing
+			 * the head must not release what is on the pool's
+			 * frag list at that time.
+			 */
+			xdp_buff_clear_frags_flag(&head->base);
 			break;
+		}
 
 		if (unlikely(bq->count == LIBETH_XDP_TX_BULK) &&
 		    unlikely(!flush_bulk(bq, LIBETH_XDP_TX_XSK))) {
-- 
2.43.0

[PATCH net 6/9] xsk: do not clear frags flag when acquiring a buffer

[Maciej Fijalkowski]

xp_alloc() and xsk_buff_set_size() reset xdp_buff::flags of every
buffer they hand out. In aligned mode, duplicate fill ring addresses
map to the same xskb, so the head of a frame still being gathered can
be handed out again and lose XDP_FLAGS_HAS_FRAGS. xsk_buff_add_frag()
rejects the duplicate and the frame is dropped, but xsk_buff_free()
leaves its frags on pool->xskb_list, where they are taken as frags of
the next frame.

Keep XDP_FLAGS_HAS_FRAGS on hand-out and clear it when the frags are
released, in xsk_buff_free() and xsk_rcv_zc(). The driver XDP_TX paths,
bpf_xdp_adjust_tail() and bpf_xdp_pull_data() already clear it, and
pool->heads starts zeroed. Other flags are still reset on hand-out, as
nothing clears them on release.

Since xsk_rcv_zc() now clears the flag, i40e must count Rx bytes before
running the XDP program, like ice and libeth.

Fixes: f7f6aa8e2438 ("xsk: make xsk_buff_pool responsible for clearing xdp_buff::flags")
Signed-off-by: Maciej Fijalkowski <maciej.fijalkowski@intel.com>
---
 drivers/net/ethernet/intel/i40e/i40e_xsk.c | 2 +-
 include/net/xdp_sock_drv.h                 | 4 +++-
 net/xdp/xsk.c                              | 1 +
 net/xdp/xsk_buff_pool.c                    | 2 +-
 4 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/intel/i40e/i40e_xsk.c b/drivers/net/ethernet/intel/i40e/i40e_xsk.c
index e6ae4c7a5cb3..57b0f520fbb2 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_xsk.c
+++ b/drivers/net/ethernet/intel/i40e/i40e_xsk.c
@@ -354,7 +354,6 @@ static void i40e_handle_xdp_result_zc(struct i40e_ring *rx_ring,
 	struct sk_buff *skb;
 
 	*rx_packets = 1;
-	*rx_bytes = xdp_get_buff_len(xdp_buff);
 
 	if (likely(xdp_res == I40E_XDP_REDIR) || xdp_res == I40E_XDP_TX)
 		return;
@@ -492,6 +491,7 @@ int i40e_clean_rx_irq_zc(struct i40e_ring *rx_ring, int budget)
 
 		xsk_buff_to_i40e_ctx(first)->desc = rx_desc;
 
+		rx_bytes = xdp_get_buff_len(first);
 		xdp_res = i40e_run_xdp_zc(rx_ring, first, xdp_prog);
 		i40e_handle_xdp_result_zc(rx_ring, first, rx_desc, &rx_packets,
 					  &rx_bytes, xdp_res, &failure);
diff --git a/include/net/xdp_sock_drv.h b/include/net/xdp_sock_drv.h
index 29965e369598..a40ad3f90f13 100644
--- a/include/net/xdp_sock_drv.h
+++ b/include/net/xdp_sock_drv.h
@@ -153,6 +153,7 @@ static inline void xsk_buff_free(struct xdp_buff *xdp)
 	}
 
 	xdp_get_shared_info_from_buff(xdp)->nr_frags = 0;
+	xdp_buff_clear_frags_flag(xdp);
 out:
 	xp_free(xskb);
 }
@@ -225,7 +226,8 @@ static inline void xsk_buff_set_size(struct xdp_buff *xdp, u32 size)
 	xdp->data = xdp->data_hard_start + XDP_PACKET_HEADROOM;
 	xdp->data_meta = xdp->data;
 	xdp->data_end = xdp->data + size;
-	xdp->flags = 0;
+	/* Frags flag is cleared when the frags of a head are released */
+	xdp->flags &= XDP_FLAGS_HAS_FRAGS;
 }
 
 static inline dma_addr_t xsk_buff_raw_get_dma(struct xsk_buff_pool *pool,
diff --git a/net/xdp/xsk.c b/net/xdp/xsk.c
index 33475b180ea6..d22be82533ae 100644
--- a/net/xdp/xsk.c
+++ b/net/xdp/xsk.c
@@ -241,6 +241,7 @@ static int xsk_rcv_zc(struct xdp_sock *xs, struct xdp_buff *xdp, u32 len)
 		goto err;
 	}
 
+	xdp_buff_clear_frags_flag(xdp);
 	__xsk_rcv_zc_safe(xs, xskb, len, contd);
 	xskb_list = &xskb->pool->xskb_list;
 	list_for_each_entry_safe(pos, tmp, xskb_list, list_node) {
diff --git a/net/xdp/xsk_buff_pool.c b/net/xdp/xsk_buff_pool.c
index 9d2d94f1fb75..4318ec98199e 100644
--- a/net/xdp/xsk_buff_pool.c
+++ b/net/xdp/xsk_buff_pool.c
@@ -597,7 +597,7 @@ struct xdp_buff *xp_alloc(struct xsk_buff_pool *pool)
 
 	xskb->xdp.data = xskb->xdp.data_hard_start + XDP_PACKET_HEADROOM;
 	xskb->xdp.data_meta = xskb->xdp.data;
-	xskb->xdp.flags = 0;
+	xskb->xdp.flags &= XDP_FLAGS_HAS_FRAGS;
 
 	if (pool->dev)
 		xp_dma_sync_for_device(pool, xskb->dma, pool->frame_len);
-- 
2.43.0

[PATCH net 7/9] i40e: xsk: fix multi-buffer XDP_PASS skb construction

[Maciej Fijalkowski]

From: Chenguang Zhao <zhaochenguang@kylinos.cn>

When AF_XDP ZC receives a multi-buffer frame and the XDP program
returns XDP_PASS, i40e_construct_skb_zc() copies frags into a new
skb. The copy used skb_frag_page() as the memcpy source (page
metadata instead of packet data) and passed a virtual address to
__skb_fill_page_desc_noacc(), which expects a struct page *.

Use skb_frag_address() for the copy, attach frags with
skb_add_rx_frag() so len/data_len/truesize are updated, and on
dev_alloc_page() failure free the skb via the shared out path so
xsk_buff_free() still runs and previously attached pages are
released by kfree_skb.

Fixes: 1c9ba9c14658 ("i40e: xsk: add RX multi-buffer support")
Signed-off-by: Chenguang Zhao <zhaochenguang@kylinos.cn>
Reviewed-by: Aleksandr Loktionov <aleksandr.loktionov@intel.com>
Reviewed-by: Jason Xing <kerneljasonxing@gmail.com>
Acked-by: Maciej Fijalkowski <maciej.fijalkowski@intel.com>
Tested-by: Patryk Holda <patryk.holda@intel.com>
Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
---
 drivers/net/ethernet/intel/i40e/i40e_xsk.c | 13 +++++--------
 1 file changed, 5 insertions(+), 8 deletions(-)

diff --git a/drivers/net/ethernet/intel/i40e/i40e_xsk.c b/drivers/net/ethernet/intel/i40e/i40e_xsk.c
index 57b0f520fbb2..5f5f33b2a097 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_xsk.c
+++ b/drivers/net/ethernet/intel/i40e/i40e_xsk.c
@@ -320,22 +320,19 @@ static struct sk_buff *i40e_construct_skb_zc(struct i40e_ring *rx_ring,
 		goto out;
 
 	for (int i = 0; i < nr_frags; i++) {
-		struct skb_shared_info *skinfo = skb_shinfo(skb);
 		skb_frag_t *frag = &sinfo->frags[i];
+		unsigned int frag_size = skb_frag_size(frag);
 		struct page *page;
-		void *addr;
 
 		page = dev_alloc_page();
 		if (!page) {
 			dev_kfree_skb(skb);
-			return NULL;
+			skb = NULL;
+			goto out;
 		}
-		addr = page_to_virt(page);
 
-		memcpy(addr, skb_frag_page(frag), skb_frag_size(frag));
-
-		__skb_fill_page_desc_noacc(skinfo, skinfo->nr_frags++,
-					   addr, 0, skb_frag_size(frag));
+		memcpy(page_to_virt(page), skb_frag_address(frag), frag_size);
+		skb_add_rx_frag(skb, i, page, 0, frag_size, PAGE_SIZE);
 	}
 
 out:
-- 
2.43.0

[PATCH net 8/9] ice: xsk: free stashed Rx frame when cleaning the ring

[Maciej Fijalkowski]

ice_clean_rx_irq_zc() stashes a partially received multi-buffer frame
in rx_ring->xsk, with its frags on pool->xskb_list. next_to_clean is
already past its buffers, so ice_xsk_clean_rx_ring(), which walks
next_to_clean..next_to_use, misses the frame, and ice_vsi_cfg_rxq()
later clears the pointer via ring->xdp.data.

The buffers are lost to user space and, as the pool outlives the ring
across ifdown/ifup or a reset, the stale frags are taken as frags of
the next frame. Before commit 93f53db9f9dc ("ice: switch to Page
Pool"), the frame was tracked by rx_ring->first_desc and leaked the
same way.

Free the stashed frame when cleaning the ring.

Fixes: 1bbc04de607b ("ice: xsk: add RX multi-buffer support")
Signed-off-by: Maciej Fijalkowski <maciej.fijalkowski@intel.com>
---
 drivers/net/ethernet/intel/ice/ice_xsk.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/net/ethernet/intel/ice/ice_xsk.c b/drivers/net/ethernet/intel/ice/ice_xsk.c
index bf14ed8f87a3..0c1ce3e86a45 100644
--- a/drivers/net/ethernet/intel/ice/ice_xsk.c
+++ b/drivers/net/ethernet/intel/ice/ice_xsk.c
@@ -912,6 +912,11 @@ void ice_xsk_clean_rx_ring(struct ice_rx_ring *rx_ring)
 	if (xdp_rxq_info_is_reg(&rx_ring->xdp_rxq))
 		xdp_rxq_info_unreg(&rx_ring->xdp_rxq);
 
+	if (rx_ring->xsk) {
+		xsk_buff_free(&rx_ring->xsk->base);
+		rx_ring->xsk = NULL;
+	}
+
 	while (ntc != ntu) {
 		struct xdp_buff *xdp = *ice_xdp_buf(rx_ring, ntc);
 
-- 
2.43.0

[PATCH net 9/9] idpf: free stashed XSk Rx frame when releasing the queue

[Maciej Fijalkowski]

idpf_xskrq_poll() stashes a partially received multi-buffer frame in
rxq->xsk, with its frags on pool->xskb_list. Nothing frees or clears
rxq->xsk on queue release, as idpf_rx_desc_rel() only returns the page
stash sharing its storage for non-XSk queues.

On ifdown, the buffers are lost to user space and, as the pool outlives
the queue, the stale frags are taken as frags of the next frame. If the
pool is disabled on a running interface, the queue restarts in page
mode with rxq->xdp.data still pointing to an xskb of the pool, which is
freed later. The page path then takes it for a partial frame, writes
frag info through it and returns it to a page pool.

Free the stashed frame and clear the pointer when the XSk state of the
queue is cleared, while the pool is still attached.

Fixes: 9705d6552f58 ("idpf: implement Rx path for AF_XDP")
Signed-off-by: Maciej Fijalkowski <maciej.fijalkowski@intel.com>
---
 drivers/net/ethernet/intel/idpf/xsk.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/net/ethernet/intel/idpf/xsk.c b/drivers/net/ethernet/intel/idpf/xsk.c
index d95d3efdfd36..5a0a1d8a7a86 100644
--- a/drivers/net/ethernet/intel/idpf/xsk.c
+++ b/drivers/net/ethernet/intel/idpf/xsk.c
@@ -134,6 +134,11 @@ void idpf_xsk_clear_queue(void *q, enum virtchnl2_queue_type type)
 		if (!idpf_queue_has_clear(XSK, rxq))
 			return;
 
+		if (rxq->xsk) {
+			libeth_xsk_buff_free_slow(rxq->xsk);
+			rxq->xsk = NULL;
+		}
+
 		rxq->pool = NULL;
 		break;
 	case VIRTCHNL2_QUEUE_TYPE_RX_BUFFER:
-- 
2.43.0

Re: [PATCH net 0/9] xsk: ZC mbuf fixes

[netdev-bot+sinfo]

Hi!

This is an automated message. This series looks like a fix, but its
commit messages seem to be missing some information:

 - Whether the issue was actually triggered, or is only theoretical
   (e.g. found by code inspection). If it was triggered please include
   the symptoms, like the stack trace or error messages.

 - What hardware the change was tested on. For driver fixes please
   mention the device (and if relevant firmware version) used for
   testing, or say that the change was not tested on real hardware.

Please do not repost the series just to address the above. Instead,
reply to this email with the missing information, so that reviewers
can take it into account. If the series needs another revision for
other reasons, please include the information in the commit messages
then.

The evaluation is done by an LLM so it may be wrong, if you think
that is the case please reply and explain.

Re: [PATCH net 0/9] xsk: ZC mbuf fixes

[Maciej Fijalkowski]

On Mon, Nov 02, 2026 at 12:54:02PM +0000, netdev-bot+sinfo@kernel.org wrote:
> Hi!
> 
> This is an automated message. This series looks like a fix, but its
> commit messages seem to be missing some information:
> 
>  - Whether the issue was actually triggered, or is only theoretical
>    (e.g. found by code inspection). If it was triggered please include
>    the symptoms, like the stack trace or error messages.

whole series started from report at security ML which included KUnit
reproducer and I verified the issue is real and could be hit with ZC
driver. Part of oops is included in commit message in patch 1.

> 
>  - What hardware the change was tested on. For driver fixes please
>    mention the device (and if relevant firmware version) used for
>    testing, or say that the change was not tested on real hardware.

Ran xskxceiver's test suite via xsk.py runner on Intel E810-C (ice driver
fw revision 4.70). It does not cover the duplicate fill ring entries but
given the changes based on code inspection this run was to make sure I did
not introduce any regressions around multi-buffer logic.

> 
> Please do not repost the series just to address the above. Instead,
> reply to this email with the missing information, so that reviewers
> can take it into account. If the series needs another revision for
> other reasons, please include the information in the commit messages
> then.
> 
> The evaluation is done by an LLM so it may be wrong, if you think
> that is the case please reply and explain.

Re: [PATCH net 0/9] xsk: ZC mbuf fixes

[Björn Töpel]

Maciej Fijalkowski <maciej.fijalkowski@intel.com> writes:

> Welcome,
>
> Nagato Shiratana reported that duplicate fill ring addresses in aligned
> mode let a driver pass the same buffer to xsk_buff_add_frag() twice.
> This corrupts pool->xskb_list and can lead to a NULL pointer
> dereference in xp_free(). Patch 1 makes xsk_buff_add_frag() reject
> such buffers.
>
> Inspecting the rest of the zero-copy multi-buffer code found more
> issues:
>
> - i40e retries a freed frame, or stalls the queue on a duplicate, when
>   xsk_buff_add_frag() fails (patch 2)
> - ice leaks the buffer when a frag is empty or cannot be added (patch 3)
> - ice and libeth free the frags of a frame still being received when
>   a multi-buffer XDP_TX frame completes (patches 4-5)
> - handing out a duplicate of a frame's head clears its frags flag, so
>   its frags stay on pool->xskb_list and end up in the next frame
>   (patch 6)
> - ice and idpf leak a partially received frame on ring cleanup, and
>   its frags end up in the next frame (patches 8-9)
>
> Patch 7 is Chenguang Zhao's fix for i40e building broken skbs from
> multi-buffer frames. It was posted earlier in an IWL pull request [0]
> and is included here unchanged.
>
> Patches 1 and 6 were tested with KUnit tests based on the reporter's
> reproducer, plus I verified to-be-upstreamed xsk.py test suite passes
> on ice driver (back-to-back setup based on netns) with this set applied.

For the series,

Reviewed-by: Björn Töpel <bjorn@kernel.org>

Re: [PATCH net 1/9] xsk: fix list corruption in xsk_buff_add_frag()

[netdev-bot+sashiko]

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

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

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

- [High] At the reviewed commit, the new `xdp == head` rejection in
  xsk_buff_add_frag() (include/net/xdp_sock_drv.h:166-168) runs after…
- [High] At the reviewed commit, the new rejection in xsk_buff_add_frag()
  fails the same way every time it sees the same input.
- [Low] The kernel-doc for libeth_xsk_buff_add_frag()
  (drivers/net/ethernet/intel/libeth/xsk.c:50-51) says it returns '%NULL…

Pre-existing issues:
- [Low] When xsk_buff_add_frag() fails, ice_clean_rx_irq_zc() frees the
  partial frame and sets first = NULL but does not skip ahead to EOP.

--

xsk: fix list corruption in xsk_buff_add_frag()

This patch changes xsk_buff_add_frag() to reject a frag buffer that is the
frame head or that is already linked on pool->xskb_list or
pool->free_list. In aligned mode this happens when user space posts the
same fill ring address twice. The goal is to prevent list corruption and
the resulting NULL pointer dereference in xp_free().

> diff --git a/include/net/xdp_sock_drv.h b/include/net/xdp_sock_drv.h
> index d94aeb506379c..29965e3695983 100644
> --- a/include/net/xdp_sock_drv.h
> +++ b/include/net/xdp_sock_drv.h
> @@ -160,15 +160,18 @@ static inline void xsk_buff_free(struct xdp_buff *xdp)
>  static inline bool xsk_buff_add_frag(struct xdp_buff *head,
>  				     struct xdp_buff *xdp)
>  {
> +	struct xdp_buff_xsk *frag = container_of(xdp, struct xdp_buff_xsk, xdp);
>  	const void *data = xdp->data;
> -	struct xdp_buff_xsk *frag;
> +
> +	/* Duplicate fill ring entries map to the same xskb in aligned mode */
> +	if (unlikely(xdp == head || !list_empty(&frag->list_node)))
> +		return false;

[Severity: High]
Can the xdp == head case still leave stale frags on pool->xskb_list?

Drivers call xsk_buff_set_size() on the new buffer before they call
xsk_buff_add_frag(). For example, in ice_clean_rx_irq_zc():

    xsk_buff_set_size(xdp, size);
    xsk_buff_dma_sync_for_cpu(xdp);

    if (!first) {
        first = xdp;
    } else if (likely(size) && !xsk_buff_add_frag(first, xdp)) {
        xsk_buff_free(first);
        first = NULL;
    }

When the duplicate is the head, xsk_buff_set_size() has already reset the
flags on the live head:

include/net/xdp_sock_drv.h:xsk_buff_set_size() {
    ...
    xdp->flags = 0;
}

So XDP_FLAGS_HAS_FRAGS is gone by the time xsk_buff_free(first) runs, and
xsk_buff_free() skips draining the list:

    if (likely(!xdp_buff_has_frags(xdp)))
        goto out;

With fill ring entries A, B, A, wouldn't B stay on pool->xskb_list and get
attached to a later frame? xsk_rcv_zc() reserves nr_frags + 1 slots but
emits one descriptor per xskb_list entry, so the Rx ring accounting would
go out of sync.

A later patch in this series, "xsk: do not clear frags flag when acquiring
a buffer", seems to fix this. It makes xsk_buff_set_size() and xp_alloc()
keep XDP_FLAGS_HAS_FRAGS, and it makes xsk_buff_free() and xsk_rcv_zc()
clear it. Could that patch go before this one, or be folded into it, so
this commit works on its own?

[Severity: High]
The commit message says:

    Callers already drop the frame on failure.

Is that true for i40e? i40e_clean_rx_irq_zc() handles the failure like
this:

    else if (!xsk_buff_add_frag(first, bi)) {
        xsk_buff_free(first);
        break;
    }

It advances neither next_to_process nor next_to_clean. On the next poll the
head is rebuilt from the ring:

    if (next_to_process != next_to_clean)
        first = *i40e_rx_bi(rx_ring, next_to_clean);

The new rejection gives the same result for the same input. With duplicate
fill entries A, A, wouldn't the same head/frag pair fail on every poll,
each time starting from a head that was already freed? next_to_clean would
never move, and the ring would stop being refilled.

The next patch in the series, "i40e: xsk: fix xsk_buff_add_frag() error
handling in Rx", seems to fix this. It frees both buffers, sets
next_to_clean to next_to_process, clears first and continues. Should that
patch come before this one?

[Severity: Low]
This is a pre-existing issue and was not introduced by this patch, but
this check makes the caller failure paths reachable through duplicate fill
ring entries.

In ice_clean_rx_irq_zc(), a failed xsk_buff_add_frag() frees the partial
frame and sets first to NULL, but the remaining descriptors up to EOP are
not skipped:

    } else if (likely(size) && !xsk_buff_add_frag(first, xdp)) {
        xsk_buff_free(first);
        first = NULL;
    }

Won't the rest of the hardware frame then be assembled and delivered as a
new frame? idpf_xskrq_poll() does the same when libeth_xsk_process_buff()
returns NULL.

The i40e patch later in this series calls this intended ("The rest of the
dropped frame is received as a new frame"). It also does not appear to
corrupt kernel memory, so it may be acceptable as is.

[Severity: Low]
This isn't a bug, but the kernel-doc for libeth_xsk_buff_add_frag() in
drivers/net/ethernet/intel/libeth/xsk.c still lists running out of frag
space as the only failure reason:

 * Return: main buffer with attached frag on success, %NULL on error (no space
 * for a new frag).

With these new checks, it also returns NULL when the frag is the head, or
when it is already on pool->xskb_list or pool->free_list. Should that
comment be updated too? The text is unchanged at the end of the series.

>  
>  	if (!__xdp_buff_add_frag(head, virt_to_netmem(data),
>  				 offset_in_page(data), xdp->data_end - data,
>  				 xdp->frame_sz, false))
>  		return false;

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261102124950.958345-1-maciej.fijalkowski%40intel.com

Re: [PATCH net 2/9] i40e: xsk: fix xsk_buff_add_frag() error handling in Rx

[netdev-bot+sashiko]

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

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

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

- [High] In i40e_clean_rx_irq_zc()
  (drivers/net/ethernet/intel/i40e/i40e_xsk.c), the new error path for a…
- [Medium] When xsk_buff_add_frag() fails on a non-EOP descriptor, the
  new drop path in i40e_clean_rx_irq_zc() frees the frame, sets `first =…

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but it is a high-
  severity issue in the same ntc/ntp bookkeeping.
- [Low] Claim: the struct i40e_ring comment 'next_to_clean is updated
  only on processing EOP descriptor' (i40e_txrx.h:354-356) is now stale,…

--

i40e: xsk: fix xsk_buff_add_frag() error handling in Rx

When xsk_buff_add_frag() fails, i40e_clean_rx_irq_zc() used to free the
frame but stay on the current descriptor, which could lead to retrying a
broken frame or stalling the queue. This patch frees the current buffer
along with the frame, advances next_to_clean past the failing descriptor
and treats the remaining buffers as a new frame, as ice does.

> diff --git a/drivers/net/ethernet/intel/i40e/i40e_xsk.c b/drivers/net/ethernet/intel/i40e/i40e_xsk.c
> index 51a05ce4c7ce8..e6ae4c7a5cb39 100644
> --- a/drivers/net/ethernet/intel/i40e/i40e_xsk.c
> +++ b/drivers/net/ethernet/intel/i40e/i40e_xsk.c
> @@ -469,16 +469,24 @@ int i40e_clean_rx_irq_zc(struct i40e_ring *rx_ring, int budget)
>  		xsk_buff_set_size(bi, size);
>  		xsk_buff_dma_sync_for_cpu(bi);
>  
> -		if (!first)
> +		if (++next_to_process == count)
> +			next_to_process = 0;
> +
> +		if (!first) {
>  			first = bi;
> -		else if (!xsk_buff_add_frag(first, bi)) {
> +		} else if (!xsk_buff_add_frag(first, bi)) {
> +			/* Drop what has been gathered so far. xp_free() skips
> +			 * @bi if it duplicates a buffer that is already on a
> +			 * pool list. Remaining buffers of this packet, if any,
> +			 * will be processed as a new packet.
> +			 */
>  			xsk_buff_free(first);

[Severity: High]
Does this drop the whole frame when the duplicate is the head itself
(bi == first) and frags are already attached?

Earlier in the loop, xsk_buff_set_size(bi, size) runs, and at this commit
it does:

include/net/xdp_sock_drv.h:xsk_buff_set_size() {
    ...
	xdp->flags = 0;
}

When bi == first, this clears XDP_FLAGS_HAS_FRAGS on the head before
xsk_buff_add_frag(first, first) returns false. xsk_buff_free(first) then
goes down the fast path:

	if (likely(!xdp_buff_has_frags(xdp)))
		goto out;

So pool->xskb_list is never drained, and only the head goes to xp_free().

With an aligned-mode fill ring such as A, B, A, wouldn't B stay on
pool->xskb_list? xsk_rcv_zc() walks the whole list:

	list_for_each_entry_safe(pos, tmp, xskb_list, list_node) {

That would emit the leftover frags with the next unrelated frame. It would
also write more descriptors than the nr_frags + 1 that were reserved.

This looks to be fixed later in the series by "xsk: do not clear frags
flag when acquiring a buffer". That patch changes xsk_buff_set_size() and
xp_alloc() to do xdp->flags &= XDP_FLAGS_HAS_FRAGS, and makes
xsk_buff_free() clear the flag. At this commit, though, the new comment's
claim that the gathered frame is dropped does not hold for this case.
Would it make sense to put that patch before this one, so bisection does
not land on this state?

> -			break;
> +			xsk_buff_free(bi);
> +			next_to_clean = next_to_process;

[Severity: Low]
This isn't a bug introduced by this patch, but the struct i40e_ring
comment in i40e_txrx.h says:

	/* Next descriptor to be processed; next_to_clean is updated only on
	 * processing EOP descriptor
	 */

This assignment now moves next_to_clean forward on a descriptor that may
not be EOP. The comment was already inaccurate before this patch, because
the non-ZC i40e_clean_rx_irq() also moves next_to_clean forward on
programming-status descriptors. Should the comment be updated?

[Severity: High]
This is a pre-existing issue and was not introduced by this patch. It is
in the same next_to_clean/next_to_process bookkeeping. The
programming-status branch earlier in i40e_clean_rx_irq_zc() frees the
buffer but only advances next_to_process:

		if (i40e_rx_is_programming_status(qword)) {
			...
			bi = *i40e_rx_bi(rx_ring, next_to_process);
			xsk_buff_free(bi);
			if (++next_to_process == count)
				next_to_process = 0;
			continue;
		}

Suppose no frame is in progress, and the loop then exits through the
!size break. The ring is left with next_to_clean == X and
next_to_process == X + 1. On the next poll, the entry code treats this as
a partial frame:

	if (next_to_process != next_to_clean)
		first = *i40e_rx_bi(rx_ring, next_to_clean);

Can this make the freed programming-status buffer the head of the next
frame?

In the meantime, I40E_DESC_UNUSED() is based on next_to_clean, so slot X
is not refilled. The freed xskb sits on pool->free_list, though, and the
refill at the end of the poll can post it to hardware in another slot
through xp_alloc_batch()->xp_alloc_reused().

On XDP_REDIRECT, that UMEM address would go to user space while hardware
can still DMA into it. On XDP_DROP, it would be freed again while hardware
still owns it.

The non-ZC i40e_clean_rx_irq() handles this case:

			if (rx_ring->next_to_clean == ntp) {
				rx_ring->next_to_clean =
					rx_ring->next_to_process;
				cleaned_count++;
			}

Should the ZC path do the same?

> +			first = NULL;
> +			continue;
>  		}

[Severity: Medium]
What happens to the rest of the hardware frame when the failing
descriptor is not EOP?

After first = NULL and continue, the next descriptor of the same frame
becomes the head of a new packet. i40e only marks the last descriptor
(EOF), so this new packet starts in the middle of the original payload.

The hardware limits a frame to I40E_MAX_CHAINED_RX_BUFFERS, so a full
frags array cannot happen. The remaining trigger is a duplicate fill ring
address in a non-final slot, for example A, A, C with jumbo frames.

At the original frame's EOP, that descriptor goes to
i40e_handle_xdp_result_zc(). On XDP_PASS, that calls:

	i40e_process_skb_fields(rx_ring, rx_desc, skb);

This applies the original frame's ptype, checksum verdict, RSS hash and
VLAN to bytes the hardware never parsed as headers. On XDP_TX or
XDP_REDIRECT, a tail fragment would be sent out, or delivered to the
AF_XDP socket as a complete packet.

Documentation/networking/af_xdp.rst says:

  AF_XDP guarantees that only a complete packet (all frames in the
  packet) is sent to the application.

The non-ZC path in i40e_clean_rx_irq() keeps the hardware frame boundary:

		} else if (i40e_add_xdp_frag(xdp, &nfrags, rx_buffer, size) &&
			   !neop) {
			/* Overflowing packet: Drop all frags on EOP */

The commit message says this matches ice, and ice does behave the same
way. Even so, could the ZC drop path consume descriptors up to EOP before
starting a new frame?

>  
> -		if (++next_to_process == count)
> -			next_to_process = 0;
> -
>  		if (i40e_is_non_eop(rx_ring, rx_desc))
>  			continue;

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261102124950.958345-1-maciej.fijalkowski%40intel.com

Re: [PATCH net 3/9] ice: xsk: fix leaking Rx buffers in multi-buffer path

[netdev-bot+sashiko]

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

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

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

- [High] The new zero-length branch in ice_clean_rx_irq_zc()
  (drivers/net/ethernet/intel/ice/ice_xsk.c) is `else if…
- [High] ice: xsk: 0-length buffer assigned to first in
  ice_clean_rx_irq_zc()
- [Medium] As of this commit, if xsk_buff_add_frag() fails in
  ice_clean_rx_irq_zc() because `xdp == first` (aligned-mode duplicate…

Pre-existing issues:
- [Low] When xsk_buff_add_frag() fails on a non-EOP descriptor in
  ice_clean_rx_irq_zc(), `first` is set to NULL and the loop continues.

--

ice: xsk: fix leaking Rx buffers in multi-buffer path

This commit changes ice_clean_rx_irq_zc() to free the xsk buffer of a
zero-length frag. When xsk_buff_add_frag() fails, it now also frees the
current descriptor's buffer in addition to the frame head. It relies on
xp_free() skipping a duplicate buffer that was already freed with the
frame.

> diff --git a/drivers/net/ethernet/intel/ice/ice_xsk.c b/drivers/net/ethernet/intel/ice/ice_xsk.c
> index 0643017541c35..ea8ac0fcdf6ef 100644
> --- a/drivers/net/ethernet/intel/ice/ice_xsk.c
> +++ b/drivers/net/ethernet/intel/ice/ice_xsk.c
> @@ -618,8 +618,11 @@ int ice_clean_rx_irq_zc(struct ice_rx_ring *rx_ring,
>  
>  		if (!first) {
>  			first = xdp;
> -		} else if (likely(size) && !xsk_buff_add_frag(first, xdp)) {
> +		} else if (unlikely(!size)) {
> +			xsk_buff_free(xdp);

[Severity: High]
Does the zero-length check need to come before the !first check?

With the current ordering, a descriptor with pkt_len 0 is only freed
when a frame is already in progress. If first is NULL, the zero-length
buffer becomes the head of a new frame. If that descriptor is also EOP,
the unlikely(!first) test further down no longer skips it.

One way to get there is the xsk_buff_add_frag() failure path just
below. When xsk_buff_add_frag() fails on a non-EOP descriptor, first is
reset to NULL and the loop moves on. Suppose the next descriptor is the
EOP of the same packet and carries no data. That can happen when the
hardware strips the FCS and the last buffer held only FCS bytes. The
empty buffer is then assigned to first with data_end == data, and
ice_run_xdp_zc() runs on an empty frame.

What happens next depends on the verdict:

- XDP_TX: ice_xmit_xdp_tx_zc() would post a Tx descriptor with a zero
  buffer size and the EOP bit set. E810 may treat that as a malicious
  driver event and stop the queue.

- XDP_PASS: xdp_build_skb_from_zc() builds an skb with len 0, so
  eth_type_trans() has no header to work with. When
  ICE_RX_FLAGS_MULTIDEV is set, ice_process_skb_fields() may also
  __skb_push() ETH_HLEN, which moves skb->data in front of the frame.

- XDP_REDIRECT: the user socket gets a spurious empty descriptor.

libeth_xsk_process_buff() checks the length before anything else:

    if (unlikely(!len)) {
        libeth_xsk_buff_free_slow(xdp);
        return head;
    }

This ordering predates this patch. Since this hunk already adds a
dedicated zero-length branch, would it make sense to test size before
first here?

Then a zero-length buffer would be freed when no frame is in progress,
and first would stay NULL so the descriptor is skipped. There is also
the duplicate fill ring case where xdp aliases first. Given that, the
zero-length branch would presumably still need to avoid freeing the
buffer when it is the current head.

[Severity: High]
Can this free the live head of the frame when xdp == first?

In aligned mode, a duplicate fill ring address maps to the same
pool->heads[] xskb. xsk_buff_add_frag() guards against this explicitly:

    /* Duplicate fill ring entries map to the same xskb in aligned mode */
    if (unlikely(xdp == head || !list_empty(&frag->list_node)))
        return false;

The new zero-size branch has no such check. Suppose the descriptor after
the head comes back with pkt_len 0 and aliases first. Then
xsk_buff_set_size() resets the head's flags, and xsk_buff_free() goes
straight to xp_free(). The head is not on any list, so this:

    void xp_free(struct xdp_buff_xsk *xskb)
    {
        if (!list_empty(&xskb->list_node))
            return;

        xskb->pool->free_list_cnt++;
        list_add(&xskb->list_node, &xskb->pool->free_list);
    }

puts it on pool->free_list while first still points at it.

The frame then either goes on to ice_run_xdp_zc() (TX, REDIRECT or PASS)
or is stashed in rx_ring->xsk. In the same poll,
ice_alloc_rx_bufs_zc()->xp_alloc_batch()->xp_alloc_reused() can take the
buffer off free_list with list_del_init() and post it to the hardware Rx
ring again.

Could the NIC then DMA a later packet into a buffer that is still in use?
That buffer could be the head of the frame being built, sit on the XDP_TX
ring, or be on the user Rx ring.

Also, list_del_init() leaves list_node empty again. A later
xsk_buff_free(first) or XDP_TX completion would then put the buffer back
on free_list while it is still posted to hardware.

The commit message says "xp_free() skips a duplicate already freed with
the frame". Here, though, the duplicate is the live head and has not been
freed yet. Before this patch, the zero-length case did not touch the
buffer.

Would skipping the free when xdp == first avoid this? That would match the
guard in xsk_buff_add_frag(). For comparison, i40e_clean_rx_irq_zc() in
this series does not free a zero-length buffer by itself.

> +		} else if (!xsk_buff_add_frag(first, xdp)) {
>  			xsk_buff_free(first);
> +			xsk_buff_free(xdp);

[Severity: Medium]
Suppose xsk_buff_add_frag() fails because xdp == first, and the head
already has frags attached. Are those frags freed here?

xsk_buff_set_size() has just run on the aliased head:

    xdp->flags = 0;

This clears XDP_FLAGS_HAS_FRAGS, so xsk_buff_free(first) takes the early
exit:

    if (likely(!xdp_buff_has_frags(xdp)))
        goto out;

It then only calls xp_free() on the head. The earlier frags seem to stay
on pool->xskb_list with no owner. They are either lost, or xsk_rcv_zc()
later picks them up as part of another frame. The new zero-size branch
above has the same flag clearing when xdp == first.

This looks to be resolved later in the series by "xsk: do not clear frags
flag when acquiring a buffer". That patch changes xsk_buff_set_size() to
preserve XDP_FLAGS_HAS_FRAGS, and makes xsk_buff_free() clear the flag
after draining.

At this commit, though, the duplicate fill ring case named in the commit
message still strands frag buffers. The stranding also happened before
this patch.

>  			first = NULL;
>  		}

[Severity: Low]
This isn't a bug introduced by this patch, but there is a related
behavior. When xsk_buff_add_frag() fails on a non-EOP descriptor, first is
set to NULL and the loop continues. The next descriptor of the dropped
packet then starts a new frame through:

    if (!first) {
        first = xdp;

So the rest of the dropped frame is assembled and run through XDP as a
standalone packet that starts mid-frame.

The i40e companion patch in this series says this is intended ("Remaining
buffers of this packet, if any, will be processed as a new packet.").

Is that also the intended behavior for ice, rather than discarding
descriptors until EOP?

>

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261102124950.958345-1-maciej.fijalkowski%40intel.com

Re: [PATCH net 5/9] libeth: xsk: do not free in-flight frags on XDP_TX completion

[netdev-bot+sashiko]

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

Critical: 0 · High: 1 (1) · Medium: 1 (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).

- [High] The fix is incomplete: when the flush inside the frag loop
  fails, XDP_FLAGS_HAS_FRAGS is never cleared.
- [Medium] With a shared XDPSQ, the late flag clear still leaves a window
  in which the current frame's own head can be completed while its frags…

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but it sits on the
  same error path the patch changes.
- [Medium] This problem wasn't introduced by this patch, but
  libeth_xdp_tx_xmit_bulk() (include/net/libeth/xdp.h) trims the number…

--

libeth: xsk: do not free in-flight frags on XDP_TX completion

This patch clears XDP_FLAGS_HAS_FRAGS on the head of a multi-buffer XSk
XDP_TX frame in libeth_xsk_tx_queue_bulk(), once all of the frame's frags
are queued. Completing the head then no longer frees whatever is on
pool->xskb_list at that moment, such as frags of a frame still being
queued or of a stashed partial Rx frame.

> diff --git a/include/net/libeth/xsk.h b/include/net/libeth/xsk.h
> index a452b7828ce4e..249b1c7407881 100644
> --- a/include/net/libeth/xsk.h
> +++ b/include/net/libeth/xsk.h
> @@ -77,11 +77,17 @@ libeth_xsk_tx_queue_bulk(struct libeth_xdp_tx_bulk *bq,
>  	if (!libeth_xsk_tx_queue_head(bq, xdp))
>  		goto out;
>  
> -	for (const struct libeth_xdp_buff *head = xdp; ; ) {
> +	for (struct libeth_xdp_buff *head = xdp; ; ) {
>  		xdp = container_of(xsk_buff_get_frag(&head->base),
>  				   typeof(*xdp), base);
> -		if (!xdp)
> +		if (!xdp) {
> +			/* Each frag is now freed via its own SQE. Freeing
> +			 * the head must not release what is on the pool's
> +			 * frag list at that time.
> +			 */
> +			xdp_buff_clear_frags_flag(&head->base);

[Severity: Medium]
With a shared XDPSQ, is the flag cleared too late here, leaving a window
open?

If the frame doesn't fit in the remaining bulk space, the in-loop
flush_bulk() writes the head into the SQ through libeth_xsk_tx_fill_buf()
(sqe->xsk = xdp; sqe->type = LIBETH_SQE_XSK_TX). It then drops the xdpsq
lock in libeth_xdp_tx_xmit_bulk() while HAS_FRAGS is still set.

After that, this CPU keeps calling xsk_buff_get_frag() without a lock, and
it clears the flag only once the list is empty.

libeth_xdpsq_id() can map several CPUs to one XDPSQ (ret %= num). In that
case the other sharer could do this:

CPU A                                  CPU B (same XDPSQ)
libeth_xsk_tx_queue_bulk()
  flush_bulk() publishes head
  xsk_buff_get_frag()                  idpf_xdp_tx_finalize()
                                         set RS, bump tail
                                       idpf_xsk_tx_prep()
                                         idpf_xsksq_complete()
                                           xsk_buff_free(head)
                                             walks pool->xskb_list
  xsk_buff_get_frag()
  xdp_buff_clear_frags_flag()

The cleanup timer could run the same completion in place of CPU B.

Could this corrupt pool->xskb_list, or free frags that CPU A then queues?

It needs the HW to complete the head before CPU A has posted the frame's
EOP descriptor, so it may be hard to hit. Still, it matches the "XDPSQ can
be completed by a bulk flush while the frags of a frame are being queued"
case in the commit message.

Would clearing the flag before the head can be published close this
window? One option is to clear it when the head is queued with
LIBETH_XDP_TX_MULTI.

>  			break;
> +		}
>  
>  		if (unlikely(bq->count == LIBETH_XDP_TX_BULK) &&
>  		    unlikely(!flush_bulk(bq, LIBETH_XDP_TX_XSK))) {

[Severity: High]
Is the flag still left set when this in-loop flush fails? The new
xdp_buff_clear_frags_flag() runs only when xsk_buff_get_frag() returns
NULL, and the failure exit skips it:

	if (unlikely(bq->count == LIBETH_XDP_TX_BULK) &&
	    unlikely(!flush_bulk(bq, LIBETH_XDP_TX_XSK))) {
		ret = false;
		break;
	}

flush_bulk() returns false here when prep() reports no free descriptors
in libeth_xdp_tx_xmit_bulk():

	n = min(n, prep(xdpsq, &sq));
	if (unlikely(!n))
		goto unlock;

This flush has no LIBETH_XDP_TX_DROP, so libeth_xdp_tx_exception() keeps
the unsent entries in bq->bulk. The head therefore stays queued with
XDP_FLAGS_HAS_FRAGS set.

The frame's remaining frags stay on pool->xskb_list, and the out: label
sets LIBETH_XDP_TX_LAST on a middle frag.

At the end of the poll, __libeth_xdp_finalize_rx() calls
flush_bulk(bq, flags | LIBETH_XDP_TX_DROP). One of two things follows:

(a) The head is sent as LIBETH_SQE_XSK_TX. When it completes later (next
prep or the cleanup timer), xsk_buff_free() sees HAS_FRAGS and frees
everything on pool->xskb_list at that point. That can include the frags
of a partial Rx frame stashed in rxq->xsk. The frame is also sent
truncated, with EOP on a middle frag.

(b) The send fails and libeth_xsk_tx_return_bulk() frees the head right
away. In idpf_xskrq_poll(), idpf_xsk_finalize_rx() runs before
rxq->xsk = xdp. The frags of the frame still being assembled are on
pool->xskb_list and get freed while that frame still references them.

Isn't this the same "frags are then freed while in use ... while a
partial Rx frame is stashed" case from the commit message, still
reachable whenever the XDPSQ is full?

The orphaned frags left on pool->xskb_list would also be picked up by the
next multi-buffer frame. On XDP_TX, xsk_buff_get_frag() pops them first.
On redirect, xsk_rcv_zc() walks the list. Either way, that frame would
carry another frame's data.

Later patches in the series don't seem to touch this function.

Does this error path also need to clear the flag and free or return the
frags of this frame that were not queued?

[Severity: High]
This is a pre-existing issue, but it sits on the same error path. Each
iteration first pops a frag with xsk_buff_get_frag(), which does
list_del_init(&frag->list_node). Only then does it check whether the bulk
needs flushing.

If that flush fails, the code runs ret = false; break;. The frag that was
just popped is never passed to libeth_xsk_tx_queue_frag(), never freed,
and never put back on pool->xskb_list.

xsk_buff_free() on the head walks only pool->xskb_list, so nothing
reclaims this frag.

Does this leak one UMEM buffer each time the XDPSQ is full mid-frame?

Under sustained TX congestion with XDP_TX, the leaks would add up. That
could exhaust the buffers of the AF_XDP queue and stall Rx. A fix for the
flag handling above would probably need to cover this frag as well.

[Severity: Medium]
This isn't a bug introduced by this patch, but there is a related one in
libeth_xdp_tx_xmit_bulk(). It cuts the number of descriptors written to
the free count without rounding down to a frame boundary:

	n = min(n, prep(xdpsq, &sq));
	if (unlikely(!n))
		goto unlock;

libeth_xsk_tx_fill_buf() writes one descriptor per bulk entry, and
idpf_xdp_tx_xmit() sets EOP only on the LIBETH_XDP_TX_LAST entry:

	if (desc.flags & LIBETH_XDP_TX_LAST)
		cmd |= FIELD_PREP(IDPF_FLEX_TXD_QW1_CMD_M,
				  IDPF_TX_DESC_CMD_EOP);

Suppose the cut falls inside a multi-buffer frame during the DROP flush
in __libeth_xdp_finalize_rx(). The head and earlier frags are then left
on the ring without EOP.

libeth_xdp_tx_exception() frees the rest of the frame, including the LAST
frag, through libeth_xsk_tx_return_bulk(). idpf_xdp_tx_finalize() then
sets RS and bumps the tail.

Can this hand the device an unterminated descriptor chain, with the next
frame's descriptors appended to it?

Without DROP, the frame is split across releases of the xdpsq lock. With
shared XDPSQs, another CPU could insert its own descriptors or bump the
tail in between.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261102124950.958345-1-maciej.fijalkowski%40intel.com

Re: [PATCH net 6/9] xsk: do not clear frags flag when acquiring a buffer

[netdev-bot+sashiko]

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

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

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

- [High] The commit message says "The driver XDP_TX paths ... already
  clear it".
- [High] libeth: xsk: uncleared frags flag and frag leak in
  libeth_xsk_tx_queue_bulk()
- [Medium] xp_alloc() (xsk_buff_pool.c:632) and xsk_buff_set_size()
  (xdp_sock_drv.h:229) now keep XDP_FLAGS_HAS_FRAGS when they hand out a…
- [Low] The i40e hunk moves rx_bytes = xdp_get_buff_len(first) to before
  i40e_run_xdp_zc(), and i40e_handle_xdp_result_zc() no longer…

Pre-existing issues:
- [Critical] This problem wasn't introduced by this patch, but AF_XDP
  zero-copy multi-buffer Rx keeps its authoritative frag metadata…
- [High] At this commit, the frag loop in i40e_construct_skb_zc() does
  `memcpy(addr, skb_frag_page(frag), skb_frag_size(frag))` (line 335).

--

xsk: do not clear frags flag when acquiring a buffer

This patch keeps XDP_FLAGS_HAS_FRAGS when xp_alloc() and
xsk_buff_set_size() hand out a buffer. The flag is now cleared when the
frags are released, in xsk_buff_free() and xsk_rcv_zc(). It also moves
the i40e ZC rx_bytes accounting to before the XDP program runs.

> diff --git a/drivers/net/ethernet/intel/i40e/i40e_xsk.c b/drivers/net/ethernet/intel/i40e/i40e_xsk.c
> index e6ae4c7a5cb39..57b0f520fbb29 100644
> --- a/drivers/net/ethernet/intel/i40e/i40e_xsk.c
> +++ b/drivers/net/ethernet/intel/i40e/i40e_xsk.c
> @@ -354,7 +354,6 @@ static void i40e_handle_xdp_result_zc(struct i40e_ring *rx_ring,
>  	struct sk_buff *skb;
>  
>  	*rx_packets = 1;
> -	*rx_bytes = xdp_get_buff_len(xdp_buff);
>  
>  	if (likely(xdp_res == I40E_XDP_REDIR) || xdp_res == I40E_XDP_TX)
>  		return;
> @@ -492,6 +491,7 @@ int i40e_clean_rx_irq_zc(struct i40e_ring *rx_ring, int budget)
>  
>  		xsk_buff_to_i40e_ctx(first)->desc = rx_desc;
>  
> +		rx_bytes = xdp_get_buff_len(first);
>  		xdp_res = i40e_run_xdp_zc(rx_ring, first, xdp_prog);
>  		i40e_handle_xdp_result_zc(rx_ring, first, rx_desc, &rx_packets,
>  					  &rx_bytes, xdp_res, &failure);

[Severity: Low]

rx_bytes is now sampled before i40e_run_xdp_zc(), and
i40e_handle_xdp_result_zc() no longer recomputes it. Does XDP_PASS now
report the pre-program length instead of the length of the skb built by
i40e_construct_skb_zc()?

If the program calls bpf_xdp_adjust_head(), bpf_xdp_adjust_tail() or
bpf_xdp_pull_data() and then returns XDP_PASS, the counter would no
longer match what is passed up the stack.

The commit message says this is "like ice and libeth".
ice_clean_rx_irq_zc() uses the pre-program length only for
TX/REDIR/CONSUMED. For XDP_PASS it uses:

	total_rx_bytes += skb->len;

The i40e non-ZC path also uses skb->len for XDP_PASS. libeth does count
pre-program bytes for XDP_PASS.

The move is needed for TX and REDIR. On XDP_PASS the driver still owns
the buffer, so could that case keep using the post-program length? If
not, could the commit message mention the change?

[Severity: High]

This is a pre-existing issue and was not introduced by this patch. The
XDP_PASS path above ends up in i40e_construct_skb_zc(), and its frag
loop looks wrong at this commit:

	memcpy(addr, skb_frag_page(frag), skb_frag_size(frag));
	...
	__skb_fill_page_desc_noacc(skinfo, skinfo->nr_frags++,
				   addr, 0, skb_frag_size(frag));

Doesn't the memcpy() copy from the struct page itself rather than from
the frag data? It also ignores the frag offset.

Doesn't __skb_fill_page_desc_noacc() get a virtual address where a
struct page pointer is expected? skb->len, data_len and truesize are
not updated either.

The dev_alloc_page() failure path also returns NULL without calling
xsk_buff_free(). That leaks the head, which keeps its flag while its
frags stay on pool->xskb_list.

The next patch in the series, "i40e: xsk: fix multi-buffer XDP_PASS skb
construction", seems to fix all of this. It switches to
skb_frag_address() and skb_add_rx_frag(), and sends the allocation
failure through xsk_buff_free().

> diff --git a/include/net/xdp_sock_drv.h b/include/net/xdp_sock_drv.h
> index 29965e3695983..a40ad3f90f13c 100644
> --- a/include/net/xdp_sock_drv.h
> +++ b/include/net/xdp_sock_drv.h
> @@ -153,6 +153,7 @@ static inline void xsk_buff_free(struct xdp_buff *xdp)
>  	}
>  
>  	xdp_get_shared_info_from_buff(xdp)->nr_frags = 0;
> +	xdp_buff_clear_frags_flag(xdp);
>  out:
>  	xp_free(xskb);
>  }

[Severity: High]

The commit message says:

  "The driver XDP_TX paths, bpf_xdp_adjust_tail() and bpf_xdp_pull_data()
  already clear it"

Does that hold for libeth_xsk_tx_queue_bulk() when a flush fails partway
through a frame? The flag is only cleared once xsk_buff_get_frag()
returns NULL:

include/net/libeth/xsk.h:libeth_xsk_tx_queue_bulk() {
    ...
		if (!xdp) {
			...
			xdp_buff_clear_frags_flag(&head->base);
			break;
		}

		if (unlikely(bq->count == LIBETH_XDP_TX_BULK) &&
		    unlikely(!flush_bulk(bq, LIBETH_XDP_TX_XSK))) {
			ret = false;
			break;
		}
    ...
}

Suppose the bulk fills up mid-frame and flush_bulk() fails, for example
because the XDPSQ has no free descriptors and prep() returns 0. Then:

  - the frag just returned by xsk_buff_get_frag() is neither queued nor
    put back on pool->xskb_list. Is that UMEM buffer leaked?
  - the rest of the frame's frags stay on pool->xskb_list
  - libeth_xdp_tx_exception() without LIBETH_XDP_TX_DROP keeps the head
    and the frags already queued in the bulk (bq->count = left)
  - the head still has XDP_FLAGS_HAS_FRAGS set

__libeth_xdp_run_flush() then only changes the verdict and frees
nothing:

	if (act == LIBETH_XDP_TX && unlikely(!queue(bq, xdp, flush_bulk)))
		act = LIBETH_XDP_DROP;

The Rx loop keeps going with stale frags on the shared list, and the
next multi-buffer frame appends its frags behind them.

For XDP_REDIRECT, xsk_rcv_zc() checks for room using the head's frag
count:

	num_desc = xdp_get_shared_info_from_buff(xdp)->nr_frags + 1;

It then writes one descriptor per entry on pool->xskb_list.
__xskq_prod_reserve_desc() has no bounds check. Can this overwrite Rx
descriptors that user space has not consumed yet? For XDP_TX, the stale
frags would go out inside the next frame.

The stale head is eventually freed, either by a DROP flush or on Tx
completion via libeth_xsk_buff_free_slow(). At that point
xsk_buff_free() still sees HAS_FRAGS and drains pool->xskb_list:

	list_for_each_entry_safe(pos, tmp, xskb_list, list_node) {
		list_del_init(&pos->list_node);
		xp_free(pos);
	}

Could that release in-flight frags of a partial frame stashed in
rxq->xsk? This is the outcome described here ("leaves its frags on
pool->xskb_list, where they are taken as frags of the next frame").
It is also what the earlier series patch "libeth: xsk: do not free
in-flight frags on XDP_TX completion" aims to prevent. No later patch
in the series appears to touch include/net/libeth/xsk.h.

> @@ -225,7 +226,8 @@ static inline void xsk_buff_set_size(struct xdp_buff *xdp, u32 size)
>  	xdp->data = xdp->data_hard_start + XDP_PACKET_HEADROOM;
>  	xdp->data_meta = xdp->data;
>  	xdp->data_end = xdp->data + size;
> -	xdp->flags = 0;
> +	/* Frags flag is cleared when the frags of a head are released */
> +	xdp->flags &= XDP_FLAGS_HAS_FRAGS;
>  }

[Severity: High]
The commit message relies on the driver XDP_TX paths clearing
XDP_FLAGS_HAS_FRAGS once the frags have been handed off. However,
libeth_xsk_tx_queue_bulk() only clears it after xsk_buff_get_frag()
returns NULL:

	xdp = container_of(xsk_buff_get_frag(&head->base),
			   typeof(*xdp), base);
	if (!xdp) {
		xdp_buff_clear_frags_flag(&head->base);
		break;
	}

	if (unlikely(bq->count == LIBETH_XDP_TX_BULK) &&
	    unlikely(!flush_bulk(bq, LIBETH_XDP_TX_XSK))) {
		ret = false;
		break;
	}

xp_alloc() and xsk_buff_set_size() no longer reset the flag. So
nothing else clears it for a head that leaves this loop early.

Suppose the bulk fills up mid-frame and flush_bulk() fails:

  - the frag just unlinked by xsk_buff_get_frag() is neither queued
    nor freed. Isn't that UMEM buffer leaked?
  - the remaining frags stay on pool->xskb_list, and the next
    multi-buffer Rx frame picks them up
  - the head stays in the bulk with HAS_FRAGS set

When that head is later freed, through libeth_xsk_tx_return_bulk() or
on Tx completion, xsk_buff_free() drains whatever is on
pool->xskb_list at that point.

The successful case looks risky too. When a mid-frame flush_bulk()
succeeds, the head has already been written to sq->sqes[] as
LIBETH_SQE_XSK_TX. HAS_FRAGS is still set on it, and its remaining
frags are still on pool->xskb_list.

What happens if that SQE completes before the loop finishes? That
could come from the cleanup done by a later flush_bulk() for the same
frame. It could also come from libeth_xdpsq_run_timer() on another
CPU, which does not serialise against pool->xskb_list.

In that case, doesn't xsk_buff_free(&head->base) free the frags this
loop is about to pop? The rest of the frame would then be silently
dropped. In the timer case, the list itself would also be modified
concurrently.

Each frag gets its own SQE. Could the flag be cleared as soon as the
head is queued, before any flush can run? The flush failure path
would then also need to put the popped frag back, or free it together
with the rest of the frame. Does that match your intent here, or is
there something that keeps the head from completing mid-loop?

[Severity: Medium]

xsk_buff_set_size() and xp_alloc() now keep XDP_FLAGS_HAS_FRAGS, so a
new head no longer starts with clean frag state. Before, flags = 0
meant the first __xdp_buff_add_frag() always took the
!xdp_buff_has_frags() branch. That branch resets nr_frags,
xdp_frags_size and xdp_frags_truesize.

If a head is handed out again while still flagged, __xdp_buff_add_frag()
trusts the skb_shared_info at the tail of the UMEM chunk:

	nr_frags = sinfo->nr_frags;

The only check before indexing frags[] is nr_frags == MAX_SKB_FRAGS. A
user-written value such as 200 would let __skb_fill_netmem_desc_noacc()
write about 3 KB past sinfo. xdp_copy_frags_from_zc() and
i40e_construct_skb_zc() also loop over that nr_frags.

Later patches in the series fix the ice, i40e and idpf leak paths:

  "ice: xsk: free stashed Rx frame when cleaning the ring"
  "i40e: xsk: fix multi-buffer XDP_PASS skb construction"
  "idpf: free stashed XSk Rx frame when releasing the queue"

The libeth_xsk_tx_queue_bulk() mid-frame flush failure above still
seems to be open. Head H stays in the Tx bulk with HAS_FRAGS until the
NAPI poll ends. If user space posted H's address twice in the fill ring,
H can come back as the first buffer of a later frame in the same poll.
Its stale sinfo would then be used, with no race needed.

Would it make sense to re-initialise nr_frags, xdp_frags_size and
xdp_frags_truesize whenever a buffer becomes a new head, as a safety
net?

[Severity: Critical]

This isn't a bug introduced by this patch, but the patch now relies on
frag state surviving hand-out, and that metadata lives in user-writable
memory. xp_init_xskb_addr() places the buffer inside the UMEM mapping:

	xskb->xdp.data_hard_start = pool->addrs + addr + pool->headroom;

So xdp_get_shared_info_from_buff() points at the tail of a chunk that
user space keeps mapped writable. Once the first frag sets HAS_FRAGS, a
thread in the process that owns the UMEM can overwrite sinfo->nr_frags
before the next xsk_buff_add_frag().

__xdp_buff_add_frag() only rejects nr_frags == MAX_SKB_FRAGS (XSk passes
try_coalesce=false). __skb_fill_netmem_desc_noacc() then indexes
shinfo->frags[i] with no bounds check.

nr_frags is a u8. Can the write land up to about 3.8 KB past the chunk,
or past the end of the vmap area for the last chunk?

On XDP_PASS, xdp_copy_frags_from_zc() then does:

	nr_frags = xinfo->nr_frags;

	for (u32 i = 0; i < nr_frags; i++) {

This writes skb_shinfo(skb)->frags[i] beyond MAX_SKB_FRAGS. It also
memcpy()s from frag descriptors read out of UMEM, and
i40e_construct_skb_zc() has the same loop. Can this overflow the head
of the new skb, or copy arbitrary kernel memory into packet data?

Keeping the kernel-side HAS_FRAGS flag does not make the nr_frags stored
in UMEM trustworthy. This appears to date back to commit 24ea50127ecf.

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261102124950.958345-1-maciej.fijalkowski%40intel.com

Re: [PATCH net 9/9] idpf: free stashed XSk Rx frame when releasing the queue

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] During a live XSk pool disable (idpf_xsk_pool_setup →
  idpf_qp_switch(false) → idpf_qp_disable → idpf_clean_queue_set), the…

Pre-existing issues:
- [High] The patch now returns the stashed partial frame (rxq->xsk) to
  the XSk pool on queue release.

--

idpf: free stashed XSk Rx frame when releasing the queue

This commit frees the partial multi-buffer XSk frame stashed in rxq->xsk
and clears the pointer in idpf_xsk_clear_queue(), while the pool is still
attached. This keeps the buffers from being lost on ifdown, and keeps the
page path from reusing a stale xskb after the pool is disabled.

> diff --git a/drivers/net/ethernet/intel/idpf/xsk.c b/drivers/net/ethernet/intel/idpf/xsk.c
> index d95d3efdfd36f..5a0a1d8a7a866 100644
> --- a/drivers/net/ethernet/intel/idpf/xsk.c
> +++ b/drivers/net/ethernet/intel/idpf/xsk.c
> @@ -134,6 +134,11 @@ void idpf_xsk_clear_queue(void *q, enum virtchnl2_queue_type type)
>  		if (!idpf_queue_has_clear(XSK, rxq))
>  			return;
>  
> +		if (rxq->xsk) {
> +			libeth_xsk_buff_free_slow(rxq->xsk);

[Severity: Low]
Can this free race with the XSk SQ cleanup timer when the pool is disabled
on a running interface?

On the pool disable path:

idpf_xsk_pool_setup()
  idpf_qp_switch(false)
    idpf_qp_disable()
      idpf_clean_queue_set()

idpf_vector_to_queue_set() places the RX queues before the xsksq TX and
COMPLQ entries. So this code runs before the timer is cancelled. The only
synchronous cancel happens later, in idpf_tx_desc_rel():

	if (xdp)
		libeth_xdpsq_deinit_timer(txq->timer);

The timer runs as BH work on system_bh_highpri_wq and can return XSk
buffers to the same pool:

idpf_xsk_tx_timer()
  libeth_xdpsq_run_timer()
    idpf_xsksq_complete()
      idpf_xsksq_complete_slow()
        libeth_xdp_complete_tx()
          libeth_xsk_buff_free_slow()
            xp_free()

The timer holds only the xdpsq lock, and the Rx release path here does not
take it. xp_free() updates the pool without any locking:

	xskb->pool->free_list_cnt++;
	list_add(&xskb->list_node, &xskb->pool->free_list);

Suppose the timer is still inside idpf_xsksq_complete_slow() when
idpf_rx_desc_rel() gets here. Could both contexts then call list_add() on
pool->free_list at the same time?

The window looks narrow, since the SW marker wait drains the completion
queue first. Also, the pool is being destroyed or detached at that point.

Would it avoid this to free the stash after the xsksq timer is cancelled,
or after the Tx queues in the set are cleaned?

> +			rxq->xsk = NULL;
> +		}
> +
>  		rxq->pool = NULL;
>  		break;
>  	case VIRTCHNL2_QUEUE_TYPE_RX_BUFFER:

[Severity: High]
This isn't a bug introduced by this patch. However, the buffers still
posted on the buffer queue seem to have the same problem as the stash
fixed here. Are the xskbs in bufq->xsk_buf[] between next_to_clean and
next_to_use ever returned to the pool?

The RX_BUFFER case only clears bufq->pool. The XSK branch of
idpf_rx_buf_rel_bufq() returns early:

	if (idpf_queue_has(XSK, bufq)) {
		idpf_xskfq_rel(bufq);
		return;
	}

idpf_xskfq_rel() only calls libeth_xskfq_destroy(), which resets some
counters and then does:

	kvfree(fq->fqes);

xsk_buff_free() is never called on the buffers that libeth_xskfqe_alloc()
got from xsk_buff_alloc_batch().

For comparison, ice_xsk_clean_rx_ring() frees the stash and also every
posted buffer:

	if (rx_ring->xsk) {
		xsk_buff_free(&rx_ring->xsk->base);
		rx_ring->xsk = NULL;
	}

	while (ntc != ntu) { ... xsk_buff_free(xdp); ... }

With an AF_XDP zero-copy socket bound, wouldn't every ifdown/ifup or soft
reset (ring resize, MTU change and so on) leak up to desc_count umem frames
per buffer queue through this path?

idpf_vport_stop()
  idpf_vport_queues_rel()
    idpf_rx_desc_rel_all()
      idpf_rx_desc_rel_bufq()
        idpf_rx_buf_rel_bufq()
          idpf_xskfq_rel()
            libeth_xskfq_destroy()

The fill ring consumer has already moved past these frames. So they never
reach the Rx ring or pool->free_list. After a few resets the application
could run out of frames to post, and Rx on that queue would stop. In
unaligned mode the leaked heads also reduce pool->free_heads_cnt.

These are the same buffers "lost to user space" that the commit message
describes. Should they be released along with the stash, as
ice_xsk_clean_rx_ring() does?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261102124950.958345-1-maciej.fijalkowski%40intel.com

Re: [PATCH net 1/9] xsk: fix list corruption in xsk_buff_add_frag()

[Maciej Fijalkowski]

On Tue, Nov 03, 2026 at 01:32:29PM +0000, netdev-bot+sashiko@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 3 potential
> issue(s) to consider.
> 
> Critical: 0 · High: 2 · Medium: 0 · Low: 1 (1)
> 
> 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).
> 
> - [High] At the reviewed commit, the new `xdp == head` rejection in
>   xsk_buff_add_frag() (include/net/xdp_sock_drv.h:166-168) runs after…
> - [High] At the reviewed commit, the new rejection in xsk_buff_add_frag()
>   fails the same way every time it sees the same input.
> - [Low] The kernel-doc for libeth_xsk_buff_add_frag()
>   (drivers/net/ethernet/intel/libeth/xsk.c:50-51) says it returns '%NULL…
> 
> Pre-existing issues:
> - [Low] When xsk_buff_add_frag() fails, ice_clean_rx_irq_zc() frees the
>   partial frame and sets first = NULL but does not skip ahead to EOP.
> 

Note: I will be trimming away all the issues where beloved bot says 'a
later patch in the series seems to fix this'. This is really annoying :<

> --
> 
> xsk: fix list corruption in xsk_buff_add_frag()
> 
> This patch changes xsk_buff_add_frag() to reject a frag buffer that is the
> frame head or that is already linked on pool->xskb_list or
> pool->free_list. In aligned mode this happens when user space posts the
> same fill ring address twice. The goal is to prevent list corruption and
> the resulting NULL pointer dereference in xp_free().
> 
> > diff --git a/include/net/xdp_sock_drv.h b/include/net/xdp_sock_drv.h
> > index d94aeb506379c..29965e3695983 100644
> > --- a/include/net/xdp_sock_drv.h
> > +++ b/include/net/xdp_sock_drv.h
> > @@ -160,15 +160,18 @@ static inline void xsk_buff_free(struct xdp_buff *xdp)
> >  static inline bool xsk_buff_add_frag(struct xdp_buff *head,
> >  				     struct xdp_buff *xdp)
> >  {
> > +	struct xdp_buff_xsk *frag = container_of(xdp, struct xdp_buff_xsk, xdp);
> >  	const void *data = xdp->data;
> > -	struct xdp_buff_xsk *frag;
> > +
> > +	/* Duplicate fill ring entries map to the same xskb in aligned mode */
> > +	if (unlikely(xdp == head || !list_empty(&frag->list_node)))
> > +		return false;
> 
> [Severity: Low]
> This is a pre-existing issue and was not introduced by this patch, but
> this check makes the caller failure paths reachable through duplicate fill
> ring entries.
> 
> In ice_clean_rx_irq_zc(), a failed xsk_buff_add_frag() frees the partial
> frame and sets first to NULL, but the remaining descriptors up to EOP are
> not skipped:
> 
>     } else if (likely(size) && !xsk_buff_add_frag(first, xdp)) {
>         xsk_buff_free(first);
>         first = NULL;
>     }
> 
> Won't the rest of the hardware frame then be assembled and delivered as a
> new frame? idpf_xskrq_poll() does the same when libeth_xsk_process_buff()
> returns NULL.
> 
> The i40e patch later in this series calls this intended ("The rest of the
> dropped frame is received as a new frame"). It also does not appear to
> corrupt kernel memory, so it may be acceptable as is.

Hm, yeah seems like frame should be drained up to EOP...it's best to do on
driver side.

Clashiko 1:2 Maciej

as two issues were just bonkers, they are fixed later.

> 
> [Severity: Low]
> This isn't a bug, but the kernel-doc for libeth_xsk_buff_add_frag() in
> drivers/net/ethernet/intel/libeth/xsk.c still lists running out of frag
> space as the only failure reason:
> 
>  * Return: main buffer with attached frag on success, %NULL on error (no space
>  * for a new frag).
> 
> With these new checks, it also returns NULL when the frag is the head, or
> when it is already on pool->xskb_list or pool->free_list. Should that
> comment be updated too? The text is unchanged at the end of the series.
> 
> >  
> >  	if (!__xdp_buff_add_frag(head, virt_to_netmem(data),
> >  				 offset_in_page(data), xdp->data_end - data,
> >  				 xdp->frame_sz, false))
> >  		return false;
> 
> [ ... ]
> 
> -- 
> Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261102124950.958345-1-maciej.fijalkowski%40intel.com

Re: [PATCH net 2/9] i40e: xsk: fix xsk_buff_add_frag() error handling in Rx

[Maciej Fijalkowski]

On Tue, Nov 03, 2026 at 01:32:30PM +0000, netdev-bot+sashiko@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential
> issue(s) to consider.
> 
> Critical: 0 · High: 1 (1) · Medium: 1 · Low: 0 (1)
> 
> 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).
> 
> - [High] In i40e_clean_rx_irq_zc()
>   (drivers/net/ethernet/intel/i40e/i40e_xsk.c), the new error path for a…
> - [Medium] When xsk_buff_add_frag() fails on a non-EOP descriptor, the
>   new drop path in i40e_clean_rx_irq_zc() frees the frame, sets `first =…
> 
> Pre-existing issues:
> - [High] This problem wasn't introduced by this patch, but it is a high-
>   severity issue in the same ntc/ntp bookkeeping.
> - [Low] Claim: the struct i40e_ring comment 'next_to_clean is updated
>   only on processing EOP descriptor' (i40e_txrx.h:354-356) is now stale,…
> 
> --
> 
> i40e: xsk: fix xsk_buff_add_frag() error handling in Rx
> 
> When xsk_buff_add_frag() fails, i40e_clean_rx_irq_zc() used to free the
> frame but stay on the current descriptor, which could lead to retrying a
> broken frame or stalling the queue. This patch frees the current buffer
> along with the frame, advances next_to_clean past the failing descriptor
> and treats the remaining buffers as a new frame, as ice does.
> 
> > diff --git a/drivers/net/ethernet/intel/i40e/i40e_xsk.c b/drivers/net/ethernet/intel/i40e/i40e_xsk.c
> > index 51a05ce4c7ce8..e6ae4c7a5cb39 100644
> > --- a/drivers/net/ethernet/intel/i40e/i40e_xsk.c
> > +++ b/drivers/net/ethernet/intel/i40e/i40e_xsk.c
> > @@ -469,16 +469,24 @@ int i40e_clean_rx_irq_zc(struct i40e_ring *rx_ring, int budget)
> >  		xsk_buff_set_size(bi, size);
> >  		xsk_buff_dma_sync_for_cpu(bi);
> >  
> > -		if (!first)
> > +		if (++next_to_process == count)
> > +			next_to_process = 0;
> > +
> > +		if (!first) {
> >  			first = bi;
> > -		else if (!xsk_buff_add_frag(first, bi)) {
> > +		} else if (!xsk_buff_add_frag(first, bi)) {
> > +			/* Drop what has been gathered so far. xp_free() skips
> > +			 * @bi if it duplicates a buffer that is already on a
> > +			 * pool list. Remaining buffers of this packet, if any,
> > +			 * will be processed as a new packet.
> > +			 */
> >  			xsk_buff_free(first);
> > -			break;
> > +			xsk_buff_free(bi);
> > +			next_to_clean = next_to_process;
> 
> [Severity: Low]
> This isn't a bug introduced by this patch, but the struct i40e_ring
> comment in i40e_txrx.h says:
> 
> 	/* Next descriptor to be processed; next_to_clean is updated only on
> 	 * processing EOP descriptor
> 	 */
> 
> This assignment now moves next_to_clean forward on a descriptor that may
> not be EOP. The comment was already inaccurate before this patch, because
> the non-ZC i40e_clean_rx_irq() also moves next_to_clean forward on
> programming-status descriptors. Should the comment be updated?
> 
> [Severity: High]
> This is a pre-existing issue and was not introduced by this patch. It is
> in the same next_to_clean/next_to_process bookkeeping. The
> programming-status branch earlier in i40e_clean_rx_irq_zc() frees the
> buffer but only advances next_to_process:
> 
> 		if (i40e_rx_is_programming_status(qword)) {
> 			...
> 			bi = *i40e_rx_bi(rx_ring, next_to_process);
> 			xsk_buff_free(bi);
> 			if (++next_to_process == count)
> 				next_to_process = 0;
> 			continue;
> 		}
> 
> Suppose no frame is in progress, and the loop then exits through the
> !size break. The ring is left with next_to_clean == X and
> next_to_process == X + 1. On the next poll, the entry code treats this as
> a partial frame:
> 
> 	if (next_to_process != next_to_clean)
> 		first = *i40e_rx_bi(rx_ring, next_to_clean);
> 
> Can this make the freed programming-status buffer the head of the next
> frame?
> 
> In the meantime, I40E_DESC_UNUSED() is based on next_to_clean, so slot X
> is not refilled. The freed xskb sits on pool->free_list, though, and the
> refill at the end of the poll can post it to hardware in another slot
> through xp_alloc_batch()->xp_alloc_reused().
> 
> On XDP_REDIRECT, that UMEM address would go to user space while hardware
> can still DMA into it. On XDP_DROP, it would be freed again while hardware
> still owns it.
> 
> The non-ZC i40e_clean_rx_irq() handles this case:
> 
> 			if (rx_ring->next_to_clean == ntp) {
> 				rx_ring->next_to_clean =
> 					rx_ring->next_to_process;
> 				cleaned_count++;
> 			}
> 
> Should the ZC path do the same?
> 
> > +			first = NULL;
> > +			continue;
> >  		}
> 
> [Severity: Medium]
> What happens to the rest of the hardware frame when the failing
> descriptor is not EOP?
> 
> After first = NULL and continue, the next descriptor of the same frame
> becomes the head of a new packet. i40e only marks the last descriptor
> (EOF), so this new packet starts in the middle of the original payload.
> 
> The hardware limits a frame to I40E_MAX_CHAINED_RX_BUFFERS, so a full
> frags array cannot happen. The remaining trigger is a duplicate fill ring
> address in a non-final slot, for example A, A, C with jumbo frames.
> 
> At the original frame's EOP, that descriptor goes to
> i40e_handle_xdp_result_zc(). On XDP_PASS, that calls:
> 
> 	i40e_process_skb_fields(rx_ring, rx_desc, skb);
> 
> This applies the original frame's ptype, checksum verdict, RSS hash and
> VLAN to bytes the hardware never parsed as headers. On XDP_TX or
> XDP_REDIRECT, a tail fragment would be sent out, or delivered to the
> AF_XDP socket as a complete packet.
> 
> Documentation/networking/af_xdp.rst says:
> 
>   AF_XDP guarantees that only a complete packet (all frames in the
>   packet) is sent to the application.
> 
> The non-ZC path in i40e_clean_rx_irq() keeps the hardware frame boundary:
> 
> 		} else if (i40e_add_xdp_frag(xdp, &nfrags, rx_buffer, size) &&
> 			   !neop) {
> 			/* Overflowing packet: Drop all frags on EOP */
> 
> The commit message says this matches ice, and ice does behave the same
> way. Even so, could the ZC drop path consume descriptors up to EOP before
> starting a new frame?

This is the same issue reported as on previous patch and I will address
this. I removed one issue which is fixed later, rest are pre-existing ones
and I don't count it, so now the score is even.

> 
> >  
> > -		if (++next_to_process == count)
> > -			next_to_process = 0;
> > -
> >  		if (i40e_is_non_eop(rx_ring, rx_desc))
> >  			continue;
> 
> -- 
> Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261102124950.958345-1-maciej.fijalkowski%40intel.com

Re: [PATCH net 3/9] ice: xsk: fix leaking Rx buffers in multi-buffer path

[Maciej Fijalkowski]

On Tue, Nov 03, 2026 at 01:32:31PM +0000, netdev-bot+sashiko@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 3 potential
> issue(s) to consider.
> 
> Critical: 0 · High: 2 · Medium: 1 · Low: 0 (1)
> 
> 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).
> 
> - [High] The new zero-length branch in ice_clean_rx_irq_zc()
>   (drivers/net/ethernet/intel/ice/ice_xsk.c) is `else if…
> - [High] ice: xsk: 0-length buffer assigned to first in
>   ice_clean_rx_irq_zc()
> - [Medium] As of this commit, if xsk_buff_add_frag() fails in
>   ice_clean_rx_irq_zc() because `xdp == first` (aligned-mode duplicate…
> 
> Pre-existing issues:
> - [Low] When xsk_buff_add_frag() fails on a non-EOP descriptor in
>   ice_clean_rx_irq_zc(), `first` is set to NULL and the loop continues.
> 
> --
> 
> ice: xsk: fix leaking Rx buffers in multi-buffer path
> 
> This commit changes ice_clean_rx_irq_zc() to free the xsk buffer of a
> zero-length frag. When xsk_buff_add_frag() fails, it now also frees the
> current descriptor's buffer in addition to the frame head. It relies on
> xp_free() skipping a duplicate buffer that was already freed with the
> frame.
> 
> > diff --git a/drivers/net/ethernet/intel/ice/ice_xsk.c b/drivers/net/ethernet/intel/ice/ice_xsk.c
> > index 0643017541c35..ea8ac0fcdf6ef 100644
> > --- a/drivers/net/ethernet/intel/ice/ice_xsk.c
> > +++ b/drivers/net/ethernet/intel/ice/ice_xsk.c
> > @@ -618,8 +618,11 @@ int ice_clean_rx_irq_zc(struct ice_rx_ring *rx_ring,
> >  
> >  		if (!first) {
> >  			first = xdp;
> > -		} else if (likely(size) && !xsk_buff_add_frag(first, xdp)) {
> > +		} else if (unlikely(!size)) {
> > +			xsk_buff_free(xdp);
> 
> [Severity: High]
> Does the zero-length check need to come before the !first check?

That can not ever happen on this HW. 0 len descriptors are rare but
possible at the end of fragmented frame containing only FCS.

So this behavior is correct: while we are in frag gathering process and
get 0 len desc, release *current* buffer and continue.

> 
> With the current ordering, a descriptor with pkt_len 0 is only freed
> when a frame is already in progress. If first is NULL, the zero-length
> buffer becomes the head of a new frame. If that descriptor is also EOP,
> the unlikely(!first) test further down no longer skips it.
> 
> One way to get there is the xsk_buff_add_frag() failure path just
> below. When xsk_buff_add_frag() fails on a non-EOP descriptor, first is
> reset to NULL and the loop moves on. Suppose the next descriptor is the
> EOP of the same packet and carries no data. That can happen when the
> hardware strips the FCS and the last buffer held only FCS bytes. The
> empty buffer is then assigned to first with data_end == data, and
> ice_run_xdp_zc() runs on an empty frame.

This will be addressed with draining up to EOP logic.

> 
> What happens next depends on the verdict:
> 
> - XDP_TX: ice_xmit_xdp_tx_zc() would post a Tx descriptor with a zero
>   buffer size and the EOP bit set. E810 may treat that as a malicious
>   driver event and stop the queue.
> 
> - XDP_PASS: xdp_build_skb_from_zc() builds an skb with len 0, so
>   eth_type_trans() has no header to work with. When
>   ICE_RX_FLAGS_MULTIDEV is set, ice_process_skb_fields() may also
>   __skb_push() ETH_HLEN, which moves skb->data in front of the frame.
> 
> - XDP_REDIRECT: the user socket gets a spurious empty descriptor.
> 
> libeth_xsk_process_buff() checks the length before anything else:
> 
>     if (unlikely(!len)) {
>         libeth_xsk_buff_free_slow(xdp);
>         return head;
>     }
> 
> This ordering predates this patch. Since this hunk already adds a
> dedicated zero-length branch, would it make sense to test size before
> first here?
> 
> Then a zero-length buffer would be freed when no frame is in progress,
> and first would stay NULL so the descriptor is skipped. There is also
> the duplicate fill ring case where xdp aliases first. Given that, the
> zero-length branch would presumably still need to avoid freeing the
> buffer when it is the current head.
> 
> [Severity: High]
> Can this free the live head of the frame when xdp == first?
> 
> In aligned mode, a duplicate fill ring address maps to the same
> pool->heads[] xskb. xsk_buff_add_frag() guards against this explicitly:
> 
>     /* Duplicate fill ring entries map to the same xskb in aligned mode */
>     if (unlikely(xdp == head || !list_empty(&frag->list_node)))
>         return false;
> 
> The new zero-size branch has no such check. Suppose the descriptor after
> the head comes back with pkt_len 0 and aliases first. Then
> xsk_buff_set_size() resets the head's flags, and xsk_buff_free() goes
> straight to xp_free(). The head is not on any list, so this:

This can not happen either. This implies an impossible scenario where
non-fragmented frame is followed by 0 len desc.

> 
>     void xp_free(struct xdp_buff_xsk *xskb)
>     {
>         if (!list_empty(&xskb->list_node))
>             return;
> 
>         xskb->pool->free_list_cnt++;
>         list_add(&xskb->list_node, &xskb->pool->free_list);
>     }
> 
> puts it on pool->free_list while first still points at it.
> 
> The frame then either goes on to ice_run_xdp_zc() (TX, REDIRECT or PASS)
> or is stashed in rx_ring->xsk. In the same poll,
> ice_alloc_rx_bufs_zc()->xp_alloc_batch()->xp_alloc_reused() can take the
> buffer off free_list with list_del_init() and post it to the hardware Rx
> ring again.
> 
> Could the NIC then DMA a later packet into a buffer that is still in use?
> That buffer could be the head of the frame being built, sit on the XDP_TX
> ring, or be on the user Rx ring.
> 
> Also, list_del_init() leaves list_node empty again. A later
> xsk_buff_free(first) or XDP_TX completion would then put the buffer back
> on free_list while it is still posted to hardware.
> 
> The commit message says "xp_free() skips a duplicate already freed with
> the frame". Here, though, the duplicate is the live head and has not been
> freed yet. Before this patch, the zero-length case did not touch the
> buffer.
> 
> Would skipping the free when xdp == first avoid this? That would match the
> guard in xsk_buff_add_frag(). For comparison, i40e_clean_rx_irq_zc() in
> this series does not free a zero-length buffer by itself.
> 
> > +		} else if (!xsk_buff_add_frag(first, xdp)) {
> >  			xsk_buff_free(first);
> > +			xsk_buff_free(xdp);
> 

Two issues not possible to happen and one later fixed.
Clashiko 2:5 Maciej

> 
> [Severity: Low]
> This isn't a bug introduced by this patch, but there is a related
> behavior. When xsk_buff_add_frag() fails on a non-EOP descriptor, first is
> set to NULL and the loop continues. The next descriptor of the dropped
> packet then starts a new frame through:
> 
>     if (!first) {
>         first = xdp;
> 
> So the rest of the dropped frame is assembled and run through XDP as a
> standalone packet that starts mid-frame.
> 
> The i40e companion patch in this series says this is intended ("Remaining
> buffers of this packet, if any, will be processed as a new packet.").
> 
> Is that also the intended behavior for ice, rather than discarding
> descriptors until EOP?
> 
> >
> 
> -- 
> Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261102124950.958345-1-maciej.fijalkowski%40intel.com