[PATCH net v2 0/6] net: stmmac: XDP path fixes

[PATCH net v2 0/6] net: stmmac: XDP path fixes

[Lorenzo Bianconi]

This series adds some stmmac XDP preliminary fixes needed to introduce XDP
RX multi-buff support.

---
Changes in v2:
- Rebase on top of net main branch
- Fix commit messages
- In order to avoid possible dirty cache overwrites, sync the full page
  dropping the xdp_buff in stmmac_rx().
- Link to v1: https://lore.kernel.org/r/20261023-stmmac-xdp-fixes-v1-0-efb37e19f5bd@oss.qualcomm.com

---
James Hilliard (1):
      net: stmmac: take ownership of saved RX state at poll entry

Lorenzo Bianconi (5):
      net: stmmac: drop split frames on AF_XDP zero-copy RX
      net: stmmac: drop multi-buffer frames in XDP mode
      net: stmmac: copy XDP metadata in AF_XDP zero-copy RX
      net: stmmac: fix VLAN tag stripping on short and non-linear RX frames
      net: stmmac: fix XDP RX buffer DMA sync length

 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 | 194 ++++++++++++----------
 3 files changed, 112 insertions(+), 86 deletions(-)
---
base-commit: f75062536e2d45ee182828f0e47e0124be2fdc8d
change-id: 20261023-stmmac-xdp-fixes-00aa25d79af1

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

[PATCH net v2 1/6] net: stmmac: take ownership of saved RX state at poll entry

[Lorenzo Bianconi]

From: James Hilliard <james.hilliard1@gmail.com>

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 rewound to zero. On the resume
path the ring is only re-armed in place: stmmac_clear_descriptors()
sets the ownership bits and stmmac_init_dma_engine() restores the tail
pointer, but the RX buffer addresses are not reprogrammed. The first
poll would then resume the stale skb against a ring position that no
longer matches the descriptors it was built from.
__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>
Co-developed-by: Lorenzo Bianconi <lorenzo.bianconi@oss.qualcomm.com>
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 9ded31150fbe..fa7108919e90 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -2182,6 +2182,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;
 
@@ -5629,12 +5632,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);
@@ -5645,23 +5648,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;
@@ -5700,6 +5705,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) {
 			/* Read the extended status after the own bit */
 			dma_rmb();
@@ -5776,7 +5783,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;
@@ -5818,9 +5825,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;
@@ -5840,25 +5848,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;
@@ -5887,6 +5898,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) {
 			/* Read the extended status after the own bit */
 			dma_rmb();
@@ -5904,11 +5917,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. */
@@ -5982,9 +5994,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;
 				}
 			}
 		}
@@ -6041,10 +6051,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);
@@ -6069,14 +6077,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;
@@ -8421,6 +8430,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 v2 2/6] 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.

Use a bit test instead of an equality test for discard_frame.
Commit 644b8437ccef9e ("net: stmmac: dwmac4: report Rx checksum errors
in status") made dwmac4_wrback_get_rx_status() set discard_frame on
RDES3_ERROR_SUMMARY and OR in csum_none on RDES1_IP_HDR_ERROR or
RDES1_IP_PAYLOAD_ERROR, so it can return discard_frame | csum_none.
The old "status == discard_frame" test missed that value and delivered
frames with CRC, overflow, watchdog or giant errors to the stack in
stmmac_rx() - or to XDP/XSK in stmmac_rx_zc() - when they also failed
the IP checksum. The change therefore affects the regular RX path, not
only zero-copy. The bit test is also required for the
discard_frame | rx_not_ls value returned for non-last legacy
descriptors below.

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")
Fixes: 644b8437ccef9e ("net: stmmac: dwmac4: report Rx checksum errors in status")
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 023b54b72478..7ece8d4157a8 100644
--- a/drivers/net/ethernet/stmicro/stmmac/enh_desc.c
+++ b/drivers/net/ethernet/stmicro/stmmac/enh_desc.c
@@ -205,7 +205,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 fa7108919e90..33814672c842 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -5714,18 +5714,21 @@ static int stmmac_rx_zc(struct stmmac_priv *priv, int limit, u32 queue)
 							   &rx_q->dma_erx[entry],
 							   status);
 		}
-		if (unlikely(status == discard_frame)) {
-			xsk_buff_free(buf->xdp);
-			buf->xdp = NULL;
-			dirty++;
+
+		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;
 		if (unlikely(error)) {
+			xsk_buff_free(buf->xdp);
+			buf->xdp = NULL;
+			dirty++;
+
+			if (status & rx_not_ls)
+				goto read_again;
+
 			count++;
 			continue;
 		}
@@ -5734,8 +5737,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;
 		}
 
@@ -5907,7 +5911,8 @@ static int stmmac_rx(struct stmmac_priv *priv, int limit, u32 queue)
 							   &rx_q->dma_erx[entry],
 							   status);
 		}
-		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 v2 3/6] 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:
buffer2 is now handed to the hardware for non-SPH frames on GMAC4/XGMAC,
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. When a non-last descriptor is
seen while an XDP program is attached, mark the frame as errored and
drain the remaining descriptors, including the last. A descriptor that
uses its second buffer is dropped directly before the program runs as
well; in XDP mode it is necessarily the last descriptor of the frame,
so it does not need the error marker. Removing the re-parse lets both
XDP verdict branches simply complete the packet via next.

