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.
quoted
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.
quoted
+ } 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?
quoted
--
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261102124950.958345-1-maciej.fijalkowski%40intel.com