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

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] In igc_clean_rx_irq()
  (drivers/net/ethernet/intel/igc/igc_main.c), the new `!skb && TSIP`…

--

igc: only strip RX timestamp header from first buffer

This commit changes igc_clean_rx_irq() so that the inline RX timestamp
header is consumed only when skb is NULL. The commit treats skb == NULL as
the marker for the first buffer of a packet. As a result, continuation
descriptors that still report TSIP no longer have IGC_TS_HDR_LEN
subtracted from their size.

Link: https://lore.kernel.org/all/20260625-igb-rx-ts-fix-v3-1-99b3efa08dca@aweta.nl/ (local)

[Severity: High]
Is skb == NULL a reliable first-buffer test when an XDP program is
attached?

Suppose the XDP program returns a non-PASS verdict (DROP/ABORTED, TX or
REDIRECT) for the first buffer of a frame that spans several descriptors.
In that case igc_clean_rx_irq() never builds an skb:

	if (xdp_res) {
		switch (xdp_res) {
		case IGC_XDP_CONSUMED:
			rx_buffer->pagecnt_bias++;
			break;
		...
	}
	...
	/* fetch next buffer in frame if non-eop */
	if (igc_is_non_eop(rx_ring, rx_desc))
		continue;

skb stays NULL, so the next descriptor is handled as if it started a new
frame. The commit message says continuation descriptors keep TSIP, so
that descriptor passes the new !skb && TSIP check as well. Then:

  - ctx.rx_ts points at payload data
  - 16 bytes of payload are skipped
  - the XDP program runs again on data from the middle of the frame

The igc_fpe_handle_mpacket() continue path can also leave skb NULL across
a non-EOP descriptor.

What happens if the last fragment holds fewer than IGC_TS_HDR_LEN bytes?
Can size -= IGC_TS_HDR_LEN wrap there?

xdp_prepare_buff() takes data_len as an int, so data_end would end up
before data. On XDP_PASS, igc_build_skb() then does:

	unsigned int size = xdp->data_end - xdp->data;
	unsigned int truesize = igc_get_rx_frame_truesize(rx_ring, size);
	...
	__skb_put(skb, size);
	...
	igc_rx_buffer_flip(rx_buffer, truesize);

This would produce an skb with len around 4G and tail past end. The rx
page would also be flipped by a wrong truesize, so the hardware could DMA
into memory that the skb still owns.

Even without the wrap, wouldn't the mid-frame payload reach the stack as
its own frame after the real frame was dropped?

These multi-descriptor frames look reachable with XDP attached when
PAGE_SIZE >= 8192. igc_set_rx_mode() caps RLPML only for small pages:

#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 RLPML stays at MAX_JUMBO_FRAME_SIZE on those systems. At the same time:

  - igc_configure_rx_ring() uses IGC_RXBUFFER_3072 buffers when XDP is
    enabled
  - igc_xdp_set_prog() checks only dev->mtu > ETH_DATA_LEN, not the frame
    size the hardware accepts

With RX timestamping enabled, a remote peer sending a frame larger than
3056 bytes would take this path.

Could the frame-start state be tracked separately from skb? One option is
to discard descriptors through EOP after a non-PASS verdict. Another is to
cap RLPML whenever XDP is enabled.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261001222443.3500206-1-anthony.l.nguyen%40intel.com