The buffers consumed by the discard path are no longer returned to the
page pool, and the new SG and second-buffer drops do not return them
either. They stay in rx_q->buf_pool[] and stmmac_rx_refill() reuses them
in place, allocating only when a slot is empty. This is safe because the
drop checks run before the buffer is synced for CPU access or
prefetched. The discard-path change and the in-place reuse apply with or
without an XDP program attached.

The !error guard on the discard check also changes the accounting for
errored multi-descriptor frames on dwmac100/dwmac1000: rx_errors is
incremented once per frame instead of once per descriptor. This too is
independent of XDP mode.

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")
Fixes: 88095728511e ("net: stmmac: fix rx Scatter-Gather support")
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 | 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 33814672c842..71858af6fd66 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -5912,18 +5912,23 @@ static int stmmac_rx(struct stmmac_priv *priv, int limit, u32 queue)
 							   status);
 		}
 
-		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;
 		}
@@ -5935,6 +5940,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;
@@ -5984,17 +5997,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 v2 4/6] net: stmmac: copy XDP metadata in AF_XDP zero-copy RX

[Lorenzo Bianconi]

stmmac_construct_skb_zc() reserves headroom for the XDP metadata and
records the length reported by the XDP program, but only copies the
packet data starting at xdp->data. The bytes in
[xdp->data_meta, xdp->data) are never copied into the new skb, so the
skb advertises metadata that is not there: after eth_type_trans()
anchors mac_header, data_meta points at uninitialized headroom instead
of what the program wrote.

Copy the metadata together with the packet data starting at
xdp->data_meta, leaving skb->data at the start of the frame and the
metadata immediately before it, and record its length. This keeps
data_meta valid for the stack and TC BPF after eth_type_trans() anchors
mac_header.

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 | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index 71858af6fd66..d9fc2464a160 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -5519,7 +5519,8 @@ static struct sk_buff *stmmac_construct_skb_zc(struct stmmac_channel *ch,
 		return NULL;
 
 	skb_reserve(skb, xdp->data - xdp->data_hard_start);
-	memcpy(__skb_put(skb, datasize), xdp->data, datasize);
+	__skb_put(skb, datasize);
+	memcpy(skb->data - metasize, xdp->data_meta, datasize + metasize);
 	if (metasize)
 		skb_metadata_set(skb, metasize);
 

-- 
2.56.0

[PATCH net v2 5/6] net: stmmac: fix VLAN tag stripping on short and 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 fully present in the
linear part of the skb. XDP can change the frame length and head layout
before the tag is stripped.

An XDP program can use bpf_xdp_adjust_tail() to shrink a frame to as
little as ETH_HLEN while bytes 12-13 still contain a VLAN ethertype. The
driver then reads h_vlan_TCI past skb->len and pops the tag, leaving less
than ETH_HLEN in the skb. eth_type_trans() cannot pull the MAC header, so
a malformed skb is passed up. The TCI is also read from the wrong place
when the tag is not fully in the linear head, e.g. when it spans into a
frag.

Read h_vlan_proto after pulling ETH_HLEN into the linear part, and check
it together with the device features. Only if the tag is actually going
to be popped, pull VLAN_ETH_HLEN and re-derive the header pointer
afterwards, as pskb_may_pull() may reallocate the skb head. Return an
error so the callers can drop a frame that claims a VLAN tag but cannot
provide the full header. This also drops and counts frames shorter than
ETH_HLEN in rx_dropped, even when VLAN RX offload is disabled;
bpf_xdp_adjust_head() and bpf_xdp_adjust_tail() cannot produce such a
frame, so it only affects a runt that reaches the driver, and dropping it
is safer than handing it to eth_type_trans().

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.

Fixes: 5fabb01207a2 ("net: stmmac: Add initial XDP support")
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 | 48 ++++++++++++++++-------
 1 file changed, 33 insertions(+), 15 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index d9fc2464a160..fb3b7174599e 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -5102,22 +5102,35 @@ 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, 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;
+		u8 meta_len = skb_metadata_len(skb);
+		u16 vlanid;
+
 		/* pop the vlan tag */
+		if (!pskb_may_pull(skb, VLAN_ETH_HLEN))
+			return -EINVAL;
+
+		veth = skb_vlan_eth_hdr(skb);
 		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,
@@ -5546,12 +5559,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))
@@ -6066,12 +6082,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 v2 6/6] net: stmmac: fix XDP RX buffer DMA sync length

[Lorenzo Bianconi]

stmmac_rx() bounded the DMA sync length by the largest data_end seen
before and after the XDP program and passed it to page_pool_put_page()
on the XDP_CONSUMED path. bpf_xdp_adjust_tail() zeroes the grown area,
which dirties the CPU cache, but a later negative adjust only moves
data_end back. If the program grows and then shrinks the tail, both
endpoints miss the zeroed range, so those dirty cache lines are not
written back before the page is reused.

