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

[Maciej Fijalkowski]

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