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

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

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