The length was also wrong since commit 88095728511e ("net: stmmac: fix
rx Scatter-Gather support") moved the page_pool DMA window to offset 0
and set max_len to dma_buf_sz + page_offset: the driver still
subtracted buf->page_offset, so it synced [base, data_end - page_offset)
instead of [base, data_end).

On a non-coherent platform those dirty lines can be evicted after the
device has DMA'd the next frame into the recycled page and corrupt it.

Sync the whole page_pool DMA window on the XDP_CONSUMED path with
page_pool_recycle_direct(), as the PASS path and page_pool allocation
already do. The device only writes [page_offset, page_offset +
dma_buf_sz), so syncing [base, base + max_len) covers every cache line
it can touch, regardless of how the program adjusted the packet.

Fixes: be8b38a722e6 ("net: stmmac: Add support for XDP_TX action")
Fixes: 88095728511e ("net: stmmac: fix rx Scatter-Gather support")
Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@oss.qualcomm.com>
---
 drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 17 ++---------------
 1 file changed, 2 insertions(+), 15 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index fb3b7174599e..b038472b9817 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -5970,8 +5970,6 @@ static int stmmac_rx(struct stmmac_priv *priv, int limit, u32 queue)
 			len -= ETH_FCS_LEN;
 
 		if (!skb) {
-			unsigned int pre_len, sync_len;
-
 			/* Each frame starts here: reset the FCS handling */
 			fcs_stripped = false;
 
@@ -5989,29 +5987,18 @@ static int stmmac_rx(struct stmmac_priv *priv, int limit, u32 queue)
 			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);
+					page_pool_recycle_direct(rx_q->page_pool,
+								 virt_to_head_page(ctx.xdp.data));
 					buf->page = NULL;
 					rx_dropped++;
 					goto next;

-- 
2.56.0

Re: [PATCH net v2 0/6] net: stmmac: XDP path fixes

[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 v2 0/6] net: stmmac: XDP path fixes

[Lorenzo Bianconi]

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

On Oct 27, netdev-bot+sinfo@kernel.org wrote:
> 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 are reported by sashiko or spotted during code development

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

They are real issues reported by sashiko or found during code inspection

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

The series has been tested on 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 v2 1/6] 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 commit message gives the trigger as a saved partial packet
  completing "with a poll budget of one".

Pre-existing issues:
- [High] In stmmac_rx(), if napi_build_skb() fails on a non-last
  descriptor, the code recycles the page, does rx_dropped++ and count++,…
- [High] On resume, the RX ring is handed back to the DMA without its
  buffers being refilled or their addresses rewritten, and this patch…

--

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

This patch makes stmmac_rx() and stmmac_rx_zc() take the saved
partial-frame state at poll entry, clear it right away, and save it again
only if the frame is still incomplete when the loop exits. The in-progress
flag is now tracked separately from the status of the last descriptor
read. The saved skb is also dropped in stmmac_reset_rx_queue() and
__free_dma_rx_desc_resources().

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

[Severity: Low]
Could the commit message say how a budget of one happens on a running
system?

With limit > 1, the old else branch cleared state_saved on the second loop
iteration. So the stale pointer only survives when limit == 1. Normal
net_rx_action() polling passes the NAPI weight.

stmmac_napi_poll_rx() passes its budget straight through:

    work_done = stmmac_rx(priv, budget, chan);

Busy polling can supply a budget of 1. In ep_eventpoll_ioctl(), EPIOCSPARAMS
only asks for CAP_NET_ADMIN above NAPI_POLL_WEIGHT:

    if (epoll_params.busy_poll_budget > NAPI_POLL_WEIGHT &&
        !capable(CAP_NET_ADMIN))
            return -EPERM;

SO_BUSY_POLL_BUDGET in sk_setsockopt() can also be lowered without
privilege.

So an unprivileged local user who receives multi-descriptor frames can
reach the use-after-free. Would it be worth saying so in the changelog, so
stable reviewers can see this is a user-triggerable memory-safety fix?

> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> index 9ded31150fbe6..fa7108919e90d 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c

[ ... ]

> @@ -6041,10 +6051,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;
>  		}

[Severity: High]
This is a pre-existing issue, but what happens in stmmac_rx() when
napi_build_skb() fails on a descriptor that is not the last one in the
frame?

    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;
    }

error stays 0 and skb stays NULL, and drain_data jumps back to read_again
because rx_not_ls is set. The next descriptor of the same hardware frame
passes the error checks and takes the if (!skb) frame-start path. A new
skb is then built from only the tail of the frame.

len still counts the dropped prefix, so the pskb_trim(skb, len) above does
nothing and the FCS is not removed. The trailing payload bytes are then
delivered as a standalone Ethernet frame.

With Split Header active, stmmac_rx_buf1_len() returns 0 for any
descriptor after the first:

    if (priv->sph_active && len)
            return 0;

That leaves the rebuilt skb with no linear data, only the buf2 frag.
Wouldn't eth_type_trans() then hit the BUG() in __skb_pull()?

    eth_type_trans()
      eth_skb_pull_mac()
        skb_pull_inline(skb, ETH_HLEN)
          __skb_pull()
            skb->len < skb->data_len -> BUG()

If the stale page bytes look like a VLAN header, stmmac_rx_vlan() can hit
the same BUG() through skb_pull(skb, VLAN_HLEN).

An on-link peer sending a frame that spans several descriptors, plus a
GFP_ATOMIC allocation failure, is enough to trigger this. With this patch,
the in_progress tracking also carries this skb == NULL, error == 0 state
across a poll boundary. Nothing else in the series seems to address it.

