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

[PATCH net v6 0/3] 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/3 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 for
      3 KiB, legacy-rx and PAGE_SIZE >= 8192 buffers. It also refuses
      XDP with rx-all and legacy-rx changes while XDP is attached, as
      both would break the limit. The limit is now set from
      igb_configure() instead of igb_set_rx_mode(). With VFs, the PF
      pool limit is also written on i354.
  2/3 is v3, unchanged in code, with a note that it relies on 1/3.
      The Reviewed-by and Tested-by tags of v3 are kept for that reason.
  3/3 gives AF_XDP zero-copy rings a 2 KiB or 3 KiB hardware buffer
      instead of rounding the XSK frame size down to 1 KiB. It refuses
      XSK pools that cannot hold the current maximum frame plus the
      timestamp header, and MTU changes that would no longer fit a bound
      pool. Unlike the igc patch in [2], which checks against the
      largest XDP MTU, this checks the current MTU, as igb allows XDP up
      to MTU 3030 and a fixed check would rule out 2 KiB chunks.
      Enabling VFs, which takes queues from the PF, is refused with
      -EBUSY while an XSK pool is bound to a queue the PF would lose; the
      core refuses ethtool -L in the same case. A pool on a queue hidden
      in another way, for example after a fallback to MSI, is checked
      again when its ring is set up.

As in igc, 1/3 moves the RLPML programming out of igb_set_rx_mode(),
which can run without RTNL, into igb_configure(), which runs when the
interface is (re)configured, before Rx is enabled. This includes the PF
pool limit in VMOLR, which igb_set_rx_mode() now leaves as it is.
Attaching or removing XDP on a running interface goes through
igb_close() and igb_open(), and an MTU change through igb_down() and
igb_up(), so the limit is updated before Rx is enabled again.

The Sashiko reviews of v4 and v5 also point out three older issues
that this series does not change:

- On 2 KiB build_skb rings, igb_skb_pad() leaves exactly
  IGB_MAX_FRAME_BUILD_SKB bytes before skb_shared_info, with no room for
  the 16-byte timestamp header. With Rx timestamping on, frame and
  header can overlap skb_shared_info by up to 12 bytes, or 2 bytes at
  the XDP limit at MTU 1500. This dates from 9cbc948b5a20 ("igb: add XDP
  support"). I left it out of this series: it is a separate problem from
  the one fixed here, and a fix changes the buffer layout and the XDP
  headroom for all igb users, so it needs its own review and testing.
- igb_set_vf_rlpml() let a VF set other VMOLR bits of its own pool. That
  is fixed separately: https://lore.kernel.org/all/20261104-igb-vf-rlpml-v1-1-729621b60eee@aweta.nl/
- igb_alloc_rx_buffers_zc() does not clear the length of the next
  descriptor after a partial refill at the end of the ring, so
  igb_clean_rx_irq_zc() can read a stale descriptor. igc clears it in
  that case. This is a separate zero-copy problem as well.

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

3) AF_XDP zero-copy with 2 KiB chunks (patch 3)

  receiver:  ip link set eth0 mtu 1500
             ethtool -L eth0 combined 1
             hwstamp_ctl -i eth0 -r 1
             xdpsock -i eth0 -q 0 -r -z -N -f 2048
  peer:      ping -c 50 -i 0.02 -s 1472 <receiver>

  xdpsock is from xdp-project/bpf-examples (AF_XDP-example). It also
  takes ARP, so the peer needs a fresh ARP entry before it starts.
  Compare rx_packets (MAC) with rx_queue_0_packets in ethtool -S:

    without the series    68 frames, 118 descriptors
    this series           64 frames, 64 descriptors

  At MTU 3000, -f 4096 receives 3014-byte frames in one buffer. Binding
  with -f 2048 at MTU 3000 was refused with EOPNOTSUPP by the new check
  on the 6.14.9 test kernel; current kernels refuse it earlier, in
  xp_assign_dev(), with EINVAL.

Tested on an I350 with these patches applied to the igb driver of
kernel 6.14.9 (the series itself is against v7.1-rc7), 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 1500 and
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.
In step 3, 98-byte frames and 4 KiB chunks take one descriptor per
frame with and without the series. With XDP attached, rx-all stays off
and legacy-rx cannot be changed, and XDP attach is refused with rx-all
on, or with legacy-rx on at MTU 3000, also when it was set while the
interface was down. RLPML and the PF pool's VMOLR limit read back as
expected with and without XDP and VFs. In repeated runs of about 200 MTU
changes each with XDP and two VFs, the limit was right after every
change, except for one read in one run, where the value was not logged;
eight reruns showed no mismatch.

The steps above were repeated on kernel 7.3.1 with the series applied
to the v7.3 igb driver, with the same results, including step 2 with
VFs. There, enabling two VFs while xdpsock was bound to queue 1 was
refused with EBUSY; with the socket on queue 0 it worked. With the
driver built so that the I350 keeps two Rx queues with VFs, as an 82576
does, the VF enable with the socket on queue 1 was refused as well. The
i354 path was checked with the driver built so that the I350 takes the
i354 branches for VMOLR: with two VFs and XDP attached, the PF pool
limit kept its old value of 9728 with v5, which skipped i354, and read
back as 3026 at MTU 3000 and 1526 at MTU 1500 with v6. Only the
registers were checked; I have no i354 to test on.
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 v6:
- Patch 1: with VFs, also write the PF pool limit on i354 (Sashiko).
- Patch 3: refuse to enable VFs with -EBUSY while an XSK pool is bound
  to a queue the PF would lose (Maciej). Restore the DMA coalescing flag
  on that error. Keep the frame size check in igb_xsk_pool() only for
  queues hidden in another way, such as a fallback to MSI.
- Patch 4 dropped: with patch 3 the hardware does not split frames in
  zero-copy mode, so it only added dead code to the hot path (Maciej).
- Link to v5: https://patch.msgid.link/20261104-igb-rx-ts-fix-v5-0-bf4b4367129a@aweta.nl

Changes in v5:
- Patch 1: v4 also subtracted the timestamp header from the build_skb
  frame limit, so XDP no longer attached at MTU 1495-1510 on x86
  (1493-1508 where NET_IP_ALIGN is 2), including the default MTU 1500.
  Only subtract the header where the limit is the hardware buffer size,
  and compute it from the adapter state instead of the ring flags. Skip
  the check when XDP is removed, so a program can always be detached.
- Patch 1: refuse XDP with rx-all and legacy-rx changes with XDP
  attached, and turn rx-all back on after XDP removal if it was
  requested meanwhile. Program RLPML and the PF pool limit from
  igb_configure() instead of igb_set_rx_mode(). With XDP attached, an
  MTU change can now move from 2 KiB to 3 KiB buffers. Added
  Suggested-by: Maciej.
- Patch 1: the issues above were also reported by the Sashiko review
  of v4.
- Patch 2: no code change. Commit message: igc is fixed by Paul's
  series, not by this patch; the loss is per continuation buffer, not
  only at the tail; the possible size wrap is described (Sashiko).
- New patch 3: AF_XDP zero-copy Rx buffer size, as asked by Maciej.
- New patch 4: drop zero-copy frames that span several descriptors;
  depends on patch 3.
