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

[Paul Moses]

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?

Paul