Should the napi_build_skb() failure path set error = 1, so the rest of the
frame is thrown away?

[ ... ]

> @@ -8421,6 +8430,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 is a pre-existing issue, but this new free happens just before
stmmac_resume() re-arms the RX ring in place:

stmmac_resume()
    stmmac_reset_queues_param()
        stmmac_reset_rx_queue()    <- frees state.skb, rewinds cur/dirty
    stmmac_free_tx_skbufs()
    stmmac_clear_descriptors()
        stmmac_clear_rx_descriptors()
            stmmac_init_rx_desc()  <- sets OWN only
    stmmac_hw_setup()
        stmmac_init_dma_engine()   <- restores the tail pointer

No refill and no stmmac_set_desc_addr() runs before the DMA restarts. That
means two kinds of descriptors get re-armed with a bad buffer address:

  - slots stmmac_rx() consumed whose refill failed. stmmac_rx_refill()
    uses GFP_ATOMIC and stops at the first page_pool_alloc_pages()
    failure, so buf->page is left NULL.

  - descriptors the DMA wrote back after napi_disable() in
    stmmac_suspend() and before stmmac_stop_all_dma().

On dwmac4/xgmac, write-back overwrites des0/des1, which hold the buffer
address in read format (dwmac4_wrback_get_rx_vlan_tci() reads des0 as the
VLAN tag). dwmac4_rd_init_rx_desc() only sets OWN/BUF1V/IOC in RDES3. The
DMA would then write to whatever status data is left in those words.

On norm/enh descriptors, the old address stays in des2. The slots most
likely to have failed refill are the ones whose pages went into the saved
partial skb. The dev_kfree_skb_any() above now returns those pages, still
DMA-mapped, to the page_pool just before the hardware is re-armed to write
into them.

Can this lead to DMA into pages the pool has already handed to another
descriptor or released?

Also, if DMA completes into a slot with buf->page == NULL, stmmac_rx()
calls page_address(NULL) and napi_build_skb() on it, which would oops.

The commit message notes that "the RX buffer addresses are not
reprogrammed" on this path. Would a full fix need stmmac_resume() to
refill and reprogram every RX descriptor before setting OWN?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261027-stmmac-xdp-fixes-v2-0-10eae8a093a8%40oss.qualcomm.com

Re: [PATCH net v2 4/6] net: stmmac: copy XDP metadata in AF_XDP zero-copy RX

[netdev-bot+sashiko]

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

Critical: 0 · High: 0 · Medium: 1 · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] In this patch, stmmac_construct_skb_zc()
  (drivers/net/ethernet/stmicro/stmmac/stmmac_main.c:5521-5525) copies…

--

net: stmmac: copy XDP metadata in AF_XDP zero-copy RX

This patch changes stmmac_construct_skb_zc() to copy the XDP metadata in
[xdp->data_meta, xdp->data) into the new skb together with the packet
data. The metadata sits right before skb->data, and its length is recorded
with skb_metadata_set().

> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> index 71858af6fd66d..d9fc2464a160a 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> @@ -5519,7 +5519,8 @@ static struct sk_buff *stmmac_construct_skb_zc(struct stmmac_channel *ch,
>  		return NULL;
>  
>  	skb_reserve(skb, xdp->data - xdp->data_hard_start);
> -	memcpy(__skb_put(skb, datasize), xdp->data, datasize);
> +	__skb_put(skb, datasize);
> +	memcpy(skb->data - metasize, xdp->data_meta, datasize + metasize);
>  	if (metasize)
>  		skb_metadata_set(skb, metasize);

[Severity: Medium]
Does the metadata stay valid for VLAN tagged frames that are stripped in
software?

The commit message says:

  This keeps data_meta valid for the stack and TC BPF after eth_type_trans()
  anchors mac_header.

When priv->hw->hw_vlan_en is false, stmmac_dispatch_skb_zc() calls
stmmac_rx_vlan() after the skb is built and before eth_type_trans() runs:

stmmac_dispatch_skb_zc() {
    skb = stmmac_construct_skb_zc(ch, xdp);
    ...
    else
        /* Driver level stripping. */
        stmmac_rx_vlan(priv->dev, skb);
    skb->protocol = eth_type_trans(skb, priv->dev);
    ...
}

For an 802.1Q frame with NETIF_F_HW_VLAN_CTAG_RX set (or 802.1AD with
NETIF_F_HW_VLAN_STAG_RX), stmmac_rx_vlan() moves only the MAC addresses
and then pulls the tag:

stmmac_rx_vlan() {
    ...
    memmove(skb->data + VLAN_HLEN, veth, ETH_ALEN * 2);
    skb_pull(skb, VLAN_HLEN);
    ...
}

The metadata stays at [D - metasize, D), where D is the original frame
start. eth_type_trans() then sets mac_header to D + VLAN_HLEN. Since
skb_metadata_end() is skb_mac_header(), TC BPF and the stack would see
the window [D + 4 - metasize, D + 4).

Wouldn't that window lose the first 4 bytes of metadata and end with 4
stale destination MAC bytes?

This can happen with AF_XDP zero-copy, an XDP program that calls
bpf_xdp_adjust_meta() and returns XDP_PASS, and a VLAN tagged frame on a
core where hw_vlan_en is false. stmmac_dvr_probe() sets
NETIF_F_HW_VLAN_CTAG_RX for all cores but only sets hw_vlan_en for xmac
cores, so dwmac100/dwmac1000 use software stripping by default.

Before this patch the metadata was not copied at all, so this does not
make things worse than they were. A later patch in the same series, "net:
stmmac: fix VLAN tag stripping on short and non-linear RX frames", changes
stmmac_rx_vlan() to move meta_len + ETH_ALEN * 2 bytes starting at
skb->data - meta_len before skb_pull(), which fixes this case.

Could the commit message mention that the VLAN stripping path is fixed
separately, or could the two patches be reordered?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261027-stmmac-xdp-fixes-v2-0-10eae8a093a8%40oss.qualcomm.com

Re: [PATCH net v2 5/6] net: stmmac: fix VLAN tag stripping on short and non-linear RX frames

[netdev-bot+sashiko]

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

Critical: 0 · High: 0 · Medium: 1 · Low: 0 (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 says the VLAN-pop code now moves the XDP
  metadata written with bpf_xdp_adjust_meta() along with the MAC header,…

Pre-existing issues:
- [Low] priv->xstats.rx_dropped is a single device-wide unsigned long
  (struct stmmac_extra_stats, common.h:240).

--

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

stmmac_rx_vlan() now pulls ETH_HLEN, and then VLAN_ETH_HLEN, into the
linear area before it reads the VLAN header. It returns an error so that
stmmac_rx() and stmmac_dispatch_skb_zc() drop malformed frames. When the
tag is popped, it also moves any XDP metadata together with the MAC
addresses.

> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> index d9fc2464a160a..fb3b7174599e0 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> @@ -5102,22 +5102,35 @@ static netdev_features_t stmmac_features_check(struct sk_buff *skb,

[ ... ]

> +		__be16 vlan_proto = veth->h_vlan_proto;
> +		u8 meta_len = skb_metadata_len(skb);
> +		u16 vlanid;
> +
>  		/* pop the vlan tag */
> +		if (!pskb_may_pull(skb, VLAN_ETH_HLEN))
> +			return -EINVAL;
> +
> +		veth = skb_vlan_eth_hdr(skb);
>  		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]
The commit message says:

  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

Does that hold for frames coming from stmmac_rx(), or only for the AF_XDP
zero-copy path?

On the page_pool path, the XDP buffer is set up with meta_valid set to
true. However, the XDP_PASS skb is built without calling
skb_metadata_set():

stmmac_rx() {
    ...
	xdp_prepare_buff(&ctx.xdp, page_address(buf->page),
			 buf->page_offset, buf1_len, true);
    ...
	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);
    ...
}

napi_build_skb() clears skb_shared_info up to dataref, and meta_len sits
before dataref. So skb_metadata_len(skb) is always 0 here.

On that path, this memmove() does the same MAC-address-only move as
before. The metadata written by the XDP program stays where it was, and
TC/BPF sees an empty data_meta range.

The only skb_metadata_set() call in the driver appears to be in
stmmac_construct_skb_zc():

	if (metasize)
		skb_metadata_set(skb, metasize);

The last patch of the series does not seem to add another one either.

The missing skb_metadata_set() in stmmac_rx() predates this patch. Still,
the Fixes: tag points at 5fabb01207a2 ("net: stmmac: Add initial XDP
support"), which is the page_pool path.

Would it make sense to add something like
skb_metadata_set(skb, ctx.xdp.data - ctx.xdp.data_meta) in stmmac_rx()?
Alternatively, the commit message could say that the metadata handling
only applies to the zero-copy path.

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

[ ... ]

> @@ -5546,12 +5559,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: Low]
This is a pre-existing issue, not one introduced by this patch. Can
updates to priv->xstats.rx_dropped get lost here?

The counter is a single unsigned long in struct stmmac_extra_stats, shared
by all queues. Several per-queue NAPI contexts update it with a plain
read-modify-write:

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

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

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

With per-channel MSI or threaded NAPI, polls for different queues can run
on different CPUs at the same time. stmmac_get_stats64() then reads the
counter without any locking, so the reported rx_dropped value can be too
low.

This patch adds another writer that follows the existing pattern, and the
new VLAN drops in stmmac_rx() go through the same shared update.

> +		return;
> +	}
> +
>  	skb->protocol = eth_type_trans(skb, priv->dev);

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261027-stmmac-xdp-fixes-v2-0-10eae8a093a8%40oss.qualcomm.com

Re: [PATCH net v2 1/6] net: stmmac: take ownership of saved RX state at poll entry

[Lorenzo Bianconi]

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

> 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 gives the trigger as a saved partial packet
>   completing "with a poll budget of one".
> 
> Pre-existing issues:
> - [High] In stmmac_rx(), if napi_build_skb() fails on a non-last
>   descriptor, the code recycles the page, does rx_dropped++ and count++,…
> - [High] On resume, the RX ring is handed back to the DMA without its
>   buffers being refilled or their addresses rewritten, and this patch…
> 
> --
> 
> net: stmmac: take ownership of saved RX state at poll entry
> 
> This patch makes stmmac_rx() and stmmac_rx_zc() take the saved
> partial-frame state at poll entry, clear it right away, and save it again
> only if the frame is still incomplete when the loop exits. The in-progress
> flag is now tracked separately from the status of the last descriptor
> read. The saved skb is also dropped in stmmac_reset_rx_queue() and
> __free_dma_rx_desc_resources().
> 
> > 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.
> 
> [Severity: Low]
> Could the commit message say how a budget of one happens on a running
> system?

