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.
quoted
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?
quoted
+ 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.
quoted
- 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