Re: [PATCH net v5 4/4] igb: drop AF_XDP zero-copy frames that span several Rx buffers

[netdev-bot+sashiko@kernel.org]

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

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

- [Medium] On i354 with VFs allocated, nothing in the driver applies the
  receive length limit that the zero-copy buffer layout now relies on.

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but the new
  discard path depends on it.

--

igb: drop AF_XDP zero-copy frames that span several Rx buffers

This patch makes igb_clean_rx_irq_zc() drop AF_XDP zero-copy frames that
span more than one Rx descriptor. It frees every non-EOP buffer and the EOP
buffer that ends such a frame. A new ring flag,
IGB_RING_FLAG_RX_XSK_DISCARD, keeps the discard state so that a split
frame can be dropped across NAPI polls.

> diff --git a/drivers/net/ethernet/intel/igb/igb_xsk.c b/drivers/net/ethernet/intel/igb/igb_xsk.c
> index fb04df82272ac..7b098069fff29 100644
> --- a/drivers/net/ethernet/intel/igb/igb_xsk.c
> +++ b/drivers/net/ethernet/intel/igb/igb_xsk.c

[ ... ]

> @@ -390,6 +392,27 @@ int igb_clean_rx_irq_zc(struct igb_q_vector *q_vector,
>  		dma_rmb();
>  
>  		xdp = rx_ring->rx_buffer_info_zc[ntc];
> +
> +		/* A frame should never span several buffers here, see
> +		 * igb_xsk_frame_fits(). If one does, drop all its buffers.
> +		 */
> +		if (unlikely(!igb_test_staterr(rx_desc, E1000_RXD_STAT_EOP) ||
> +			     test_bit(IGB_RING_FLAG_RX_XSK_DISCARD,
> +				      &rx_ring->flags))) {

[Severity: Medium]
Is the receive length limit that this comment relies on applied on i354
when VFs are allocated?

After the series, igb_setup_srrctl() gives XSK rings a 2048 or 3072 byte
hardware buffer. It relies on the length limit to keep frames inside the
XSK frame, which is 1792 bytes with a 2 KiB chunk pool.

When vfn != 0, igb_set_rlpml() leaves the global RLPML large:

	if (igb_xdp_is_enabled(adapter)) {
		pool_rlpml = min(pool_rlpml, adapter->max_frame_size);
		if (!vfn)
			rlpml = min(rlpml, adapter->max_frame_size);
	}
	wr32(E1000_RLPML, rlpml);

On i354 it then returns before it writes the PF pool's VMOLR, because
e1000_i354 comes after e1000_i350:

	if (hw->mac.type < e1000_82576 || hw->mac.type > e1000_i350)
		return;

igb_set_rx_mode() also skips VMOLR on i354, and igb_probe_vfs() does not
exclude i354. An earlier commit in this series (bfd45cda) notes that the
PF pool's VMOLR is still not written on i354. The ZC path does not seem to
account for that.

igb_xsk_pool() and igb_xsk_pool_enable() only check that XDP is enabled
and that igb_xsk_frame_fits() passes:

	return max_frame + IGB_TS_HDR_LEN <= xsk_pool_get_rx_frame_size(pool);

This check assumes the MAC enforces max_frame_size.

Suppose the i354 PF pool accepts long frames by default. Can a peer frame
of 1793 to 2048 bytes then land in a single descriptor with EOP set, pass
this check, and reach xsk_buff_set_size(xdp, size) with a size larger than
the XSK frame? By then the NIC would already have written into the next
UMEM chunk or past the end of the UMEM page.

Longer frames are dropped here, but only after each of their buffers has
been filled up to the hardware buffer size. The commit message of "igb: do
not split AF_XDP zero-copy frames over several Rx buffers" says: "Without
it, the hardware can write past the XSK frame."

The i354 reset default of the PF pool's VMOLR.LPE/RLPML could not be
confirmed, since the driver never writes that register on this MAC.

Before the series, ZC rings used the XSK frame size rounded down to 1 KiB,
so the hardware could not write past the frame.

> +			if (igb_test_staterr(rx_desc, E1000_RXD_STAT_EOP))
> +				clear_bit(IGB_RING_FLAG_RX_XSK_DISCARD,
> +					  &rx_ring->flags);
> +			else
> +				set_bit(IGB_RING_FLAG_RX_XSK_DISCARD,
> +					&rx_ring->flags);
> +
> +			xsk_buff_free(xdp);
> +			ntc++;
> +			if (ntc == rx_ring->count)
> +				ntc = 0;
> +			continue;
> +		}

[Severity: High]
This isn't a bug introduced by this patch, but the new discard path
depends on a length == 0 sentinel that igb_alloc_rx_buffers_zc() does not
always set.

A refill can wrap around the end of the ring and get fewer buffers from
xsk_buff_alloc_batch() than it asked for (but at least one). In that case
the early exit skips clearing the length of the new next_to_use
descriptor:

	if (ntu + count >= rx_ring->count) {
		nb_buffs_extra = igb_fill_rx_descs(xsk_pool, xdp, rx_desc,
						   rx_ring->count - ntu);
		if (nb_buffs_extra != rx_ring->count - ntu) {
			ntu += nb_buffs_extra;
			goto exit;
		}
	...
	/* clear the length for the next_to_use descriptor */
	rx_desc = IGB_RX_DESC(rx_ring, ntu);
	rx_desc->wb.upper.length = 0;

exit:

That descriptor is in the region software has already processed.
igb_clean_rx_irq_zc() never clears the length after use, so after the
first lap the descriptor still holds an old write-back with a non-zero
length.

Once the hardware has completed every posted descriptor,
igb_clean_rx_irq_zc() reaches ntc == next_to_use and only checks:

	size = le16_to_cpu(rx_desc->wb.upper.length);
	if (!size)
		break;

rx_buffer_info_zc[ntc] is never set to NULL after use (ixgbe does
bi->xdp = NULL). It can still point to an xdp_buff that was already freed,
given to the socket, or reposted on another descriptor.

Can this deliver the same UMEM chunk to user space twice, or return it to
the pool while hardware or user space still owns it? In unaligned mode,
xp_release() would push the same xskb onto free_heads[] a second time.

Every descriptor in the unused region is stale, so the loop can run until
the budget is used up. That leaves next_to_clean past next_to_use, which
breaks igb_desc_unused() and later refills.

With this patch, a stale non-EOP descriptor also takes the new branch.
xsk_buff_free() is called on the stale pointer and
IGB_RING_FLAG_RX_XSK_DISCARD is set, so the next valid frame is dropped.

The branch also continues without counting against the budget. That
leaves the missing length sentinel as the only thing that ends the loop.

igc_alloc_rx_buffers_zc() clears the next descriptor's length after every
fill, including partial ones. Would doing the same here close this?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261104-igb-rx-ts-fix-v5-0-bf4b4367129a%40aweta.nl