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

[Simon Horman]

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@xxxxxxxxxx <netdev-bot+sashiko@xxxxxxxxxx> 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:
> > 
> > 		/* 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.