[PATCH net 1/6] igc: Fix RX HW timestamp reporting when NET_RX_BUSY_POLL is disabled

[PATCH net 1/6] igc: Fix RX HW timestamp reporting when NET_RX_BUSY_POLL is disabled

[Tony Nguyen]

From: Ding Meng <meng.ding@siemens.com>

When CONFIG_NET_RX_BUSY_POLL is deactivated, fetching RX HW timestamps
from the NIC no longer works as expected, often resulting in incorrect
or negative values such as "HW raw -121948.050407424".

This occurs because disabling CONFIG_NET_RX_BUSY_POLL disables the
SKB NAPI mapping in __skb_mark_napi_id(). Consequently, get_timestamp()
fails to perform its driver lookup, and the igc driver's struct
net_device_ops::ndo_get_tstamp is never invoked.

Instead, get_timestamp() falls back to use shhwtstamps(skb)->hwtstamp,
a field that the driver has not populated. This results in incorrect
timestamps.

Fix this by populating the hwtstamp field with the correct timestamp
in the default timer when CONFIG_NET_RX_BUSY_POLL is disabled.
The "igc_adapter" is passed to igc_construct_skb() to enable
igc_ptp_rx_pktstamp() to access the necessary adapter details for
adjusting the timestamp.

Test case:
 Disable CONFIG_NET_RX_BUSY_POLL.
 Sender:
 # tools/testing/selftests/net/timestamping en0 \
        SOF_TIMESTAMPING_TX_HARDWARE PTPV2 IP_MULTICAST_LOOP
 Receiver:
 # tools/testing/selftests/net/timestamping en0 \
        SOF_TIMESTAMPING_RX_HARDWARE SOF_TIMESTAMPING_RAW_HARDWARE PTPV2

Before patch, receiver prints
 HW raw -121948.050407424
After patch, receiver prints
 HW raw 1760648763.746974064

Fixes: 069b142f5819 ("igc: Add support for PTP .getcyclesx64()")
Cc: stable@vger.kernel.org
Co-developed-by: Florian Bezdeka <florian.bezdeka@siemens.com>
Signed-off-by: Florian Bezdeka <florian.bezdeka@siemens.com>
Signed-off-by: Ding Meng <meng.ding@siemens.com>
Reviewed-by: Aleksandr Loktionov <aleksandr.loktionov@intel.com>
Reviewed-by: Piotr Kwapulinski <piotr.kwapulinski@intel.com>
Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
---
 drivers/net/ethernet/intel/igc/igc_main.c | 41 ++++++++++++++++-------
 1 file changed, 29 insertions(+), 12 deletions(-)

diff --git a/drivers/net/ethernet/intel/igc/igc_main.c b/drivers/net/ethernet/intel/igc/igc_main.c
index 1fb5f3cbe93c..95f7747b347b 100644
--- a/drivers/net/ethernet/intel/igc/igc_main.c
+++ b/drivers/net/ethernet/intel/igc/igc_main.c
@@ -1989,7 +1989,29 @@ static struct sk_buff *igc_build_skb(struct igc_ring *rx_ring,
 	return skb;
 }
 
