[PATCH net v4 0/2] igb: only strip the Rx timestamp header from the first buffer of a frame

[PATCH net v4 0/2] igb: only strip the Rx timestamp header from the first buffer of a frame

[Tjerk Kusters via B4 Relay]

With Rx hardware timestamping enabled (e.g. by ptp4l), igb strips the
16-byte inline timestamp header from every Rx buffer of a frame instead
of only from the first one. A continuation buffer that starts with 8
zero bytes loses 16 bytes, so jumbo frames with zero payload arrive
short and are dropped.

v3 used skb == NULL to find the first buffer of a frame. The Sashiko
AI review pointed out that this does not hold when an XDP program
consumes the first buffer of a frame that spans several buffers, so v3
was sent back for rework.

The same question came up for igc. There, Maciej suggested not to track
frame boundaries in the data path, but to make sure that frames never
span several buffers while XDP is attached [1]. Paul Moses' igc v3 does
that [2]. This version follows the same approach for igb:

  1/2 limits the receive length (RLPML, or the PF pool's VMOLR with VFs)
      to the configured frame size while XDP is attached, and leaves
      room for the timestamp header in the XDP buffer size checks.
  2/2 is v3, unchanged in code, with a note that it relies on 1/2.
      The Reviewed-by and Tested-by tags of v3 are kept for that reason.

Not addressed here: the AF_XDP zero-copy Rx path. igb_clean_rx_irq_zc()
has the same unguarded strip, and with 2 KiB UMEM chunks the hardware
buffer is rounded down to 1 KiB, so frames span descriptors there. The
igc counterpart is Maciej's buffer-size patch in [2]. I do not use
AF_XDP and cannot test it, so I would prefer to leave that to someone
who can.

How to reproduce
----------------
Receiver with igb (eth0), peer with MTU 8228. hwstamp_ctl turns on Rx
timestamping with filter ALL, no ptp4l needed; this ping method is
taken from Paul's igc report. One host with two ports also works if
the sending port is moved into its own network namespace.

1) Truncated jumbo frames (patch 2)

  receiver:  ip link set eth0 mtu 8228
             hwstamp_ctl -i eth0 -r 1
  peer:      ping -c 5 -M do -s 8200 -p 00 <receiver>

  Without the series there are no replies, and IpExtInTruncatedPkts
  goes up by one per frame. With the series all requests are answered.

2) XDP consumes the first buffer (patch 1)

  receiver:  ip link set eth0 mtu 3000
             hwstamp_ctl -i eth0 -r 1
             xdp-filter load -m native eth0
             xdp-filter ip <peer> -m src
             tcpdump -nn -e -i eth0 'ether src 00:00:00:00:00:00'
  peer:      ping -c 3 -M do -s 8200 -p 00 <receiver>

  At MTU 3000 XDP can attach, but without patch 1 the MAC still accepts
  the 8242-byte frames, which then span three Rx buffers. xdp-filter
  drops the first one, and the rest reaches the stack as a separate
  frame with all-zero MAC addresses:

    without the series    5154 bytes (header stripped twice)
    v3 alone              5170 bytes (stripped once)
    this series           no frame; rx_long_length_errors +1 per frame

Tested on an I350, kernel 6.14.9 with these patches, with the steps
above: results as listed. The v3 line is from an earlier run with an
XDP program dropping ICMP. Also checked: XDP attach at MTU 3030 works,
at MTU 3046 it is refused, as is an MTU change to 3046 with XDP
attached. With two VFs allocated (booted with pci=realloc), step 2
gives the same picture: three leftover frames without the series, none
with it, as the PF pool limit in VMOLR drops the frames.
Not tested: PAGE_SIZE >= 8192.

The patches, their descriptions and this cover letter were prepared
with the help of an AI coding assistant (Claude). I reviewed them and
ran the tests above on my test bench.

[1] https://lore.kernel.org/all/20261025065154.2037149-1-p@1g4.org/
[2] https://lore.kernel.org/all/20261028083344.3047350-1-p@1g4.org/

Changes in v4:
- New patch 1: limit the receive length while XDP is attached, so that
  skb == NULL reliably marks the first buffer (Sashiko review), as
  done for igc.
- Patch 2: no code change; added a note that it relies on patch 1.
- Link to v3: https://patch.msgid.link/20260625-igb-rx-ts-fix-v3-1-99b3efa08dca@aweta.nl

---
Tjerk Kusters (2):
      igb: do not let XDP frames span more than one Rx buffer
      igb: only strip Rx timestamp header on the first buffer of a frame

 drivers/net/ethernet/intel/igb/igb_main.c | 37 ++++++++++++++++++++++---------
 1 file changed, 26 insertions(+), 11 deletions(-)
---
base-commit: 2d3090a8aeb596a26935db0955d46c9a5db5c6ce
change-id: 20260619-igb-rx-ts-fix-cd70585ee316

Best regards,
--  
Tjerk Kusters <tkusters@aweta.nl>

[PATCH net v4 1/2] igb: do not let XDP frames span more than one Rx buffer

[Tjerk Kusters via B4 Relay]

From: Tjerk Kusters <tkusters@aweta.nl>

igb does not support XDP multi-buffer: igb_xdp_setup() only accepts an
MTU whose frame fits in one Rx buffer, and the XDP program only sees
the first buffer of a frame. The maximum receive length is not limited
accordingly, though. RLPML is only lowered when the frame fits a build_skb
buffer on systems with PAGE_SIZE < 8192 and no VFs, and the PF pool limit
in VMOLR is set the same way. Otherwise the MAC still accepts frames of
up to MAX_JUMBO_FRAME_SIZE while XDP is attached, for example at MTU
3000, at any MTU when PAGE_SIZE >= 8192, or when VFs are allocated.

