Re: [PATCH net v2] igc: only strip RX timestamp header from first buffer

[netdev-bot+sashiko@kernel.org]

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

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

- [High] The patch fixes the TSIP length underflow only in
  igc_clean_rx_irq().
- [High] In igc_clean_rx_irq(), the new condition `if (!skb &&
  igc_test_staterr(rx_desc, IGC_RXDADV_STAT_TSIP))` uses `!skb` to mean…

--

igc: only strip RX timestamp header from first buffer

This patch changes igc_clean_rx_irq() so the 16-byte inline RX timestamp
header is only stripped when no skb is in progress. Before, a continuation
buffer shorter than 16 bytes could underflow the unsigned length and
produce a skb fragment of almost 4 GiB.

> diff --git a/drivers/net/ethernet/intel/igc/igc_main.c b/drivers/net/ethernet/intel/igc/igc_main.c
> index 1fb5f3cbe93c5..41b293dc10c3a 100644
> --- a/drivers/net/ethernet/intel/igc/igc_main.c
> +++ b/drivers/net/ethernet/intel/igc/igc_main.c
> @@ -2639,7 +2639,8 @@ static int igc_clean_rx_irq(struct igc_q_vector *q_vector, const int budget)
>  
>  		pktbuf = page_address(rx_buffer->page) + rx_buffer->page_offset;
>  
> -		if (igc_test_staterr(rx_desc, IGC_RXDADV_STAT_TSIP)) {
> +		if (!skb &&
> +		    igc_test_staterr(rx_desc, IGC_RXDADV_STAT_TSIP)) {

[Severity: High]
Is !skb a reliable first-buffer test when an XDP program is attached?

Suppose the XDP program returns a non-PASS verdict (CONSUMED, TX or
REDIRECT) for the first, non-EOP buffer of a multi-descriptor frame. No skb
gets built, and the loop moves on with skb still NULL:

		/* fetch next buffer in frame if non-eop */
		if (igc_is_non_eop(rx_ring, rx_desc))
			continue;

On the next continuation descriptor, !skb is still true, so the TSIP branch
subtracts IGC_TS_HDR_LEN again. If that continuation holds fewer than 16
bytes, size wraps.

The wrapped size then goes to xdp_prepare_buff(), and the continuation is
run through XDP as a new frame with data_end before data. On XDP_PASS, both
igc_build_skb() and igc_construct_skb() compute:

	unsigned int size = xdp->data_end - xdp->data;

That comes out to about 4 GiB:

- igc_build_skb() passes it to __skb_put(). On PAGE_SIZE >= 8192,
  igc_get_rx_frame_truesize() also wraps to a tiny truesize, so
  napi_build_skb() would place skb_shared_info inside the packet data.
- igc_construct_skb() turns it into a fragment of almost 4 GiB. That looks
  like the same corrupted skb the commit message describes.

Can this be reached from the wire? On PAGE_SIZE >= 8192, igc_set_rx_mode()
never clamps RLPML:

#if (PAGE_SIZE < 8192)
	if (adapter->max_frame_size <= IGC_MAX_FRAME_BUILD_SKB)
		rlpml = IGC_MAX_FRAME_BUILD_SKB;
#endif
	wr32(IGC_RLPML, rlpml);

So the hardware still accepts jumbo frames even though XDP limits the MTU to
1500, and with XDP the ring uses 3072-byte buffers. A remote frame that
leaves 1 to 15 bytes in the second descriptor would hit this.

Would it be safer to track start-of-frame explicitly, or to drop the rest of
the frame's descriptors after a non-PASS verdict? Another option is to
reject size < IGC_TS_HDR_LEN before the subtraction.

>  			ctx.rx_ts = pktbuf;
>  			pkt_offset = IGC_TS_HDR_LEN;
>  			size -= IGC_TS_HDR_LEN;

[Severity: High]
The same unchecked subtraction is still present in the AF_XDP zero-copy
path. igc_clean_rx_irq_zc() strips the header from every descriptor with
TSIP set. There is no first-buffer gate and no size check:

		if (igc_test_staterr(desc, IGC_RXDADV_STAT_TSIP)) {
			ctx->rx_ts = bi->xdp->data;

			bi->xdp->data += IGC_TS_HDR_LEN;
			...
			size -= IGC_TS_HDR_LEN;
		} else {
			ctx->rx_ts = NULL;
		}

		bi->xdp->data_end = bi->xdp->data + size;

Can zero-copy frames span descriptors at MTU 1500?
igc_configure_rx_ring() programs the XSK frame size through
IGC_SRRCTL_BSIZEPKT(), which is (x) / 1024. With 2048-byte chunks the frame
size is 1792, so the hardware buffer becomes 1 KB.

igc_xdp_enable_pool() only checks:

	if (frame_size < ETH_FRAME_LEN + VLAN_HLEN * 2) {

That accepts 1792 without allowing for the 1 KB rounding or the 16-byte
inline timestamp, even though the comment there says frames should not
span multiple buffers.

On 4K pages RLPML is IGC_MAX_FRAME_BUILD_SKB. A received frame of about
1009 to 1023 bytes, plus the timestamp header, would leave 1 to 15 bytes in
a second descriptor.

The reproduction in the commit message suggests TSIP is set on such
continuation descriptors. If so, size wraps here and data_end ends up about
4 GiB past the XSK chunk.

Wouldn't that let the XDP program's packet bounds checks pass for offsets
past the chunk? On XDP_REDIRECT, it would also report a bogus length to the
XSK socket.

Should igc_clean_rx_irq_zc() get a similar fix? Should
igc_xdp_enable_pool() also account for the 1 KB SRRCTL granularity and the
inline timestamp?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261013010854.4068874-1-p%401g4.org