Re: [PATCH net v3 1/2] igc: only strip RX timestamp header from first buffer

[Maciej Fijalkowski]

On Fri, Oct 30, 2026 at 04:47:36PM +0000, Paul Moses wrote:
> 
> 
> 
> 
> On Friday, October 30th, 2026 at 6:45 AM, Maciej Fijalkowski <maciej.fijalkowski@xxxxxxxxx> wrote:
> 
> > On Thu, Oct 29, 2026 at 11:37:20PM +0000, netdev-bot+sashiko@xxxxxxxxxx wrote:
> > > Thank you for your contribution! Sashiko AI review found 1 potential
> > > issue(s) to consider.
> > >
> > > Critical: 0 · High: 1 · 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] At this commit by itself, the new first-buffer test `!skb &&
> > >   igc_test_staterr(rx_desc, IGC_RXDADV_STAT_TSIP)` in igc_clean_rx_irq()…
> > >
> > > --
> > >
> > > igc: only strip RX timestamp header from first buffer
> > >
> > > igc_clean_rx_irq() used to subtract IGC_TS_HDR_LEN from every descriptor
> > > with TSIP set, which truncated multi-buffer packets. With this patch the
> > > timestamp header is only stripped when skb is NULL, which the commit treats
> > > as marking the first buffer of a new packet.
> > >
> > > Link: https://lore.kernel.org/all/20260625-igb-rx-ts-fix-v3-1-99b3efa08dca@xxxxxxxx/
> > >
> > > > 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)) {
> > > >  			ctx.rx_ts = pktbuf;
> > > >  			pkt_offset = IGC_TS_HDR_LEN;
> > > >  			size -= IGC_TS_HDR_LEN;
> > >
> > > [Severity: High]
> > >
> > > Is skb == NULL a reliable first-buffer marker when an XDP program is
> > > attached? The commit message says:
> > >
> > >     Only consume the timestamp header when skb is NULL, which identifies the
> > >     first buffer of a new packet.
> > >
> > > Suppose igc_xdp_run_prog() returns IGC_XDP_CONSUMED, IGC_XDP_TX or
> > > IGC_XDP_REDIRECT for the first, non-EOP buffer of a multi-descriptor frame.
> > > Then no skb is built, and igc_clean_rx_irq() moves on to the continuation
> > > descriptor with skb still NULL:
> > >
> > > 		/* fetch next buffer in frame if non-eop */
> > > 		if (igc_is_non_eop(rx_ring, rx_desc))
> > > 			continue;
> > >
> > > The continuation descriptor still has TSIP set, so this check strips
> > > IGC_TS_HDR_LEN a second time. If that descriptor holds fewer than 16 bytes,
> > > can the unsigned size wrap here?
> > >
> > > The continuation buffer then goes through the new-frame path:
> > >
> > > 		if (!skb) {
> > > 			xdp_init_buff(&ctx.xdp, truesize, &rx_ring->xdp_rxq);
> > > 			xdp_prepare_buff(&ctx.xdp, pktbuf - igc_rx_offset(rx_ring),
> > > 					 igc_rx_offset(rx_ring) + pkt_offset,
> > > 					 size, true);
> > >
> > > In that case the XDP program would see a data_end about 4 GiB past data.
> > > On XDP_PASS, the same size would reach igc_build_skb() or
> > > igc_construct_skb(), and the __skb_put() there does not check it.
> > >
> > > At this commit, XDP with multi-descriptor frames looks possible on kernels
> > > with PAGE_SIZE >= 8192. igc_set_rx_mode() only clamps RLPML under the
> > > small-page #if:
> > >
> > > #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);
> > >
> > > On large-page kernels, RLPML therefore stays at MAX_JUMBO_FRAME_SIZE while
> > > igc_rx_bufsz() returns IGC_RXBUFFER_2048. igc_xdp_set_prog() only checks
> > > dev->mtu > ETH_DATA_LEN, not the hardware receive length limit.
> > >
> > > This needs RX hardware timestamping enabled, an XDP program that returns a
> > > non-PASS verdict for a first buffer, and a peer that sends frames larger
> > > than about 2032 bytes.
> > >
> > > The next patch in this series, "igc: fix AF_XDP receive buffer size for 2K
> > > UMEM chunks", adds igc_set_rlpml(). It caps RLPML at 1526 bytes whenever
> > > XDP is enabled, whatever PAGE_SIZE is. It is called from igc_setup_rctl()
> > > and igc_xdp_set_prog(). With that change, XDP frames always fit in a single
> > > descriptor, and skb == NULL becomes a correct first-buffer marker.
> > 
> > Wow - finally.
> > 
> > >
> > > This patch is tagged Cc: stable and could be backported on its own. Should
> > > it depend on that follow-up change, or should the two patches be swapped in
> > > the series?
> > 
> > How would we make sure both commits from this set get actually backported
> > together?
> > 
> > >
> > > --
> > > Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261028083344.3047350-1-p%401g4.org
> > 
> 
> I did forget to tag yours for stable, so maybe the will satisfy it?
> 
> I think ultimately, it sees all related bugs as one big problem in
> an effort to catch adjacent. My understanding has been that unless
> there's true interdependence of patches, ie. kernel will be left broken
> unless taken together, a series in itself adequately satisfies this requirement.
> 
> Should I do a v4 to add the tag? Or can this be rectified through 
> intel's process?

CC: Jake

Jake could you make sure we tag 2/2 as stable backport candidate?

Let's not send v4 and start the whole process again, bots are rather happy
so the ball is on tester's side now.

Thanks a lot Paul!

> 
> Paul