> -----Original Message----- > From: Intel-wired-lan <[email protected]> On Behalf Of Tjerk > Kusters via B4 Relay > Sent: Wednesday, June 24, 2026 10:24 PM > To: Nguyen, Anthony L <[email protected]>; Kitszel, Przemyslaw > <[email protected]>; Andrew Lunn <[email protected]>; David S. > Miller <[email protected]>; Eric Dumazet <[email protected]>; Jakub > Kicinski <[email protected]>; Paolo Abeni <[email protected]>; Richard Cochran > <[email protected]>; Jesper Dangaard Brouer <[email protected]>; Kurt > Kanzenbach <[email protected]> > Cc: [email protected]; [email protected]; > [email protected]; [email protected]; Kwapulinski, Piotr > <[email protected]>; Loktionov, Aleksandr > <[email protected]>; Tjerk Kusters <[email protected]> > Subject: [Intel-wired-lan] [PATCH net v3] igb: only strip Rx timestamp header > on the first buffer of a frame > > From: Tjerk Kusters <[email protected]> > > 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, ×tamp); > 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: [email protected] > Reviewed-by: Piotr Kwapulinski <[email protected]> > Reviewed-by: Aleksandr Loktionov <[email protected]> > Reviewed-by: Kurt Kanzenbach <[email protected]> > Signed-off-by: Tjerk Kusters <[email protected]> > --- > 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/[email protected] > > 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/pawpr05mb1069106d52f4e17f1edb99c67b9...@pawpr05mb10691.eurprd05.prod.outlook.com/ > --- > drivers/net/ethernet/intel/igb/igb_main.c | 7 +++++-- > 1 file changed, 5 insertions(+), 2 deletions(-)
Tested-by: Alexander Nowlin <[email protected]>