I guess this comment is minor, I can amend it if I need to rebase.

> 
> With limit > 1, the old else branch cleared state_saved on the second loop
> iteration. So the stale pointer only survives when limit == 1. Normal
> net_rx_action() polling passes the NAPI weight.
> 
> stmmac_napi_poll_rx() passes its budget straight through:
> 
>     work_done = stmmac_rx(priv, budget, chan);
> 
> Busy polling can supply a budget of 1. In ep_eventpoll_ioctl(), EPIOCSPARAMS
> only asks for CAP_NET_ADMIN above NAPI_POLL_WEIGHT:
> 
>     if (epoll_params.busy_poll_budget > NAPI_POLL_WEIGHT &&
>         !capable(CAP_NET_ADMIN))
>             return -EPERM;
> 
> SO_BUSY_POLL_BUDGET in sk_setsockopt() can also be lowered without
> privilege.
> 
> So an unprivileged local user who receives multi-descriptor frames can
> reach the use-after-free. Would it be worth saying so in the changelog, so
> stable reviewers can see this is a user-triggerable memory-safety fix?
> 
> > diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > index 9ded31150fbe6..fa7108919e90d 100644
> > --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> 
> [ ... ]
> 
> > @@ -6041,10 +6051,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;
> >  		}
> 
> [Severity: High]
> This is a pre-existing issue, but what happens in stmmac_rx() when
> napi_build_skb() fails on a descriptor that is not the last one in the
> frame?

This issue has not been introduced by this patch and it needs to be fixed in a
dedicated patch.

> 
>     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;
>     }
> 
> error stays 0 and skb stays NULL, and drain_data jumps back to read_again
> because rx_not_ls is set. The next descriptor of the same hardware frame
> passes the error checks and takes the if (!skb) frame-start path. A new
> skb is then built from only the tail of the frame.
> 
> len still counts the dropped prefix, so the pskb_trim(skb, len) above does
> nothing and the FCS is not removed. The trailing payload bytes are then
> delivered as a standalone Ethernet frame.
> 
> With Split Header active, stmmac_rx_buf1_len() returns 0 for any
> descriptor after the first:
> 
>     if (priv->sph_active && len)
>             return 0;
> 
> That leaves the rebuilt skb with no linear data, only the buf2 frag.
> Wouldn't eth_type_trans() then hit the BUG() in __skb_pull()?
> 
>     eth_type_trans()
>       eth_skb_pull_mac()
>         skb_pull_inline(skb, ETH_HLEN)
>           __skb_pull()
>             skb->len < skb->data_len -> BUG()
> 
> If the stale page bytes look like a VLAN header, stmmac_rx_vlan() can hit
> the same BUG() through skb_pull(skb, VLAN_HLEN).
> 
> An on-link peer sending a frame that spans several descriptors, plus a
> GFP_ATOMIC allocation failure, is enough to trigger this. With this patch,
> the in_progress tracking also carries this skb == NULL, error == 0 state
> across a poll boundary. Nothing else in the series seems to address it.
> 
> Should the napi_build_skb() failure path set error = 1, so the rest of the
> frame is thrown away?
> 
> [ ... ]
> 
> > @@ -8421,6 +8430,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 is a pre-existing issue, but this new free happens just before
> stmmac_resume() re-arms the RX ring in place:

This issue has not been introduced by this patch and it needs to be fixed in a
dedicated patch.

Regards,
Lorenzo

> 
> stmmac_resume()
>     stmmac_reset_queues_param()
>         stmmac_reset_rx_queue()    <- frees state.skb, rewinds cur/dirty
>     stmmac_free_tx_skbufs()
>     stmmac_clear_descriptors()
>         stmmac_clear_rx_descriptors()
>             stmmac_init_rx_desc()  <- sets OWN only
>     stmmac_hw_setup()
>         stmmac_init_dma_engine()   <- restores the tail pointer
> 
> No refill and no stmmac_set_desc_addr() runs before the DMA restarts. That
> means two kinds of descriptors get re-armed with a bad buffer address:
> 
>   - slots stmmac_rx() consumed whose refill failed. stmmac_rx_refill()
>     uses GFP_ATOMIC and stops at the first page_pool_alloc_pages()
>     failure, so buf->page is left NULL.
> 
>   - descriptors the DMA wrote back after napi_disable() in
>     stmmac_suspend() and before stmmac_stop_all_dma().
> 
> On dwmac4/xgmac, write-back overwrites des0/des1, which hold the buffer
> address in read format (dwmac4_wrback_get_rx_vlan_tci() reads des0 as the
> VLAN tag). dwmac4_rd_init_rx_desc() only sets OWN/BUF1V/IOC in RDES3. The
> DMA would then write to whatever status data is left in those words.
> 
> On norm/enh descriptors, the old address stays in des2. The slots most
> likely to have failed refill are the ones whose pages went into the saved
> partial skb. The dev_kfree_skb_any() above now returns those pages, still
> DMA-mapped, to the page_pool just before the hardware is re-armed to write
> into them.
> 
> Can this lead to DMA into pages the pool has already handed to another
> descriptor or released?
> 
> Also, if DMA completes into a slot with buf->page == NULL, stmmac_rx()
> calls page_address(NULL) and napi_build_skb() on it, which would oops.
> 
> The commit message notes that "the RX buffer addresses are not
> reprogrammed" on this path. Would a full fix need stmmac_resume() to
> refill and reprogram every RX descriptor before setting OWN?
> 
> -- 
> Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261027-stmmac-xdp-fixes-v2-0-10eae8a093a8%40oss.qualcomm.com

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

