Re: [PATCH net] igb: only strip Rx timestamp header on the first buffer of a frame

[Tony Nguyen <anthony.l.nguyen@intel.com>]

On 6/15/2026 12:43 AM, Kurt Kanzenbach wrote:
> Hi,
> 
> On Fri Jun 12 2026, Tjerk Kusters wrote:
>> Hi,
>>
>> The patch is attached (0001-igb-only-strip-Rx-timestamp-header-on-the-first-buff.patch)
>> as my mail setup cannot send it inline via git send-email; apologies for the
>> attachment.
> 
> b4 has a web submission endpoint. Maybe you can use that one:
> 
> https://b4.docs.kernel.org/en/latest/contributor/send.html
Hi Tjerk,

It would be great if you could get this setup as it makes patch handling 
easier.

> [snip]
> 
>>  From fee3e3452dfcd7e109332369672a3e0090cadeb3 Mon Sep 17 00:00:00 2001
>> From: T Kusters <tkusters@aweta.nl>
>> Date: Tue, 9 Jun 2026 14:06:24 +0200
>> Subject: [PATCH net] igb: only strip Rx timestamp header on the first buffer
>>   of a frame
>>
>> 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@vger.kernel.org
>> Signed-off-by: T Kusters <tkusters@aweta.nl>

Sign off should be your full name.

Thanks,
Tony

> Great explanation! igb_clean_rx_irq_zc() does not need the same
> treatment, correct?
> 
> Reviewed-by: Kurt Kanzenbach <kurt@linutronix.de>
> 
>> ---
>>   drivers/net/ethernet/intel/igb/igb_main.c | 3 ++-
>>   1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/net/ethernet/intel/igb/igb_main.c b/drivers/net/ethernet/intel/igb/igb_main.c
>> index ce91dda00ec0..abb55cd589a9 100644
>> --- a/drivers/net/ethernet/intel/igb/igb_main.c
>> +++ b/drivers/net/ethernet/intel/igb/igb_main.c
>> @@ -9061,7 +9061,8 @@ static int igb_clean_rx_irq(struct igb_q_vector *q_vector, const int budget)
>>   		pktbuf = page_address(rx_buffer->page) + rx_buffer->page_offset;
>>   
>>   		/* pull rx packet timestamp if available and valid */
>> -		if (igb_test_staterr(rx_desc, E1000_RXDADV_STAT_TSIP)) {
>> +		if (!skb &&
>> +		    igb_test_staterr(rx_desc, E1000_RXDADV_STAT_TSIP)) {
>>   			int ts_hdr_len;
>>   
>>   			ts_hdr_len = igb_ptp_rx_pktstamp(rx_ring->q_vector,
>> -- 
>> 2.27.0
>>