[PATCH net-next v8 0/5] net: stmmac: introduce XDP rx multi-buff support

[PATCH net-next v8 0/5] net: stmmac: introduce XDP rx multi-buff support

[Lorenzo Bianconi]

Rework stmmac_rx() to build the xdp_buff from all the descriptors that make
up a frame, so the XDP program runs on the whole (possibly fragmented)
packet instead of the first buffer only. Fix possible rx_q state leaks
when closing/opening the interface.

---
Changes in v8:
- Add patch 'net: stmmac: fix VLAN tag stripping on non-linear RX
  frames' to fix sw rx vlan parsing.
- Rebase on top of net-next main branch.
- Link to v7: https://lore.kernel.org/r/20261014-stmmac-rx-mb-v7-0-5d2d70f052cd@oss.qualcomm.com

Changes in v7:
- Take into account the case where buf2_len is not 0 for the single
  descriptor case if the interface is running in xdp mode.
- Report rx_not_ls bit if the packet is splitted over multiple
  descriptors for dwmac100 and dwmac1000 cores.
- Link to v6: https://lore.kernel.org/r/20261012-stmmac-rx-mb-v6-0-2225050915c1@oss.qualcomm.com

Changes in v6:
- Drop patch 'net: stmmac: allow non-linear xdp_buff in XDP mode' since
  it requires some XDP fixes.
- Introduce patch 'net: stmmac: drop split frames on AF_XDP zero-copy
  RX' to always drop XDP multi-buff frames for AF_XDP mode since it not
  yet supported there.
- Introduce patch 'net: stmmac: drop multi-buffer frames in XDP mode' to
  fix pending bugs due to missing XDP multi-buffer support before patch
  'net: stmmac: rework stmmac_rx to support XDP rx multi-buff'
- Link to v5: https://lore.kernel.org/r/20261009-stmmac-rx-mb-v5-0-c38fa4eaa138@oss.qualcomm.com

Changes in v5:
- Fix possible read after bound issue in patch 1/3.
- Do not allow parsing non-linear XDP buffers if the program loaded does
  not support frags.
- Add patch 3/3 to remove MTU constraint for XDP prog.
- Link to v4: https://lore.kernel.org/r/20261006-stmmac-rx-mb-v4-0-efa4ca974e3d@oss.qualcomm.com

Changes in v4:
- Reset in_progress flag when the packet is fully consumed.
- Advetise NETDEV_XDP_ACT_RX_SG capability.
- Link to v3: https://lore.kernel.org/r/20261004-stmmac-rx-mb-v3-0-50fa171af9ec@oss.qualcomm.com

Changes in v3:
- Return secondary page in case of error.
- Introduce stmmac_xdp_shrink_tail() to remove the ethernet FCS.
- Add patch 'net: stmmac: take ownership of saved RX state at poll
  entry' from James.
- Link to v2: https://lore.kernel.org/r/20260921-stmmac-rx-mb-v2-1-6e826e1ff306@oss.qualcomm.com

Changes in v2:
- Rely on xdp_buff_add_frag() to create xdp fragments
- Fix bugs in rx_q state recording
- Fix the corner case where we receive more than MAX_SKB_FRAGS fragments
- Cosmetics
- Link to v1: https://lore.kernel.org/r/20260918-stmmac-rx-mb-v1-1-0b4517d404af@oss.qualcomm.com

---
Lorenzo Bianconi (5):
      net: stmmac: take ownership of saved RX state at poll entry
      net: stmmac: drop split frames on AF_XDP zero-copy RX
      net: stmmac: drop multi-buffer frames in XDP mode
      net: stmmac: fix VLAN tag stripping on non-linear RX frames
      net: stmmac: rework stmmac_rx to support XDP rx multi-buff

 drivers/net/ethernet/stmicro/stmmac/enh_desc.c    |   2 +-
 drivers/net/ethernet/stmicro/stmmac/norm_desc.c   |   2 +-
 drivers/net/ethernet/stmicro/stmmac/stmmac.h      |   3 +-
 drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 453 ++++++++++++++--------
 4 files changed, 289 insertions(+), 171 deletions(-)
---
base-commit: 98c2ad4f5d805dbc1ea436a334ee59311806a48c
change-id: 20260918-stmmac-rx-mb-16469a2714ea

Best regards,
-- 
Lorenzo Bianconi <lorenzo.bianconi@oss.qualcomm.com>

[PATCH net-next v8 1/5] net: stmmac: take ownership of saved RX state at poll entry

[Lorenzo Bianconi]

When a saved partial packet completes with a poll budget of one, the
old loop can leave state_saved and state.skb pointing at an skb that
has already been delivered or freed. The next poll then reuses that
pointer, causing a use-after-free or double free.

Take the saved state at poll entry and clear the stored ownership
immediately. Save it again only if the packet remains incomplete,
including when the next descriptor is still DMA-owned.

Track the frame-in-progress flag independently of the status read from
the next descriptor. The old code keyed the save on the status of the
last descriptor looked at, so a loop stopped at a DMA-owned descriptor
(rx status not yet valid) could drop the saved error/length bookkeeping
in stmmac_rx_zc().

Drop the saved state both when the queue parameters are reset and when
the ring is destroyed. stmmac_reset_rx_queue() is reached from
__stmmac_open(), stmmac_xdp_open(), stmmac_resume() and
stmmac_enable_rx_queue(); without draining it there, a partial frame
saved before suspend (or before a queue is disabled and re-enabled)
survives while cur_rx and dirty_rx are re-armed, and the first poll
resumes that stale skb with descriptors from the freshly initialized
ring. __free_dma_rx_desc_resources() releases it on teardown.

Apply the same state handling to stmmac_rx_zc(), which also leaves
state_saved set when a saved frame finishes at the budget boundary.
That path saves only error and length bookkeeping, not an skb pointer.