Re: [PATCH net v2 4/6] net: stmmac: copy XDP metadata in AF_XDP zero-copy RX

[Lorenzo Bianconi]

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

On Oct 30, netdev-bot+sashiko@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential
> issue(s) to consider.
> 
> Critical: 0 · High: 0 · Medium: 1 · Low: 0
> 
> If you believe none of the issues are legitimate, please reply to each
> issue of Medium severity or higher and explain why the AI is wrong.
> If any of them are legitimate and you plan to respin, please let patchwork
> know by including "pw-bot: cr" as a separate line at the end of your reply
> (one such reply per series is enough).
> 
> - [Medium] In this patch, stmmac_construct_skb_zc()
>   (drivers/net/ethernet/stmicro/stmmac/stmmac_main.c:5521-5525) copies…
> 
> --
> 
> net: stmmac: copy XDP metadata in AF_XDP zero-copy RX
> 
> This patch changes stmmac_construct_skb_zc() to copy the XDP metadata in
> [xdp->data_meta, xdp->data) into the new skb together with the packet
> data. The metadata sits right before skb->data, and its length is recorded
> with skb_metadata_set().
> 
> > diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > index 71858af6fd66d..d9fc2464a160a 100644
> > --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > @@ -5519,7 +5519,8 @@ static struct sk_buff *stmmac_construct_skb_zc(struct stmmac_channel *ch,
> >  		return NULL;
> >  
> >  	skb_reserve(skb, xdp->data - xdp->data_hard_start);
> > -	memcpy(__skb_put(skb, datasize), xdp->data, datasize);
> > +	__skb_put(skb, datasize);
> > +	memcpy(skb->data - metasize, xdp->data_meta, datasize + metasize);
> >  	if (metasize)
> >  		skb_metadata_set(skb, metasize);
> 
> [Severity: Medium]
> Does the metadata stay valid for VLAN tagged frames that are stripped in
> software?
> 
> The commit message says:
> 
>   This keeps data_meta valid for the stack and TC BPF after eth_type_trans()
>   anchors mac_header.
> 
> When priv->hw->hw_vlan_en is false, stmmac_dispatch_skb_zc() calls
> stmmac_rx_vlan() after the skb is built and before eth_type_trans() runs:
> 
> stmmac_dispatch_skb_zc() {
>     skb = stmmac_construct_skb_zc(ch, xdp);
>     ...
>     else
>         /* Driver level stripping. */
>         stmmac_rx_vlan(priv->dev, skb);
>     skb->protocol = eth_type_trans(skb, priv->dev);
>     ...
> }
> 
> For an 802.1Q frame with NETIF_F_HW_VLAN_CTAG_RX set (or 802.1AD with
> NETIF_F_HW_VLAN_STAG_RX), stmmac_rx_vlan() moves only the MAC addresses
> and then pulls the tag:
> 
> stmmac_rx_vlan() {
>     ...
>     memmove(skb->data + VLAN_HLEN, veth, ETH_ALEN * 2);
>     skb_pull(skb, VLAN_HLEN);
>     ...
> }
> 
> The metadata stays at [D - metasize, D), where D is the original frame
> start. eth_type_trans() then sets mac_header to D + VLAN_HLEN. Since
> skb_metadata_end() is skb_mac_header(), TC BPF and the stack would see
> the window [D + 4 - metasize, D + 4).
> 
> Wouldn't that window lose the first 4 bytes of metadata and end with 4
> stale destination MAC bytes?
> 
> This can happen with AF_XDP zero-copy, an XDP program that calls
> bpf_xdp_adjust_meta() and returns XDP_PASS, and a VLAN tagged frame on a
> core where hw_vlan_en is false. stmmac_dvr_probe() sets
> NETIF_F_HW_VLAN_CTAG_RX for all cores but only sets hw_vlan_en for xmac
> cores, so dwmac100/dwmac1000 use software stripping by default.
> 
> Before this patch the metadata was not copied at all, so this does not
> make things worse than they were. A later patch in the same series, "net:
> stmmac: fix VLAN tag stripping on short and non-linear RX frames", changes
> stmmac_rx_vlan() to move meta_len + ETH_ALEN * 2 bytes starting at
> skb->data - meta_len before skb_pull(), which fixes this case.
> 
> Could the commit message mention that the VLAN stripping path is fixed
> separately, or could the two patches be reordered?

I guess the reported issue is fixed in the following patch: "net: stmmac: fix
VLAN tag stripping on short and non-linear RX frames".

Regards,
Lorenzo

> 
> -- 
> Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261027-stmmac-xdp-fixes-v2-0-10eae8a093a8%40oss.qualcomm.com

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

Re: [PATCH net v2 5/6] net: stmmac: fix VLAN tag stripping on short and non-linear RX frames

[Lorenzo Bianconi]

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

> Thank you for your contribution! Sashiko AI review found 1 potential
> issue(s) to consider.
> 
> Critical: 0 · High: 0 · Medium: 1 · Low: 0 (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 says the VLAN-pop code now moves the XDP
>   metadata written with bpf_xdp_adjust_meta() along with the MAC header,…
> 
> Pre-existing issues:
> - [Low] priv->xstats.rx_dropped is a single device-wide unsigned long
>   (struct stmmac_extra_stats, common.h:240).
> 
> --
> 
> net: stmmac: fix VLAN tag stripping on short and non-linear RX frames
> 
> stmmac_rx_vlan() now pulls ETH_HLEN, and then VLAN_ETH_HLEN, into the
> linear area before it reads the VLAN header. It returns an error so that
> stmmac_rx() and stmmac_dispatch_skb_zc() drop malformed frames. When the
> tag is popped, it also moves any XDP metadata together with the MAC
> addresses.
> 
> > diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > index d9fc2464a160a..fb3b7174599e0 100644
> > --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > @@ -5102,22 +5102,35 @@ static netdev_features_t stmmac_features_check(struct sk_buff *skb,
> 
> [ ... ]
> 
> > +		__be16 vlan_proto = veth->h_vlan_proto;
> > +		u8 meta_len = skb_metadata_len(skb);
> > +		u16 vlanid;
> > +
> >  		/* pop the vlan tag */
> > +		if (!pskb_may_pull(skb, VLAN_ETH_HLEN))
> > +			return -EINVAL;
> > +
> > +		veth = skb_vlan_eth_hdr(skb);
> >  		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]
> The commit message says:
> 
>   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
> 
> Does that hold for frames coming from stmmac_rx(), or only for the AF_XDP
> zero-copy path?
> 
> On the page_pool path, the XDP buffer is set up with meta_valid set to
> true. However, the XDP_PASS skb is built without calling
> skb_metadata_set():
> 
> stmmac_rx() {
>     ...
> 	xdp_prepare_buff(&ctx.xdp, page_address(buf->page),
> 			 buf->page_offset, buf1_len, true);
>     ...
> 	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);
>     ...
> }
> 
> napi_build_skb() clears skb_shared_info up to dataref, and meta_len sits
> before dataref. So skb_metadata_len(skb) is always 0 here.
> 
> On that path, this memmove() does the same MAC-address-only move as
> before. The metadata written by the XDP program stays where it was, and
> TC/BPF sees an empty data_meta range.
> 
> The only skb_metadata_set() call in the driver appears to be in
> stmmac_construct_skb_zc():
> 
> 	if (metasize)
> 		skb_metadata_set(skb, metasize);
> 
> The last patch of the series does not seem to add another one either.
> 
> The missing skb_metadata_set() in stmmac_rx() predates this patch. Still,
> the Fixes: tag points at 5fabb01207a2 ("net: stmmac: Add initial XDP
> support"), which is the page_pool path.
> 
> Would it make sense to add something like
> skb_metadata_set(skb, ctx.xdp.data - ctx.xdp.data_meta) in stmmac_rx()?
> Alternatively, the commit message could say that the metadata handling
> only applies to the zero-copy path.

I agree this is real issue and it needs to be fixed, but it has not been
introduced by this patch and it needs to be fixed with a dedicated patch.

> 
> >  		skb_pull(skb, VLAN_HLEN);
> >  		__vlan_hwaccel_put_tag(skb, vlan_proto, vlanid);
> >  	}
> 
> [ ... ]
> 
> > @@ -5546,12 +5559,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: Low]
> This is a pre-existing issue, not one introduced by this patch. Can
> updates to priv->xstats.rx_dropped get lost here?

This issue has not been introduced by this patch and it needs to be fixed in a
dedicated patch.

Regards,
Lorenzo

> 
> The counter is a single unsigned long in struct stmmac_extra_stats, shared
> by all queues. Several per-queue NAPI contexts update it with a plain
> read-modify-write:
> 
> stmmac_dispatch_skb_zc():
> 	priv->xstats.rx_dropped++;
> 
> stmmac_rx_zc():
> 	priv->xstats.rx_dropped += rx_dropped;
> 
> stmmac_rx():
> 	priv->xstats.rx_dropped += rx_dropped;
> 
> With per-channel MSI or threaded NAPI, polls for different queues can run
> on different CPUs at the same time. stmmac_get_stats64() then reads the
> counter without any locking, so the reported rx_dropped value can be too
> low.
> 
> This patch adds another writer that follows the existing pattern, and the
> new VLAN drops in stmmac_rx() go through the same shared update.
> 
> > +		return;
> > +	}
> > +
> >  	skb->protocol = eth_type_trans(skb, priv->dev);
> 
> [ ... ]
> 
> -- 
> Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261027-stmmac-xdp-fixes-v2-0-10eae8a093a8%40oss.qualcomm.com

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