RE: [Intel-wired-lan] [PATCH net v3] igb: only strip Rx timestamp header on the first buffer of a frame

[Nowlin, Alexander]

> -----Original Message-----
> From: Intel-wired-lan <intel-wired-lan-bounces@xxxxxxxxxx> On Behalf Of Tjerk Kusters via B4 Relay
> Sent: Wednesday, June 24, 2026 10:24 PM
> To: Nguyen, Anthony L <anthony.l.nguyen@xxxxxxxxx>; Kitszel, Przemyslaw <przemyslaw.kitszel@xxxxxxxxx>; Andrew Lunn <andrew+netdev@xxxxxxx>; David S. Miller <davem@xxxxxxxxxxxxx>; Eric Dumazet <edumazet@xxxxxxxxxx>; Jakub Kicinski <kuba@xxxxxxxxxx>; Paolo Abeni <pabeni@xxxxxxxxxx>; Richard Cochran <richardcochran@xxxxxxxxx>; Jesper Dangaard Brouer <hawk@xxxxxxxxxx>; Kurt Kanzenbach <kurt@xxxxxxxxxxxxx>
> Cc: intel-wired-lan@xxxxxxxxxxxxxxxx; netdev@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; stable@xxxxxxxxxxxxxxx; Kwapulinski, Piotr <piotr.kwapulinski@xxxxxxxxx>; Loktionov, Aleksandr <aleksandr.loktionov@xxxxxxxxx>; Tjerk Kusters <tkusters@xxxxxxxx>
> Subject: [Intel-wired-lan] [PATCH net v3] igb: only strip Rx timestamp header on the first buffer of a frame
> 
> From: Tjerk Kusters <tkusters@xxxxxxxx>
> 
> When Rx hardware timestamping is enabled (e.g. ptp4l, which configures HWTSTAMP_FILTER_ALL), the NIC prepends a 16-byte timestamp header to the first Rx buffer of every received frame. igb_clean_rx_irq()  strips this header inside its per-buffer loop:
> 
> 	if (igb_test_staterr(rx_desc, E1000_RXDADV_STAT_TSIP)) {
> 		ts_hdr_len = igb_ptp_rx_pktstamp(rx_ring->q_vector,
> 						 pktbuf, &timestamp);
> 		pkt_offset += ts_hdr_len;
> 		size -= ts_hdr_len;
> 	}
> 
> For a frame that spans more than one Rx buffer (e.g. a jumbo frame), this block runs once per buffer. The timestamp header only exists at the start of the first buffer, but igb_ptp_rx_pktstamp() is called for every buffer.
> 
> On a continuation buffer the data is packet payload, not a timestamp header. igb_ptp_rx_pktstamp() already has two guards against acting on a non-header buffer: it returns 0 if PTP is disabled, and returns 0 if the reserved dwords (the first 8 bytes) are non-zero. Neither is sufficient
> here: PTP is enabled, and a continuation buffer whose payload happens to begin with 8 zero bytes passes the reserved-dword check. In that case the payload is mistaken for a valid timestamp header and igb_ptp_rx_pktstamp() returns IGB_TS_HDR_LEN, so the caller strips 16 bytes of real data from that buffer. A frame spanning N buffers whose continuation buffers start with zero bytes therefore loses 16 * (N - 1) bytes from its tail.
> 
> This is easily triggered by a GigE Vision camera streaming dark frames (mostly 0x00 pixel data) over jumbo UDP with PTP active on the receiver:
> the all-zero frames arrive truncated while frames with non-zero content are fine. There is no error indication.
> 
> No content-based check can reliably tell a continuation buffer that begins with zero bytes from a real timestamp header, because both are all zero.
> Fix it structurally instead: only attempt the strip on the first buffer of a frame, which is the only buffer that can contain a timestamp header. In
> igb_clean_rx_irq() skb is NULL until the first buffer has been processed, so guarding the strip with !skb restricts it to the first buffer regardless of payload content.
> 
> Fixes: 5379260852b0 ("igb: Fix XDP with PTP enabled")
> Cc: stable@xxxxxxxxxxxxxxx
> Reviewed-by: Piotr Kwapulinski <piotr.kwapulinski@xxxxxxxxx>
> Reviewed-by: Aleksandr Loktionov <aleksandr.loktionov@xxxxxxxxx>
> Reviewed-by: Kurt Kanzenbach <kurt@xxxxxxxxxxxxx>
> Signed-off-by: Tjerk Kusters <tkusters@xxxxxxxx>
> ---
> Changes in v3:
> - update the rx-timestamp comment to note it only applies to the first
>   buffer of a frame (Piotr Kwapulinski)
> - add Reviewed-by from Aleksandr Loktionov and Piotr Kwapulinski
> - no functional change
> - Link to v2: https://patch.msgid.link/20260619-igb-rx-ts-fix-v2-1-d3b8d605ca62@xxxxxxxx
> 
> igb: only strip Rx timestamp header on the first buffer of a frame
> 
> Changes in v2:
>  - resend via b4 (v1 was sent with a mail client)
>  - use full author name "Tjerk Kusters" (Jacob Keller)
>  - add Reviewed-by from Kurt Kanzenbach
>  - no functional change
> 
> Link to v1: https://lore.kernel.org/all/PAWPR05MB1069106D52F4E17F1EDB99C67B9182@xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx/
> ---
>  drivers/net/ethernet/intel/igb/igb_main.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)

Tested-by: Alexander Nowlin <alexander.nowlin@xxxxxxxxx>