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

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

On Tue, Oct 20, 2026 at 06:06:49PM +0100, Simon Horman wrote:
> On Wed, Oct 14, 2026 at 05:43:10AM +0000, Paul Moses wrote:
> > On Tuesday, October 13th, 2026 at 8:59 PM, netdev-bot+sashiko@kernel.org <netdev-bot+sashiko@kernel.org> wrote:
> > 
> > > 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:

What the heck?

1. This driver does NOT support XDP multi-buffer.
2. XDP programs do NOT run on non-EOP buffers. Usually whole packet is
gathered together *up to* EOP frame and represented via linear xdp_buff
part + frags sitting in tailroom in skb_shared_info.

Hallucination at its best! :D There is still hope for us, humans.

However, the ZC side needs addressing. I would go with data-path fix same
as this patch carries.

The rx frame size issue is real, but let us fix it separately.

Seems like no one has tested igc xsk against 2k chunk size. In this case
indeed for standard MTU we would get fragmented (!) frame. BSIZEPKT value
after 1k division gets round down and HW descriptor will actually hold upt
1024 bytes...

Thanks,
Maciej

> > > 
> > > 		/* 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
> > > 
> > 
> > Yes, there's work needed elsewhere, but it's entirely independent of my fix. It's 
> > questionable how much of this is even reachable and if any impact whatsoever. 
> > 
> > Here's what I can reproduce with and without kasan and what my patch fixes. You are on your 
> > own on the rest.
> > 
> > I'm sorry, if only there were more time...
> > 
> > Good luck.
> 
> Hi Paul,
> 
> Thanks a lot for your patch.
> 
> I  do accept that there are some pre-existing issues in the AI-generated
> review that are not strictly related to this patch.
> 
> But I do think that it would be best to make this patch more complete
> in so far as preventing the overrun described by making a wider guard
> for calling igc_test_staterr().
> 
> And including igc_clean_rx_irq_zc() in the fix.
> 
> My slight preference would be for explicitly tracking the first buffer.
> But perhaps that degrades performance.