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

[Maciej Fijalkowski <maciej.fijalkowski@intel.com>]

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