- Link to v4: https://patch.msgid.link/20261102-igb-rx-ts-fix-v4-0-8785124c5d23@aweta.nl

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 (3):
      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
      igb: do not split AF_XDP zero-copy frames over several Rx buffers

 drivers/net/ethernet/intel/igb/igb.h         |   9 ++
 drivers/net/ethernet/intel/igb/igb_ethtool.c |   9 ++
 drivers/net/ethernet/intel/igb/igb_main.c    | 207 +++++++++++++++++++++------
 drivers/net/ethernet/intel/igb/igb_xsk.c     |  20 ++-
 4 files changed, 197 insertions(+), 48 deletions(-)
---
base-commit: 2d3090a8aeb596a26935db0955d46c9a5db5c6ce
change-id: 20260619-igb-rx-ts-fix-cd70585ee316

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

[PATCH net v6 1/3] 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. Otherwise
the MAC still accepts frames of up to MAX_JUMBO_FRAME_SIZE while XDP is
attached, for example at MTU 3000 on 3 KiB buffers, and at any MTU when
PAGE_SIZE >= 8192. With VFs allocated, RLPML always stays at
MAX_JUMBO_FRAME_SIZE and the PF pool limit in VMOLR is only lowered for
frames that fit a build_skb buffer, so the same happens at MTU 3000.

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 make sure that the limit holds:

- 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, where the
  limit is the hardware buffer size. This lowers the largest MTU
  accepted with XDP by 16 bytes: from 3046 to 3030 on 3 KiB buffers,
  and from 2022 to 2006 with legacy-rx or PAGE_SIZE >= 8192. Setups that
  use XDP at those MTUs will have to lower the MTU. The build_skb frame
  limit used with 2 KiB buffers is kept, so XDP still attaches up to MTU
  1510 on x86. The header is only prepended by 82580 and later MACs; the
  room is reserved on all MACs for simplicity.
- Base that check on the buffer size that igb_set_rx_buffer_len() will
  select, not on the ring flags, which are only updated when the rings
  are configured and can be stale on an interface that is down. As a
  result, an MTU change with XDP attached can now also move from 2 KiB
  to 3 KiB buffers. The check is skipped when XDP is removed.
- Refuse XDP while rx-all is enabled, and keep rx-all off while XDP is
  attached, as the MAC then also stores frames longer than RLPML. If
  rx-all was requested meanwhile, it is turned on when XDP is removed.
- Refuse to change the legacy-rx private flag while XDP is attached, as
  that changes the Rx buffer size without checking it against the MTU.
- Program both limits from igb_configure(), which runs when the
  interface is (re)configured, before Rx is enabled, and no longer from
  igb_set_rx_mode(), which can also run without RTNL, for example on an
  IPv6 multicast join or a VF mailbox request. Otherwise such a call
  during an XDP removal or an MTU change could apply the new limit while
  the rings still use the old buffers.
  Attaching or removing an XDP program on a running interface goes
  through igb_close() and igb_open(), and an MTU change through
  igb_down() and igb_up(), so igb_configure() runs in both cases.
  igb_set_rx_mode() still updates other bits of the PF pool's VMOLR. The
  core calls it under the address lock, so the limit is changed under
  that lock. The VF mailbox path calls it from the interrupt handler
  without the lock, but interrupts are masked while igb_configure()
  runs.

This follows the same approach as the igc fix that limits RLPML while
XDP is attached. igb_set_rx_mode() does not touch VMOLR on i354, but
i354 can have VFs, and then the PF pool limit is the only one. So it is
also written on i354 when VFs are allocated, with or without XDP.

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 1500 and 3030 still
works, at MTU 3046 it is refused, as is an MTU change from 3030 to 3046
with XDP attached; an MTU change from 1500 to 3000 with XDP attached
now works. With XDP attached, rx-all stays off and the legacy-rx flag
cannot be changed. XDP attach is refused with rx-all on, and at MTU
3000 with legacy-rx on, also when the flag was set while the interface
was down. MTU changes between 1500 and 3000 with XDP attached, while
the peer sent 3000-byte zero-payload frames, let no frame through as a
separate frame.

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. Read back from the registers, RLPML then stays at 9728
and the PF pool limit is 3026 at MTU 3000 and 1526 at MTU 1500 with XDP
attached. In repeated runs of about 200 MTU changes each, the limit was
right after every change, except for one read in one run, where the
value was not logged; eight reruns showed no mismatch.

The i354 path was checked with the driver built so that the I350 takes
the i354 branches for VMOLR. With two VFs and XDP attached, the PF pool
limit kept its old value of 9728 when the VMOLR write was skipped on
i354, and read back as 3026 at MTU 3000 and 1526 at MTU 1500 with this
patch. Only the registers were checked; I have no i354 to test on.

Link: https://lore.kernel.org/all/20261028083344.3047350-3-p@1g4.org/
Fixes: 9cbc948b5a20 ("igb: add XDP support")
Cc: stable@vger.kernel.org
Suggested-by: Maciej Fijalkowski <maciej.fijalkowski@intel.com>
Signed-off-by: Tjerk Kusters <tkusters@aweta.nl>
---
 drivers/net/ethernet/intel/igb/igb_ethtool.c |   9 ++
 drivers/net/ethernet/intel/igb/igb_main.c    | 149 +++++++++++++++++++--------
 2 files changed, 116 insertions(+), 42 deletions(-)

diff --git a/drivers/net/ethernet/intel/igb/igb_ethtool.c b/drivers/net/ethernet/intel/igb/igb_ethtool.c
index f7938c1da835..4f2891fca282 100644
--- a/drivers/net/ethernet/intel/igb/igb_ethtool.c
+++ b/drivers/net/ethernet/intel/igb/igb_ethtool.c
@@ -3438,6 +3438,15 @@ static int igb_set_priv_flags(struct net_device *netdev, u32 priv_flags)
 		flags |= IGB_FLAG_RX_LEGACY;
 
 	if (flags != adapter->flags) {
+		/* the Rx buffer size depends on this flag, see
+		 * igb_xdp_setup()
+		 */
+		if (igb_xdp_is_enabled(adapter)) {
+			netdev_warn(netdev,
+				    "Cannot change legacy-rx while XDP is attached\n");
+			return -EOPNOTSUPP;
+		}
+
 		adapter->flags = flags;
 
 		/* reset interface to repopulate queues */
diff --git a/drivers/net/ethernet/intel/igb/igb_main.c b/drivers/net/ethernet/intel/igb/igb_main.c
index ce91dda00ec0..711a67f0bd80 100644
--- a/drivers/net/ethernet/intel/igb/igb_main.c
+++ b/drivers/net/ethernet/intel/igb/igb_main.c
@@ -1964,6 +1964,57 @@ static void igb_setup_tx_mode(struct igb_adapter *adapter)
 		   "enabled" : "disabled");
 }
 