-static struct sk_buff *igc_construct_skb(struct igc_ring *rx_ring,
+static void igc_construct_skb_timestamps(struct igc_adapter *adapter,
+					 struct sk_buff *skb,
+					 struct igc_xdp_buff *ctx)
+{
+#ifndef CONFIG_NET_RX_BUSY_POLL
+	struct igc_inline_rx_tstamps *tstamps;
+#endif
+
+	if (!ctx->rx_ts)
+		return;
+
+#ifndef CONFIG_NET_RX_BUSY_POLL
+	tstamps = ctx->rx_ts;
+	skb_hwtstamps(skb)->hwtstamp = igc_ptp_rx_pktstamp(adapter,
+							   tstamps->timer0);
+#else
+	skb_shinfo(skb)->tx_flags |= SKBTX_HW_TSTAMP_NETDEV;
+	skb_hwtstamps(skb)->netdev_data = ctx->rx_ts;
+#endif
+}
+
+static struct sk_buff *igc_construct_skb(struct igc_adapter *adapter,
+					 struct igc_ring *rx_ring,
 					 struct igc_rx_buffer *rx_buffer,
 					 struct igc_xdp_buff *ctx)
 {
@@ -2010,10 +2032,7 @@ static struct sk_buff *igc_construct_skb(struct igc_ring *rx_ring,
 	if (unlikely(!skb))
 		return NULL;
 
-	if (ctx->rx_ts) {
-		skb_shinfo(skb)->tx_flags |= SKBTX_HW_TSTAMP_NETDEV;
-		skb_hwtstamps(skb)->netdev_data = ctx->rx_ts;
-	}
+	igc_construct_skb_timestamps(adapter, skb, ctx);
 
 	/* Determine available headroom for copy */
 	headlen = size;
@@ -2683,7 +2702,7 @@ static int igc_clean_rx_irq(struct igc_q_vector *q_vector, const int budget)
 		else if (ring_uses_build_skb(rx_ring))
 			skb = igc_build_skb(rx_ring, rx_buffer, &ctx.xdp);
 		else
-			skb = igc_construct_skb(rx_ring, rx_buffer, &ctx);
+			skb = igc_construct_skb(adapter, rx_ring, rx_buffer, &ctx);
 
 		/* exit if we failed to retrieve a buffer */
 		if (!xdp_res && !skb) {
@@ -2735,7 +2754,8 @@ static int igc_clean_rx_irq(struct igc_q_vector *q_vector, const int budget)
 	return total_packets;
 }
 
-static struct sk_buff *igc_construct_skb_zc(struct igc_ring *ring,
+static struct sk_buff *igc_construct_skb_zc(struct igc_adapter *adapter,
+					    struct igc_ring *ring,
 					    struct igc_xdp_buff *ctx)
 {
 	struct xdp_buff *xdp = &ctx->xdp;
@@ -2757,10 +2777,7 @@ static struct sk_buff *igc_construct_skb_zc(struct igc_ring *ring,
 		__skb_pull(skb, metasize);
 	}
 
-	if (ctx->rx_ts) {
-		skb_shinfo(skb)->tx_flags |= SKBTX_HW_TSTAMP_NETDEV;
-		skb_hwtstamps(skb)->netdev_data = ctx->rx_ts;
-	}
+	igc_construct_skb_timestamps(adapter, skb, ctx);
 
 	return skb;
 }
@@ -2772,7 +2789,7 @@ static void igc_dispatch_skb_zc(struct igc_q_vector *q_vector,
 	struct igc_ring *ring = q_vector->rx.ring;
 	struct sk_buff *skb;
 
-	skb = igc_construct_skb_zc(ring, ctx);
+	skb = igc_construct_skb_zc(q_vector->adapter, ring, ctx);
 	if (!skb) {
 		ring->rx_stats.alloc_failed++;
 		set_bit(IGC_RING_FLAG_RX_ALLOC_FAILED, &ring->flags);
-- 
2.47.1

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

[Tony Nguyen]

From: Paul Moses <p@1g4.org>

igc_clean_rx_irq() strips IGC_TS_HDR_LEN whenever a descriptor reports
IGC_RXDADV_STAT_TSIP. For multi-buffer packets, continuation descriptors
retain TSIP even though the inline timestamp is present only in the first
RX buffer.

Subtracting the header length from each continuation buffer truncates
jumbo packets by 16 bytes per continuation and leaves the packet length
larger than the received data.

Only consume the timestamp header when skb is NULL, which identifies the
first buffer of a new packet. An skb carried in rx_ring->skb remains
non-NULL when packet assembly resumes in a later NAPI poll.

Link: https://lore.kernel.org/all/20260625-igb-rx-ts-fix-v3-1-99b3efa08dca@aweta.nl/
Fixes: e1ed4f92a625 ("igc: Refactor Rx timestamp handling")
Cc: stable@vger.kernel.org
Signed-off-by: Paul Moses <p@1g4.org>
Reviewed-by: Aleksandr Loktionov <aleksandr.loktionov@intel.com>
Acked-by: Maciej Fijalkowski <maciej.fijalkowski@intel.com>
Tested-by: Avigail Dahan <avigailx.dahan@intel.com>
Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
---
 drivers/net/ethernet/intel/igc/igc_main.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/intel/igc/igc_main.c b/drivers/net/ethernet/intel/igc/igc_main.c
index 95f7747b347b..b94a08791102 100644
--- a/drivers/net/ethernet/intel/igc/igc_main.c
+++ b/drivers/net/ethernet/intel/igc/igc_main.c
@@ -2658,7 +2658,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;
-- 
2.47.1

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

[Tony Nguyen]

From: Tjerk Kusters <tkusters@aweta.nl>

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
Reviewed-by: Piotr Kwapulinski <piotr.kwapulinski@intel.com>
Reviewed-by: Aleksandr Loktionov <aleksandr.loktionov@intel.com>
Reviewed-by: Kurt Kanzenbach <kurt@linutronix.de>
Signed-off-by: Tjerk Kusters <tkusters@aweta.nl>
Tested-by: Alexander Nowlin <alexander.nowlin@intel.com>
Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
---
 drivers/net/ethernet/intel/igb/igb_main.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/intel/igb/igb_main.c b/drivers/net/ethernet/intel/igb/igb_main.c
index d4a897a8c82c..5c09dc4a2566 100644
--- a/drivers/net/ethernet/intel/igb/igb_main.c
+++ b/drivers/net/ethernet/intel/igb/igb_main.c
@@ -9069,8 +9069,11 @@ static int igb_clean_rx_irq(struct igb_q_vector *q_vector, const int budget)
 		rx_buffer = igb_get_rx_buffer(rx_ring, size, &rx_buf_pgcnt);
 		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)) {
+		/* pull rx packet timestamp if available and valid; it is only
+		 * present on the first buffer of a frame
+		 */
+		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.47.1

[PATCH net 6/6] e1000e: add system to disable K1 list

[Tony Nguyen]

Javier reports seeing packet loss issues related to the re-enablement
of K1; disabling it makes the issues go away. Add the reported system
to have K1 disabled by default.

Reproduction steps (from the Link):
Plug the cable and ping the default gateway on the LAN. No suspend/resume
involved, machine freshly booted and on AC power. Nothing e1000e-related in
dmesg apart from the link up/down messages; no "Hardware Unit Hang".

Default (K1 enabled):

  $ ping -c 30 192.168.10.1
  --- 192.168.10.1 ping statistics ---
  30 packets transmitted, 18 received, 40% packet loss, time 29734ms
  rtt min/avg/max/mdev = 0.211/0.315/0.384/0.046 ms

The lost packets are spread over the run (seq 6, 8, 9, 11, 13, 14, 16, 19,
20, 24, 25, 30), not a single burst. DHCP on this link also takes 1-2
minutes to get a lease, consistent with incoming packets being dropped.

With K1 disabled at runtime:

  # ethtool --set-priv-flags enp0s31f6 disable-k1 on
  $ ethtool --show-priv-flags enp0s31f6
  Private flags for enp0s31f6:
  s0ix-enabled: on
  disable-k1  : on

  $ ping -c 30 192.168.10.1
  --- 192.168.10.1 ping statistics ---
  30 packets transmitted, 30 received, 0% packet loss, time 29713ms
  rtt min/avg/max/mdev = 0.169/0.251/0.468/0.054 ms

  $ ping -c 30 192.168.10.1
  --- 192.168.10.1 ping statistics ---
  30 packets transmitted, 30 received, 0% packet loss, time 29677ms
  rtt min/avg/max/mdev = 0.101/0.222/0.360/0.064 ms

I have only compile tested this, but Javier has tested that disabling K1
on his system (Lenovo ThinkPad P14s Gen 5 (Intel), I219-LM [8086:550a])
resolves his issues.

Cc: stable@vger.kernel.org
Fixes: 578294b8b60d ("e1000e: Reconfigure PLL clock gate timeout and re-enable K1 on Meteor Lake")
Reported-by: Javier Herrera <javier.herrera@afronta.com>
Link: https://lore.kernel.org/intel-wired-lan/CAKRuCQLWEcci9hPgNUmvq44Bd2H30T5_vP=exMjsknXMN5Yh9A@mail.gmail.com/
Reviewed-by: Aleksandr Loktionov <aleksandr.loktionov@intel.com>
Signed-off-by: Tony Nguyen <anthony.l.nguyen@intel.com>
---
 drivers/net/ethernet/intel/e1000e/netdev.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/net/ethernet/intel/e1000e/netdev.c b/drivers/net/ethernet/intel/e1000e/netdev.c
index ad9b88c9af22..6f85dab5c55c 100644
--- a/drivers/net/ethernet/intel/e1000e/netdev.c
+++ b/drivers/net/ethernet/intel/e1000e/netdev.c
@@ -67,6 +67,13 @@ static const struct dmi_system_id disable_k1_list[] = {
 			DMI_MATCH(DMI_PRODUCT_NAME, "Dell Pro 16 Plus PB16250"),
 		},
 	},
+	{
+		.ident = "Lenovo ThinkPad P14s Gen 5",
+		.matches = {
+			DMI_MATCH(DMI_SYS_VENDOR, "LENOVO"),
+			DMI_MATCH(DMI_PRODUCT_VERSION, "ThinkPad P14s Gen 5"),
+		},
+	},
 	{}
 };
 
-- 
2.47.1

Re: [PATCH net 1/6] igc: Fix RX HW timestamp reporting when NET_RX_BUSY_POLL is disabled

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider.

Critical: 0 · High: 1 (3) · 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] When CONFIG_NET_RX_BUSY_POLL=n, the fix still returns wrong RX
  hardware timestamps to sockets bound to a PTP virtual clock…

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but the root cause
  the commit message names (get_timestamp() falling back to…
- [High] This problem wasn't introduced by this patch, but with
  CONFIG_NET_RX_BUSY_POLL=y (the common distro default)…
- [High] This problem wasn't introduced by this patch, but the
  CONFIG_NET_RX_BUSY_POLL=y branch of igc_construct_skb_timestamps()…

--

igc: Fix RX HW timestamp reporting when NET_RX_BUSY_POLL is disabled

When CONFIG_NET_RX_BUSY_POLL is disabled, get_timestamp() cannot find the
driver through the NAPI id. It then falls back to
skb_hwtstamps(skb)->hwtstamp, which igc never fills in. This patch makes
igc_construct_skb() and igc_construct_skb_zc() put the Timer0 timestamp
into hwtstamp directly in that configuration, and keeps the
SKBTX_HW_TSTAMP_NETDEV / netdev_data scheme when BUSY_POLL is enabled.

> diff --git a/drivers/net/ethernet/intel/igc/igc_main.c b/drivers/net/ethernet/intel/igc/igc_main.c
> index 1fb5f3cbe93c5..95f7747b347b1 100644
> --- a/drivers/net/ethernet/intel/igc/igc_main.c
> +++ b/drivers/net/ethernet/intel/igc/igc_main.c
> @@ -1989,7 +1989,29 @@ static struct sk_buff *igc_build_skb(struct igc_ring *rx_ring,
>  	return skb;
>  }
>  
> -static struct sk_buff *igc_construct_skb(struct igc_ring *rx_ring,
> +static void igc_construct_skb_timestamps(struct igc_adapter *adapter,
> +					 struct sk_buff *skb,
> +					 struct igc_xdp_buff *ctx)
> +{
> +#ifndef CONFIG_NET_RX_BUSY_POLL
> +	struct igc_inline_rx_tstamps *tstamps;
> +#endif
> +
> +	if (!ctx->rx_ts)
> +		return;
> +
> +#ifndef CONFIG_NET_RX_BUSY_POLL
> +	tstamps = ctx->rx_ts;
> +	skb_hwtstamps(skb)->hwtstamp = igc_ptp_rx_pktstamp(adapter,
> +							   tstamps->timer0);

[Severity: High]
With CONFIG_NET_RX_BUSY_POLL=n, can sockets bound to a PTP virtual clock
with SOF_TIMESTAMPING_BIND_PHC still get wrong RX timestamps?

This branch always stores the Timer0 value and does not set
SKBTX_HW_TSTAMP_NETDEV. igc_get_tstamp() normally chooses the timer for
each socket:

drivers/net/ethernet/intel/igc/igc_main.c:igc_get_tstamp() {
    ...
	if (cycles)
		timestamp = igc_ptp_rx_pktstamp(adapter, tstamp->timer1);
	else
		timestamp = igc_ptp_rx_pktstamp(adapter, tstamp->timer0);
    ...
}

With the flag clear, __sock_recv_timestamp() uses hwtstamp as it is. For
BIND_PHC sockets it then converts the value:

net/socket.c:__sock_recv_timestamp() {
    ...
		if (tsflags & SOF_TIMESTAMPING_BIND_PHC)
			hwtstamp = ptp_convert_timestamp(&hwtstamp,
							 READ_ONCE(sk->sk_bind_phc));
    ...
}

ptp_convert_timestamp() passes the value to timecounter_cyc2time() as a
cycle count from the vclock's counter. For igc that counter is Timer1,
which igc_ptp_getcyclesx64() reads from IGC_SYSTIML_1/IGC_SYSTIMH_1.

igc provides getcyclesx64, so ptp_clock_freerun() returns false. That
means Timer0 can still be adjusted while vclocks exist, and Timer0 and
Timer1 can drift apart.

The Fixes: commit 069b142f5819 was added so that taprio/etf offload could
coexist with PTP vclocks. Does that use case stay broken when BUSY_POLL is
disabled?

[Severity: High]
This isn't a bug introduced by this patch, but the root cause in the
commit message seems to affect the other in-tree ndo_get_tstamp user,
tsnep, too. With CONFIG_NET_RX_BUSY_POLL=n, skb_napi_id() always returns 0:

include/linux/skbuff.h:skb_napi_id() {
#ifdef CONFIG_NET_RX_BUSY_POLL
	return skb->napi_id;
#else
	return 0;
#endif
}

So get_timestamp() in net/socket.c always falls back to
shhwtstamps->hwtstamp. tsnep_build_skb() in
drivers/net/ethernet/engleder/tsnep_main.c does:

		skb_shinfo(skb)->tx_flags |=
			SKBTX_HW_TSTAMP_NETDEV;
		memset(hwtstamps, 0, sizeof(*hwtstamps));
		hwtstamps->netdev_data = rx_inline;

Wouldn't tsnep then report the rx_inline kernel address as the RX
hardware timestamp, which is the same symptom described here? Once
hardware timestamping is enabled, an unprivileged socket using
SO_TIMESTAMPING with RX_HARDWARE|RAW_HARDWARE can receive this value.

The workaround here is an #ifndef inside igc only. Was a fix in
get_timestamp() in the core considered, so that tsnep is covered as well?

> +#else
> +	skb_shinfo(skb)->tx_flags |= SKBTX_HW_TSTAMP_NETDEV;
> +	skb_hwtstamps(skb)->netdev_data = ctx->rx_ts;

[Severity: High]
This is a pre-existing issue, but with CONFIG_NET_RX_BUSY_POLL=y this
branch stores only the kernel pointer ctx->rx_ts in the
netdev_data/hwtstamp union. hwtstamp never holds a real ktime. What
happens to code that reads ->hwtstamp without calling ndo_get_tstamp?

tpacket_get_timestamp() in net/packet/af_packet.c does not check
SKBTX_HW_TSTAMP_NETDEV:

	if (shhwtstamps &&
	    (flags & SOF_TIMESTAMPING_RAW_HARDWARE) &&
	    ktime_to_timespec64_cond(shhwtstamps->hwtstamp, ts))
		return TP_STATUS_TS_RAW_HARDWARE;

So it looks like a TPACKET_V2/V3 ring with PACKET_TIMESTAMP set to
SOF_TIMESTAMPING_RAW_HARDWARE (for example tcpdump -j adapter_unsynced)
gets the RX buffer's kernel address in tp_sec/tp_nsec. Opening such a
socket needs CAP_NET_RAW.

get_timestamp() in net/socket.c also falls back to shhwtstamps->hwtstamp
with BUSY_POLL=y when dev_get_by_napi_id() fails. That can happen if
igc_reset_q_vector() and netif_napi_del() have removed the NAPI before
recvmsg runs.

[Severity: High]
This is also a pre-existing issue, but can netdev_data outlive the buffer
it points to? ctx->rx_ts points into the RX page, or into the UMEM frame
on the XSK path. The skb takes no reference on the bytes that hold the
timestamp header. igc_get_tstamp() dereferences the pointer later, at
recvmsg time:

	tstamp = hwtstamps->netdev_data;

	if (cycles)
		timestamp = igc_ptp_rx_pktstamp(adapter, tstamp->timer1);
	...

On the non-ZC path, a frame of up to IGC_RX_HDR_LEN bytes is copied
entirely into the linear area, so igc_construct_skb() takes this branch:

	} else {
		rx_buffer->pagecnt_bias++;
	}

No frag is attached in that case. igc_put_rx_buffer() then either gives
the same page half back to hardware through igc_reuse_rx_page(), or
frees it with __page_frag_cache_drain() when the page is not reusable.
igc_clean_rx_ring() also frees the pages on ifdown.

On the XSK path, igc_clean_rx_irq_zc() frees the frame right after the
skb is built:

		case IGC_XDP_PASS:
			igc_dispatch_skb_zc(q_vector, desc, ctx);
			fallthrough;
		case IGC_XDP_CONSUMED:
			xsk_buff_free(bi->xdp);

Later, xdp_umem_addr_unmap() vunmaps the UMEM.

In these cases, could igc_get_tstamp() read another packet's timestamp
or the contents of a freed page, or fault on an unmapped UMEM address?
The new !BUSY_POLL branch reads the timestamp synchronously and avoids
this, but the BUSY_POLL=y branch keeps the old behavior.

> +#endif
> +}

[ ... ]

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

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

[netdev-bot+sashiko]

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/

> diff --git a/drivers/net/ethernet/intel/igc/igc_main.c b/drivers/net/ethernet/intel/igc/igc_main.c
> index 95f7747b347b1..b94a08791102c 100644
> --- a/drivers/net/ethernet/intel/igc/igc_main.c
> +++ b/drivers/net/ethernet/intel/igc/igc_main.c
> @@ -2658,7 +2658,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 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

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

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider.

Critical: 0 · High: 1 (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] The fix is incomplete.

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but the same
  unguarded strip exists in igb's AF_XDP zero-copy Rx path.

--

igb: only strip Rx timestamp header on the first buffer of a frame

When Rx hardware timestamping is enabled, igb_clean_rx_irq() tries to strip
the 16-byte timestamp header from every Rx buffer of a frame. A continuation
buffer whose payload starts with 8 zero bytes then loses 16 bytes of data.
The patch adds a !skb check so that the strip only runs on the first buffer
of a frame.

> diff --git a/drivers/net/ethernet/intel/igb/igb_main.c b/drivers/net/ethernet/intel/igb/igb_main.c
> index d4a897a8c82c6..5c09dc4a25660 100644
> --- a/drivers/net/ethernet/intel/igb/igb_main.c
> +++ b/drivers/net/ethernet/intel/igb/igb_main.c
> @@ -9069,8 +9069,11 @@ static int igb_clean_rx_irq(struct igb_q_vector *q_vector, const int budget)
>  		rx_buffer = igb_get_rx_buffer(rx_ring, size, &rx_buf_pgcnt);
>  		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)) {
> +		/* pull rx packet timestamp if available and valid; it is only
> +		 * present on the first buffer of a frame
> +		 */
> +		if (!skb &&
> +		    igb_test_staterr(rx_desc, E1000_RXDADV_STAT_TSIP)) {

[Severity: High]
Is !skb a reliable stand-in for "first buffer of a frame" when an XDP
program is attached?

The commit message says "skb is NULL until the first buffer has been
processed". However, when igb_run_xdp() returns a verdict other than PASS
(DROP, ABORTED, TX or REDIRECT) for the first buffer of a multi-descriptor
frame, skb is never assigned:

igb_clean_rx_irq() {
    ...
		if (xdp_res) {
			if (xdp_res & (IGB_XDP_TX | IGB_XDP_REDIR)) {
				xdp_xmit |= xdp_res;
				igb_rx_buffer_flip(rx_ring, rx_buffer, size);
			} else {
				rx_buffer->pagecnt_bias++;
			}
    ...
		if (igb_is_non_eop(rx_ring, rx_desc))
			continue;
    ...
}

So the continuation descriptor goes through the loop with skb still NULL.
If the budget runs out mid-frame, rx_ring->skb = NULL carries that state
into the next poll.

On 82580 and newer, any Rx timestamp filter is upgraded to
HWTSTAMP_FILTER_ALL. Per the commit message, continuation descriptors also
carry TSIP. That means the !skb && TSIP check passes on payload, and
igb_ptp_rx_pktstamp() returns IGB_TS_HDR_LEN whenever the first 8 payload
bytes are zero.

Won't that still strip 16 bytes from the continuation buffer?

Now suppose the final fragment is shorter than 16 bytes, for example 8 zero
bytes chosen by the sender. Can size -= ts_hdr_len wrap the unsigned size?
xdp_prepare_buff() takes an int data_len, so data_end would end up before
data:

include/net/xdp.h:xdp_prepare_buff() {
    ...
	xdp->data_end = data + data_len;
    ...
}

On XDP_PASS, igb_build_skb() then does:

	__skb_put(skb, xdp->data_end - xdp->data);

As an unsigned length that is roughly 4 GiB. With legacy-rx,
igb_construct_skb() passes a frag of similar size to skb_add_rx_frag(). On
XDP_TX/REDIRECT, the bogus length goes into the xdp_frame and into
igb_rx_buffer_flip().

Multi-descriptor frames do seem to reach XDP rings:

- igb_xdp_setup() and igb_change_mtu() only check
  mtu + IGB_ETH_PKT_HDR_PAD against igb_rx_bufsz(). That check ignores the
  16-byte in-buffer timestamp that igb_setup_srrctl() enables with
  E1000_SRRCTL_TIMESTAMP.
- igb_setup_rctl() always sets E1000_RCTL_LPE.
- igb_set_rx_mode() leaves RLPML at MAX_JUMBO_FRAME_SIZE when the MTU is
  above about 1514 on 4K pages, when VFs are allocated, or when
  PAGE_SIZE >= 8192.

The underflow was already possible on this path before this patch, and the
patch doesn't change it. Still, the continuation-buffer strip that this
commit sets out to fix looks reachable here.

Would explicit first-buffer state work better? Two options are a flag set
at frame start and cleared after the first descriptor, or discarding the
rest of the frame once XDP has consumed the head. Checking that
size >= ts_hdr_len before the subtraction might also help.

[Severity: High]
This isn't a bug introduced by this patch, but the AF_XDP zero-copy Rx path
has the same unguarded strip, and this patch only changes
igb_clean_rx_irq():

drivers/net/ethernet/intel/igb/igb_xsk.c:igb_clean_rx_irq_zc() {
    ...
		xsk_buff_set_size(xdp, size);
		...
		if (igb_test_staterr(rx_desc, E1000_RXDADV_STAT_TSIP)) {
			int ts_hdr_len;

			ts_hdr_len = igb_ptp_rx_pktstamp(rx_ring->q_vector,
							 xdp->data,
							 &timestamp);

			xdp->data += ts_hdr_len;
			xdp->data_meta += ts_hdr_len;
			size -= ts_hdr_len;
		}
    ...
}

This code runs on every TSIP descriptor. It has no first-buffer check and
no size >= IGB_TS_HDR_LEN check, and it treats each descriptor as its own
frame.

In ZC mode, igb_setup_srrctl() sizes the hardware buffer from
xsk_pool_get_rx_frame_size(), rounded down to 1 KB. With 2K chunks, a
normal 1500-byte frame therefore spans more than one descriptor.

Can a zero-prefixed final fragment shorter than 16 bytes push xdp->data and
xdp->data_meta past data_end here?

On XDP_PASS, igb_construct_skb_zc() does:

	unsigned int totalsize = xdp->data_end - xdp->data_meta;
	...
	skb = napi_alloc_skb(&rx_ring->q_vector->napi, totalsize);
	...
	memcpy(__skb_put(skb, totalsize), xdp->data_meta,
	       ALIGN(totalsize, sizeof(long)));

For an 8-byte fragment, totalsize would be 0xFFFFFFF8. napi_alloc_skb()
adds NET_SKB_PAD + NET_IP_ALIGN in unsigned int, which wraps to a small
value, so the allocation succeeds. For fragments of 1 to 8 bytes, the
memcpy length then stays at 0xFFFFFFF8.

Would that overflow the skb head?

For fragments of 9 to 15 bytes, the copy length wraps to 0, but the skb
still ends up with a len of about 4 GiB. Fragments of 16 bytes or more lose
16 bytes of payload, the same data loss described in this commit message.

The kernel-doc for igb_ptp_rx_pktstamp() also says it is meant to read the
timestamp from the first buffer of an incoming frame, and the ZC caller
doesn't follow that.

[ ... ]

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

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

[Paul Moses]

On Monday, October 5th, 2026 at 6:02 PM, netdev-bot+sashiko@kernel.org <netdev-bot+sashiko@kernel.org> wrote:

> 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/
> 
> > diff --git a/drivers/net/ethernet/intel/igc/igc_main.c b/drivers/net/ethernet/intel/igc/igc_main.c
> > index 95f7747b347b1..b94a08791102c 100644
> > --- a/drivers/net/ethernet/intel/igc/igc_main.c
> > +++ b/drivers/net/ethernet/intel/igc/igc_main.c
> > @@ -2658,7 +2658,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 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
> 

This seems maybe legit, but considering the fixes would be entirely independent 
of one another and that this is a theoretical scenario affecting exotic system 
configurations, I'd rather not entangle it with a real user facing issue.

I have some idea and maybe hardware to try to reproduce, but nothing setup 
currently.

More appropriate to be a followup patch. 

Thanks
Paul

Re: [PATCH net 1/6] igc: Fix RX HW timestamp reporting when NET_RX_BUSY_POLL is disabled

[Jakub Kicinski]

On Thu,  1 Oct 2026 15:24:34 -0700 Tony Nguyen wrote:
> When CONFIG_NET_RX_BUSY_POLL is deactivated, fetching RX HW timestamps
> from the NIC no longer works as expected, often resulting in incorrect
> or negative values such as "HW raw -121948.050407424".
> 
> This occurs because disabling CONFIG_NET_RX_BUSY_POLL disables the
> SKB NAPI mapping in __skb_mark_napi_id(). Consequently, get_timestamp()
> fails to perform its driver lookup, and the igc driver's struct
> net_device_ops::ndo_get_tstamp is never invoked.
> 
> Instead, get_timestamp() falls back to use shhwtstamps(skb)->hwtstamp,
> a field that the driver has not populated. This results in incorrect
> timestamps.
> 
> Fix this by populating the hwtstamp field with the correct timestamp
> in the default timer when CONFIG_NET_RX_BUSY_POLL is disabled.
> The "igc_adapter" is passed to igc_construct_skb() to enable
> igc_ptp_rx_pktstamp() to access the necessary adapter details for
> adjusting the timestamp.

looks like a driver workaround for a generic problem
a better fix is likely needed here