Fixes: ec222003bd94 ("net: stmmac: Prepare to add Split Header support")
Fixes: bba2556efad6 ("net: stmmac: Enable RX via AF_XDP zero-copy")
Signed-off-by: James Hilliard <james.hilliard1@gmail.com>
Reviewed-by: Nicolai Buchwitz <nb@tipi-net.de>
Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@oss.qualcomm.com>
---
 drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 72 +++++++++++++----------
 1 file changed, 42 insertions(+), 30 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index 53782ab55e12..8fd2acb25fb6 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -2180,6 +2180,9 @@ static void __free_dma_rx_desc_resources(struct stmmac_priv *priv,
 	else
 		dma_free_rx_skbufs(priv, dma_conf, queue);
 
+	dev_kfree_skb_any(rx_q->state.skb);
+	rx_q->state.skb = NULL;
+	rx_q->state_saved = false;
 	rx_q->buf_alloc_num = 0;
 	rx_q->xsk_pool = NULL;
 
@@ -5614,12 +5617,12 @@ static int stmmac_rx_zc(struct stmmac_priv *priv, int limit, u32 queue)
 	unsigned int count = 0, error = 0, len = 0;
 	int dirty = stmmac_rx_dirty(priv, queue);
 	unsigned int next_entry = rx_q->cur_rx;
+	bool in_progress = rx_q->state_saved;
 	u32 rx_errors = 0, rx_dropped = 0;
 	unsigned int desc_size;
 	struct bpf_prog *prog;
 	bool failure = false;
 	int xdp_status = 0;
-	int status = 0;
 
 	if (netif_msg_rx_status(priv)) {
 		void *rx_head = stmmac_get_rx_desc(priv, rx_q, 0);
@@ -5630,23 +5633,25 @@ static int stmmac_rx_zc(struct stmmac_priv *priv, int limit, u32 queue)
 		stmmac_display_ring(priv, rx_head, priv->dma_conf.dma_rx_size, true,
 				    rx_q->dma_rx_phy, desc_size);
 	}
+
+	if (rx_q->state_saved) {
+		error = rx_q->state.error;
+		len = rx_q->state.len;
+		rx_q->state_saved = false;
+	}
+
 	while (count < limit) {
 		struct stmmac_rx_buffer *buf;
 		struct stmmac_xdp_buff *ctx;
 		unsigned int buf1_len = 0;
 		struct dma_desc *np, *p;
-		int entry;
+		int entry, status;
 		int res;
 
-		if (!count && rx_q->state_saved) {
-			error = rx_q->state.error;
-			len = rx_q->state.len;
-		} else {
-			rx_q->state_saved = false;
+		if (!in_progress) {
 			error = 0;
 			len = 0;
 		}
-
 read_again:
 		if (count >= limit)
 			break;
@@ -5685,6 +5690,8 @@ static int stmmac_rx_zc(struct stmmac_priv *priv, int limit, u32 queue)
 		if (!buf->xdp)
 			break;
 
+		in_progress = status & rx_not_ls;
+
 		if (priv->extend_desc)
 			stmmac_rx_extended_status(priv, &priv->xstats,
 						  rx_q->dma_erx + entry);
@@ -5757,7 +5764,7 @@ static int stmmac_rx_zc(struct stmmac_priv *priv, int limit, u32 queue)
 		count++;
 	}
 
-	if (status & rx_not_ls) {
+	if (in_progress) {
 		rx_q->state_saved = true;
 		rx_q->state.error = error;
 		rx_q->state.len = len;
@@ -5799,9 +5806,10 @@ static int stmmac_rx(struct stmmac_priv *priv, int limit, u32 queue)
 	struct stmmac_rx_queue *rx_q = &priv->dma_conf.rx_queue[queue];
 	struct stmmac_channel *ch = &priv->channel[queue];
 	unsigned int count = 0, error = 0, len = 0;
-	int status = 0, coe = priv->hw->rx_csum;
 	unsigned int next_entry = rx_q->cur_rx;
+	bool in_progress = rx_q->state_saved;
 	enum dma_data_direction dma_dir;
+	int coe = priv->hw->rx_csum;
 	unsigned int desc_size;
 	struct sk_buff *skb = NULL;
 	struct stmmac_xdp_buff ctx;
@@ -5821,25 +5829,28 @@ static int stmmac_rx(struct stmmac_priv *priv, int limit, u32 queue)
 		stmmac_display_ring(priv, rx_head, priv->dma_conf.dma_rx_size, true,
 				    rx_q->dma_rx_phy, desc_size);
 	}
+
+	if (rx_q->state_saved) {
+		skb = rx_q->state.skb;
+		error = rx_q->state.error;
+		len = rx_q->state.len;
+		rx_q->state.skb = NULL;
+		rx_q->state_saved = false;
+	}
+
 	while (count < limit) {
 		unsigned int buf1_len = 0, buf2_len = 0;
 		enum pkt_hash_types hash_type;
 		struct stmmac_rx_buffer *buf;
 		struct dma_desc *np, *p;
-		int entry;
+		int entry, status;
 		u32 hash;
 
-		if (!count && rx_q->state_saved) {
-			skb = rx_q->state.skb;
-			error = rx_q->state.error;
-			len = rx_q->state.len;
-		} else {
-			rx_q->state_saved = false;
+		if (!in_progress) {
 			skb = NULL;
 			error = 0;
 			len = 0;
 		}
-
 read_again:
 		if (count >= limit)
 			break;
@@ -5868,6 +5879,8 @@ static int stmmac_rx(struct stmmac_priv *priv, int limit, u32 queue)
 
 		prefetch(np);
 
+		in_progress = status & rx_not_ls;
+
 		if (priv->extend_desc)
 			stmmac_rx_extended_status(priv, &priv->xstats, rx_q->dma_erx + entry);
 		if (unlikely(status == discard_frame)) {
@@ -5880,11 +5893,10 @@ static int stmmac_rx(struct stmmac_priv *priv, int limit, u32 queue)
 
 		if (unlikely(error && (status & rx_not_ls)))
 			goto read_again;
+
 		if (unlikely(error)) {
 			dev_kfree_skb(skb);
-			skb = NULL;
-			count++;
-			continue;
+			goto next;
 		}
 
 		/* Buffer is good. Go on. */
@@ -5958,9 +5970,7 @@ static int stmmac_rx(struct stmmac_priv *priv, int limit, u32 queue)
 						      STMMAC_XDP_REDIRECT)) {
 					xdp_status |= xdp_res;
 					buf->page = NULL;
-					skb = NULL;
-					count++;
-					continue;
+					goto next;
 				}
 			}
 		}
@@ -6017,10 +6027,8 @@ static int stmmac_rx(struct stmmac_priv *priv, int limit, u32 queue)
 		/* Remove FCS if needed */
 		if (!fcs_stripped && pskb_trim(skb, len)) {
 			dev_kfree_skb_any(skb);
-			skb = NULL;
 			rx_dropped++;
-			count++;
-			continue;
+			goto next;
 		}
 
 		stmmac_get_rx_hwtstamp(priv, p, np, skb);
@@ -6045,14 +6053,15 @@ static int stmmac_rx(struct stmmac_priv *priv, int limit, u32 queue)
 
 		skb_record_rx_queue(skb, queue);
 		napi_gro_receive(&ch->rx_napi, skb);
-		skb = NULL;
-
 		rx_packets++;
 		rx_bytes += len;
+next:
+		in_progress = false;
+		skb = NULL;
 		count++;
 	}
 
-	if (status & rx_not_ls || skb) {
+	if (in_progress || skb) {
 		rx_q->state_saved = true;
 		rx_q->state.skb = skb;
 		rx_q->state.error = error;
@@ -8399,6 +8408,9 @@ static void stmmac_reset_rx_queue(struct stmmac_priv *priv, u32 queue)
 {
 	struct stmmac_rx_queue *rx_q = &priv->dma_conf.rx_queue[queue];
 
+	dev_kfree_skb_any(rx_q->state.skb);
+	rx_q->state.skb = NULL;
+	rx_q->state_saved = false;
 	rx_q->cur_rx = 0;
 	rx_q->dirty_rx = 0;
 }

-- 
2.56.0

[PATCH net-next v8 2/5] net: stmmac: drop split frames on AF_XDP zero-copy RX

[Lorenzo Bianconi]

stmmac_rx_zc() assumes a 1:1 mapping between a received frame and an
XSK pool buffer and drops any frame that spans more than one RX
descriptor:

        /* XSK pool expects RX frame 1:1 mapped to XSK buffer */
        if (likely(status & rx_not_ls)) {
                xsk_buff_free(buf->xdp);
                buf->xdp = NULL;
                dirty++;
                count++;
                goto read_again;
        }

The non-last descriptors are freed and dropped, but the frame is not
marked as errored. When the poll reaches the last descriptor of the
same frame, error is still 0, so the fragment is handled as a complete
frame: it is passed to the XDP program, copied into an skb on XDP_PASS,
or converted for XDP_TX. The tail starts in the middle of the original
payload at a predictable offset and, parsed as a fresh frame (len == 0),
stmmac_rx_buf1_len() reports the full dma_buf_sz length for it even
though the DMA engine only wrote the tail remainder into that buffer, so
the reads extend past the received bytes and can reach beyond the end of
the XSK pool buffer. GMAC4 always enables jumbo frames (GMAC_CONFIG_JE
in GMAC_CORE_INIT), so an on-link peer can trigger such a split even
when the interface MTU is left at 1500.

Set error when the 1:1 mapping check drops a descriptor, so the
remaining descriptors of the frame - including the last one - are
drained and released with xsk_buff_free() instead of being processed as
a new frame. Account every freed buffer with dirty and count the
dropped frame once in the poll budget / rx_pkt_n. The direct 1:1
mapping drop also increments rx_dropped.

Normalize the legacy normal and enhanced descriptor callbacks to report
rx_not_ls together with discard_frame for non-last descriptors. This
allows both stmmac_rx_zc() and stmmac_rx() to drain split frames on
dwmac100 and dwmac1000 instead of reparsing the final fragment as a new
packet.

Fixes: bba2556efad6 ("net: stmmac: Enable RX via AF_XDP zero-copy")
Reviewed-by: Nicolai Buchwitz <nb@tipi-net.de>
Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@oss.qualcomm.com>
---
 drivers/net/ethernet/stmicro/stmmac/enh_desc.c    |  2 +-
 drivers/net/ethernet/stmicro/stmmac/norm_desc.c   |  2 +-
 drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 21 +++++++++++++--------
 3 files changed, 15 insertions(+), 10 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/enh_desc.c b/drivers/net/ethernet/stmicro/stmmac/enh_desc.c
index 051253601225..297e2428a8f9 100644
--- a/drivers/net/ethernet/stmicro/stmmac/enh_desc.c
+++ b/drivers/net/ethernet/stmicro/stmmac/enh_desc.c
@@ -184,7 +184,7 @@ static int enh_desc_get_rx_status(struct stmmac_extra_stats *x,
 
 	if (unlikely(!(rdes0 & RDES0_LAST_DESCRIPTOR))) {
 		x->rx_length++;
-		return discard_frame;
+		return discard_frame | rx_not_ls;
 	}
 
 	if (unlikely(rdes0 & RDES0_ERROR_SUMMARY)) {
diff --git a/drivers/net/ethernet/stmicro/stmmac/norm_desc.c b/drivers/net/ethernet/stmicro/stmmac/norm_desc.c
index c4b613564f87..d19bf7ca4db3 100644
--- a/drivers/net/ethernet/stmicro/stmmac/norm_desc.c
+++ b/drivers/net/ethernet/stmicro/stmmac/norm_desc.c
@@ -70,7 +70,7 @@ static int ndesc_get_rx_status(struct stmmac_extra_stats *x,
 
 	if (unlikely(!(rdes0 & RDES0_LAST_DESCRIPTOR))) {
 		x->rx_length++;
-		return discard_frame;
+		return discard_frame | rx_not_ls;
 	}
 
 	if (unlikely(rdes0 & RDES0_ERROR_SUMMARY)) {
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index 8fd2acb25fb6..efa88cd359da 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -5695,18 +5695,21 @@ static int stmmac_rx_zc(struct stmmac_priv *priv, int limit, u32 queue)
 		if (priv->extend_desc)
 			stmmac_rx_extended_status(priv, &priv->xstats,
 						  rx_q->dma_erx + entry);
-		if (unlikely(status == discard_frame)) {
-			xsk_buff_free(buf->xdp);
-			buf->xdp = NULL;
-			dirty++;
+
+		if (unlikely(status & discard_frame)) {
 			error = 1;
 			if (!priv->hwts_rx_en)
 				rx_errors++;
 		}
 
-		if (unlikely(error && (status & rx_not_ls)))
-			goto read_again;
 		if (unlikely(error)) {
+			xsk_buff_free(buf->xdp);
+			buf->xdp = NULL;
+			dirty++;
+
+			if (status & rx_not_ls)
+				goto read_again;
+
 			count++;
 			continue;
 		}
@@ -5715,8 +5718,9 @@ static int stmmac_rx_zc(struct stmmac_priv *priv, int limit, u32 queue)
 		if (likely(status & rx_not_ls)) {
 			xsk_buff_free(buf->xdp);
 			buf->xdp = NULL;
+			rx_dropped++;
+			error = 1;
 			dirty++;
-			count++;
 			goto read_again;
 		}
 
@@ -5883,7 +5887,8 @@ static int stmmac_rx(struct stmmac_priv *priv, int limit, u32 queue)
 
 		if (priv->extend_desc)
 			stmmac_rx_extended_status(priv, &priv->xstats, rx_q->dma_erx + entry);
-		if (unlikely(status == discard_frame)) {
+
+		if (unlikely(status & discard_frame)) {
 			page_pool_put_page(rx_q->page_pool, buf->page, 0, true);
 			buf->page = NULL;
 			error = 1;

-- 
2.56.0

[PATCH net-next v8 3/5] net: stmmac: drop multi-buffer frames in XDP mode

[Lorenzo Bianconi]

stmmac_rx() runs the XDP program once per descriptor buffer and uses
skb == NULL as the only marker for the start of a frame. A frame larger
than one buffer is split by the hardware across several descriptors;
GMAC4 enables jumbo frames unconditionally (GMAC_CONFIG_JE in
GMAC_CORE_INIT), so an on-link peer can deliver such a frame even when
the interface MTU is left at 1500.

When an XDP verdict ends a non-last descriptor, skb is cleared and the
next descriptor of the same hardware frame is parsed as a new frame.
For XDP_CONSUMED this happens via read_again with len preserved, so
stmmac_rx_buf1_len() then returns min(dma_buf_sz, plen - len) for the
last descriptor, which can be as small as 0-3 bytes. buf1_len -=
ETH_FCS_LEN underflows and xdp_prepare_buff() puts data_end far past
data; on XDP_PASS this produces an skb whose length is close to 4G that
reaches the network stack (skb->tail and skb->len are 32-bit and wrap
below skb->end, so skb_over_panic() does not fire). A 4+ byte tail is
instead offered to the XDP program and delivered as a standalone
mid-payload Ethernet frame on XDP_PASS. XDP_TX/XDP_REDIRECT instead
falls through to the next iteration, resetting the running length, so
the tail is re-parsed as a full-size new frame.

The same fault exists when a single descriptor uses its second buffer:
GMAC4/XGMAC always provide a second data buffer per descriptor, so a
frame up to 2 * dma_buf_sz can land on a single descriptor while the XDP
program still only sees the first buffer. XDP_TX then transmits a
truncated frame, and XDP_PASS appends the second buffer only after the
program has already run over the first one.

XDP does not support scatter-gather frames until the multi-buffer
rework, so drop the whole frame instead: mark it as errored as soon as
a non-last descriptor, or a descriptor using its second buffer, is seen
while an XDP program is attached, drain and release every descriptor -
including the last. Removing the re-parse lets both XDP verdict branches
simply complete the packet via next.

Note: the subsequent "net: stmmac: rework stmmac_rx to support XDP rx
multi-buff" accumulates the whole frame into a single xdp_buff and
supersedes this patch; the fix is kept so the net portion of the series
stands on its own.

Fixes: 5fabb01207a2 ("net: stmmac: Add initial XDP support")
Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@oss.qualcomm.com>
---
 drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 35 ++++++++++++-----------
 1 file changed, 19 insertions(+), 16 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index efa88cd359da..e929cc0f2312 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -5888,18 +5888,23 @@ static int stmmac_rx(struct stmmac_priv *priv, int limit, u32 queue)
 		if (priv->extend_desc)
 			stmmac_rx_extended_status(priv, &priv->xstats, rx_q->dma_erx + entry);
 
-		if (unlikely(status & discard_frame)) {
-			page_pool_put_page(rx_q->page_pool, buf->page, 0, true);
-			buf->page = NULL;
+		if (unlikely(!error && (status & discard_frame))) {
 			error = 1;
 			if (!priv->hwts_rx_en)
 				rx_errors++;
 		}
 
-		if (unlikely(error && (status & rx_not_ls)))
-			goto read_again;
+		/* We do not support SG in XDP mode */
+		if (unlikely(!error && (status & rx_not_ls) &&
+			     stmmac_xdp_is_enabled(priv))) {
+			rx_dropped++;
+			error = 1;
+		}
 
 		if (unlikely(error)) {
+			if (status & rx_not_ls)
+				goto read_again;
+
 			dev_kfree_skb(skb);
 			goto next;
 		}
@@ -5911,6 +5916,14 @@ static int stmmac_rx(struct stmmac_priv *priv, int limit, u32 queue)
 		buf2_len = stmmac_rx_buf2_len(priv, p, status, len);
 		len += buf2_len;
 
+		/* Drop the packet if it is split over buf1 and buf2 on a
+		 * single descriptor if the interface is running in XDP mode.
+		 */
+		if (unlikely(stmmac_xdp_is_enabled(priv) && buf2_len)) {
+			rx_dropped++;
+			goto next;
+		}
+
 		/* ACS is disabled; strip manually. */
 		if (likely(!(status & rx_not_ls)))
 			len -= ETH_FCS_LEN;
@@ -5960,17 +5973,7 @@ static int stmmac_rx(struct stmmac_priv *priv, int limit, u32 queue)
 							   sync_len, true);
 					buf->page = NULL;
 					rx_dropped++;
-
-					/* Clear skb as it was set as
-					 * status by XDP program.
-					 */
-					skb = NULL;
-
-					if (unlikely((status & rx_not_ls)))
-						goto read_again;
-
-					count++;
-					continue;
+					goto next;
 				} else if (xdp_res & (STMMAC_XDP_TX |
 						      STMMAC_XDP_REDIRECT)) {
 					xdp_status |= xdp_res;

-- 
2.56.0

[PATCH net-next v8 4/5] net: stmmac: fix VLAN tag stripping on non-linear RX frames

[Lorenzo Bianconi]

stmmac_rx_vlan() reads the VLAN header straight from skb->data and
removes it with skb_pull(), assuming the header is always in the linear
part of the skb. That is not true once non-linear skbs can reach it: a
program loaded with BPF_F_XDP_HAS_FRAGS can pass up a fragmented frame
whose linear head has been shortened to as little as ETH_HLEN by
bpf_xdp_adjust_head(), and the unguarded skb_pull() then drives skb->len
below skb->data_len, tripping BUG() in __skb_pull().

Pull the VLAN header into the linear part with pskb_may_pull() before
touching it and re-derive the header pointer afterwards, as
pskb_may_pull() may reallocate the skb head. Also move the XDP metadata
written with bpf_xdp_adjust_meta() together with the MAC header when the
tag is popped, so data_meta still points at the metadata after
eth_type_trans() anchors mac_header, instead of at the first bytes of
the destination MAC. Return an error so the callers can drop the frame
when the header cannot be pulled.

Fixes: bba2556efad6 ("net: stmmac: Enable RX via AF_XDP zero-copy")
Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@oss.qualcomm.com>
---
 drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 45 +++++++++++++++--------
 1 file changed, 29 insertions(+), 16 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index e929cc0f2312..04dc6dcaa435 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -5095,22 +5095,30 @@ static netdev_features_t stmmac_features_check(struct sk_buff *skb,
 	return vlan_features_check(skb, features);
 }
 
-static void stmmac_rx_vlan(struct net_device *dev, struct sk_buff *skb)
+static int stmmac_rx_vlan(struct net_device *dev, struct sk_buff *skb)
 {
-	struct vlan_ethhdr *veth = skb_vlan_eth_hdr(skb);
-	__be16 vlan_proto = veth->h_vlan_proto;
-	u16 vlanid;
+	struct vlan_ethhdr *veth;
 
-	if ((vlan_proto == htons(ETH_P_8021Q) &&
+	if (!pskb_may_pull(skb, VLAN_ETH_HLEN))
+		return -EINVAL;
+
+	veth = skb_vlan_eth_hdr(skb);
+	if ((veth->h_vlan_proto == htons(ETH_P_8021Q) &&
 	     dev->features & NETIF_F_HW_VLAN_CTAG_RX) ||
-	    (vlan_proto == htons(ETH_P_8021AD) &&
+	    (veth->h_vlan_proto == htons(ETH_P_8021AD) &&
 	     dev->features & NETIF_F_HW_VLAN_STAG_RX)) {
+		__be16 vlan_proto = veth->h_vlan_proto;
+		u16 vlanid = ntohs(veth->h_vlan_TCI);
+		u8 meta_len = skb_metadata_len(skb);
+
 		/* pop the vlan tag */
-		vlanid = ntohs(veth->h_vlan_TCI);
-		memmove(skb->data + VLAN_HLEN, veth, ETH_ALEN * 2);
+		memmove(skb->data - meta_len + VLAN_HLEN,
+			skb->data - meta_len, meta_len + ETH_ALEN * 2);
 		skb_pull(skb, VLAN_HLEN);
 		__vlan_hwaccel_put_tag(skb, vlan_proto, vlanid);
 	}
+
+	return 0;
 }
 
 static bool stmmac_rx_need_ic(struct stmmac_priv *priv,
@@ -5530,12 +5538,15 @@ static void stmmac_dispatch_skb_zc(struct stmmac_priv *priv, u32 queue,
 	}
 
 	stmmac_get_rx_hwtstamp(priv, p, np, skb);
-	if (priv->hw->hw_vlan_en)
+	if (priv->hw->hw_vlan_en) {
 		/* MAC level stripping. */
 		stmmac_rx_hw_vlan(priv, priv->hw, p, skb);
-	else
-		/* Driver level stripping. */
-		stmmac_rx_vlan(priv->dev, skb);
+	} else if (stmmac_rx_vlan(priv->dev, skb)) {
+		dev_kfree_skb_any(skb);
+		priv->xstats.rx_dropped++;
+		return;
+	}
+
 	skb->protocol = eth_type_trans(skb, priv->dev);
 
 	if (unlikely(!coe) || !stmmac_has_ip_ethertype(skb))
@@ -6041,12 +6052,14 @@ static int stmmac_rx(struct stmmac_priv *priv, int limit, u32 queue)
 
 		stmmac_get_rx_hwtstamp(priv, p, np, skb);
 
-		if (priv->hw->hw_vlan_en)
+		if (priv->hw->hw_vlan_en) {
 			/* MAC level stripping. */
 			stmmac_rx_hw_vlan(priv, priv->hw, p, skb);
-		else
-			/* Driver level stripping. */
-			stmmac_rx_vlan(priv->dev, skb);
+		} else if (stmmac_rx_vlan(priv->dev, skb)) {
+			dev_kfree_skb_any(skb);
+			rx_dropped++;
+			goto next;
+		}
 
 		skb->protocol = eth_type_trans(skb, priv->dev);
 

-- 
2.56.0

[PATCH net-next v8 5/5] net: stmmac: rework stmmac_rx to support XDP rx multi-buff

[Lorenzo Bianconi]

The preceding commit dropped frames split across multiple descriptors, or
using a second buffer, while XDP was enabled. This prevented the
per-descriptor XDP path from re-parsing frame tails and from applying FCS
handling to truncated buffers.

Rework stmmac_rx() to accumulate all the descriptors that make up a frame
in an xdp_buff, so the XDP program runs on the full (possibly fragmented)
packet instead of just the first buffer. Frames split across descriptors
or buffers are now passed to programs loaded with BPF_F_XDP_HAS_FRAGS;
programs without that capability continue to drop fragmented frames. When
the frame is not consumed by the program, assemble the skb from the head
buffer and the collected fragments via napi_build_skb()/
xdp_update_skb_frags_info().

Note that XDP still rejects an MTU larger than ETH_DATA_LEN
(stmmac_xdp_set_prog(), stmmac_change_mtu()) and disables SPH, so an
ordinary frame fits in a single buffer. This path is therefore exercised
by frames that do span several descriptors (for example jumbo frames the
MAC admits through its jumbo-enable bit) and is groundwork for extending
XDP to jumbo/SPH; it does not enable XDP jumbo support by itself.

To do so, store the in-progress xdp_buff in rx_q->state instead of the
partially built skb, so the accumulated head and fragments survive a NAPI
poll boundary (mid-frame dma_own or dirty_rx break). The state is saved
only while a frame is in progress and cleared once it completes, leaving
it untouched when the poll does not process anything (e.g. netpoll invoked
with a zero budget).

In addition:
- advertise NETDEV_XDP_ACT_RX_SG, since the RX path can now deliver
  non-linear buffers to XDP;
- build the skb head with napi_build_skb() passing xdp->frame_sz, so
  skb_shinfo() lands on the same shared_info the fragments were accumulated
  into;
- carry the XDP metadata written with bpf_xdp_adjust_meta() into the skb
  via skb_metadata_set(), so it is no longer dropped and TC/BPF programs
  can read it through data_meta;
- attach fragments with xdp_buff_add_frag(), which initializes all the
  shared_info fields and takes care of the pfmemalloc bit;
- charge fragments the whole page_pool page size (xdp->frame_sz) as
  truesize instead of the old dma_buf_sz; with SPH/buf2 most frames carry
  a fragment, so skb->truesize grows by roughly 1.5x on 4K pages and 2x on
  64K pages, which reduces the effective socket receive buffer. The new
  value is more accurate now that a fragment is a full page_pool page, but
  it is a behavior change;
- release the buffers collected in the xdp_buff when a frame is dropped
  on RX errors; the previous code relied on dev_kfree_skb() recycling the
  skb frags, which no longer applies now that the head and fragments live
  in an xdp_buff rather than a partially built skb;
- drop the whole frame when it would exceed MAX_SKB_FRAGS, where the
  previous skb_add_rx_frag() calls were unbounded;
- strip the Ethernet FCS from the accumulated xdp_buff with a driver-local
  tail shrink (stmmac_xdp_shrink_tail()) for both linear and fragmented
  frames, instead of trimming the skb or the first descriptor length.

Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@oss.qualcomm.com>
---
 drivers/net/ethernet/stmicro/stmmac/stmmac.h      |   3 +-
 drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 336 ++++++++++++++--------
 2 files changed, 212 insertions(+), 127 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac.h b/drivers/net/ethernet/stmicro/stmmac/stmmac.h
index 08b68441f9c8..4ac596a2d904 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac.h
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac.h
@@ -132,7 +132,8 @@ struct stmmac_rx_queue {
 	dma_addr_t dma_rx_phy;
 	unsigned int state_saved;
 	struct {
-		struct sk_buff *skb;
+		struct xdp_buff xdp;
+		unsigned int frames;
 		unsigned int len;
 		unsigned int error;
 	} state;
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index 04dc6dcaa435..e9c61524fc74 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -158,6 +158,9 @@ static void stmmac_flush_tx_descriptors(struct stmmac_priv *priv, int queue);
 static void stmmac_set_dma_operation_mode(struct stmmac_priv *priv, u32 txmode,
 					  u32 rxmode, u32 chan);
 static void stmmac_vlan_restore(struct stmmac_priv *priv);
+static void stmmac_xdp_put_buff(struct stmmac_rx_queue *rx_q,
+				struct xdp_buff *xdp, int sync_len,
+				bool allow_direct);
 
 #ifdef CONFIG_DEBUG_FS
 static const struct net_device_ops stmmac_netdev_ops;
@@ -2180,8 +2183,10 @@ static void __free_dma_rx_desc_resources(struct stmmac_priv *priv,
 	else
 		dma_free_rx_skbufs(priv, dma_conf, queue);
 
-	dev_kfree_skb_any(rx_q->state.skb);
-	rx_q->state.skb = NULL;
+	if (rx_q->state.frames && !rx_q->xsk_pool) {
+		stmmac_xdp_put_buff(rx_q, &rx_q->state.xdp, -1, false);
+		rx_q->state.frames = 0;
+	}
 	rx_q->state_saved = false;
 	rx_q->buf_alloc_num = 0;
 	rx_q->xsk_pool = NULL;
@@ -5470,8 +5475,8 @@ static int __stmmac_xdp_run_prog(struct stmmac_priv *priv,
 static struct sk_buff *stmmac_xdp_run_prog(struct stmmac_priv *priv,
 					   struct xdp_buff *xdp)
 {
+	int res = STMMAC_XDP_CONSUMED;
 	struct bpf_prog *prog;
-	int res;
 
 	prog = READ_ONCE(priv->xdp_prog);
 	if (!prog) {
@@ -5479,7 +5484,8 @@ static struct sk_buff *stmmac_xdp_run_prog(struct stmmac_priv *priv,
 		goto out;
 	}
 
-	res = __stmmac_xdp_run_prog(priv, prog, xdp);
+	if (likely(!xdp_buff_has_frags(xdp) || prog->aux->xdp_has_frags))
+		res = __stmmac_xdp_run_prog(priv, prog, xdp);
 out:
 	return ERR_PTR(-res);
 }
@@ -5806,6 +5812,119 @@ static int stmmac_rx_zc(struct stmmac_priv *priv, int limit, u32 queue)
 	return failure ? limit : (int)count;
 }
 
+static void
+stmmac_xdp_put_buff(struct stmmac_rx_queue *rx_q, struct xdp_buff *xdp,
+		    int sync_len, bool allow_direct)
+{
+	struct skb_shared_info *sinfo = xdp_get_shared_info_from_buff(xdp);
+	int i;
+
+	if (likely(!xdp_buff_has_frags(xdp)))
+		goto out;
+
+	for (i = 0; i < sinfo->nr_frags; i++)
+		page_pool_put_full_page(rx_q->page_pool,
+					skb_frag_page(&sinfo->frags[i]),
+					allow_direct);
+out:
+	page_pool_put_page(rx_q->page_pool, virt_to_head_page(xdp->data),
+			   sync_len, allow_direct);
+}
+
+static struct sk_buff *stmmac_build_skb(struct xdp_buff *xdp)
+{
+	struct skb_shared_info *sinfo = xdp_get_shared_info_from_buff(xdp);
+	u32 metasize = xdp->data - xdp->data_meta;
+	struct sk_buff *skb;
+	u8 num_frags = 0;
+
+	if (unlikely(xdp_buff_has_frags(xdp)))
+		num_frags = sinfo->nr_frags;
+
+	skb = napi_build_skb(xdp->data_hard_start, xdp->frame_sz);
+	if (!skb)
+		return NULL;
+
+	skb_mark_for_recycle(skb);
+	skb_reserve(skb, xdp->data - xdp->data_hard_start);
+	skb_put(skb, xdp->data_end - xdp->data);
+	if (metasize)
+		skb_metadata_set(skb, metasize);
+
+	if (unlikely(xdp_buff_has_frags(xdp)))
+		xdp_update_skb_frags_info(skb, num_frags, sinfo->xdp_frags_size,
+					  sinfo->xdp_frags_truesize,
+					  xdp_buff_get_skb_flags(xdp));
+	return skb;
+}
+
+static bool stmmac_build_xdp_frags(struct stmmac_priv *priv,
+				   struct stmmac_rx_queue *rx_q,
+				   unsigned int len, struct page *page,
+				   unsigned int offset,
+				   enum dma_data_direction dma_dir,
+				   struct xdp_buff *xdp)
+{
+	dma_addr_t dma_addr = page_pool_get_dma_addr(page) + offset;
+
+	dma_sync_single_for_cpu(priv->device, dma_addr, len, dma_dir);
+	if (!xdp_buff_add_frag(xdp, page_to_netmem(page), offset, len,
+			       xdp->frame_sz)) {
+		page_pool_put_full_page(rx_q->page_pool, page, true);
+		return false;
+	}
+
+	return true;
+}
+
+static int stmmac_xdp_shrink_tail(struct stmmac_rx_queue *rx_q,
+				  struct xdp_buff *xdp, int offset)
+{
+	struct skb_shared_info *sinfo;
+	int i;
+
+	if (unlikely(offset < 0 ||
+		     offset > (int)xdp_get_buff_len(xdp) - ETH_HLEN))
+		return -EINVAL;
+
+	if (likely(!xdp_buff_has_frags(xdp))) {
+		xdp->data_end -= offset;
+		return 0;
+	}
+
+	sinfo = xdp_get_shared_info_from_buff(xdp);
+	for (i = sinfo->nr_frags - 1; i >= 0 && offset > 0; i--) {
+		skb_frag_t *frag = &sinfo->frags[i];
+		int delta = min_t(int, offset, skb_frag_size(frag));
+
+		if (delta == skb_frag_size(frag)) {
+			/* The whole frag is consumed by the strip: return it
+			 * to the page pool right away. Its ring slot still
+			 * carries the page DMA address until stmmac_rx_refill()
+			 * re-arms it at the end of the NAPI poll. This is
+			 * safe because HW cannot touch the slot until then.
+			 */
+			page_pool_put_full_page(rx_q->page_pool,
+						skb_frag_page(frag), true);
+			sinfo->nr_frags--;
+			sinfo->xdp_frags_truesize -= xdp->frame_sz;
+		} else {
+			skb_frag_size_sub(frag, delta);
+		}
+
+		sinfo->xdp_frags_size -= delta;
+		offset -= delta;
+	}
+
+	if (unlikely(!sinfo->nr_frags)) {
+		xdp_buff_clear_frags_flag(xdp);
+		xdp_buff_clear_frag_pfmemalloc(xdp);
+		xdp->data_end -= offset;
+	}
+
+	return 0;
+}
+
 /**
  * stmmac_rx - manage the receive process
  * @priv: driver private structure
@@ -5819,21 +5938,20 @@ static int stmmac_rx(struct stmmac_priv *priv, int limit, u32 queue)
 	u32 rx_errors = 0, rx_dropped = 0, rx_bytes = 0, rx_packets = 0;
 	struct stmmac_rxq_stats *rxq_stats = &priv->xstats.rxq_stats[queue];
 	struct stmmac_rx_queue *rx_q = &priv->dma_conf.rx_queue[queue];
+	unsigned int frames = 0, next_entry = rx_q->cur_rx;
 	struct stmmac_channel *ch = &priv->channel[queue];
 	unsigned int count = 0, error = 0, len = 0;
-	unsigned int next_entry = rx_q->cur_rx;
 	bool in_progress = rx_q->state_saved;
 	enum dma_data_direction dma_dir;
 	int coe = priv->hw->rx_csum;
-	unsigned int desc_size;
-	struct sk_buff *skb = NULL;
 	struct stmmac_xdp_buff ctx;
-	bool fcs_stripped = false;
+	unsigned int desc_size;
 	int xdp_status = 0;
 	int bufsz;
 
 	dma_dir = page_pool_get_dma_dir(rx_q->page_pool);
-	bufsz = DIV_ROUND_UP(priv->dma_conf.dma_buf_sz, PAGE_SIZE) * PAGE_SIZE;
+	bufsz = rx_q->napi_skb_frag_size;
+	ctx.priv = priv;
 
 	if (netif_msg_rx_status(priv)) {
 		void *rx_head = stmmac_get_rx_desc(priv, rx_q, 0);
@@ -5846,23 +5964,25 @@ static int stmmac_rx(struct stmmac_priv *priv, int limit, u32 queue)
 	}
 
 	if (rx_q->state_saved) {
-		skb = rx_q->state.skb;
+		ctx.xdp = rx_q->state.xdp;
 		error = rx_q->state.error;
+		frames = rx_q->state.frames;
 		len = rx_q->state.len;
-		rx_q->state.skb = NULL;
 		rx_q->state_saved = false;
+		rx_q->state.frames = 0;
 	}
 
 	while (count < limit) {
 		unsigned int buf1_len = 0, buf2_len = 0;
+		unsigned int pre_len, sync_len;
 		enum pkt_hash_types hash_type;
 		struct stmmac_rx_buffer *buf;
 		struct dma_desc *np, *p;
+		struct sk_buff *skb;
 		int entry, status;
 		u32 hash;
 
 		if (!in_progress) {
-			skb = NULL;
 			error = 0;
 			len = 0;
 		}
@@ -5905,19 +6025,11 @@ static int stmmac_rx(struct stmmac_priv *priv, int limit, u32 queue)
 				rx_errors++;
 		}
 
-		/* We do not support SG in XDP mode */
-		if (unlikely(!error && (status & rx_not_ls) &&
-			     stmmac_xdp_is_enabled(priv))) {
-			rx_dropped++;
-			error = 1;
-		}
-
 		if (unlikely(error)) {
 			if (status & rx_not_ls)
 				goto read_again;
 
-			dev_kfree_skb(skb);
-			goto next;
+			goto error_free_frag;
 		}
 
 		/* Buffer is good. Go on. */
@@ -5927,129 +6039,89 @@ static int stmmac_rx(struct stmmac_priv *priv, int limit, u32 queue)
 		buf2_len = stmmac_rx_buf2_len(priv, p, status, len);
 		len += buf2_len;
 
-		/* Drop the packet if it is split over buf1 and buf2 on a
-		 * single descriptor if the interface is running in XDP mode.
-		 */
-		if (unlikely(stmmac_xdp_is_enabled(priv) && buf2_len)) {
-			rx_dropped++;
-			goto next;
-		}
-
-		/* ACS is disabled; strip manually. */
-		if (likely(!(status & rx_not_ls)))
-			len -= ETH_FCS_LEN;
-
-		if (!skb) {
-			unsigned int pre_len, sync_len;
-
-			/* Each frame starts here: reset the FCS handling */
-			fcs_stripped = false;
-
+		if (!frames) {
 			dma_sync_single_for_cpu(priv->device, buf->addr,
 						buf1_len, dma_dir);
 			net_prefetch(page_address(buf->page) +
 				     buf->page_offset);
 
-			if (stmmac_xdp_is_enabled(priv) && !buf2_len) {
-				buf1_len -= ETH_FCS_LEN;
-				fcs_stripped = true;
-			}
-
 			xdp_init_buff(&ctx.xdp, bufsz, &rx_q->xdp_rxq);
 			xdp_prepare_buff(&ctx.xdp, page_address(buf->page),
 					 buf->page_offset, buf1_len, true);
-
-			pre_len = ctx.xdp.data_end - ctx.xdp.data_hard_start -
-				  buf->page_offset;
-
-			ctx.priv = priv;
-			ctx.desc = p;
-			ctx.ndesc = np;
-
-			skb = stmmac_xdp_run_prog(priv, &ctx.xdp);
-			/* Due xdp_adjust_tail: DMA sync for_device
-			 * cover max len CPU touch
-			 */
-			sync_len = ctx.xdp.data_end - ctx.xdp.data_hard_start -
-				   buf->page_offset;
-			sync_len = max(sync_len, pre_len);
-
-			/* For Not XDP_PASS verdict */
-			if (IS_ERR(skb)) {
-				unsigned int xdp_res = -PTR_ERR(skb);
-
-				if (xdp_res & STMMAC_XDP_CONSUMED) {
-					page_pool_put_page(rx_q->page_pool,
-							   virt_to_head_page(ctx.xdp.data),
-							   sync_len, true);
-					buf->page = NULL;
-					rx_dropped++;
-					goto next;
-				} else if (xdp_res & (STMMAC_XDP_TX |
-						      STMMAC_XDP_REDIRECT)) {
-					xdp_status |= xdp_res;
-					buf->page = NULL;
-					goto next;
-				}
-			}
-		}
-
-		if (!skb) {
-			unsigned int head_pad_len;
-
-			/* XDP program may expand or reduce tail */
-			buf1_len = ctx.xdp.data_end - ctx.xdp.data;
-
-			skb = napi_build_skb(page_address(buf->page),
-					     rx_q->napi_skb_frag_size);
-			if (!skb) {
-				page_pool_recycle_direct(rx_q->page_pool,
-							 buf->page);
-				buf->page = NULL;
-				rx_dropped++;
-				count++;
-				goto drain_data;
-			}
-
-			/* XDP program may adjust header */
-			head_pad_len = ctx.xdp.data - ctx.xdp.data_hard_start;
-			skb_reserve(skb, head_pad_len);
-			skb_put(skb, buf1_len);
-			skb_mark_for_recycle(skb);
 			buf->page = NULL;
 		} else if (buf1_len) {
-			dma_sync_single_for_cpu(priv->device, buf->addr,
-						buf1_len, dma_dir);
-			skb_add_rx_frag(skb, skb_shinfo(skb)->nr_frags,
-					buf->page, buf->page_offset, buf1_len,
-					priv->dma_conf.dma_buf_sz);
+			if (!stmmac_build_xdp_frags(priv, rx_q, buf1_len,
+						    buf->page,
+						    buf->page_offset,
+						    dma_dir, &ctx.xdp)) {
+				if (!error)
+					rx_dropped++;
+				error = 1;
+			}
 			buf->page = NULL;
 		}
 
 		if (buf2_len) {
-			dma_sync_single_for_cpu(priv->device, buf->sec_addr,
-						buf2_len, dma_dir);
-			skb_add_rx_frag(skb, skb_shinfo(skb)->nr_frags,
-					buf->sec_page, 0, buf2_len,
-					priv->dma_conf.dma_buf_sz);
+			if (!stmmac_build_xdp_frags(priv, rx_q, buf2_len,
+						    buf->sec_page, 0,
+						    dma_dir, &ctx.xdp)) {
+				if (!error)
+					rx_dropped++;
+				error = 1;
+			}
 			buf->sec_page = NULL;
 		}
+		frames++;
 
-drain_data:
 		if (likely(status & rx_not_ls))
 			goto read_again;
-		if (!skb)
-			continue;
 
-		/* Got entire packet into SKB. Finish it. */
+		/* ACS is disabled; strip manually. */
+		len -= ETH_FCS_LEN;
+		if (stmmac_xdp_shrink_tail(rx_q, &ctx.xdp, ETH_FCS_LEN)) {
+			if (!error)
+				rx_dropped++;
+			error = 1;
+		}
+
+		if (unlikely(error))
+			goto error_free_frag;
+
+		pre_len = ctx.xdp.data_end - ctx.xdp.data_hard_start;
+
+		ctx.desc = p;
+		ctx.ndesc = np;
+
+		skb = stmmac_xdp_run_prog(priv, &ctx.xdp);
+		/* Due xdp_adjust_tail: DMA sync for_device
+		 * cover max len CPU touch
+		 */
+		sync_len = ctx.xdp.data_end - ctx.xdp.data_hard_start;
+		sync_len = max(sync_len, pre_len);
+
+		/* For Not XDP_PASS verdict */
+		if (IS_ERR(skb)) {
+			unsigned int xdp_res = -PTR_ERR(skb);
+
+			if (xdp_res & STMMAC_XDP_CONSUMED) {
+				stmmac_xdp_put_buff(rx_q, &ctx.xdp, sync_len,
+						    true);
+				rx_dropped++;
+			} else if (xdp_res & (STMMAC_XDP_TX |
+					      STMMAC_XDP_REDIRECT)) {
+				xdp_status |= xdp_res;
+			}
 
-		/* Remove FCS if needed */
-		if (!fcs_stripped && pskb_trim(skb, len)) {
-			dev_kfree_skb_any(skb);
-			rx_dropped++;
 			goto next;
 		}
 
+		skb = stmmac_build_skb(&ctx.xdp);
+		if (!skb) {
+			rx_dropped++;
+			goto error_free_frag;
+		}
+
+		/* Got entire packet into SKB. Finish it. */
 		stmmac_get_rx_hwtstamp(priv, p, np, skb);
 
 		if (priv->hw->hw_vlan_en) {
@@ -6078,15 +6150,23 @@ static int stmmac_rx(struct stmmac_priv *priv, int limit, u32 queue)
 		rx_bytes += len;
 next:
 		in_progress = false;
-		skb = NULL;
+		frames = 0;
+		count++;
+		continue;
+error_free_frag:
+		if (frames)
+			stmmac_xdp_put_buff(rx_q, &ctx.xdp, -1, true);
+		in_progress = false;
+		frames = 0;
 		count++;
 	}
 
-	if (in_progress || skb) {
-		rx_q->state_saved = true;
-		rx_q->state.skb = skb;
+	if (in_progress || frames) {
+		rx_q->state.xdp = ctx.xdp;
+		rx_q->state.frames = frames;
 		rx_q->state.error = error;
 		rx_q->state.len = len;
+		rx_q->state_saved = true;
 	}
 
 	stmmac_finalize_xdp_rx(priv, xdp_status);
@@ -8106,6 +8186,7 @@ static int __stmmac_dvr_probe(struct device *device,
 	ndev->hw_features = NETIF_F_SG | NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM |
 			    NETIF_F_RXCSUM;
 	ndev->xdp_features = NETDEV_XDP_ACT_BASIC | NETDEV_XDP_ACT_REDIRECT |
+			     NETDEV_XDP_ACT_RX_SG |
 			     NETDEV_XDP_ACT_XSK_ZEROCOPY;
 
 	/* Default qdisc num_tx_queues */
@@ -8429,9 +8510,12 @@ static void stmmac_reset_rx_queue(struct stmmac_priv *priv, u32 queue)
 {
 	struct stmmac_rx_queue *rx_q = &priv->dma_conf.rx_queue[queue];
 
-	dev_kfree_skb_any(rx_q->state.skb);
-	rx_q->state.skb = NULL;
+	if (rx_q->state.frames && !rx_q->xsk_pool) {
+		stmmac_xdp_put_buff(rx_q, &rx_q->state.xdp, -1, false);
+		rx_q->state.frames = 0;
+	}
 	rx_q->state_saved = false;
+
 	rx_q->cur_rx = 0;
 	rx_q->dirty_rx = 0;
 }

-- 
2.56.0

Re: [PATCH net-next v8 0/5] net: stmmac: introduce XDP rx multi-buff support

[netdev-bot+sinfo]

Hi!

This is an automated message. This series looks like a fix, but its
commit messages seem to be missing some information:

 - How the issue was discovered, e.g. hit in production, hit during
   development, syzbot report, manual code inspection, LLM or static
   analysis tool scan.

 - Whether the issue was actually triggered, or is only theoretical
   (e.g. found by code inspection). If it was triggered please include
   the symptoms, like the stack trace or error messages.

 - What hardware the change was tested on. For driver fixes please
   mention the device (and if relevant firmware version) used for
   testing, or say that the change was not tested on real hardware.

Please do not repost the series just to address the above. Instead,
reply to this email with the missing information, so that reviewers
can take it into account. If the series needs another revision for
other reasons, please include the information in the commit messages
then.

The evaluation is done by an LLM so it may be wrong, if you think
that is the case please reply and explain.

Re: [PATCH net-next v8 0/5] net: stmmac: introduce XDP rx multi-buff support

[Lorenzo Bianconi]

[-- Attachment #1: Type: text/plain, Size: 1317 bytes --]

> Hi!
> 
> This is an automated message. This series looks like a fix, but its
> commit messages seem to be missing some information:
> 
>  - How the issue was discovered, e.g. hit in production, hit during
>    development, syzbot report, manual code inspection, LLM or static
>    analysis tool scan.

The issues have been reported by sashiko.

> 
>  - Whether the issue was actually triggered, or is only theoretical
>    (e.g. found by code inspection). If it was triggered please include
>    the symptoms, like the stack trace or error messages.

The issues are real bugs spotted by sashiko.

> 
>  - What hardware the change was tested on. For driver fixes please
>    mention the device (and if relevant firmware version) used for
>    testing, or say that the change was not tested on real hardware.

I tested them on a Qualcomm RB3-gen2 board.

Regards,
Lorenzo

> 
> Please do not repost the series just to address the above. Instead,
> reply to this email with the missing information, so that reviewers
> can take it into account. If the series needs another revision for
> other reasons, please include the information in the commit messages
> then.
> 
> The evaluation is done by an LLM so it may be wrong, if you think
> that is the case please reply and explain.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH net-next v8 3/5] net: stmmac: drop multi-buffer frames in XDP mode

[Nicolai Buchwitz]

On 18.10.2026 19:07, Lorenzo Bianconi wrote:
> stmmac_rx() runs the XDP program once per descriptor buffer and uses
> skb == NULL as the only marker for the start of a frame. A frame larger
> than one buffer is split by the hardware across several descriptors;
> GMAC4 enables jumbo frames unconditionally (GMAC_CONFIG_JE in
> GMAC_CORE_INIT), so an on-link peer can deliver such a frame even when
> the interface MTU is left at 1500.
> 
> When an XDP verdict ends a non-last descriptor, skb is cleared and the
> next descriptor of the same hardware frame is parsed as a new frame.
> For XDP_CONSUMED this happens via read_again with len preserved, so
> stmmac_rx_buf1_len() then returns min(dma_buf_sz, plen - len) for the
> last descriptor, which can be as small as 0-3 bytes. buf1_len -=
> ETH_FCS_LEN underflows and xdp_prepare_buff() puts data_end far past
> data; on XDP_PASS this produces an skb whose length is close to 4G that
> reaches the network stack (skb->tail and skb->len are 32-bit and wrap
> below skb->end, so skb_over_panic() does not fire). A 4+ byte tail is
> instead offered to the XDP program and delivered as a standalone
> mid-payload Ethernet frame on XDP_PASS. XDP_TX/XDP_REDIRECT instead
> falls through to the next iteration, resetting the running length, so
> the tail is re-parsed as a full-size new frame.
> 
> The same fault exists when a single descriptor uses its second buffer:
> GMAC4/XGMAC always provide a second data buffer per descriptor, so a
> frame up to 2 * dma_buf_sz can land on a single descriptor while the 
> XDP
> program still only sees the first buffer. XDP_TX then transmits a
> truncated frame, and XDP_PASS appends the second buffer only after the
> program has already run over the first one.
> 
> XDP does not support scatter-gather frames until the multi-buffer
> rework, so drop the whole frame instead: mark it as errored as soon as
> a non-last descriptor, or a descriptor using its second buffer, is seen
> while an XDP program is attached, drain and release every descriptor -
> including the last. Removing the re-parse lets both XDP verdict 
> branches
> simply complete the packet via next.
> 
> Note: the subsequent "net: stmmac: rework stmmac_rx to support XDP rx
> multi-buff" accumulates the whole frame into a single xdp_buff and
> supersedes this patch; the fix is kept so the net portion of the series
> stands on its own.
> 
> Fixes: 5fabb01207a2 ("net: stmmac: Add initial XDP support")
> Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@oss.qualcomm.com>
> ---
> [...]

Reviewed-by: Nicolai Buchwitz <nb@tipi-net.de>

Thanks,
Nicolai

Re: [PATCH net-next v8 4/5] net: stmmac: fix VLAN tag stripping on non-linear RX frames

[Nicolai Buchwitz]

On 18.10.2026 19:07, Lorenzo Bianconi wrote:
> stmmac_rx_vlan() reads the VLAN header straight from skb->data and
> removes it with skb_pull(), assuming the header is always in the linear
> part of the skb. That is not true once non-linear skbs can reach it: a
> program loaded with BPF_F_XDP_HAS_FRAGS can pass up a fragmented frame
> whose linear head has been shortened to as little as ETH_HLEN by
> bpf_xdp_adjust_head(), and the unguarded skb_pull() then drives 
> skb->len
> below skb->data_len, tripping BUG() in __skb_pull().
> 
> Pull the VLAN header into the linear part with pskb_may_pull() before
> touching it and re-derive the header pointer afterwards, as
> pskb_may_pull() may reallocate the skb head. Also move the XDP metadata
> written with bpf_xdp_adjust_meta() together with the MAC header when 
> the
> tag is popped, so data_meta still points at the metadata after
> eth_type_trans() anchors mac_header, instead of at the first bytes of
> the destination MAC. Return an error so the callers can drop the frame
> when the header cannot be pulled.
> 
> Fixes: bba2556efad6 ("net: stmmac: Enable RX via AF_XDP zero-copy")
> Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@oss.qualcomm.com>

> [...]

Reviewed-by: Nicolai Buchwitz <nb@tipi-net.de>

Thanks,
Nicolai

Re: [PATCH net-next v8 5/5] net: stmmac: rework stmmac_rx to support XDP rx multi-buff

[Nicolai Buchwitz]

On 18.10.2026 19:07, Lorenzo Bianconi wrote:
> The preceding commit dropped frames split across multiple descriptors, 
> or
> using a second buffer, while XDP was enabled. This prevented the
> per-descriptor XDP path from re-parsing frame tails and from applying 
> FCS
> handling to truncated buffers.
> 
> Rework stmmac_rx() to accumulate all the descriptors that make up a 
> frame
> in an xdp_buff, so the XDP program runs on the full (possibly 
> fragmented)
> packet instead of just the first buffer. Frames split across 
> descriptors
> or buffers are now passed to programs loaded with BPF_F_XDP_HAS_FRAGS;
> programs without that capability continue to drop fragmented frames. 
> When
> the frame is not consumed by the program, assemble the skb from the 
> head
> buffer and the collected fragments via napi_build_skb()/
> xdp_update_skb_frags_info().
> 
> Note that XDP still rejects an MTU larger than ETH_DATA_LEN
> (stmmac_xdp_set_prog(), stmmac_change_mtu()) and disables SPH, so an
> ordinary frame fits in a single buffer. This path is therefore 
> exercised
> by frames that do span several descriptors (for example jumbo frames 
> the
> MAC admits through its jumbo-enable bit) and is groundwork for 
> extending
> XDP to jumbo/SPH; it does not enable XDP jumbo support by itself.
> 
> To do so, store the in-progress xdp_buff in rx_q->state instead of the
> partially built skb, so the accumulated head and fragments survive a 
> NAPI
> poll boundary (mid-frame dma_own or dirty_rx break). The state is saved
> only while a frame is in progress and cleared once it completes, 
> leaving
> it untouched when the poll does not process anything (e.g. netpoll 
> invoked
> with a zero budget).
> 
> In addition:
> - advertise NETDEV_XDP_ACT_RX_SG, since the RX path can now deliver
>   non-linear buffers to XDP;
> - build the skb head with napi_build_skb() passing xdp->frame_sz, so
>   skb_shinfo() lands on the same shared_info the fragments were 
> accumulated
>   into;
> - carry the XDP metadata written with bpf_xdp_adjust_meta() into the 
> skb
>   via skb_metadata_set(), so it is no longer dropped and TC/BPF 
> programs
>   can read it through data_meta;
> - attach fragments with xdp_buff_add_frag(), which initializes all the
>   shared_info fields and takes care of the pfmemalloc bit;
> - charge fragments the whole page_pool page size (xdp->frame_sz) as
>   truesize instead of the old dma_buf_sz; with SPH/buf2 most frames 
> carry
>   a fragment, so skb->truesize grows by roughly 1.5x on 4K pages and 2x 
> on
>   64K pages, which reduces the effective socket receive buffer. The new
>   value is more accurate now that a fragment is a full page_pool page, 
> but
>   it is a behavior change;
> - release the buffers collected in the xdp_buff when a frame is dropped
>   on RX errors; the previous code relied on dev_kfree_skb() recycling 
> the
>   skb frags, which no longer applies now that the head and fragments 
> live
>   in an xdp_buff rather than a partially built skb;
> - drop the whole frame when it would exceed MAX_SKB_FRAGS, where the
>   previous skb_add_rx_frag() calls were unbounded;
> - strip the Ethernet FCS from the accumulated xdp_buff with a 
> driver-local
>   tail shrink (stmmac_xdp_shrink_tail()) for both linear and fragmented
>   frames, instead of trimming the skb or the first descriptor length.
> 
> Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@oss.qualcomm.com>

> [...]

Reviewed-by: Nicolai Buchwitz <nb@tipi-net.de>

Thanks,
Nicolai

Re: [PATCH net-next v8 2/5] net: stmmac: drop split frames on AF_XDP zero-copy RX

[Nicolai Buchwitz]

Hi Lorenzo

On 18.10.2026 19:07, Lorenzo Bianconi wrote:
> stmmac_rx_zc() assumes a 1:1 mapping between a received frame and an
> XSK pool buffer and drops any frame that spans more than one RX
> descriptor:
> 
>         /* XSK pool expects RX frame 1:1 mapped to XSK buffer */
>         if (likely(status & rx_not_ls)) {
>                 xsk_buff_free(buf->xdp);
>                 buf->xdp = NULL;
>                 dirty++;
>                 count++;
>                 goto read_again;
>         }
> 
> The non-last descriptors are freed and dropped, but the frame is not
> marked as errored. When the poll reaches the last descriptor of the
> same frame, error is still 0, so the fragment is handled as a complete
> frame: it is passed to the XDP program, copied into an skb on XDP_PASS,
> or converted for XDP_TX. The tail starts in the middle of the original
> payload at a predictable offset and, parsed as a fresh frame (len == 
> 0),
> stmmac_rx_buf1_len() reports the full dma_buf_sz length for it even
> though the DMA engine only wrote the tail remainder into that buffer, 
> so
> the reads extend past the received bytes and can reach beyond the end 
> of
> the XSK pool buffer. GMAC4 always enables jumbo frames (GMAC_CONFIG_JE
> in GMAC_CORE_INIT), so an on-link peer can trigger such a split even
> when the interface MTU is left at 1500.
> 
> Set error when the 1:1 mapping check drops a descriptor, so the
> remaining descriptors of the frame - including the last one - are
> drained and released with xsk_buff_free() instead of being processed as
> a new frame. Account every freed buffer with dirty and count the
> dropped frame once in the poll budget / rx_pkt_n. The direct 1:1
> mapping drop also increments rx_dropped.
> 
> Normalize the legacy normal and enhanced descriptor callbacks to report
> rx_not_ls together with discard_frame for non-last descriptors. This
> allows both stmmac_rx_zc() and stmmac_rx() to drain split frames on
> dwmac100 and dwmac1000 instead of reparsing the final fragment as a new
> packet.
> 
> Fixes: bba2556efad6 ("net: stmmac: Enable RX via AF_XDP zero-copy")
> Reviewed-by: Nicolai Buchwitz <nb@tipi-net.de>

This grew quite a bit since v6 (legacy desc changes, stmmac_rx() etc),
so my R-b should have been dropped. I had another look and two small
notes below.

> Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@oss.qualcomm.com>
> ---

> [...]

> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> @@ -5695,18 +5695,21 @@ static int stmmac_rx_zc(struct stmmac_priv 
> *priv, int limit, u32 queue)
>  		if (priv->extend_desc)
>  			stmmac_rx_extended_status(priv, &priv->xstats,
>  						  rx_q->dma_erx + entry);
> -		if (unlikely(status == discard_frame)) {
> -			xsk_buff_free(buf->xdp);
> -			buf->xdp = NULL;
> -			dirty++;
> +
> +		if (unlikely(status & discard_frame)) {
>  			error = 1;
>  			if (!priv->hwts_rx_en)
>  				rx_errors++;
>  		}

There's no !error check here, so on the legacy descriptors a split
frame now counts once per descriptor in rx_errors. stmmac_rx() in 3/5
counts it once. Could you add the !error check here too?

> [...]

> @@ -5883,7 +5887,8 @@ static int stmmac_rx(struct stmmac_priv *priv, 
> int limit, u32 queue)
> 
>  		if (priv->extend_desc)
>  			stmmac_rx_extended_status(priv, &priv->xstats, rx_q->dma_erx + 
> entry);
> -		if (unlikely(status == discard_frame)) {
> +
> +		if (unlikely(status & discard_frame)) {

AFAIU this also changes GMAC4: a frame with an error plus an IP header
error returns discard_frame | csum_none, which the old "==" let
through. Dropping it is right, but maybe worth a line in the commit
message?

> [...]

With the !error check added:

Reviewed-by: Nicolai Buchwitz <nb@tipi-net.de>

Thanks,
Nicolai

Re: [PATCH net-next v8 2/5] net: stmmac: drop split frames on AF_XDP zero-copy RX

[Lorenzo Bianconi]

[-- Attachment #1: Type: text/plain, Size: 4324 bytes --]

> Hi Lorenzo
> 
> On 18.10.2026 19:07, Lorenzo Bianconi wrote:
> > stmmac_rx_zc() assumes a 1:1 mapping between a received frame and an
> > XSK pool buffer and drops any frame that spans more than one RX
> > descriptor:
> > 
> >         /* XSK pool expects RX frame 1:1 mapped to XSK buffer */
> >         if (likely(status & rx_not_ls)) {
> >                 xsk_buff_free(buf->xdp);
> >                 buf->xdp = NULL;
> >                 dirty++;
> >                 count++;
> >                 goto read_again;
> >         }
> > 
> > The non-last descriptors are freed and dropped, but the frame is not
> > marked as errored. When the poll reaches the last descriptor of the
> > same frame, error is still 0, so the fragment is handled as a complete
> > frame: it is passed to the XDP program, copied into an skb on XDP_PASS,
> > or converted for XDP_TX. The tail starts in the middle of the original
> > payload at a predictable offset and, parsed as a fresh frame (len == 0),
> > stmmac_rx_buf1_len() reports the full dma_buf_sz length for it even
> > though the DMA engine only wrote the tail remainder into that buffer, so
> > the reads extend past the received bytes and can reach beyond the end of
> > the XSK pool buffer. GMAC4 always enables jumbo frames (GMAC_CONFIG_JE
> > in GMAC_CORE_INIT), so an on-link peer can trigger such a split even
> > when the interface MTU is left at 1500.
> > 
> > Set error when the 1:1 mapping check drops a descriptor, so the
> > remaining descriptors of the frame - including the last one - are
> > drained and released with xsk_buff_free() instead of being processed as
> > a new frame. Account every freed buffer with dirty and count the
> > dropped frame once in the poll budget / rx_pkt_n. The direct 1:1
> > mapping drop also increments rx_dropped.
> > 
> > Normalize the legacy normal and enhanced descriptor callbacks to report
> > rx_not_ls together with discard_frame for non-last descriptors. This
> > allows both stmmac_rx_zc() and stmmac_rx() to drain split frames on
> > dwmac100 and dwmac1000 instead of reparsing the final fragment as a new
> > packet.
> > 
> > Fixes: bba2556efad6 ("net: stmmac: Enable RX via AF_XDP zero-copy")
> > Reviewed-by: Nicolai Buchwitz <nb@tipi-net.de>
> 
> This grew quite a bit since v6 (legacy desc changes, stmmac_rx() etc),
> so my R-b should have been dropped. I had another look and two small
> notes below.

Hi Nicolai,

ack, right.

> 
> > Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@oss.qualcomm.com>
> > ---
> 
> > [...]
> 
> > --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > @@ -5695,18 +5695,21 @@ static int stmmac_rx_zc(struct stmmac_priv
> > *priv, int limit, u32 queue)
> >  		if (priv->extend_desc)
> >  			stmmac_rx_extended_status(priv, &priv->xstats,
> >  						  rx_q->dma_erx + entry);
> > -		if (unlikely(status == discard_frame)) {
> > -			xsk_buff_free(buf->xdp);
> > -			buf->xdp = NULL;
> > -			dirty++;
> > +
> > +		if (unlikely(status & discard_frame)) {
> >  			error = 1;
> >  			if (!priv->hwts_rx_en)
> >  				rx_errors++;
> >  		}
> 
> There's no !error check here, so on the legacy descriptors a split
> frame now counts once per descriptor in rx_errors. stmmac_rx() in 3/5
> counts it once. Could you add the !error check here too?

ack, I agree we should align stmmac_rx() and stmmac_rx_zc(). In order to avoid
reposting the full series for this change, do you agree to address it as a
follow-up patch?

Regards,
Lorenzo

> 
> > [...]
> 
> > @@ -5883,7 +5887,8 @@ static int stmmac_rx(struct stmmac_priv *priv, int
> > limit, u32 queue)
> > 
> >  		if (priv->extend_desc)
> >  			stmmac_rx_extended_status(priv, &priv->xstats, rx_q->dma_erx +
> > entry);
> > -		if (unlikely(status == discard_frame)) {
> > +
> > +		if (unlikely(status & discard_frame)) {
> 
> AFAIU this also changes GMAC4: a frame with an error plus an IP header
> error returns discard_frame | csum_none, which the old "==" let
> through. Dropping it is right, but maybe worth a line in the commit
> message?
> 
> > [...]
> 
> With the !error check added:
> 
> Reviewed-by: Nicolai Buchwitz <nb@tipi-net.de>
> 
> Thanks,
> Nicolai

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH net-next v8 2/5] net: stmmac: drop split frames on AF_XDP zero-copy RX

[Nicolai Buchwitz]

On 19.10.2026 14:17, Lorenzo Bianconi wrote:
>> Hi Lorenzo
>> 
>> On 18.10.2026 19:07, Lorenzo Bianconi wrote:
>> > stmmac_rx_zc() assumes a 1:1 mapping between a received frame and an
>> > XSK pool buffer and drops any frame that spans more than one RX
>> > descriptor:
>> >
>> >         /* XSK pool expects RX frame 1:1 mapped to XSK buffer */
>> >         if (likely(status & rx_not_ls)) {
>> >                 xsk_buff_free(buf->xdp);
>> >                 buf->xdp = NULL;
>> >                 dirty++;
>> >                 count++;
>> >                 goto read_again;
>> >         }
>> >
>> > The non-last descriptors are freed and dropped, but the frame is not
>> > marked as errored. When the poll reaches the last descriptor of the
>> > same frame, error is still 0, so the fragment is handled as a complete
>> > frame: it is passed to the XDP program, copied into an skb on XDP_PASS,
>> > or converted for XDP_TX. The tail starts in the middle of the original
>> > payload at a predictable offset and, parsed as a fresh frame (len == 0),
>> > stmmac_rx_buf1_len() reports the full dma_buf_sz length for it even
>> > though the DMA engine only wrote the tail remainder into that buffer, so
>> > the reads extend past the received bytes and can reach beyond the end of
>> > the XSK pool buffer. GMAC4 always enables jumbo frames (GMAC_CONFIG_JE
>> > in GMAC_CORE_INIT), so an on-link peer can trigger such a split even
>> > when the interface MTU is left at 1500.
>> >
>> > Set error when the 1:1 mapping check drops a descriptor, so the
>> > remaining descriptors of the frame - including the last one - are
>> > drained and released with xsk_buff_free() instead of being processed as
>> > a new frame. Account every freed buffer with dirty and count the
>> > dropped frame once in the poll budget / rx_pkt_n. The direct 1:1
>> > mapping drop also increments rx_dropped.
>> >
>> > Normalize the legacy normal and enhanced descriptor callbacks to report
>> > rx_not_ls together with discard_frame for non-last descriptors. This
>> > allows both stmmac_rx_zc() and stmmac_rx() to drain split frames on
>> > dwmac100 and dwmac1000 instead of reparsing the final fragment as a new
>> > packet.
>> >
>> > Fixes: bba2556efad6 ("net: stmmac: Enable RX via AF_XDP zero-copy")
>> > Reviewed-by: Nicolai Buchwitz <nb@tipi-net.de>
>> 
>> This grew quite a bit since v6 (legacy desc changes, stmmac_rx() etc),
>> so my R-b should have been dropped. I had another look and two small
>> notes below.
> 
> Hi Nicolai,
> 
> ack, right.
> 
>> 
>> > Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@oss.qualcomm.com>
>> > ---
>> 
>> > [...]
>> 
>> > --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
>> > +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
>> > @@ -5695,18 +5695,21 @@ static int stmmac_rx_zc(struct stmmac_priv
>> > *priv, int limit, u32 queue)
>> >  		if (priv->extend_desc)
>> >  			stmmac_rx_extended_status(priv, &priv->xstats,
>> >  						  rx_q->dma_erx + entry);
>> > -		if (unlikely(status == discard_frame)) {
>> > -			xsk_buff_free(buf->xdp);
>> > -			buf->xdp = NULL;
>> > -			dirty++;
>> > +
>> > +		if (unlikely(status & discard_frame)) {
>> >  			error = 1;
>> >  			if (!priv->hwts_rx_en)
>> >  				rx_errors++;
>> >  		}
>> 
>> There's no !error check here, so on the legacy descriptors a split
>> frame now counts once per descriptor in rx_errors. stmmac_rx() in 3/5
>> counts it once. Could you add the !error check here too?
> 
> ack, I agree we should align stmmac_rx() and stmmac_rx_zc(). In order 
> to avoid
> reposting the full series for this change, do you agree to address it 
> as a
> follow-up patch?

Would be ok for me if nothing else is flagged in this version. In the 
end it will
be Maxime's / the net dev maintainers decision :)

> 
> Regards,
> Lorenzo
> 
>> 
>> > [...]
>> 
>> > @@ -5883,7 +5887,8 @@ static int stmmac_rx(struct stmmac_priv *priv, int
>> > limit, u32 queue)
>> >
>> >  		if (priv->extend_desc)
>> >  			stmmac_rx_extended_status(priv, &priv->xstats, rx_q->dma_erx +
>> > entry);
>> > -		if (unlikely(status == discard_frame)) {
>> > +
>> > +		if (unlikely(status & discard_frame)) {
>> 
>> AFAIU this also changes GMAC4: a frame with an error plus an IP header
>> error returns discard_frame | csum_none, which the old "==" let
>> through. Dropping it is right, but maybe worth a line in the commit
>> message?
>> 
>> > [...]
>> 
>> With the !error check added:
>> 
>> Reviewed-by: Nicolai Buchwitz <nb@tipi-net.de>
>> 
>> Thanks,
>> Nicolai

Re: [PATCH net-next v8 4/5] net: stmmac: fix VLAN tag stripping on non-linear RX frames

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] net: stmmac: unconditional VLAN_ETH_HLEN pull in stmmac_rx_vlan()
--

commit 0a2d776ad3e3751ad20c9dd1144b0c000e67acc1
Author: Lorenzo Bianconi <lorenzo.bianconi@oss.qualcomm.com>

net: stmmac: fix VLAN tag stripping on non-linear RX frames

This commit updates stmmac_rx_vlan() to linearize the VLAN header with
pskb_may_pull(), preserve XDP metadata when popping the VLAN tag, and drop
the frame in stmmac_rx() and stmmac_dispatch_skb_zc() if pulling fails.

> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> index e929cc0f23125..04dc6dcaa435d 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> @@ -5095,22 +5095,30 @@ static netdev_features_t stmmac_features_check(struct sk_buff *skb,
>  	return vlan_features_check(skb, features);
>  }
>  
> -static void stmmac_rx_vlan(struct net_device *dev, struct sk_buff *skb)
> +static int stmmac_rx_vlan(struct net_device *dev, struct sk_buff *skb)
>  {
> -	struct vlan_ethhdr *veth = skb_vlan_eth_hdr(skb);
> -	__be16 vlan_proto = veth->h_vlan_proto;
> -	u16 vlanid;
> +	struct vlan_ethhdr *veth;
>  
> -	if ((vlan_proto == htons(ETH_P_8021Q) &&
> +	if (!pskb_may_pull(skb, VLAN_ETH_HLEN))
> +		return -EINVAL;
> +
> +	veth = skb_vlan_eth_hdr(skb);
> +	if ((veth->h_vlan_proto == htons(ETH_P_8021Q) &&
>  	     dev->features & NETIF_F_HW_VLAN_CTAG_RX) ||
> -	    (vlan_proto == htons(ETH_P_8021AD) &&
> +	    (veth->h_vlan_proto == htons(ETH_P_8021AD) &&
>  	     dev->features & NETIF_F_HW_VLAN_STAG_RX)) {

[Severity: Medium]
Can calling pskb_may_pull(skb, VLAN_ETH_HLEN) unconditionally at the start
of stmmac_rx_vlan() cause valid untagged frames to be dropped, or force
unnecessary fragment pulls on non-linear untagged packets?

When priv->hw->hw_vlan_en is false, stmmac_rx() and stmmac_dispatch_skb_zc()
call stmmac_rx_vlan() on every received frame, even when VLAN RX stripping
offloads are disabled in dev->features or the packet is untagged.

XDP helpers such as bpf_xdp_adjust_head(), bpf_xdp_adjust_tail(), and
bpf_xdp_frags_shrink_tail() allow shrinking a frame down to ETH_HLEN (14
bytes):

net/core/filter.c:bpf_xdp_adjust_head() {
	...
	if (unlikely(data < data_start ||
		     data > xdp->data_end - ETH_HLEN))
		return -EINVAL;
	...
}

For an untagged frame of 14 to 17 bytes (where ETH_HLEN <= skb->len <
VLAN_ETH_HLEN), pskb_may_pull(skb, VLAN_ETH_HLEN) fails and returns -EINVAL,
causing both stmmac_rx() and stmmac_dispatch_skb_zc() to free and drop the
skb via dev_kfree_skb_any(skb).

Additionally, for non-linear untagged frames with skb->len >= VLAN_ETH_HLEN
whose linear head is ETH_HLEN bytes (for example, after bpf_xdp_adjust_head()
with BPF_F_XDP_HAS_FRAGS, or with GMAC4 split header mode splitting after the
14-byte L2 header), pulling VLAN_ETH_HLEN before checking h_vlan_proto forces
__pskb_pull_tail() on every untagged packet.

Since veth->h_vlan_proto sits within the first ETH_HLEN bytes of the header,
should stmmac_rx_vlan() only pull ETH_HLEN initially (or check dev->features
first), and defer pskb_may_pull(skb, VLAN_ETH_HLEN) and reloading veth until
after confirming that veth->h_vlan_proto matches a VLAN ethertype to be
stripped?

> +		__be16 vlan_proto = veth->h_vlan_proto;
> +		u16 vlanid = ntohs(veth->h_vlan_TCI);
> +		u8 meta_len = skb_metadata_len(skb);
> +
>  		/* pop the vlan tag */
> -		vlanid = ntohs(veth->h_vlan_TCI);
> -		memmove(skb->data + VLAN_HLEN, veth, ETH_ALEN * 2);
> +		memmove(skb->data - meta_len + VLAN_HLEN,
> +			skb->data - meta_len, meta_len + ETH_ALEN * 2);
>  		skb_pull(skb, VLAN_HLEN);
>  		__vlan_hwaccel_put_tag(skb, vlan_proto, vlanid);
>  	}
> +
> +	return 0;
>  }

[ ... ]

> @@ -5530,12 +5538,15 @@ static void stmmac_dispatch_skb_zc(struct stmmac_priv *priv, u32 queue,
>  	}
>  
>  	stmmac_get_rx_hwtstamp(priv, p, np, skb);
> -	if (priv->hw->hw_vlan_en)
> +	if (priv->hw->hw_vlan_en) {
>  		/* MAC level stripping. */
>  		stmmac_rx_hw_vlan(priv, priv->hw, p, skb);
> -	else
> -		/* Driver level stripping. */
> -		stmmac_rx_vlan(priv->dev, skb);
> +	} else if (stmmac_rx_vlan(priv->dev, skb)) {
> +		dev_kfree_skb_any(skb);
> +		priv->xstats.rx_dropped++;
> +		return;
> +	}
> +
>  	skb->protocol = eth_type_trans(skb, priv->dev);
>  
>  	if (unlikely(!coe) || !stmmac_has_ip_ethertype(skb))
> @@ -6041,12 +6052,14 @@ static int stmmac_rx(struct stmmac_priv *priv, int limit, u32 queue)
>  
>  		stmmac_get_rx_hwtstamp(priv, p, np, skb);
>  
> -		if (priv->hw->hw_vlan_en)
> +		if (priv->hw->hw_vlan_en) {
>  			/* MAC level stripping. */
>  			stmmac_rx_hw_vlan(priv, priv->hw, p, skb);
> -		else
> -			/* Driver level stripping. */
> -			stmmac_rx_vlan(priv->dev, skb);
> +		} else if (stmmac_rx_vlan(priv->dev, skb)) {
> +			dev_kfree_skb_any(skb);
> +			rx_dropped++;
> +			goto next;
> +		}
>  
>  		skb->protocol = eth_type_trans(skb, priv->dev);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261018-stmmac-rx-mb-v8-0-5f58c0b25849@oss.qualcomm.com?part=4

Re: [PATCH net-next v8 1/5] net: stmmac: take ownership of saved RX state at poll entry

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] The authorship and Signed-off-by chain disagree.

Pre-existing issues:
- [High] At this commit, stmmac_rx() still uses `skb == NULL` to decide
  that a descriptor starts a new frame.
- [High] This problem wasn't introduced by this patch, but the commit
  message relies on it being absent.

--

net: stmmac: take ownership of saved RX state at poll entry

This commit makes stmmac_rx() and stmmac_rx_zc() take the saved
partial-frame state at poll entry. A new in_progress flag tracks whether
a frame is still incomplete, and the state is saved again only in that
case. It also frees the saved skb in stmmac_reset_rx_queue() and in
__free_dma_rx_desc_resources().

> Signed-off-by: James Hilliard <james.hilliard1@gmail.com>
> Reviewed-by: Nicolai Buchwitz <nb@tipi-net.de>
> Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@oss.qualcomm.com>

[Severity: Low]
This isn't a bug, but the author field and the Signed-off-by chain don't
match. The commit author is Lorenzo Bianconi. The first Signed-off-by is
James Hilliard, and there is no Co-developed-by tag.

If James wrote the patch, should it carry a "From: James Hilliard
<james.hilliard1@gmail.com>" line so git credits him as the author? If
Lorenzo is the author, does James's sign-off need a matching
Co-developed-by tag?

The other patches in the series carry only Lorenzo's sign-off. That
suggests this patch was written by James and picked into the series.

> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> index 53782ab55e125..8fd2acb25fb60 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c

[ ... ]

> @@ -5958,9 +5970,7 @@ static int stmmac_rx(struct stmmac_priv *priv, int limit, u32 queue)
>  						      STMMAC_XDP_REDIRECT)) {
>  					xdp_status |= xdp_res;
>  					buf->page = NULL;
> -					skb = NULL;
> -					count++;
> -					continue;
> +					goto next;
>  				}
>  			}
>  		}

[Severity: High]
This is a pre-existing issue, and this patch doesn't change how it
behaves. What happens here when the XDP verdict is taken on a descriptor
that still has rx_not_ls set?

stmmac_rx() uses skb == NULL to mean a new frame is starting. The goto
next now also clears in_progress, and error stays 0. The remaining
descriptors of the same hardware frame would then each be handled as the
first descriptor of a new frame.

The XDP_CONSUMED branch just above has the same shape:

	skb = NULL;

	if (unlikely((status & rx_not_ls)))
		goto read_again;

So does the napi_build_skb() failure path:

	if (!skb) {
		page_pool_recycle_direct(rx_q->page_pool,
					 buf->page);
		buf->page = NULL;
		rx_dropped++;
		count++;
		goto drain_data;
	}

In each of these three cases, could the middle and tail descriptors be
passed to XDP and built into an skb with a stale or zero len? Could
buf1_len then underflow and produce an skb length near 4G?

An on-link peer can reach this by sending a frame that spans several
descriptors while an XDP program returns a non-PASS verdict. A
napi_build_skb() failure on such a frame reaches it too.

Later patches in this series seem to fix it. "net: stmmac: drop
multi-buffer frames in XDP mode" sets error on non-last descriptors when
XDP is attached. "net: stmmac: rework stmmac_rx to support XDP rx
multi-buff" runs XDP and stmmac_build_skb() only on the last descriptor
and sends build failures to error_free_frag.

[ ... ]

> @@ -8399,6 +8408,9 @@ static void stmmac_reset_rx_queue(struct stmmac_priv *priv, u32 queue)
>  {
>  	struct stmmac_rx_queue *rx_q = &priv->dma_conf.rx_queue[queue];
>  
> +	dev_kfree_skb_any(rx_q->state.skb);
> +	rx_q->state.skb = NULL;
> +	rx_q->state_saved = false;
>  	rx_q->cur_rx = 0;
>  	rx_q->dirty_rx = 0;
>  }

[Severity: High]
This isn't a bug introduced by this patch, but the commit message says
that, without this change, the first poll "resumes that stale skb with
descriptors from the freshly initialized ring". Is the ring actually
re-initialized on the stmmac_resume() path?

stmmac_resume() does:

	stmmac_reset_queues_param(priv);

	stmmac_free_tx_skbufs(priv);
	stmmac_clear_descriptors(priv, &priv->dma_conf);

	ret = stmmac_hw_setup(ndev);

None of these calls refills buf_pool or rewrites the descriptor buffer
addresses. stmmac_clear_rx_descriptors() calls stmmac_init_rx_desc() on
every entry. On dwmac4 that only sets the owner bits:

static void dwmac4_rd_init_rx_desc(struct dma_desc *p, int disable_rx_ic,
				   u8 descriptor_mode, int end, int bfsize)
{
	dwmac4_set_rx_owner(p, disable_rx_ic);
}

Suppose stmmac_rx_refill() hit a page_pool allocation failure before
suspend:

		if (!buf->page) {
			buf->page = page_pool_alloc_pages(rx_q->page_pool, gfp);
			if (!buf->page)
				break;
		}

Then the entries between dirty_rx and cur_rx still have buf->page ==
NULL. On resume, cur_rx and dirty_rx are reset to 0 and every descriptor
goes back to the DMA. When one of those entries completes, can
stmmac_rx() call page_address(buf->page) and napi_build_skb() on a NULL
page?

There is a similar problem on dwmac4/xgmac. Descriptors that the hardware
writes back between napi_disable() in stmmac_suspend() and
stmmac_stop_all_dma() have RDES0/RDES1 overwritten with status words.
If those descriptors are given back to the DMA, would it write to a
bogus buffer address?

The resume sequence is the same at the end of the series.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261018-stmmac-rx-mb-v8-0-5f58c0b25849%40oss.qualcomm.com

Re: [PATCH net-next v8 2/5] net: stmmac: drop split frames on AF_XDP zero-copy RX

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] The patch changes the discard test from `status == discard_frame`
  to `status & discard_frame` in both stmmac_rx() and stmmac_rx_zc().
- [Low] After this patch, stmmac_rx_zc() counts the same event (a frame
  split across RX descriptors on an AF_XDP zero-copy queue) differently…
- [Low] In stmmac_rx_zc() on dwmac4/XGMAC, a split frame can now be
  counted in both rx_dropped and rx_errors.

Pre-existing issues:
- [High] At this commit, stmmac_rx() (non-ZC, XDP attached, GMAC4/XGMAC
  whose non-last descriptors report bare rx_not_ls) still drops a non-…
- [Medium] stmmac_rx_zc() and stmmac_rx() add their local
  rx_dropped/rx_errors into the device-wide `priv->xstats.rx_dropped`…

--

net: stmmac: drop split frames on AF_XDP zero-copy RX

In stmmac_rx_zc(), when the 1:1 XSK mapping check drops a non-last
descriptor, this patch now marks the frame as errored. The rest of the
split frame is then drained, and its tail is no longer handled as a new
frame. The patch also makes the legacy enhanced and normal descriptor
callbacks report rx_not_ls together with discard_frame, and changes the
discard test in stmmac_rx_zc() and stmmac_rx() from == to &.

> diff --git a/drivers/net/ethernet/stmicro/stmmac/enh_desc.c b/drivers/net/ethernet/stmicro/stmmac/enh_desc.c
> index 0512536012251..297e2428a8f9b 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/enh_desc.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/enh_desc.c
> @@ -184,7 +184,7 @@ static int enh_desc_get_rx_status(struct stmmac_extra_stats *x,
>  
>  	if (unlikely(!(rdes0 & RDES0_LAST_DESCRIPTOR))) {
>  		x->rx_length++;
> -		return discard_frame;
> +		return discard_frame | rx_not_ls;
>  	}

[Severity: Low]

With this change and the matching one in ndesc_get_rx_status(), does
stmmac_rx_zc() now count the same split frame differently depending on
the descriptor format?

On dwmac4 and xgmac, dwmac4_wrback_get_rx_status() and
dwxgmac2_get_rx_status() return a bare rx_not_ls for a non-last
descriptor:

	if (likely(!(rdes3 & RDES3_LAST_DESCRIPTOR)))
		return rx_not_ls;

So the frame reaches the new 1:1 mapping drop and rx_dropped is
incremented once.

On dwmac100/dwmac1000, non-last descriptors now carry discard_frame. The
status & discard_frame block in stmmac_rx_zc() runs first and increments
rx_errors for each non-last descriptor (when hwts_rx_en is off). The new
rx_dropped++ in the 1:1 path is never reached on these cores.

An N-descriptor jumbo frame would then show up as 1 rx_dropped on
GMAC4/XGMAC, but as N-1 rx_errors on dwmac1000. Both counters are
exported through stmmac_get_stats64(). Is this asymmetry intended?

The legacy cores already counted rx_errors per descriptor before this
patch; what is new is the different rx_dropped accounting. Later in the
series, "net: stmmac: rework stmmac_rx to support XDP rx multi-buff"
changes stmmac_rx() to test !error && (status & discard_frame), so it
counts once per frame. stmmac_rx_zc() still counts per descriptor at the
end of the series.

[ ... ]

> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> index 8fd2acb25fb60..efa88cd359dac 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c

[ ... ]

> @@ -5715,8 +5718,9 @@ static int stmmac_rx_zc(struct stmmac_priv *priv, int limit, u32 queue)
>  		if (likely(status & rx_not_ls)) {
>  			xsk_buff_free(buf->xdp);
>  			buf->xdp = NULL;
> +			rx_dropped++;
> +			error = 1;
>  			dirty++;
> -			count++;
>  			goto read_again;
>  		}
>  

[Severity: Low]

Can one split frame on dwmac4/XGMAC now be counted in both rx_dropped and
rx_errors?

The first non-last descriptor takes this path, which increments
rx_dropped and sets error = 1. Suppose the last descriptor of the same
frame then reports discard_frame (RDES3_ERROR_SUMMARY, for example a CRC
or giant error on an oversized frame). The earlier block in
stmmac_rx_zc() still runs:

		if (unlikely(status & discard_frame)) {
			error = 1;
			if (!priv->hwts_rx_en)
				rx_errors++;
		}

This happens even though error is already set for this frame.
stmmac_get_stats64() exports both totals as rx_dropped and rx_errors.

Later in the series, "net: stmmac: rework stmmac_rx to support XDP rx
multi-buff" avoids this in stmmac_rx() by testing !error && (status &
discard_frame). stmmac_rx_zc() is not changed, so the double count is
still there at the end of the series.

[Severity: Medium]

This is a pre-existing issue and was not introduced by this patch. The
new rx_dropped++ feeds this accumulation at the end of stmmac_rx_zc():

	priv->xstats.rx_dropped += rx_dropped;
	priv->xstats.rx_errors += rx_errors;

These are plain unsigned long fields in struct stmmac_extra_stats,
shared by the whole device. Each RX channel has its own NAPI instance,
so stmmac_rx_zc() and stmmac_rx() can run at the same time for different
queues on different CPUs.

Can these non-atomic read-modify-writes lose updates?
stmmac_get_stats64() also reads the fields without any synchronization.

stmmac_rx() does the same accumulation, and it is unchanged at the end
of the series.

> @@ -5883,7 +5887,8 @@ static int stmmac_rx(struct stmmac_priv *priv, int limit, u32 queue)
>  
>  		if (priv->extend_desc)
>  			stmmac_rx_extended_status(priv, &priv->xstats, rx_q->dma_erx + entry);
> -		if (unlikely(status == discard_frame)) {
> +
> +		if (unlikely(status & discard_frame)) {

[Severity: Low]

Changing == to & here and in stmmac_rx_zc() also changes the normal RX
path on dwmac4/GMAC4/GMAC5. Should the commit message mention this?

dwmac4_wrback_get_rx_status() sets ret = discard_frame when
RDES3_ERROR_SUMMARY is set. It then ORs in csum_none for IP header or
payload errors:

	if (rdes1 & RDES1_IP_HDR_ERROR) {
		x->ip_hdr_err++;
		ret |= csum_none;
	}

So a frame with both an L2 error and an IP checksum error reports
discard_frame | csum_none.

Before this patch, such a frame failed the == test. It was run through
XDP and passed to napi_gro_receive() with CHECKSUM_NONE. Now it is
dropped and counted in rx_errors.

The new behaviour looks correct. It stops errored frames from reaching
the stack, which has been possible since commit 644b8437ccef9e ("net:
stmmac: dwmac4: report Rx checksum errors in status").

However, the commit message presents the operator change only as a
result of the enh_desc/norm_desc normalization, and limits the scope to
AF_XDP zero-copy. Could it describe this GMAC4 change and reference
644b8437ccef9e?

>  			page_pool_put_page(rx_q->page_pool, buf->page, 0, true);
>  			buf->page = NULL;
>  			error = 1;

[Severity: High]

This isn't a bug introduced by this patch. The commit message says the
change lets stmmac_rx() drain split frames, which holds for
dwmac100/dwmac1000.

GMAC4/XGMAC report non-last descriptors as a bare rx_not_ls. On those
cores, with an XDP program attached, are there still paths in
stmmac_rx() that drop a non-last descriptor without setting error?

When the XDP verdict is STMMAC_XDP_CONSUMED:

				if (xdp_res & STMMAC_XDP_CONSUMED) {
					...
					skb = NULL;

					if (unlikely((status & rx_not_ls)))
						goto read_again;

error is still 0 here, so the next fragment goes through XDP as a new
frame. With a 1-3 byte tail, wouldn't buf1_len -= ETH_FCS_LEN underflow?

For XDP_TX/XDP_REDIRECT:

				} else if (xdp_res & (STMMAC_XDP_TX |
						      STMMAC_XDP_REDIRECT)) {
					xdp_status |= xdp_res;
					buf->page = NULL;
					goto next;
				}

goto next clears in_progress even when rx_not_ls is set. The loop head
then resets error and len, and the tail is reparsed with len == 0.

When napi_build_skb() fails:

				rx_dropped++;
				count++;
				goto drain_data;

error is not set here either, so a later descriptor can build an skb from
only the end of the frame.

This is the same class of bug the patch fixes in stmmac_rx_zc(). An
on-link peer can trigger it, since GMAC_CONFIG_JE is always set on
GMAC4.

It looks like later patches in the same series fix it: "net: stmmac:
drop multi-buffer frames in XDP mode" and "net: stmmac: rework stmmac_rx
to support XDP rx multi-buff". After those, XDP and stmmac_build_skb()
only run on the last descriptor, and failures go to error_free_frag.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261018-stmmac-rx-mb-v8-0-5f58c0b25849%40oss.qualcomm.com

Re: [PATCH net-next v8 3/5] net: stmmac: drop multi-buffer frames in XDP mode

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] The commit message does not describe what the patch actually
  does, and it hides changes to the non-XDP RX path.

--

net: stmmac: drop multi-buffer frames in XDP mode

When an XDP program is attached, stmmac_rx() now drops any frame that
spans more than one descriptor, or that uses the second buffer of a
descriptor. This prevents the XDP program from running on partial
frames. Both XDP_CONSUMED and XDP_TX/XDP_REDIRECT now finish the packet
via next instead of re-parsing the following descriptor.

> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> index efa88cd359dac..e929cc0f23125 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> @@ -5888,18 +5888,23 @@ static int stmmac_rx(struct stmmac_priv *priv, int limit, u32 queue)
>  		if (priv->extend_desc)
>  			stmmac_rx_extended_status(priv, &priv->xstats, rx_q->dma_erx + entry);
>  
> -		if (unlikely(status & discard_frame)) {
> -			page_pool_put_page(rx_q->page_pool, buf->page, 0, true);
> -			buf->page = NULL;
> +		if (unlikely(!error && (status & discard_frame))) {
>  			error = 1;
>  			if (!priv->hwts_rx_en)
>  				rx_errors++;
>  		}

[Severity: Low]
The commit message says the patch will:

  "mark it as errored as soon as a non-last descriptor, or a descriptor
  using its second buffer, is seen while an XDP program is attached,
  drain and release every descriptor - including the last."

Is this description accurate? This hunk removes the only
page_pool_put_page() call on the error path in stmmac_rx(). After the
patch, none of the drop paths returns a page to the page_pool. That
covers the hardware discard_frame drop, the new XDP SG drop and the new
buf2 drop.

The dropped buffers stay in rx_q->buf_pool[], and stmmac_rx_refill()
reuses them in place because it only allocates when the slot is empty:

stmmac_rx_refill() {
    ...
		if (!buf->page) {
			buf->page = page_pool_alloc_pages(rx_q->page_pool, gfp);
    ...
}

The reuse itself looks safe. All three drop checks run before any
dma_sync_single_for_cpu() or net_prefetch() on the buffer.

The new !error guard also changes rx_errors from once per descriptor
carrying discard_frame to once per frame. Both changes apply whether or
not an XDP program is attached, but the subject says "in XDP mode".

On dwmac100/dwmac1000, the previous patch in this series makes non-last
descriptors report discard_frame | rx_not_ls. So an errored frame
spread over N descriptors used to add N-1 or N to rx_errors, and now
adds 1.

Separately, the new buf2 drop in the later hunk doesn't set error, even
though the message says the frame is marked as errored:

	if (unlikely(stmmac_xdp_is_enabled(priv) && buf2_len)) {
		rx_dropped++;
		goto next;
	}

It only behaves the same way because, in XDP mode, that descriptor is
always the last one.

This patch carries a Fixes: tag for net/stable. Could the commit message
describe the in-place buffer reuse and the rx_errors accounting change
for the non-XDP path? The later "net: stmmac: rework stmmac_rx to
support XDP rx multi-buff" patch keeps both the !error guard and the
in-place reuse, so it doesn't change the picture here.

> -		if (unlikely(error && (status & rx_not_ls)))
> -			goto read_again;
> +		/* We do not support SG in XDP mode */
> +		if (unlikely(!error && (status & rx_not_ls) &&
> +			     stmmac_xdp_is_enabled(priv))) {
> +			rx_dropped++;
> +			error = 1;
> +		}

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261018-stmmac-rx-mb-v8-0-5f58c0b25849%40oss.qualcomm.com

Re: [PATCH net-next v8 4/5] net: stmmac: fix VLAN tag stripping on non-linear RX frames

[netdev-bot+sashiko]

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

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

- [Medium] The commit message and Fixes: tag describe a crash that cannot
  happen in any tree this patch would be backported to.
- [Medium] The commit message says that moving meta_len bytes together
  with the MAC addresses in stmmac_rx_vlan() keeps data_meta pointing at…
- [Low] stmmac_rx_vlan() now calls pskb_may_pull(skb, VLAN_ETH_HLEN)
  before checking h_vlan_proto or dev->features.

Pre-existing issues:
- [Medium] This problem wasn't introduced by this patch, but
  priv->xstats.rx_dropped is a plain 'unsigned long' (common.h:241)…

--

net: stmmac: fix VLAN tag stripping on non-linear RX frames

This patch makes stmmac_rx_vlan() call pskb_may_pull(skb, VLAN_ETH_HLEN)
before it reads the VLAN header. When the tag is popped, any XDP metadata
is now moved along with the MAC addresses. If the pull fails, both RX
callers drop the frame.

> stmmac_rx_vlan() reads the VLAN header straight from skb->data and
> removes it with skb_pull(), assuming the header is always in the linear
> part of the skb. That is not true once non-linear skbs can reach it: a
> program loaded with BPF_F_XDP_HAS_FRAGS can pass up a fragmented frame
> whose linear head has been shortened to as little as ETH_HLEN by
> bpf_xdp_adjust_head(), and the unguarded skb_pull() then drives skb->len
> below skb->data_len, tripping BUG() in __skb_pull().

[Severity: Medium]
Can this scenario happen in a tree that matches the Fixes: tag?

On the AF_XDP zero-copy path from bba2556efad6, stmmac_construct_skb_zc()
always builds a linear skb:

	skb = napi_alloc_skb(&ch->rxtx_napi,
			     xdp->data_end - xdp->data_hard_start);
	...
	memcpy(__skb_put(skb, datasize), xdp->data, datasize);

On the regular stmmac_rx() path with XDP enabled, multi-descriptor frames
and buf2 frames are both dropped:

		/* We do not support SG in XDP mode */
		if (unlikely(!error && (status & rx_not_ls) &&
			     stmmac_xdp_is_enabled(priv))) {
		...
		if (unlikely(stmmac_xdp_is_enabled(priv) && buf2_len)) {
			rx_dropped++;
			goto next;
		}

The XDP_PASS skb there is built with napi_build_skb() plus
skb_put(buf1_len) and has no frags. XDP_PASS skbs with frags seem to
appear only with the later "net: stmmac: rework stmmac_rx to support XDP
rx multi-buff" patch in this series.

Even with a 14 byte linear head, skb_pull(skb, VLAN_HLEN) here would leave
10 bytes of headlen and would not BUG. Wouldn't the BUG() come from
eth_type_trans()->eth_skb_pull_mac()->skb_pull_inline(skb, ETH_HLEN)
instead?

On current trees, the patch fixes a different case. An XDP program can
use bpf_xdp_adjust_tail() to shrink a linear frame to 14..17 bytes while
bytes 12-13 still hold a VLAN ethertype. The old code then reads
h_vlan_TCI past skb->len and pops the tag. That leaves skb->len below
ETH_HLEN, and eth_type_trans() passes a malformed skb up the stack.

Could the commit message describe that case, and could the Fixes: tag be
rechecked? As written, stable maintainers may conclude that 5.13+ kernels
hit a BUG() they do not have.

> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> index e929cc0f23125..04dc6dcaa435d 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> @@ -5095,22 +5095,30 @@ static netdev_features_t stmmac_features_check(struct sk_buff *skb,
>  	return vlan_features_check(skb, features);
>  }
>  
> -static void stmmac_rx_vlan(struct net_device *dev, struct sk_buff *skb)
> +static int stmmac_rx_vlan(struct net_device *dev, struct sk_buff *skb)
>  {
> -	struct vlan_ethhdr *veth = skb_vlan_eth_hdr(skb);
> -	__be16 vlan_proto = veth->h_vlan_proto;
> -	u16 vlanid;
> +	struct vlan_ethhdr *veth;
>  
> -	if ((vlan_proto == htons(ETH_P_8021Q) &&
> +	if (!pskb_may_pull(skb, VLAN_ETH_HLEN))
> +		return -EINVAL;

[Severity: Low]
Does this drop every 14..17 byte frame, including untagged ones?

The pull happens before h_vlan_proto and dev->features are checked. Any
skb with skb->len < VLAN_ETH_HLEN now fails here, and both stmmac_rx()
and stmmac_dispatch_skb_zc() free it and count it in rx_dropped.

This also covers interfaces set with "ethtool -K rxvlan off". There,
stmmac_set_features() clears hw_vlan_en, so stmmac_rx_vlan() runs for
every frame. It also covers non-xmac cores, where hw_vlan_en is never
set.

Such frames can come from an XDP program that shrinks a frame with
bpf_xdp_adjust_tail(), which allows lengths down to ETH_HLEN, and then
returns XDP_PASS. Before this patch they went through eth_type_trans()
unchanged.

Would it work to check the ethertype and features first? ETH_HLEN is
enough to read h_vlan_proto, and VLAN_ETH_HLEN would only be needed
when the tag is about to be popped.

> +
> +	veth = skb_vlan_eth_hdr(skb);
> +	if ((veth->h_vlan_proto == htons(ETH_P_8021Q) &&
>  	     dev->features & NETIF_F_HW_VLAN_CTAG_RX) ||
> -	    (vlan_proto == htons(ETH_P_8021AD) &&
> +	    (veth->h_vlan_proto == htons(ETH_P_8021AD) &&
>  	     dev->features & NETIF_F_HW_VLAN_STAG_RX)) {
> +		__be16 vlan_proto = veth->h_vlan_proto;
> +		u16 vlanid = ntohs(veth->h_vlan_TCI);
> +		u8 meta_len = skb_metadata_len(skb);
> +
>  		/* pop the vlan tag */
> -		vlanid = ntohs(veth->h_vlan_TCI);
> -		memmove(skb->data + VLAN_HLEN, veth, ETH_ALEN * 2);
> +		memmove(skb->data - meta_len + VLAN_HLEN,
> +			skb->data - meta_len, meta_len + ETH_ALEN * 2);

[Severity: Medium]
On the AF_XDP zero-copy path, do the meta_len bytes before skb->data
actually hold the XDP metadata?

stmmac_construct_skb_zc() records the metadata length but never copies
the metadata bytes:

	skb_reserve(skb, xdp->data - xdp->data_hard_start);
	memcpy(__skb_put(skb, datasize), xdp->data, datasize);
	if (metasize)
		skb_metadata_set(skb, metasize);

The skb comes from napi_alloc_skb(), which does not zero the headroom,
so the bytes in front of skb->data are stale. This memmove() moves those
stale bytes in front of the relocated MAC header.

After eth_type_trans(), bpf_compute_data_pointers() sets data_meta to
skb->data - skb_metadata_len(skb). TC BPF programs would then read
uninitialized headroom instead of what the XDP program wrote.

On the regular stmmac_rx() path, skb_metadata_set() is never called at
this commit. meta_len is therefore always 0 there, and this part of the
change does nothing until the later rework patch adds the call in
stmmac_build_skb(). stmmac_construct_skb_zc() is still unchanged at the
end of the series.

Peer drivers such as igc_construct_skb_zc() copy starting at
xdp->data_meta and then call __skb_pull(skb, metasize). Should
stmmac_construct_skb_zc() do the same, or should the metadata claim come
out of the commit message?

>  		skb_pull(skb, VLAN_HLEN);
>  		__vlan_hwaccel_put_tag(skb, vlan_proto, vlanid);
>  	}

[ ... ]

> @@ -5530,12 +5538,15 @@ static void stmmac_dispatch_skb_zc(struct stmmac_priv *priv, u32 queue,
>  	}
>  
>  	stmmac_get_rx_hwtstamp(priv, p, np, skb);
> -	if (priv->hw->hw_vlan_en)
> +	if (priv->hw->hw_vlan_en) {
>  		/* MAC level stripping. */
>  		stmmac_rx_hw_vlan(priv, priv->hw, p, skb);
> -	else
> -		/* Driver level stripping. */
> -		stmmac_rx_vlan(priv->dev, skb);
> +	} else if (stmmac_rx_vlan(priv->dev, skb)) {
> +		dev_kfree_skb_any(skb);
> +		priv->xstats.rx_dropped++;

[Severity: Medium]
This isn't a bug introduced by this patch, but priv->xstats.rx_dropped is
a plain unsigned long shared by all RX queues. Per-queue NAPI contexts,
which can run at the same time on different CPUs, update it with
unsynchronized read-modify-write:

stmmac_dispatch_skb_zc():
	priv->xstats.rx_dropped++;

stmmac_rx_zc() and stmmac_rx():
	priv->xstats.rx_dropped += rx_dropped;

stmmac_get_stats64() also reads it without READ_ONCE().

Can concurrent updates from two queues lose increments here? The new
error path adds one more writer that follows the existing pattern.

> +		return;
> +	}

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261018-stmmac-rx-mb-v8-0-5f58c0b25849%40oss.qualcomm.com

Re: [PATCH net-next v8 5/5] net: stmmac: rework stmmac_rx to support XDP rx multi-buff

[netdev-bot+sashiko]

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

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

- [Medium] The patch fixes a DMA under-sync on the XDP drop path but does
  not say so.
- [Medium] Fragment truesize is undercounted when num_pages is not a
  power of two.
- [Low] The patch advertises NETDEV_XDP_ACT_RX_SG (line 8189) and now
  runs frags-aware programs on multi-buffer xdp_buffs…

Pre-existing issues:
- [High] XDP_TX completions are recycled into the RX page_pool's lockless
  alloc cache from a different NAPI context.
- [Medium] The AF_XDP zero-copy XDP_PASS path reports XDP metadata whose
  bytes it never copies.

--

net: stmmac: rework stmmac_rx to support XDP rx multi-buff

This reworks stmmac_rx() to collect every descriptor of a frame in an
xdp_buff, so the XDP program runs on the whole frame, including any
fragments. The skb is then built from the xdp_buff with napi_build_skb()
and xdp_update_skb_frags_info(). The in-progress xdp_buff is saved in
rx_q->state across NAPI polls, and NETDEV_XDP_ACT_RX_SG is advertised.

> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> index 04dc6dcaa435d..e9c61524fc740 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c

[ ... ]

> @@ -5806,6 +5812,119 @@ static int stmmac_rx_zc(struct stmmac_priv *priv, int limit, u32 queue)

[ ... ]

> +static struct sk_buff *stmmac_build_skb(struct xdp_buff *xdp)
> +{
> +	struct skb_shared_info *sinfo = xdp_get_shared_info_from_buff(xdp);
> +	u32 metasize = xdp->data - xdp->data_meta;

[ ... ]

> +	skb_mark_for_recycle(skb);
> +	skb_reserve(skb, xdp->data - xdp->data_hard_start);
> +	skb_put(skb, xdp->data_end - xdp->data);
> +	if (metasize)
> +		skb_metadata_set(skb, metasize);

[Severity: Medium]
This isn't a bug introduced by this patch. Now that the page_pool path
keeps the metadata, though, the AF_XDP zero-copy XDP_PASS path no longer
matches it. Does stmmac_construct_skb_zc() pass stale headroom bytes to
TC/BPF as metadata?

stmmac_construct_skb_zc() {
    ...
	skb_reserve(skb, xdp->data - xdp->data_hard_start);
	memcpy(__skb_put(skb, datasize), xdp->data, datasize);
	if (metasize)
		skb_metadata_set(skb, metasize);
    ...
}

Only [data, data_end) is copied into the new skb, but the metasize bytes
in front of skb->data are still marked as metadata. A TC/BPF program that
reads data_meta on frames from stmmac_rx_zc()->stmmac_dispatch_skb_zc()
would see leftover bytes from the napi_alloc_skb() headroom instead of
what the XDP program wrote.

Other zero-copy drivers copy metasize + datasize bytes starting at
xdp->data_meta and then call __skb_pull(skb, metasize). Would the same
approach work here?

> +
> +	if (unlikely(xdp_buff_has_frags(xdp)))
> +		xdp_update_skb_frags_info(skb, num_frags, sinfo->xdp_frags_size,
> +					  sinfo->xdp_frags_truesize,
> +					  xdp_buff_get_skb_flags(xdp));
> +	return skb;
> +}
> +
> +static bool stmmac_build_xdp_frags(struct stmmac_priv *priv,
> +				   struct stmmac_rx_queue *rx_q,
> +				   unsigned int len, struct page *page,
> +				   unsigned int offset,
> +				   enum dma_data_direction dma_dir,
> +				   struct xdp_buff *xdp)
> +{
> +	dma_addr_t dma_addr = page_pool_get_dma_addr(page) + offset;
> +
> +	dma_sync_single_for_cpu(priv->device, dma_addr, len, dma_dir);
> +	if (!xdp_buff_add_frag(xdp, page_to_netmem(page), offset, len,
> +			       xdp->frame_sz)) {

[Severity: Medium]
The commit message says:

  charge fragments the whole page_pool page size (xdp->frame_sz) as
  truesize instead of the old dma_buf_sz

Is xdp->frame_sz the page_pool page size in every configuration?

frame_sz comes from bufsz = rx_q->napi_skb_frag_size in stmmac_rx(). That
value is set in __alloc_dma_rx_desc_resources():

	rx_q->napi_skb_frag_size = num_pages * PAGE_SIZE;
	...
	pp_params.order = order_base_2(num_pages);

When num_pages is not a power of two, each page_pool buffer is
PAGE_SIZE << order, which is larger than napi_skb_frag_size. Two examples
with 4K pages and no XDP:

- dma_buf_sz of 8KiB: num_pages = 3, order 2, so each 16K page is charged
  as 12K.
- dma_buf_sz of 16KiB: num_pages = 5, order 3, so each 32K page is charged
  as 20K.

stmmac_xdp_shrink_tail() subtracts the same xdp->frame_sz. Then
stmmac_build_skb() adds sinfo->xdp_frags_truesize to skb->truesize
through xdp_update_skb_frags_info(). As a result, skb->truesize is lower
than the memory the fragments actually pin.

Could the fragment truesize use PAGE_SIZE << pp_params.order instead? Or
could num_pages be rounded up to a power of two?

[ ... ]

> @@ -5927,129 +6039,89 @@ static int stmmac_rx(struct stmmac_priv *priv, int limit, u32 queue)

[ ... ]

> +		pre_len = ctx.xdp.data_end - ctx.xdp.data_hard_start;
> +
> +		ctx.desc = p;
> +		ctx.ndesc = np;
> +
> +		skb = stmmac_xdp_run_prog(priv, &ctx.xdp);
> +		/* Due xdp_adjust_tail: DMA sync for_device
> +		 * cover max len CPU touch
> +		 */
> +		sync_len = ctx.xdp.data_end - ctx.xdp.data_hard_start;
> +		sync_len = max(sync_len, pre_len);

[Severity: Medium]
The old pre_len and sync_len subtracted buf->page_offset:

	sync_len = ctx.xdp.data_end - ctx.xdp.data_hard_start -
		   buf->page_offset;

The new ones don't. This looks like a separate DMA sync fix, and the
commit message doesn't mention it.

Commit 88095728511e ("net: stmmac: fix rx Scatter-Gather support") set
pp_params.offset = 0, and __page_pool_dma_sync_for_device() syncs from
dma_addr + pool->p.offset. So with the old formula, the XDP_CONSUMED path
synced [0, L) for an L-byte frame at XDP_PACKET_HEADROOM. The bytes the
CPU had touched were at [256, 256 + L).

Take a non-coherent platform and an XDP program that writes the packet,
grows the tail or writes metadata, and then returns XDP_DROP or
XDP_ABORTED, or fails a redirect. Could that leave dirty cache lines in
the recycled page? Those lines could later be written back over a frame
the device has DMA'd into the page.

The new formula looks like the correct one. Stable kernels that picked up
88095728511e would keep the under-sync. Could this go out as a separate
net fix with:

Fixes: 88095728511e ("net: stmmac: fix rx Scatter-Gather support")

or at least be described in the commit message?

> +
> +		/* For Not XDP_PASS verdict */
> +		if (IS_ERR(skb)) {
> +			unsigned int xdp_res = -PTR_ERR(skb);
> +
> +			if (xdp_res & STMMAC_XDP_CONSUMED) {
> +				stmmac_xdp_put_buff(rx_q, &ctx.xdp, sync_len,
> +						    true);
> +				rx_dropped++;
> +			} else if (xdp_res & (STMMAC_XDP_TX |
> +					      STMMAC_XDP_REDIRECT)) {
> +				xdp_status |= xdp_res;
> +			}

[Severity: High]
This is a pre-existing issue, but this patch sends fragment pages down the
same path. Is it safe for tx_napi to recycle XDP_TX completions directly
into this RX queue's page_pool?

stmmac_tx_clean() runs from ch->tx_napi and does:

	if (xdpf &&
	    tx_q->tx_skbuff_dma[entry].buf_type == STMMAC_TXBUF_T_XDP_TX) {
		xdp_return_frame_rx_napi(xdpf);
		tx_q->xdpf[entry] = NULL;
	}

That call ends up here:

xdp_return_frame_rx_napi()
  __xdp_return(..., napi_direct=true)
    page_pool_put_full_netmem(pp, netmem, true)
      page_pool_recycle_in_cache()
        pool->alloc.cache[pool->alloc.count++] = netmem;

The page_pool belongs to ch->rx_napi, which is a separate napi_struct.
stmmac_xdp_get_tx_queue() picks the XDP_TX queue by CPU. tx_napi can also
be scheduled on another CPU by stmmac_tx_timer() or by a per-channel TX
MSI vector. stmmac does not set pp_params.napi, so page_pool cannot catch
this.

At the same time, stmmac_rx() on rx_napi uses the same alloc cache. It
does so through stmmac_rx_refill() and through the new allow_direct=true
puts in stmmac_xdp_put_buff(), stmmac_build_xdp_frags() and
stmmac_xdp_shrink_tail().

Can these two contexts race on alloc.count and the cache slots? That
could lose a page, or cache the same page twice so that it gets handed to
two RX descriptors.

With multi-buffer frames, xdp_return_frame_rx_napi() now also returns
every fragment page with napi_direct=true, so more pages go through this
race.

[ ... ]

> @@ -8106,6 +8186,7 @@ static int __stmmac_dvr_probe(struct device *device,
>  	ndev->hw_features = NETIF_F_SG | NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM |
>  			    NETIF_F_RXCSUM;
>  	ndev->xdp_features = NETDEV_XDP_ACT_BASIC | NETDEV_XDP_ACT_REDIRECT |
> +			     NETDEV_XDP_ACT_RX_SG |
>  			     NETDEV_XDP_ACT_XSK_ZEROCOPY;

[Severity: Low]
With NETDEV_XDP_ACT_RX_SG advertised, frags-aware programs now see
multi-buffer frames. However, __alloc_dma_rx_desc_resources() still
registers the rxq with:

	ret = xdp_rxq_info_reg(&rx_q->xdp_rxq, priv->dev, queue, napi_id);

That leaves frag_size at 0, so bpf_xdp_frags_increase_tail() always
returns early:

	if (!rxq->frag_size || rxq->frag_size > xdp->frame_sz)
		return -EOPNOTSUPP;

Is it intended that a positive bpf_xdp_adjust_tail() on a multi-buffer
stmmac frame always fails, even when the last fragment page has room?
Other RX_SG drivers such as bnxt behave the same way, so this may just be
a limitation. Would registering with __xdp_rxq_info_reg() and a real
frag_size be worth considering?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261018-stmmac-rx-mb-v8-0-5f58c0b25849%40oss.qualcomm.com