+/**
+ *  igb_set_rlpml - set the receive length limits
+ *  @adapter: private board structure
+ *
+ *  Called from igb_configure(), before Rx is enabled. igb_set_rx_mode() can
+ *  also run without RTNL, so it does not change these limits.
+ **/
+static void igb_set_rlpml(struct igb_adapter *adapter)
+{
+	u32 rlpml = MAX_JUMBO_FRAME_SIZE, pool_rlpml = MAX_JUMBO_FRAME_SIZE;
+	unsigned int vfn = adapter->vfs_allocated_count;
+	struct e1000_hw *hw = &adapter->hw;
+	u32 vmolr;
+
+#if (PAGE_SIZE < 8192)
+	/* restrict as needed to support build_skb */
+	if (adapter->max_frame_size <= IGB_MAX_FRAME_BUILD_SKB) {
+		pool_rlpml = IGB_MAX_FRAME_BUILD_SKB;
+		if (!vfn)
+			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.
+	 */
+	if (igb_xdp_is_enabled(adapter)) {
+		pool_rlpml = min(pool_rlpml, adapter->max_frame_size);
+		if (!vfn)
+			rlpml = min(rlpml, adapter->max_frame_size);
+	}
+	wr32(E1000_RLPML, rlpml);
+
+	/* the PF pool's VMOLR is used on these MACs, see igb_set_rx_mode(),
+	 * and with VFs it carries the only limit for the PF, also on i354
+	 */
+	if (hw->mac.type < e1000_82576 ||
+	    (hw->mac.type > e1000_i350 && !vfn))
+		return;
+
+	/* igb_set_rx_mode() also updates this register, and the core calls
+	 * it under the address lock
+	 */
+	netif_addr_lock_bh(adapter->netdev);
+	vmolr = rd32(E1000_VMOLR(vfn));
+	vmolr &= ~E1000_VMOLR_RLPML_MASK;
+	vmolr |= pool_rlpml | E1000_VMOLR_LPE;
+	wr32(E1000_VMOLR(vfn), vmolr);
+	netif_addr_unlock_bh(adapter->netdev);
+}
+
 /**
  *  igb_configure - configure the hardware for RX and TX
  *  @adapter: private board structure
@@ -1981,6 +2032,7 @@ static void igb_configure(struct igb_adapter *adapter)
 
 	igb_setup_tctl(adapter);
 	igb_setup_mrqc(adapter);
+	igb_set_rlpml(adapter);
 	igb_setup_rctl(adapter);
 
 	igb_nfc_filter_restore(adapter);
@@ -2468,6 +2520,14 @@ void igb_reset(struct igb_adapter *adapter)
 static netdev_features_t igb_fix_features(struct net_device *netdev,
 	netdev_features_t features)
 {
+	struct igb_adapter *adapter = netdev_priv(netdev);
+
+	/* with rx-all the MAC also stores frames longer than RLPML, which
+	 * XDP cannot handle
+	 */
+	if (igb_xdp_is_enabled(adapter))
+		features &= ~NETIF_F_RXALL;
+
 	/* Since there is no support for separate Rx/Tx vlan accel
 	 * enable/disable make sure Tx flag is always in same state as Rx.
 	 */
@@ -2886,6 +2946,27 @@ static int igb_setup_tc(struct net_device *dev, enum tc_setup_type type,
 	}
 }
 
+/* Check that a frame fits in one Rx buffer together with the inline Rx
+ * timestamp header, for the buffer size igb_set_rx_buffer_len() selects.
+ * The ring flags are only updated when the rings are configured, so they
+ * are not used here. For build_skb rings the frame is limited to
+ * IGB_MAX_FRAME_BUILD_SKB, which is below the 2 KiB hardware buffer size
+ * even with the header.
+ */
+static bool igb_xdp_frame_fits(struct igb_adapter *adapter, int max_frame)
+{
+#if (PAGE_SIZE < 8192)
+	if (!(adapter->flags & IGB_FLAG_RX_LEGACY)) {
+		if (max_frame <= IGB_MAX_FRAME_BUILD_SKB &&
+		    !IGB_2K_TOO_SMALL_WITH_PADDING)
+			return true;
+
+		return max_frame + IGB_TS_HDR_LEN <= IGB_RXBUFFER_3072;
+	}
+#endif
+	return max_frame + IGB_TS_HDR_LEN <= IGB_RXBUFFER_2048;
+}
+
 static int igb_xdp_setup(struct net_device *dev, struct netdev_bpf *bpf)
 {
 	int i, frame_size = dev->mtu + IGB_ETH_PKT_HDR_PAD;
@@ -2894,17 +2975,21 @@ 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 */
-	for (i = 0; i < adapter->num_rx_queues; i++) {
-		struct igb_ring *ring = adapter->rx_ring[i];
+	/* with rx-all the MAC also stores frames longer than RLPML */
+	if (prog && (dev->features & NETIF_F_RXALL)) {
+		NL_SET_ERR_MSG_MOD(bpf->extack, "XDP is not supported with rx-all");
+		return -EOPNOTSUPP;
+	}
 
-		if (frame_size > 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",
-				    igb_rx_bufsz(ring), frame_size);
-			return -EINVAL;
-		}
+	/* verify igb ring attributes are sufficient for XDP, leaving room
+	 * for the inline Rx timestamp header
+	 */
+	if (prog && !igb_xdp_frame_fits(adapter, frame_size)) {
+		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 is too small for the frame size %d\n",
+			    frame_size);
+		return -EINVAL;
 	}
 
 	old_prog = xchg(&adapter->xdp_prog, prog);
@@ -2935,6 +3020,10 @@ static int igb_xdp_setup(struct net_device *dev, struct netdev_bpf *bpf)
 	if (running)
 		igb_open(dev);
 
+	/* rx-all may have been requested while XDP was attached */
+	if (!prog && (dev->wanted_features & NETIF_F_RXALL))
+		netdev_update_features(dev);
+
 	return 0;
 }
 
@@ -5323,7 +5412,7 @@ static void igb_set_rx_mode(struct net_device *netdev)
 	struct igb_adapter *adapter = netdev_priv(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 rctl = 0, vmolr = 0;
 	int count;
 
 	/* Check for Promiscuous and All Multicast modes */
@@ -5380,14 +5469,6 @@ static void igb_set_rx_mode(struct net_device *netdev)
 				     E1000_RCTL_VFE);
 	wr32(E1000_RCTL, rctl);
 
-#if (PAGE_SIZE < 8192)
-	if (!adapter->vfs_allocated_count) {
-		if (adapter->max_frame_size <= IGB_MAX_FRAME_BUILD_SKB)
-			rlpml = IGB_MAX_FRAME_BUILD_SKB;
-	}
-#endif
-	wr32(E1000_RLPML, rlpml);
-
 	/* In order to support SR-IOV and eventually VMDq it is necessary to set
 	 * the VMOLR to enable the appropriate modes.  Without this workaround
 	 * we will have issues with VLAN tag stripping not being done for frames
@@ -5399,19 +5480,10 @@ static void igb_set_rx_mode(struct net_device *netdev)
 	/* set UTA to appropriate mode */
 	igb_set_uta(adapter, !!(vmolr & E1000_VMOLR_ROPE));
 
+	/* keep the receive length limit set by igb_set_rlpml() */
 	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;
-#if (PAGE_SIZE < 8192)
-	if (adapter->max_frame_size <= IGB_MAX_FRAME_BUILD_SKB)
-		vmolr |= IGB_MAX_FRAME_BUILD_SKB;
-	else
-#endif
-		vmolr |= MAX_JUMBO_FRAME_SIZE;
-	vmolr |= E1000_VMOLR_LPE;
-
 	wr32(E1000_VMOLR(vfn), vmolr);
 
 	igb_restore_vf_multicasts(adapter);
@@ -6707,19 +6779,12 @@ static int igb_change_mtu(struct net_device *netdev, int new_mtu)
 	struct igb_adapter *adapter = netdev_priv(netdev);
 	int max_frame = new_mtu + IGB_ETH_PKT_HDR_PAD;
 
