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

[netdev-bot+sashiko@kernel.org]

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@aweta.nl/

> 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.

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?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261028083344.3047350-1-p%401g4.org