Such frames span several descriptors. When the program does not return
XDP_PASS for the first buffer, no skb is built, and igb_clean_rx_irq()
handles the next buffer of the same frame as the start of a new frame:
it runs the program on it and may pass it to the stack as a bogus frame
made from the middle of the original payload.

Limit the receive length to the configured maximum frame size while XDP
is attached: in RLPML when no VFs are allocated, and in the PF pool's
VMOLR otherwise, as RLPML then has to stay large for the VFs. Larger
frames are dropped by the MAC. Also leave room for the inline Rx
timestamp header when checking that the frame fits in one Rx buffer, on
XDP setup and on MTU change; this lowers the largest MTU accepted with
XDP on 3 KiB buffers by 16 bytes.

Both registers are programmed from igb_set_rx_mode(), which
igb_configure() calls, and attaching or removing an XDP program on a
running interface goes through igb_close() and igb_open(), as does an
MTU change.

This follows the same approach as the igc fix that limits RLPML while
XDP is attached.

Tested on an I350 at MTU 3000 with Rx timestamping on and xdp-filter
dropping all IPv4 from the peer: 8242-byte frames are now dropped by the
MAC (rx_long_length_errors), instead of their last two buffers being
delivered as a separate frame. XDP attach at MTU 3030 still works, at
MTU 3046 it is refused, as is an MTU change from 3030 to 3046 with XDP
attached.

With two VFs allocated, the same frames are counted by the MAC but
dropped by the PF pool limit in VMOLR before they reach the Rx queue;
without this patch their last two buffers are again delivered as a
separate frame.

Link: https://lore.kernel.org/all/20261028083344.3047350-3-p@1g4.org/
Fixes: 9cbc948b5a20 ("igb: add XDP support")
Cc: stable@vger.kernel.org
Assisted-by: Claude:claude-opus-5-5
Signed-off-by: Tjerk Kusters <tkusters@aweta.nl>
---
 drivers/net/ethernet/intel/igb/igb_main.c | 30 +++++++++++++++++++++---------
 1 file changed, 21 insertions(+), 9 deletions(-)

diff --git a/drivers/net/ethernet/intel/igb/igb_main.c b/drivers/net/ethernet/intel/igb/igb_main.c
index ce91dda00ec0..5bf489cd7b06 100644
--- a/drivers/net/ethernet/intel/igb/igb_main.c
+++ b/drivers/net/ethernet/intel/igb/igb_main.c
@@ -2894,11 +2894,13 @@ static int igb_xdp_setup(struct net_device *dev, struct netdev_bpf *bpf)
 	bool running = netif_running(dev);
 	bool need_reset;
 