-	if (igb_xdp_is_enabled(adapter)) {
-		int i;
-
-		for (i = 0; i < adapter->num_rx_queues; i++) {
-			struct igb_ring *ring = adapter->rx_ring[i];
-
-			if (max_frame > igb_rx_bufsz(ring)) {
-				netdev_warn(adapter->netdev,
-					    "Requested MTU size is not supported with XDP. Max frame size is %d\n",
-					    max_frame);
-				return -EINVAL;
-			}
-		}
+	if (igb_xdp_is_enabled(adapter) &&
+	    !igb_xdp_frame_fits(adapter, max_frame)) {
+		netdev_warn(adapter->netdev,
+			    "Requested MTU size is not supported with XDP. Frame size is %d\n",
+			    max_frame);
+		return -EINVAL;
 	}
 
 	/* adjust max frame to be at least the size of a standard frame */

-- 
2.27.0

[PATCH net v6 2/3] 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, and the caller reduces that buffer's size by 16.
igb_add_rx_frag() adds a continuation buffer from its original offset, so
the buffer loses its last 16 bytes and the data of the next buffer
follows directly. A frame spanning N buffers loses 16 bytes per affected
continuation buffer, in the middle of the payload when N > 2. If the last
buffer of a frame holds 8 to 15 bytes, all zero, the unsigned size wraps
and a fragment of about 4 GiB is added to the skb. The sender chooses the
frame length and content, so this can be triggered remotely.

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 are dropped as truncated while frames with non-zero
content are fine. The driver reports no error; only the IP layer counts
the frames in IpExtInTruncatedPkts.

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; Paul Moses' igc series, linked below, fixes it 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>
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 711a67f0bd80..e787095d3cdb 100644
--- a/drivers/net/ethernet/intel/igb/igb_main.c
+++ b/drivers/net/ethernet/intel/igb/igb_main.c
@@ -9125,8 +9125,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

[PATCH net v6 3/3] igb: do not split AF_XDP zero-copy frames over several Rx buffers

[Tjerk Kusters via B4 Relay]

From: Tjerk Kusters <tkusters@aweta.nl>

SRRCTL.BSIZEPKT has a granularity of 1 KiB. For AF_XDP zero-copy rings,
igb_setup_srrctl() programs xsk_pool_get_rx_frame_size(), which is
rounded down. With 2 KiB UMEM chunks and the default headroom this is
1792 bytes, so the hardware buffer becomes 1 KiB and a full-size frame
spans two Rx buffers. igb_clean_rx_irq_zc() handles every descriptor as
a complete frame, so the AF_XDP socket gets such a frame as two
packets, the second one without headers. With Rx timestamping enabled,
the timestamp header is also stripped from the second buffer, and a
short final buffer makes the size wrap.

Program the same buffer size as for the other rings, 2 KiB or 3 KiB.
This depends on "igb: do not let XDP frames span more than one Rx
buffer": the receive length limit set there while XDP is attached keeps
the hardware from writing more than the frame plus the timestamp header
into the larger buffer. Without it, the hardware can write past the XSK
frame.

Refuse an XSK pool whose frame size cannot hold that, and an MTU change
that would no longer fit a bound pool, also while no XDP program is
attached. The current MTU is checked rather than the largest XDP MTU,
as igb allows XDP up to MTU 3030 and that would rule out 2 KiB chunks.
With VFs, the PF keeps a single Tx queue, and one Rx queue on i350 or
two on 82576. A pool bound to a queue above that would lose its Rx
queue, which the MTU check then no longer sees, or its Tx queue, which
breaks the socket. The core refuses ethtool -L in that case (with
-EINVAL). Likewise, refuse to enable VFs with -EBUSY while an XSK pool
is bound to a queue the PF would lose. On that error, igb_enable_sriov()
also restores the DMA coalescing flag it cleared. The MAC filter
entries it reserved for the VFs before the check stay reserved until
the next reset.

A queue can also be hidden without such a request, for example when the
driver falls back from MSI-X to MSI and keeps one queue. The MTU check
does not see a pool on that queue, so igb_xsk_pool() checks the frame
size again when the ring is set up, and does not use zero-copy if the
frame does not fit. The socket then receives through the copy path,
and its Tx wakeup fails with EINVAL, until the ring is set up again
with an MTU that fits. This path was not tested.

This is the igb counterpart of the igc XSK buffer change.

Tested on an I350 with xdpsock from bpf-examples in rxdrop zero-copy
mode on one queue, Rx timestamping on and 1514-byte frames from the
peer. With 2 KiB chunks, the receive descriptors went from about two
per frame to one per frame; 98-byte frames and 4 KiB chunks were one
per frame with and without this patch. At MTU 3000, 4 KiB chunks
receive 3014-byte frames in one buffer. A 2 KiB chunk pool at MTU 3000
was refused by the new check on the 6.14.9 test kernel; current kernels
already refuse it in xp_assign_dev(). There, with 2 KiB chunks and the
default headroom, the new check refuses MTU 1751 to 1766, which the core
accepts.

On 7.3.1, with xdpsock bound to queue 1 of an I350, enabling two VFs is
now refused with EBUSY and the socket keeps its queue; with the socket on
queue 0, which the PF keeps, the VFs are enabled. The 82576 case, with
two Rx queues and one Tx queue for the PF, was checked with the driver
built so that the I350 keeps two Rx queues with VFs: with the socket on
queue 1, the VF enable is refused as well. Only the refusal was checked.

Link: https://lore.kernel.org/all/20261028083344.3047350-3-p@1g4.org/
Fixes: 2c6196013f84 ("igb: Add AF_XDP zero-copy Rx support")
Cc: stable@vger.kernel.org
Suggested-by: Maciej Fijalkowski <maciej.fijalkowski@intel.com>
Signed-off-by: Tjerk Kusters <tkusters@aweta.nl>
---
 drivers/net/ethernet/intel/igb/igb.h      |  9 ++++++
 drivers/net/ethernet/intel/igb/igb_main.c | 51 +++++++++++++++++++++++++++++--
 drivers/net/ethernet/intel/igb/igb_xsk.c  | 20 +++++++++++-
 3 files changed, 76 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/intel/igb/igb.h b/drivers/net/ethernet/intel/igb/igb.h
index 0fff1df81b7b..0495c02544d6 100644
--- a/drivers/net/ethernet/intel/igb/igb.h
+++ b/drivers/net/ethernet/intel/igb/igb.h
@@ -856,6 +856,15 @@ int igb_add_mac_steering_filter(struct igb_adapter *adapter,
 int igb_del_mac_steering_filter(struct igb_adapter *adapter,
 				const u8 *addr, u8 queue, u8 flags);
 
+/* XDP does not support frames that span more than one Rx buffer, so a
+ * frame plus the inline Rx timestamp header must fit in one XSK buffer.
+ */
+static inline bool igb_xsk_frame_fits(struct xsk_buff_pool *pool,
+				      int max_frame)
+{
+	return max_frame + IGB_TS_HDR_LEN <= xsk_pool_get_rx_frame_size(pool);
+}
+
 struct xsk_buff_pool *igb_xsk_pool(struct igb_adapter *adapter,
 				   struct igb_ring *ring);
 int igb_xsk_pool_setup(struct igb_adapter *adapter,
diff --git a/drivers/net/ethernet/intel/igb/igb_main.c b/drivers/net/ethernet/intel/igb/igb_main.c
index e787095d3cdb..c9351a8c4f5e 100644
--- a/drivers/net/ethernet/intel/igb/igb_main.c
+++ b/drivers/net/ethernet/intel/igb/igb_main.c
@@ -3777,9 +3777,29 @@ static int igb_sriov_reinit(struct pci_dev *dev)
 	struct net_device *netdev = pci_get_drvdata(dev);
 	struct igb_adapter *adapter = netdev_priv(netdev);
 	struct pci_dev *pdev = adapter->pdev;
+	unsigned int i, rxq, txq, end;
 
 	rtnl_lock();
 
+	/* With VFs the PF gets fewer Rx queues and a single Tx queue, see
+	 * igb_set_interrupt_capability(). Refuse that while an XSK pool is
+	 * bound to a queue the PF would lose; the core refuses ethtool -L in
+	 * the same case.
+	 */
+	rxq = min_t(unsigned int, igb_get_max_rss_queues(adapter),
+		    num_online_cpus());
+	txq = adapter->vfs_allocated_count ? 1 : rxq;
+	end = max(netdev->real_num_rx_queues, netdev->real_num_tx_queues);
+	for (i = min(rxq, txq); i < end; i++) {
+		if (xsk_get_pool_from_qid(netdev, i)) {
+			rtnl_unlock();
+			dev_err(&pdev->dev,
+				"Cannot change the number of queues, an AF_XDP socket is bound to queue %u\n",
+				i);
+			return -EBUSY;
+		}
+	}
+
 	if (netif_running(netdev))
 		igb_close(netdev);
 	else
@@ -3844,6 +3864,7 @@ static int igb_enable_sriov(struct pci_dev *pdev, int num_vfs, bool reinit)
 {
 	struct net_device *netdev = pci_get_drvdata(pdev);
 	struct igb_adapter *adapter = netdev_priv(netdev);
+	bool dmac = adapter->flags & IGB_FLAG_DMAC;
 	int old_vfs = pci_num_vf(pdev);
 	struct vf_mac_filter *mac_list;
 	int err = 0;
@@ -3933,6 +3954,8 @@ static int igb_enable_sriov(struct pci_dev *pdev, int num_vfs, bool reinit)
 	kfree(adapter->vf_data);
 	adapter->vf_data = NULL;
 	adapter->vfs_allocated_count = 0;
+	if (dmac)
+		adapter->flags |= IGB_FLAG_DMAC;
 out:
 	return err;
 }
@@ -4836,9 +4859,13 @@ void igb_setup_srrctl(struct igb_adapter *adapter, struct igb_ring *ring)
 	u32 srrctl = 0;
 	u32 buf_size;
 
-	if (ring->xsk_pool)
-		buf_size = xsk_pool_get_rx_frame_size(ring->xsk_pool);
-	else if (ring_uses_large_buffer(ring))
+	/* SRRCTL.BSIZEPKT has a 1 KiB granularity, so programming the XSK
+	 * frame size would round it down and split frames over several
+	 * buffers. XSK rings use the same buffer size as the other rings.
+	 * While XDP is attached, the receive length limit keeps frames
+	 * within the XSK frame, see igb_xsk_frame_fits().
+	 */
+	if (ring_uses_large_buffer(ring))
 		buf_size = IGB_RXBUFFER_3072;
 	else
 		buf_size = IGB_RXBUFFER_2048;
@@ -6778,6 +6805,7 @@ static int igb_change_mtu(struct net_device *netdev, int new_mtu)
 {
 	struct igb_adapter *adapter = netdev_priv(netdev);
 	int max_frame = new_mtu + IGB_ETH_PKT_HDR_PAD;
+	int i;
 
 	if (igb_xdp_is_enabled(adapter) &&
 	    !igb_xdp_frame_fits(adapter, max_frame)) {
@@ -6791,6 +6819,23 @@ static int igb_change_mtu(struct net_device *netdev, int new_mtu)
 	if (max_frame < (ETH_FRAME_LEN + ETH_FCS_LEN))
 		max_frame = ETH_FRAME_LEN + ETH_FCS_LEN;
 
+	/* the frame must fit the buffers of a bound XSK pool, also while no
+	 * XDP program is attached, as attaching one does not check it again
+	 */
+	for (i = 0; i < adapter->num_rx_queues; i++) {
+		struct xsk_buff_pool *pool = xsk_get_pool_from_qid(netdev, i);
+
+		if (!pool || !pool->dev)
+			continue;
+
+		if (!igb_xsk_frame_fits(pool, max_frame)) {
+			netdev_warn(adapter->netdev,
+				    "Requested MTU size is not supported with AF_XDP. Frame size is %d\n",
+				    max_frame);
+			return -EINVAL;
+		}
+	}
+
 	while (test_and_set_bit(__IGB_RESETTING, &adapter->state))
 		usleep_range(1000, 2000);
 
diff --git a/drivers/net/ethernet/intel/igb/igb_xsk.c b/drivers/net/ethernet/intel/igb/igb_xsk.c
index ce4a7b58cad2..7f99e834c1ed 100644
--- a/drivers/net/ethernet/intel/igb/igb_xsk.c
+++ b/drivers/net/ethernet/intel/igb/igb_xsk.c
@@ -91,7 +91,21 @@ struct xsk_buff_pool *igb_xsk_pool(struct igb_adapter *adapter,
 	if (!igb_xdp_is_enabled(adapter))
 		return NULL;
 
-	return (pool && pool->dev) ? pool : NULL;
+	if (!pool || !pool->dev)
+		return NULL;
+
+	/* igb_change_mtu() does not see a pool on a queue that is hidden, for
+	 * example after a fallback from MSI-X to MSI. The hardware buffer can
+	 * be larger than the XSK frame, so check again when the ring is set up.
+	 */
+	if (!igb_xsk_frame_fits(pool, adapter->max_frame_size)) {
+		netdev_warn_once(adapter->netdev,
+				 "XSK frame size is too small for the MTU, queue %d is not used in zero-copy mode\n",
+				 qid);
+		return NULL;
+	}
+
+	return pool;
 }
 
 static int igb_xsk_pool_enable(struct igb_adapter *adapter,
@@ -110,6 +124,10 @@ static int igb_xsk_pool_enable(struct igb_adapter *adapter,
 	    qid >= netdev->real_num_tx_queues)
 		return -EINVAL;
 
+	/* checked again on MTU changes and when the ring is set up */
+	if (!igb_xsk_frame_fits(pool, adapter->max_frame_size))
+		return -EOPNOTSUPP;
+
 	err = xsk_pool_dma_map(pool, &adapter->pdev->dev, IGB_RX_DMA_ATTR);
 	if (err)
 		return err;

-- 
2.27.0

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

[netdev-bot+sashiko]

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

Critical: 0 · High: 0 (3) · 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).

- [Medium] igb_xdp_setup() now calls netdev_update_features(dev) without
  first checking that the device is running or awake.
- [Medium] igb_xdp_setup() ignores the return value of igb_open() and
  then calls the new netdev_update_features(dev) unconditionally.

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but
  igb_set_rx_mode() runs without netif_addr_lock_bh() from several paths…
- [High] This problem wasn't introduced by this patch, but
  igb_set_vf_rlpml() takes the VF-supplied msgbuf[1] (u32) into 'int…
- [High] This problem wasn't introduced by this patch, but on 2 KiB
  build_skb Rx rings (PAGE_SIZE < 8192, legacy-rx off,…

--

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

This commit limits the receive length to the configured maximum frame
size while XDP is attached, so XDP frames cannot span more than one Rx
buffer. The limit goes in RLPML, or in the PF pool's VMOLR when VFs are
allocated. The length programming moves from igb_set_rx_mode() into a
new igb_set_rlpml() called from igb_configure(), the XDP fit check now
reserves room for the Rx timestamp header, and rx-all and legacy-rx are
kept from changing while XDP is attached.

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..711a67f0bd801 100644
> --- a/drivers/net/ethernet/intel/igb/igb_main.c
> +++ b/drivers/net/ethernet/intel/igb/igb_main.c
> @@ -1964,6 +1964,57 @@ static void igb_setup_tx_mode(struct igb_adapter *adapter)

[ ... ]

> +static void igb_set_rlpml(struct igb_adapter *adapter)
> +{

[ ... ]

> +	/* the PF pool's VMOLR is used on these MACs, see igb_set_rx_mode(),
> +	 * and with VFs it carries the only limit for the PF, also on i354
> +	 */
> +	if (hw->mac.type < e1000_82576 ||
> +	    (hw->mac.type > e1000_i350 && !vfn))
> +		return;
> +
> +	/* igb_set_rx_mode() also updates this register, and the core calls
> +	 * it under the address lock
> +	 */
> +	netif_addr_lock_bh(adapter->netdev);

[Severity: High]
This isn't a bug introduced by this patch, but the core is not the only
caller of igb_set_rx_mode(). Several others run it without
netif_addr_lock_bh(): igb_configure() just before this call,
igb_set_vf_multicasts() and igb_vf_reset() from the VF mailbox interrupt
path (holding only vfs_lock), and __igb_shutdown().

igb_set_rx_mode() walks dev->mc without the lock:

igb_write_mc_addr_list() {
    ...
	mta_list = kcalloc(netdev_mc_count(netdev), 6, GFP_ATOMIC);
	if (!mta_list)
		return -ENOMEM;

	/* The shared function expects a packed array of only addresses. */
	i = 0;
	netdev_for_each_mc_addr(ha, netdev)
		memcpy(mta_list + (i++ * ETH_ALEN), ha->addr, ETH_ALEN);
    ...
}

Suppose dev_mc_add() adds an entry between netdev_mc_count() and the
walk, for example on an IPv6 multicast join that holds only
addr_list_lock. Can this then write past the end of mta_list?

A VF can reach the unlocked path whenever it wants by sending
E1000_VF_SET_MULTICAST:

igb_msg_task()->igb_rcv_msg_from_vf()->igb_set_vf_multicasts()
  ->igb_set_rx_mode()->igb_write_mc_addr_list()

The unlocked __dev_uc_sync()->__hw_addr_sync_dev() can also race with a
locked __dev_uc_sync() on another CPU. Both update sync_cnt and refcount
and can delete entries on dev->uc. An entry freed with
list_del_rcu()/kfree_rcu() during the unlocked walk could also be used
after it is freed.

> +	vmolr = rd32(E1000_VMOLR(vfn));
> +	vmolr &= ~E1000_VMOLR_RLPML_MASK;
> +	vmolr |= pool_rlpml | E1000_VMOLR_LPE;
> +	wr32(E1000_VMOLR(vfn), vmolr);
> +	netif_addr_unlock_bh(adapter->netdev);
> +}

[Severity: High]
This is a pre-existing issue, but it sits next to the VMOLR length logic
reworked here. igb_set_vf_rlpml() writes the VF's own VMOLR from a
mailbox word that the VF controls:

igb_set_vf_rlpml(struct igb_adapter *adapter, int size, int vfn) {
    ...
	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;
	wr32(E1000_VMOLR(vfn), vmolr);
    ...
}

For E1000_VF_SET_LPE, igb_rcv_msg_from_vf() passes msgbuf[1] straight
through. A value of 0x80000000 or more becomes a negative int, skips the
clamp, and is ORed into VMOLR without being masked to
E1000_VMOLR_RLPML_MASK.

Can a malicious VF use this to set ROPE, AUPE, MPME, ROMPE, BAM, STRVLAN
and other pool bits that the PF otherwise controls? igb_set_vf_promisc()
explicitly clears ROPE, and igb_set_vmolr() clears AUPE when a port VLAN
is assigned.

With AUPE set on a port-VLAN VF, the pool would accept untagged frames.
With ROPE set and UTA all-ones (82576 with the PF in promiscuous mode,
or RAR overflow), the VF would receive unicast traffic for other MACs.

Would treating size as unsigned and masking it with
E1000_VMOLR_RLPML_MASK be enough here?

[ ... ]

> @@ -2886,6 +2946,27 @@ static int igb_setup_tc(struct net_device *dev, enum tc_setup_type type,
>  	}
>  }
>  
> +/* Check that a frame fits in one Rx buffer together with the inline Rx
> + * timestamp header, for the buffer size igb_set_rx_buffer_len() selects.
> + * The ring flags are only updated when the rings are configured, so they
> + * are not used here. For build_skb rings the frame is limited to
> + * IGB_MAX_FRAME_BUILD_SKB, which is below the 2 KiB hardware buffer size
> + * even with the header.
> + */
> +static bool igb_xdp_frame_fits(struct igb_adapter *adapter, int max_frame)
> +{
> +#if (PAGE_SIZE < 8192)
> +	if (!(adapter->flags & IGB_FLAG_RX_LEGACY)) {
> +		if (max_frame <= IGB_MAX_FRAME_BUILD_SKB &&
> +		    !IGB_2K_TOO_SMALL_WITH_PADDING)
> +			return true;

[Severity: High]
This is a pre-existing issue, but the comment's "even with the header"
compares against the 2 KiB SRRCTL buffer size. Does it also hold for
the build_skb data area?

igb_skb_pad() leaves exactly IGB_MAX_FRAME_BUILD_SKB bytes between the
DMA start (page_offset = IGB_SKB_PAD) and the skb_shared_info that
igb_build_skb() places at SKB_WITH_OVERHEAD(2048):

igb_skb_pad() {
    ...
		rx_buf_len = IGB_RXBUFFER_1536;

	/* if needed make room for NET_IP_ALIGN */
	rx_buf_len -= NET_IP_ALIGN;

	return igb_compute_pad(rx_buf_len);
}

On 82580 and later MACs, Rx hardware timestamping makes the MAC prepend
the 16-byte timestamp header. RLPML (or the PF pool limit) is
IGB_MAX_FRAME_BUILD_SKB here. A 1525 to 1536 byte frame from the wire
can then put up to 16 + (1536 - 4) = 1548 bytes after the DMA start on
x86_64.

Could this write up to 12 bytes into the shinfo area? Then
napi_build_skb(xdp->data_hard_start, truesize) zeroes shinfo over the
end of the payload, and __skb_put() in igb_build_skb() leaves tail past
end.

With XDP attached and the new RLPML of max_frame_size, data_end would
also land past data_hard_end at MTU 1510, or with QinQ frames at MTU
1500.

Since igb_xdp_frame_fits() now accepts this layout explicitly, should
the build_skb path also leave room for IGB_TS_HDR_LEN?

[ ... ]

> @@ -2935,6 +3020,10 @@ static int igb_xdp_setup(struct net_device *dev, struct netdev_bpf *bpf)
>  	if (running)
>  		igb_open(dev);
>  
> +	/* rx-all may have been requested while XDP was attached */
> +	if (!prog && (dev->wanted_features & NETIF_F_RXALL))
> +		netdev_update_features(dev);

[Severity: Medium]
Can this new call touch a device that is runtime-suspended?

Take an interface that is down, where rx-all was requested while XDP was
attached. When XDP is removed, igb_fix_features() stops masking
NETIF_F_RXALL. igb_set_features() then sees it in changed and takes the
!netif_running() branch:

igb_set_features() {
    ...
	if (netif_running(netdev))
		igb_reinit_locked(adapter);
	else
		igb_reset(adapter);
    ...
}

igb_reset() accesses MMIO registers right away. The ndo_bpf path
(dev_xdp_install()->bpf_op()) does not take a runtime-PM reference. The
ethtool paths that reach igb_set_features() do take one.

With power/control=auto, a sequence like this seems to reach it:

  ethtool -K <if> rx-all on   (XDP attached, only wanted_features changes)
  ip link set <if> down       (igb_runtime_idle() schedules suspend
                               after 5s without link, device in D3hot)
  ip link set <if> xdp off
    igb_xdp_setup()
      netdev_update_features()
        igb_set_features()
          igb_reset()

igb_rd32() would then read all-F, set hw->hw_addr = NULL and warn about
the lost PCIe link. __igb_resume() does not restore hw_addr. Does the
NIC then stay unusable until the driver is rebound?

Before this patch, igb_xdp_setup() did not touch the hardware on a down
interface.

[Severity: Medium]
What happens here if igb_open() fails? Its return value is ignored, and
the new netdev_update_features() call runs anyway.

When XDP is removed from a running interface, igb_close()->igb_down()
disables NAPI on every q_vector and the rings are freed. __igb_open()
can then fail in Tx/Rx resource allocation, igb_request_irq() or
netif_set_real_num_*_queues(). Its error labels free the resources and
return without napi_enable(), but netif_running() is still true.

If rx-all was requested while XDP was attached, igb_set_features() then
calls igb_reinit_locked()->igb_down()->napi_disable() on NAPI instances
that are already disabled. napi_disable_locked() waits for:

	while (val & (NAPIF_STATE_SCHED | NAPIF_STATE_NPSVC)) {
		usleep_range(20, 200);
		val = READ_ONCE(n->state);
	}

Would this spin forever with RTNL held? Even past that point,
igb_up()->igb_configure()->igb_alloc_rx_buffers() would use rings whose
descriptors and rx_buffer_info were freed.

Ignoring the igb_open() error is older than this patch, and the broken
state would also hang on a later link down. This call, though, turns a
single failed XDP removal straight into the hang. Should igb_xdp_setup()
return the igb_open() error and skip netdev_update_features() when it
fails?

> +
>  	return 0;
>  }

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261111-igb-rx-ts-fix-v6-0-ba5b9903859c%40aweta.nl

Re: [PATCH net v6 3/3] igb: do not split AF_XDP zero-copy frames over several Rx buffers

[netdev-bot+sashiko]

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

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

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 new XSK queue check sits in igb_sriov_reinit(), and that
  function is shared by the SR-IOV enable and disable paths.
- [Medium] The new -EBUSY return from igb_sriov_reinit() gives userspace
  a deterministic way into igb_enable_sriov()'s err_out path while the…
- [Low] This patch raises the XSK ring hardware buffer in
  igb_setup_srrctl() to 2 KiB or 3 KiB, while the XSK frame can be as…
- [Low] The new -EBUSY refusal in igb_sriov_reinit() runs after
  igb_enable_sriov() has already called igb_vf_configure() for each VF.
- [Low] The new restore `if (dmac) adapter->flags |= IGB_FLAG_DMAC;` in
  igb_enable_sriov() err_out is a non-atomic read-modify-write of…

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but it sits in
  igb_xsk_pool(), which the patch edits.
- [Medium] This problem wasn't introduced by this patch.

--

igb: do not split AF_XDP zero-copy frames over several Rx buffers

AF_XDP zero-copy Rx rings now get the same 2 KiB or 3 KiB SRRCTL buffer
size as the other rings, so frames are no longer split over two
buffers. The patch adds checks that refuse an XSK pool, an MTU change,
or VF enablement when a bound pool's frame size or queue would no
longer fit. When the frame does not fit at ring setup, igb_xsk_pool()
falls back to copy mode.

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 e787095d3cdbe..c9351a8c4f5e5 100644
> --- a/drivers/net/ethernet/intel/igb/igb_main.c
> +++ b/drivers/net/ethernet/intel/igb/igb_main.c
> @@ -3777,9 +3777,29 @@ static int igb_sriov_reinit(struct pci_dev *dev)
>  	struct net_device *netdev = pci_get_drvdata(dev);
>  	struct igb_adapter *adapter = netdev_priv(netdev);
>  	struct pci_dev *pdev = adapter->pdev;
> +	unsigned int i, rxq, txq, end;
>  
>  	rtnl_lock();
>  
> +	/* With VFs the PF gets fewer Rx queues and a single Tx queue, see
> +	 * igb_set_interrupt_capability(). Refuse that while an XSK pool is
> +	 * bound to a queue the PF would lose; the core refuses ethtool -L in
> +	 * the same case.
> +	 */
> +	rxq = min_t(unsigned int, igb_get_max_rss_queues(adapter),
> +		    num_online_cpus());
> +	txq = adapter->vfs_allocated_count ? 1 : rxq;
> +	end = max(netdev->real_num_rx_queues, netdev->real_num_tx_queues);
> +	for (i = min(rxq, txq); i < end; i++) {
> +		if (xsk_get_pool_from_qid(netdev, i)) {
> +			rtnl_unlock();
> +			dev_err(&pdev->dev,
> +				"Cannot change the number of queues, an AF_XDP socket is bound to queue %u\n",
> +				i);
> +			return -EBUSY;
> +		}
> +	}

[Severity: High]
igb_sriov_reinit() is shared by igb_enable_sriov() and
igb_disable_sriov(). Can this check also fire on the disable path? The
commit message only describes refusing the enable case.

igb_disable_sriov() calls igb_sriov_reinit() only after the VFs have
already been torn down:

igb_disable_sriov() {
    ...
		pci_disable_sriov(pdev);
    ...
		kfree(adapter->vf_data);
		adapter->vf_data = NULL;
		adapter->vfs_allocated_count = 0;
    ...
		wr32(E1000_IOVCTL, E1000_IOVCTL_REUSE_VFQ);
    ...
	return reinit ? igb_sriov_reinit(pdev) : 0;
}

One possible path is an i350 with the interface down.
real_num_rx_queues and real_num_tx_queues stay at 8 from
alloc_etherdev_mq(). Enable VFs, then bind a copy-mode XSK to queue 5.
The loop doesn't check pool->dev, so copy-mode pools count too. On a
host with fewer than 8 CPUs, writing 0 to sriov_numvfs then lands queue
5 inside [min(rxq, txq), end) and returns -EBUSY, even though the VFs
are already gone. On 82576, ethtool -L combined 2 on a 1-CPU host, or
offlining a CPU, gives a similar range.

The -EBUSY return skips igb_clear_interrupt_scheme(),
igb_init_queue_configuration() and igb_init_interrupt_scheme(). The PF
keeps a single Tx queue. The rings also keep the reg_idx offset that
igb_cache_ring_register() computed from the old vfs_allocated_count,
while vfs_allocated_count is now 0.

Would the next igb_configure() then program non-VT MRQC that steers to
hardware queue 0, while the only PF Rx ring sits at the old register
index, so PF Rx stops?

Could this check run only on the enable path, ideally at the start of
igb_enable_sriov() before any VF state is set up?

[ ... ]

> @@ -3933,6 +3954,8 @@ static int igb_enable_sriov(struct pci_dev *pdev, int num_vfs, bool reinit)
>  	kfree(adapter->vf_data);
>  	adapter->vf_data = NULL;
>  	adapter->vfs_allocated_count = 0;

[Severity: Medium]
The new -EBUSY return lets userspace reliably reach this err_out
teardown while the PF is still up. The check runs before igb_close(),
so the watchdog and rx_mode keep running. Here rtnl has already been
dropped inside igb_sriov_reinit(), and vfs_lock is not taken.

Some readers walk vf_data without either lock, for example
igb_watchdog_task()->igb_ping_all_vfs():

	for (i = 0 ; i < adapter->vfs_allocated_count; i++) {
		ping = E1000_PF_CONTROL_MSG;
		if (adapter->vf_data[i].flags & IGB_VF_FLAG_CTS)

igb_set_rx_mode()->igb_restore_vf_multicasts() does the same under
addr_list_lock only. rtnl-only VF ops such as igb_ndo_set_vf_mac() also
write vf_data[vf].

vf_data is set to NULL before vfs_allocated_count is cleared. Can a
concurrent reader see a non-zero count and either use the freed
vf_data or dereference NULL plus an offset?

This unsynchronized reader pattern also exists against
igb_disable_sriov(), but this patch adds a user-triggerable way into
it. Moving the XSK check before vfs_allocated_count and vf_data are
published would avoid that.

[Severity: Low]
The commit message says the MAC filter entries reserved for the VFs
stay reserved until the next reset. They come from
igb_vf_configure()->igb_set_vf_mac(), which runs before
igb_sriov_reinit():

	adapter->mac_table[rar_entry].queue = vf;
	adapter->mac_table[rar_entry].state |= IGB_MAC_STATE_IN_USE;
	igb_rar_set_index(adapter, rar_entry);

err_out does not release them. With vfs_allocated_count back at 0, the
PF loses up to num_vfs - 1 unicast filter slots.

If the check ran at the top of igb_enable_sriov() (the PF queue limit
can be derived from num_vfs), would this leak and the disable-path
problem above both go away?

[Severity: Medium]
This isn't a bug introduced by this patch, but when igb_sriov_reinit()
succeeds and pci_enable_sriov() then fails:

	if (!old_vfs) {
		err = pci_enable_sriov(pdev, adapter->vfs_allocated_count);
		if (err)
			goto err_out;
	}

err_out clears vfs_allocated_count without running igb_sriov_reinit()
again. The interrupt scheme, the ring reg_idx offsets and the VT
register programming all stay in VF mode while software believes there
are no VFs.

The new DMAC restore matches the intended no-VF state and does not
make this worse. Should err_out redo the reinit in this case?

> +	if (dmac)
> +		adapter->flags |= IGB_FLAG_DMAC;

[Severity: Low]
This is a non-atomic read-modify-write of adapter->flags from sysfs
context, and the interface is still up after the -EBUSY return.

igb_watchdog_task() updates the same word
(IGB_FLAG_NEED_LINK_UPDATE, IGB_FLAG_MEDIA_RESET, IGB_FLAG_EEE), and so
does igb_set_rx_mode() (IGB_FLAG_VLAN_PROMISC) under addr_list_lock.

Can an overlapping update lose either the DMAC bit or the other
context's bit? The earlier "adapter->flags &= ~IGB_FLAG_DMAC" in this
function already follows the same pattern.

>  out:
>  	return err;
>  }

[ ... ]

> @@ -4836,9 +4859,13 @@ void igb_setup_srrctl(struct igb_adapter *adapter, struct igb_ring *ring)
>  	u32 srrctl = 0;
>  	u32 buf_size;
>  
> -	if (ring->xsk_pool)
> -		buf_size = xsk_pool_get_rx_frame_size(ring->xsk_pool);
> -	else if (ring_uses_large_buffer(ring))
> +	/* SRRCTL.BSIZEPKT has a 1 KiB granularity, so programming the XSK
> +	 * frame size would round it down and split frames over several
> +	 * buffers. XSK rings use the same buffer size as the other rings.
> +	 * While XDP is attached, the receive length limit keeps frames
> +	 * within the XSK frame, see igb_xsk_frame_fits().
> +	 */
> +	if (ring_uses_large_buffer(ring))
>  		buf_size = IGB_RXBUFFER_3072;
>  	else
>  		buf_size = IGB_RXBUFFER_2048;

[Severity: Low]
This is only safe together with the receive length limit from the
earlier patch. The commit message says:

    This depends on "igb: do not let XDP frames span more than one Rx
    buffer" ... Without it, the hardware can write past the XSK frame.

The two patches carry different Fixes: tags (9cbc948b5a20 and
2c6196013f84), and the Cc: stable tag here has no prerequisite note. If
a stable tree takes this patch without the prerequisite, could the NIC
DMA up to 2 KiB or 3 KiB into a 1792-byte XSK frame and overrun the
next UMEM chunk?

Would a stable prerequisite annotation on the Cc: stable line make the
dependency explicit?

> diff --git a/drivers/net/ethernet/intel/igb/igb_xsk.c b/drivers/net/ethernet/intel/igb/igb_xsk.c
> index ce4a7b58cad2f..7f99e834c1edb 100644
> --- a/drivers/net/ethernet/intel/igb/igb_xsk.c
> +++ b/drivers/net/ethernet/intel/igb/igb_xsk.c
> @@ -91,7 +91,21 @@ struct xsk_buff_pool *igb_xsk_pool(struct igb_adapter *adapter,
>  	if (!igb_xdp_is_enabled(adapter))
>  		return NULL;
>  
> -	return (pool && pool->dev) ? pool : NULL;
> +	if (!pool || !pool->dev)
> +		return NULL;
> +
> +	/* igb_change_mtu() does not see a pool on a queue that is hidden, for
> +	 * example after a fallback from MSI-X to MSI. The hardware buffer can
> +	 * be larger than the XSK frame, so check again when the ring is set up.
> +	 */
> +	if (!igb_xsk_frame_fits(pool, adapter->max_frame_size)) {
> +		netdev_warn_once(adapter->netdev,
> +				 "XSK frame size is too small for the MTU, queue %d is not used in zero-copy mode\n",
> +				 qid);
> +		return NULL;
> +	}
> +
> +	return pool;
>  }

[Severity: High]
This is a pre-existing issue that was not introduced by this patch.
igb_xsk_pool() picks the pool from ring->queue_index alone.
igb_setup_desc_rings() in igb_ethtool.c never sets
test_rx_ring.queue_index, so it stays 0. It then calls
igb_configure_rx_ring(), which sets ring->xsk_pool from
igb_xsk_pool().

Suppose XDP is attached and a zero-copy pool is bound to queue 0. Then
ethtool -t ethX offline takes this path:

igb_diag_test()
  igb_loopback_test()
    igb_setup_desc_rings()
      igb_configure_rx_ring()   test ring gets the queue 0 pool
      igb_alloc_rx_buffers()    fills igb_rx_buffer page entries
    ...
    igb_free_desc_rings()
      igb_free_rx_resources()
        igb_clean_rx_ring()
          igb_clean_rx_ring_zc()

igb_clean_rx_ring_zc() then does:

	struct xdp_buff *xdp = rx_ring->rx_buffer_info_zc[ntc];

	xsk_buff_free(xdp);

Can this call xsk_buff_free() on words of the igb_rx_buffer array (DMA
addresses, struct page pointers) as if they were struct xdp_buff
pointers?

The test's pages would also leak and stay DMA-mapped. The live pool's
rxq info also gets pointed at the test ring's xdp_rxq. The new
netdev_warn_once() above can also fire for this test ring.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261111-igb-rx-ts-fix-v6-0-ba5b9903859c%40aweta.nl