-	/* verify igb ring attributes are sufficient for XDP */
+	/* verify igb ring attributes are sufficient for XDP, leaving room
+	 * for the inline Rx timestamp header
+	 */
 	for (i = 0; i < adapter->num_rx_queues; i++) {
 		struct igb_ring *ring = adapter->rx_ring[i];
 
-		if (frame_size > igb_rx_bufsz(ring)) {
+		if (frame_size + IGB_TS_HDR_LEN > igb_rx_bufsz(ring)) {
 			NL_SET_ERR_MSG_MOD(bpf->extack,
 					   "The RX buffer size is too small for the frame size");
 			netdev_warn(dev, "XDP RX buffer size %d is too small for the frame size %d\n",
@@ -5324,6 +5326,7 @@ static void igb_set_rx_mode(struct net_device *netdev)
 	struct e1000_hw *hw = &adapter->hw;
 	unsigned int vfn = adapter->vfs_allocated_count;
 	u32 rctl = 0, vmolr = 0, rlpml = MAX_JUMBO_FRAME_SIZE;
+	u32 pool_rlpml;
 	int count;
 
 	/* Check for Promiscuous and All Multicast modes */
@@ -5386,6 +5389,12 @@ static void igb_set_rx_mode(struct net_device *netdev)
 			rlpml = IGB_MAX_FRAME_BUILD_SKB;
 	}
 #endif
+	/* XDP does not support frames that span more than one Rx buffer.
+	 * With VFs the global limit stays large and the PF pool is limited
+	 * through VMOLR below.
+	 */
+	if (!adapter->vfs_allocated_count && igb_xdp_is_enabled(adapter))
+		rlpml = min_t(u32, rlpml, adapter->max_frame_size);
 	wr32(E1000_RLPML, rlpml);
 
 	/* In order to support SR-IOV and eventually VMDq it is necessary to set
@@ -5402,15 +5411,18 @@ static void igb_set_rx_mode(struct net_device *netdev)
 	vmolr |= rd32(E1000_VMOLR(vfn)) &
 		 ~(E1000_VMOLR_ROPE | E1000_VMOLR_MPME | E1000_VMOLR_ROMPE);
 
-	/* enable Rx jumbo frames, restrict as needed to support build_skb */
-	vmolr &= ~E1000_VMOLR_RLPML_MASK;
+	/* enable Rx jumbo frames, restrict as needed to support build_skb
+	 * and XDP
+	 */
+	pool_rlpml = MAX_JUMBO_FRAME_SIZE;
 #if (PAGE_SIZE < 8192)
 	if (adapter->max_frame_size <= IGB_MAX_FRAME_BUILD_SKB)
-		vmolr |= IGB_MAX_FRAME_BUILD_SKB;
-	else
+		pool_rlpml = IGB_MAX_FRAME_BUILD_SKB;
 #endif
-		vmolr |= MAX_JUMBO_FRAME_SIZE;
-	vmolr |= E1000_VMOLR_LPE;
+	if (igb_xdp_is_enabled(adapter))
+		pool_rlpml = min_t(u32, pool_rlpml, adapter->max_frame_size);
+	vmolr &= ~E1000_VMOLR_RLPML_MASK;
+	vmolr |= pool_rlpml | E1000_VMOLR_LPE;
 
 	wr32(E1000_VMOLR(vfn), vmolr);
 
@@ -6713,7 +6725,7 @@ static int igb_change_mtu(struct net_device *netdev, int new_mtu)
 		for (i = 0; i < adapter->num_rx_queues; i++) {
 			struct igb_ring *ring = adapter->rx_ring[i];
 
-			if (max_frame > igb_rx_bufsz(ring)) {
+			if (max_frame + IGB_TS_HDR_LEN > igb_rx_bufsz(ring)) {
 				netdev_warn(adapter->netdev,
 					    "Requested MTU size is not supported with XDP. Max frame size is %d\n",
 					    max_frame);

-- 
2.27.0

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

[Tjerk Kusters via B4 Relay]

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.

This relies on the previous patch. Without it, an XDP program that does
not return XDP_PASS for the first buffer of a multi-buffer frame leaves
skb NULL for the rest of that frame. With the receive length limited
while XDP is attached, such frames no longer reach the XDP path.

igc has the same bug and is fixed the same way.

Link: https://lore.kernel.org/all/20261028083344.3047350-2-p@1g4.org/
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>
Tested-by: Alexander Nowlin <alexander.nowlin@intel.com>
Assisted-by: Claude:claude-opus-5-5
Signed-off-by: Tjerk Kusters <tkusters@aweta.nl>
---
 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 5bf489cd7b06..c4eb9f589fbe 100644
--- a/drivers/net/ethernet/intel/igb/igb_main.c
+++ b/drivers/net/ethernet/intel/igb/igb_main.c
@@ -9072,8 +9072,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.27.0

Re: [PATCH net v4 0/2] igb: only strip the Rx timestamp header from the first buffer of a frame

[Maciej Fijalkowski]

On Mon, Nov 02, 2026 at 10:58:12AM +0100, Tjerk Kusters via B4 Relay wrote:
> With Rx hardware timestamping enabled (e.g. by ptp4l), igb strips the
> 16-byte inline timestamp header from every Rx buffer of a frame instead
> of only from the first one. A continuation buffer that starts with 8
> zero bytes loses 16 bytes, so jumbo frames with zero payload arrive
> short and are dropped.
> 
> v3 used skb == NULL to find the first buffer of a frame. The Sashiko
> AI review pointed out that this does not hold when an XDP program
> consumes the first buffer of a frame that spans several buffers, so v3
> was sent back for rework.
> 
> The same question came up for igc. There, Maciej suggested not to track
> frame boundaries in the data path, but to make sure that frames never
> span several buffers while XDP is attached [1]. Paul Moses' igc v3 does
> that [2]. This version follows the same approach for igb:
> 
>   1/2 limits the receive length (RLPML, or the PF pool's VMOLR with VFs)
>       to the configured frame size while XDP is attached, and leaves
>       room for the timestamp header in the XDP buffer size checks.
>   2/2 is v3, unchanged in code, with a note that it relies on 1/2.
>       The Reviewed-by and Tested-by tags of v3 are kept for that reason.
> 
> Not addressed here: the AF_XDP zero-copy Rx path. igb_clean_rx_irq_zc()
> has the same unguarded strip, and with 2 KiB UMEM chunks the hardware
> buffer is rounded down to 1 KiB, so frames span descriptors there. The
> igc counterpart is Maciej's buffer-size patch in [2]. I do not use
> AF_XDP and cannot test it, so I would prefer to leave that to someone
> who can.

I assume review bots will give you a hard time if you don't address this
within your series. Seems you have HW handy plus you do use LLM so I don't
see why you couldn't ask it to address AF_XDP zc rx ?

FWIW blessed user space tool has always been xdpsock:
https://github.com/xdp-project/bpf-examples/tree/main/AF_XDP-example

so you can ask llm to run this in rxdrop mode on receiver side and feed it
with some traffic from peer side and then see that frames were not
fragmented.

It's easier for you to cover this as you have HW.

> 
> How to reproduce
> ----------------
> Receiver with igb (eth0), peer with MTU 8228. hwstamp_ctl turns on Rx
> timestamping with filter ALL, no ptp4l needed; this ping method is
> taken from Paul's igc report. One host with two ports also works if
> the sending port is moved into its own network namespace.
> 
> 1) Truncated jumbo frames (patch 2)
> 
>   receiver:  ip link set eth0 mtu 8228
>              hwstamp_ctl -i eth0 -r 1
>   peer:      ping -c 5 -M do -s 8200 -p 00 <receiver>
> 
>   Without the series there are no replies, and IpExtInTruncatedPkts
>   goes up by one per frame. With the series all requests are answered.
> 
> 2) XDP consumes the first buffer (patch 1)
> 
>   receiver:  ip link set eth0 mtu 3000
>              hwstamp_ctl -i eth0 -r 1
>              xdp-filter load -m native eth0
>              xdp-filter ip <peer> -m src
>              tcpdump -nn -e -i eth0 'ether src 00:00:00:00:00:00'
>   peer:      ping -c 3 -M do -s 8200 -p 00 <receiver>
> 
>   At MTU 3000 XDP can attach, but without patch 1 the MAC still accepts
>   the 8242-byte frames, which then span three Rx buffers. xdp-filter
>   drops the first one, and the rest reaches the stack as a separate
>   frame with all-zero MAC addresses:
> 
>     without the series    5154 bytes (header stripped twice)
>     v3 alone              5170 bytes (stripped once)
>     this series           no frame; rx_long_length_errors +1 per frame
> 
> Tested on an I350, kernel 6.14.9 with these patches, with the steps
> above: results as listed. The v3 line is from an earlier run with an
> XDP program dropping ICMP. Also checked: XDP attach at MTU 3030 works,
> at MTU 3046 it is refused, as is an MTU change to 3046 with XDP
> attached. With two VFs allocated (booted with pci=realloc), step 2
> gives the same picture: three leftover frames without the series, none
> with it, as the PF pool limit in VMOLR drops the frames.
> Not tested: PAGE_SIZE >= 8192.
> 
> The patches, their descriptions and this cover letter were prepared
> with the help of an AI coding assistant (Claude). I reviewed them and
> ran the tests above on my test bench.
> 
> [1] https://lore.kernel.org/all/20261025065154.2037149-1-p@1g4.org/
> [2] https://lore.kernel.org/all/20261028083344.3047350-1-p@1g4.org/
> 
> Changes in v4:
> - New patch 1: limit the receive length while XDP is attached, so that
>   skb == NULL reliably marks the first buffer (Sashiko review), as
>   done for igc.
> - Patch 2: no code change; added a note that it relies on patch 1.
> - Link to v3: https://patch.msgid.link/20260625-igb-rx-ts-fix-v3-1-99b3efa08dca@aweta.nl
> 
> ---
> Tjerk Kusters (2):
>       igb: do not let XDP frames span more than one Rx buffer
>       igb: only strip Rx timestamp header on the first buffer of a frame
> 
>  drivers/net/ethernet/intel/igb/igb_main.c | 37 ++++++++++++++++++++++---------
>  1 file changed, 26 insertions(+), 11 deletions(-)
> ---
> base-commit: 2d3090a8aeb596a26935db0955d46c9a5db5c6ce
> change-id: 20260619-igb-rx-ts-fix-cd70585ee316
> 
> Best regards,
> --  
> Tjerk Kusters <tkusters@aweta.nl>
> 
> 

RE: [PATCH net v4 0/2] igb: only strip the Rx timestamp header from the first buffer of a frame

[Tjerk Kusters]

> Not addressed here: the AF_XDP zero-copy Rx path.
> igb_clean_rx_irq_zc() has the same unguarded strip, and with 2 KiB
> UMEM chunks the hardware buffer is rounded down to 1 KiB, so frames
> span descriptors there. The igc counterpart is Maciej's buffer-size
> patch in [2]. I do not use AF_XDP and cannot test it, so I would
> prefer to leave that to someone who can.

> I assume review bots will give you a hard time if you don't address
> this within your series. Seems you have HW handy plus you do use LLM
> so I don't see why you couldn't ask it to address AF_XDP zc rx ?
>
> FWIW blessed user space tool has always been xdpsock:
> https://github.com/xdp-project/bpf-examples/tree/main/AF_XDP-example
>
> so you can ask llm to run this in rxdrop mode on receiver side and
> feed it with some traffic from peer side and then see that frames
> were not fragmented.


OK, fair point. I will look into it, and I will hold v5 until that is done and tested.

Thanks,
Tjerk

Re: [PATCH net v4 1/2] igb: do not let XDP frames span more than one Rx buffer

[netdev-bot+sashiko]

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

Critical: 0 · High: 4 (1) · Medium: 2 · 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] This patch breaks XDP at the default MTU of 1500 on 4K-page
  systems, and can leave a program that cannot be removed.
- [High] The fix misses AF_XDP zero-copy rings.
- [High] Turning on the legacy-rx private flag while XDP is attached
  brings back multi-buffer XDP frames.
- [High] With rx-all enabled, RCTL.SBP defeats the new hardware length
  limit.
- [Medium] During XDP detach, the hardware limit can be dropped while the
  rings still run the old program.
- [Medium] Lowering the MTU gets around the new timestamp-aware check in
  igb_change_mtu(), which validates the new max_frame against the ring…

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but a malicious VF
  can set arbitrary VMOLR bits for its own pool.

--

igb: do not let XDP frames span more than one Rx buffer

While XDP is attached, this patch caps the maximum receive length at
max_frame_size. The cap goes in RLPML when no VFs are allocated, and in
the PF pool's VMOLR otherwise, so the MAC drops frames that would span
several descriptors. It also adds IGB_TS_HDR_LEN to the one-buffer
checks in igb_xdp_setup() and igb_change_mtu().

Link: https://lore.kernel.org/all/20261028083344.3047350-3-p@1g4.org/

> diff --git a/drivers/net/ethernet/intel/igb/igb_main.c b/drivers/net/ethernet/intel/igb/igb_main.c
> index ce91dda00ec0e..5bf489cd7b066 100644
> --- a/drivers/net/ethernet/intel/igb/igb_main.c
> +++ b/drivers/net/ethernet/intel/igb/igb_main.c
> @@ -2894,11 +2894,13 @@ static int igb_xdp_setup(struct net_device *dev, struct netdev_bpf *bpf)
>  	bool running = netif_running(dev);
>  	bool need_reset;
>  
> -	/* verify igb ring attributes are sufficient for XDP */
> +	/* verify igb ring attributes are sufficient for XDP, leaving room
> +	 * for the inline Rx timestamp header
> +	 */
>  	for (i = 0; i < adapter->num_rx_queues; i++) {
>  		struct igb_ring *ring = adapter->rx_ring[i];
>  
> -		if (frame_size > igb_rx_bufsz(ring)) {
> +		if (frame_size + IGB_TS_HDR_LEN > igb_rx_bufsz(ring)) {

[Severity: High]
Does this refuse XDP at the default MTU of 1500 on 4K page systems?

At MTU 1500, max_frame_size is 1500 + IGB_ETH_PKT_HDR_PAD = 1526. That
is <= IGB_MAX_FRAME_BUILD_SKB, so igb_set_rx_buffer_len() sets up
build_skb rings without large buffers. igb_rx_bufsz() then returns
IGB_MAX_FRAME_BUILD_SKB (1536 - NET_IP_ALIGN):

igb_rx_bufsz() {
	...
	if (ring_uses_build_skb(ring))
		return IGB_MAX_FRAME_BUILD_SKB;
	...
}

The new check is 1526 + 16 = 1542 > 1536. So attaching XDP to an
interface that has been opened now fails with -EINVAL, where it passed
before. This seems to affect every MTU from about 1495 to 1510.
igb_change_mtu() also refuses those MTUs while XDP is attached.

The check also runs before the xchg() when bpf->prog is NULL. Can a
program get stuck attached? There are two ways to get there:

  - attach while the interface has never been opened (the ring flags
    are clear, so the 2048 check passes), then bring it up

  - attach at MTU 3000 on 3072 rings, then set MTU 1500.
    igb_change_mtu() checks against the old 3072 rings, and igb_up()
    then switches them to 1536 build_skb rings

After that, "ip link set xdp off" fails. On unregister,
dev_xdp_uninstall() hits:

	WARN_ON(dev_xdp_install(dev, mode, bpf_op, NULL, 0, NULL));

and the reference held in adapter->xdp_prog is never dropped.

The extra 16 bytes do seem to be needed for the skb layout:
igb_skb_pad() gives all the slack to headroom, which leaves exactly
1536 bytes before skb_shared_info.

Could igb_set_rx_buffer_len() instead pick a layout with room for the
timestamp, rather than refusing the default configuration and blocking
removal? For example, it could use 3K buffers when XDP is attached and
max_frame_size + IGB_TS_HDR_LEN > IGB_MAX_FRAME_BUILD_SKB.

The commit message only mentions a 16-byte reduction on 3 KiB buffers,
and none of the tests described ran at MTU 1500.

>  			NL_SET_ERR_MSG_MOD(bpf->extack,
>  					   "The RX buffer size is too small for the frame size");
>  			netdev_warn(dev, "XDP RX buffer size %d is too small for the frame size %d\n",

[ ... ]

> @@ -5386,6 +5389,12 @@ static void igb_set_rx_mode(struct net_device *netdev)
>  			rlpml = IGB_MAX_FRAME_BUILD_SKB;
>  	}
>  #endif
> +	/* XDP does not support frames that span more than one Rx buffer.
> +	 * With VFs the global limit stays large and the PF pool is limited
> +	 * through VMOLR below.
> +	 */
> +	if (!adapter->vfs_allocated_count && igb_xdp_is_enabled(adapter))
> +		rlpml = min_t(u32, rlpml, adapter->max_frame_size);

[Severity: High]
Are AF_XDP zero-copy rings covered by this limit?

For a ring with an XSK pool, igb_setup_srrctl() programs the pool frame
size rounded down to whole KiB:

	srrctl |= buf_size >> E1000_SRRCTL_BSIZEPKT_SHIFT;

With 2048-byte UMEM chunks, the frame size is
2048 - XDP_PACKET_HEADROOM = 1792, which gives a 1 KiB hardware buffer.
xp_assign_dev() accepts that pool at MTU 1500, and
igb_xsk_pool_enable() does no size check.

The limit here is max_frame_size (1526), so frames from 1025 to 1526
bytes still span two descriptors. igb_rx_bufsz() has no xsk_pool case,
so the checks in igb_xdp_setup() and igb_change_mtu() don't see that
buffer size either.

igb_clean_rx_irq_zc() never checks E1000_RXD_STAT_EOP. It runs
igb_run_xdp_zc() on each descriptor as if it were a full frame. On
XDP_PASS it builds an skb from the continuation buffer with
igb_construct_skb_zc(), or it redirects that buffer to the socket.

Doesn't that leave the bogus-frame problem from the commit title in
place for zero-copy?

[Severity: High]
What happens if the legacy-rx private flag is turned on while XDP is
attached?

igb_set_priv_flags() sets IGB_FLAG_RX_LEGACY and calls
igb_reinit_locked() without checking XDP or the MTU.
igb_set_rx_buffer_len() then returns early, which leaves 2048-byte
buffers:

	if (adapter->flags & IGB_FLAG_RX_LEGACY)
		return;

RLPML here, or the PF VMOLR, is still min(..., max_frame_size). For
example, attach XDP at MTU 3000 on 3072 rings (3026 + 16 <= 3072
passes), then run:

  ethtool --set-priv-flags ethX legacy-rx on

RLPML is now 3026 while the buffers are 2048, so a 3026-byte frame
takes two descriptors. In igb_clean_rx_irq(), if the first buffer gets
a non-PASS verdict, the second buffer is handled as a new frame.

The same happens if legacy-rx is turned on while the interface is down,
after it ran with 3K buffers. igb_xdp_setup() checks against the stale
large-buffer flag and passes, and igb_open() then builds 2048-byte
legacy rings.

Should the limit follow the buffer size the rings actually use? Or
should legacy-rx be refused while XDP is attached?

[Severity: High]
Does rx-all get around this limit?

When NETIF_F_RXALL is set, igb_setup_rctl() sets E1000_RCTL_SBP:

	rctl |= (E1000_RCTL_SBP | /* Receive bad packets */

With SBP set, the MAC stores frames longer than RLPML instead of
dropping them. Commit bb5ed01cd242 ("igb: set max size RX buffer when
store bad packet is enabled") already notes this, and
igb_set_rx_buffer_len() forces 3K buffers for that reason. Nothing
stops RXALL and XDP from being enabled together.

Frames larger than 3072 bytes then span several descriptors. In
igb_clean_rx_irq(), igb_run_xdp() runs on the first buffer before
igb_is_non_eop() and igb_cleanup_headers() are reached. On a non-PASS
verdict, the continuation buffer is handled as a new frame.

Would it make sense to refuse XDP while RXALL is on, to clear SBP while
XDP is attached, or to drop buffers until EOP after a non-PASS verdict?

>  	wr32(E1000_RLPML, rlpml);
>  
>  	/* In order to support SR-IOV and eventually VMDq it is necessary to set
> @@ -5402,15 +5411,18 @@ static void igb_set_rx_mode(struct net_device *netdev)
>  	vmolr |= rd32(E1000_VMOLR(vfn)) &
>  		 ~(E1000_VMOLR_ROPE | E1000_VMOLR_MPME | E1000_VMOLR_ROMPE);
>  
> -	/* enable Rx jumbo frames, restrict as needed to support build_skb */
> -	vmolr &= ~E1000_VMOLR_RLPML_MASK;
> +	/* enable Rx jumbo frames, restrict as needed to support build_skb
> +	 * and XDP
> +	 */
> +	pool_rlpml = MAX_JUMBO_FRAME_SIZE;
>  #if (PAGE_SIZE < 8192)
>  	if (adapter->max_frame_size <= IGB_MAX_FRAME_BUILD_SKB)
> -		vmolr |= IGB_MAX_FRAME_BUILD_SKB;
> -	else
> +		pool_rlpml = IGB_MAX_FRAME_BUILD_SKB;
>  #endif
> -		vmolr |= MAX_JUMBO_FRAME_SIZE;
> -	vmolr |= E1000_VMOLR_LPE;
> +	if (igb_xdp_is_enabled(adapter))
> +		pool_rlpml = min_t(u32, pool_rlpml, adapter->max_frame_size);

[Severity: Medium]
Can the limit be lifted during XDP detach while the rings are still
running the old program?

This check and the RLPML one above read adapter->xdp_prog. The Rx path
runs rx_ring->xdp_prog, which is only refreshed on open. igb_xdp_setup()
publishes NULL before igb_close() stops Rx:

	old_prog = xchg(&adapter->xdp_prog, prog);
	...
	if (need_reset && running) {
		igb_close(dev);

igb_set_rx_mode() can also be called without RTNL. One path is the VF
mailbox:

igb_msix_other()
  igb_msg_task()
    igb_rcv_msg_from_vf()
      igb_set_vf_multicasts() or igb_vf_reset_event()
        igb_set_rx_mode()

Another is __dev_set_rx_mode(), from callers such as MLD that hold
mc_lock rather than RTNL.

Suppose one of these runs between the xchg() and the point where
igb_down() clears E1000_RCTL_EN. RLPML (no VFs) or the PF VMOLR (with
VFs) is then written as MAX_JUMBO_FRAME_SIZE while the old program is
still running, and oversized frames arriving in that window would be
split the same way. The window is small and only opens during a detach.

> +	vmolr &= ~E1000_VMOLR_RLPML_MASK;
> +	vmolr |= pool_rlpml | E1000_VMOLR_LPE;

[Severity: High]
This isn't a bug introduced by this patch, but it involves the same
VMOLR RLPML field. Can a VF set arbitrary VMOLR bits for its own pool
through igb_set_vf_rlpml()?

igb_rcv_msg_from_vf() passes the mailbox word, which the VF controls,
straight through:

	case E1000_VF_SET_LPE:
		retval = igb_set_vf_rlpml(adapter, msgbuf[1], vf);

igb_set_vf_rlpml() takes it as int size, clamps it with a signed
compare, and ORs it in without masking:

	if (size > MAX_JUMBO_FRAME_SIZE)
		size = MAX_JUMBO_FRAME_SIZE;

	vmolr = rd32(E1000_VMOLR(vfn));
	vmolr &= ~E1000_VMOLR_RLPML_MASK;
	vmolr |= size | E1000_VMOLR_LPE;

A value with bit 31 set, such as 0x81000000, is negative and passes the
clamp. Its upper bits then land in VMOLR: AUPE, ROMPE, ROPE, BAM, MPME
and STRVLAN.

Take a VF with a port VLAN assigned by the PF, where igb_set_vmolr()
clears AUPE to enforce tagged-only receive. That VF could set AUPE
again and receive untagged traffic from outside its VLAN. It could also
set ROPE, even though igb_set_vf_promisc() only offers multicast
promiscuous mode to VFs.

Should size be checked as unsigned, or masked with
E1000_VMOLR_RLPML_MASK, before it is ORed in?

>  
>  	wr32(E1000_VMOLR(vfn), vmolr);
>  
> @@ -6713,7 +6725,7 @@ static int igb_change_mtu(struct net_device *netdev, int new_mtu)
>  		for (i = 0; i < adapter->num_rx_queues; i++) {
>  			struct igb_ring *ring = adapter->rx_ring[i];
>  
> -			if (max_frame > igb_rx_bufsz(ring)) {
> +			if (max_frame + IGB_TS_HDR_LEN > igb_rx_bufsz(ring)) {

[Severity: Medium]
Is this check run against the ring layout from before the MTU change?

Suppose XDP is attached at MTU 3000 (3072 rings) and the MTU is then
set to 1500. The check passes (1542 <= 3072). Then
igb_up()->igb_configure()->igb_set_rx_buffer_len() picks 1536 build_skb
rings, because that selection doesn't account for IGB_TS_HDR_LEN.

On those rings, igb_skb_pad() gives all the slack to headroom, which
leaves exactly IGB_MAX_FRAME_BUILD_SKB bytes before skb_shared_info.
With inline Rx timestamps on, take a frame accepted by the new RLPML of
1526 (1522 bytes stored after CRC stripping). With the 16-byte
timestamp that is 1538 bytes.

igb_build_skb() then does:

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

Can skb->tail end up about 2 bytes past skb->end here, overlapping
skb_shared_info?

The same overlap, up to 12 bytes at RLPML 1536, appears to exist
without XDP on build_skb rings with timestamping. So the layout problem
itself predates this patch. Still, this path lets frame plus timestamp
exceed the buffer, which the new check is meant to prevent.

>  				netdev_warn(adapter->netdev,
>  					    "Requested MTU size is not supported with XDP. Max frame size is %d\n",
>  					    max_frame);

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261102-igb-rx-ts-fix-v4-0-8785124c5d23%40aweta.nl

Re: [PATCH net v4 2/2] 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 3 potential
issue(s) to consider.

Critical: 0 · High: 2 (1) · Medium: 0 · Low: 1

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 commit message says "igc has the same bug and is fixed the
  same way." The patch does not change igc.
- [High] igb: XDP setup and MTU checks in igb_xdp_setup() reject default
  MTU 1500
- [Low] The commit message understates what the pre-patch bug does.

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but it is the same
  failure this patch fixes for igb, on igb's other Rx path.

--

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

This commit guards the Rx timestamp header strip in igb_clean_rx_irq()
with !skb, so it only runs on the first buffer of a frame. Before this, a
continuation buffer whose payload started with 8 zero bytes could be
mistaken for a timestamp header and lose 16 bytes.

Link: https://lore.kernel.org/all/20261028083344.3047350-2-p@1g4.org/

> 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.

[ ... ]

> the all-zero frames arrive truncated while frames with non-zero content
> are fine. There is no error indication.

[Severity: Low]
Is this an accurate description of the pre-patch behaviour?

On a continuation buffer (skb != NULL), the old strip only reduced size.
pkt_offset is only used by xdp_prepare_buff() in the !skb branch.
igb_add_rx_frag() then does:

	skb_add_rx_frag(skb, skb_shinfo(skb)->nr_frags, rx_buffer->page,
			rx_buffer->page_offset, size, truesize);

So each affected buffer loses its last 16 bytes, and the data after it
shifts left. For frames that span more than two buffers, the corruption is
in the middle of the payload, not at the tail.

Also, igb_ptp_rx_pktstamp() only checks that the first 8 bytes are zero. It
returns IGB_TS_HDR_LEN without comparing against the descriptor length.
Suppose the last descriptor of a frame holds k bytes, with 8 <= k < 16, and
they are all zero. The old code would then do:

	size -= ts_hdr_len;

The unsigned size wraps to 0xFFFFFFF0 + k. That gives a frag of roughly
4 GiB that no longer matches skb->data_len. The sender picks the frame
length, so a remote sender can reach this.

This patch closes both cases. Could the commit message mention the
mid-frame loss and the possible size wrap, for stable and security
triage?

[ ... ]

> igc has the same bug and is fixed the same way.

[Severity: High]
Is igc actually fixed here? The diffstat only lists
drivers/net/ethernet/intel/igb/igb_main.c. Neither this patch nor the
previous one in the series touches drivers/net/ethernet/intel/igc/.

igc_clean_rx_irq() still strips on every descriptor that has TSIP set:

drivers/net/ethernet/intel/igc/igc_main.c:igc_clean_rx_irq() {
    ...
		if (igc_test_staterr(rx_desc, IGC_RXDADV_STAT_TSIP)) {
			ctx.rx_ts = pktbuf;
			pkt_offset = IGC_TS_HDR_LEN;
			size -= IGC_TS_HDR_LEN;
		}
    ...
		} else if (skb)
			igc_add_rx_frag(rx_ring, rx_buffer, skb, size);
    ...
}

There is no !skb guard here. There is also no reserved dword check like
the one in igb_ptp_rx_pktstamp().

If i225/i226 set TSIP on continuation descriptors the way igb does, every
continuation buffer of a timestamped multi-buffer frame would lose its last
16 bytes, whatever its content. Is that the case?

There is also no size check before the unsigned subtraction. A last buffer
shorter than 16 bytes would make igc_add_rx_frag() add a frag of about
4 GiB.

Should this patch add the same !skb && guard to igc_clean_rx_irq()? The
alternative is to drop this sentence and fix igc in a separate patch with
its own Fixes: tag. igc already limits RLPML while XDP is attached, so the
guard looks safe there.

> diff --git a/drivers/net/ethernet/intel/igb/igb_main.c b/drivers/net/ethernet/intel/igb/igb_main.c
> index 5bf489cd7b066..c4eb9f589fbe7 100644
> --- a/drivers/net/ethernet/intel/igb/igb_main.c
> +++ b/drivers/net/ethernet/intel/igb/igb_main.c
> @@ -9072,8 +9072,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,

[Severity: High]
This patch depends on the receive length limit from the previous patch
in the series, so I am raising this here. The checks that patch adds to
igb_xdp_setup() and igb_change_mtu() look like they reject the default
MTU of 1500 on systems with PAGE_SIZE < 8192.

At MTU 1500, frame_size in igb_xdp_setup() is 1500 +
IGB_ETH_PKT_HDR_PAD = 1526.

igb_set_rx_buffer_len() only selects 3K buffers when
adapter->max_frame_size > IGB_MAX_FRAME_BUILD_SKB, and that test does
not include IGB_TS_HDR_LEN. With 4 KiB pages and SBP clear, the ring
stays on build_skb buffers. igb_rx_bufsz() then returns
IGB_MAX_FRAME_BUILD_SKB, which is 1536 - NET_IP_ALIGN.

The new check

	if (frame_size + IGB_TS_HDR_LEN > igb_rx_bufsz(ring)) {

compares 1542 against 1536 (or 1534) and returns -EINVAL. Does that
mean XDP can no longer be attached to an igb interface at the default
MTU? Every MTU from 1495 to 1510 is affected the same way, or 1493 to
1508 when NET_IP_ALIGN is 2.

The loop in igb_xdp_setup() also runs when bpf->prog is NULL. Two cases
leave a program attached at MTU 1500:

  - It was attached while the interface was down, before the first
    open. ring->flags is 0 then, so igb_rx_bufsz() returns 2048.
  - It was attached at a larger MTU with 3K buffers, and the MTU was
    then lowered to 1500.

Can that program still be detached or replaced?

igb_change_mtu() has a related problem. It checks max_frame +
IGB_TS_HDR_LEN against igb_rx_bufsz() using the ring flags for the old
MTU. igb_set_rx_buffer_len() has not yet picked the buffer size for the
new MTU at that point.

With XDP attached and the MTU at 1494 or below, every larger MTU is
rejected. That includes MTUs from 1511 to 3030, which would fit in the
3K buffers that igb_up() would then select. Going from 3000 down to
1500 is allowed, though. That leaves XDP running on 1536 byte buffers,
where the detach check above then fails.

Should igb_set_rx_buffer_len() include IGB_TS_HDR_LEN when it decides
whether to use 3K buffers, so that the ring sizing and the XDP checks
agree? Should the detach path skip the buffer size check entirely? And
should igb_change_mtu() check against the buffer size the new MTU will
get, rather than the current ring flags?

[Severity: High]
This is a pre-existing issue and was not introduced by this patch. The
igb AF_XDP zero-copy Rx path seems to have the same problem, and the new
first-buffer rule does not cover it.

igb_clean_rx_irq_zc() still strips on every descriptor that has TSIP set,
and it treats each descriptor as its own packet:

drivers/net/ethernet/intel/igb/igb_xsk.c:igb_clean_rx_irq_zc() {
    ...
		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;
		}
    ...
}

Frames can span descriptors in this mode:

  - igb_setup_srrctl() takes the hardware buffer size from
    xsk_pool_get_rx_frame_size() in 1 KiB units, so 2048-byte chunks give
    1 KiB buffers.
  - igb_xsk_pool_enable() does not check the pool frame size against
    max_frame_size + IGB_TS_HDR_LEN.
  - The RLPML limit from the previous patch is based on
    adapter->max_frame_size, not on the pool's buffer size.

Suppose a continuation descriptor holds 8 to 15 bytes, all zero. Then
xdp->data ends up past xdp->data_end. 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)));

totalsize wraps to about 4 GiB. napi_alloc_skb() adds NET_SKB_PAD, so the
allocation length wraps to a small value (56 bytes on x86_64 for k = 8).

Can the memcpy() then overflow the skb head? For k = 9 to 15, ALIGN()
wraps to 0 and the skb is left with skb->len near 4 GiB.

The zero-copy path does not track frames, so a !skb style guard would not
work there directly. One option is to reject pool frame sizes that let
max_frame_size + IGB_TS_HDR_LEN span descriptors. Another is to handle EOP
there and only strip on the first descriptor, with a size check. Would
either of those make sense?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261102-igb-rx-ts-fix-v4-0-8785124c5d23%40aweta.nl

RE: [PATCH net v4 1/2] igb: do not let XDP frames span more than one Rx buffer

[Tjerk Kusters]

Thanks for the review. v5 will address the issues in these patches:

- XDP attach and detach at MTU 1500, and the MTU checks, now use the
  buffer size the rings will get;
- AF_XDP zero-copy rings get full-size buffers, and split zero-copy
  frames are dropped (two new patches);
- XDP is refused with rx-all, legacy-rx cannot change while XDP is
  attached;
- the receive length limit is programmed from igb_configure() instead
  of igb_set_rx_mode();
- patch 2's commit message is corrected (igc, mid-frame loss, size
  wrap).

Of the two pre-existing issues, the VF frame size in igb_set_vf_rlpml()
gets a separate fix, and the build_skb layout overlap is described in
the v5 cover letter.

pw-bot: cr