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

[PATCH net-next v5 0/3] 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.
Enable XDP support with MTU bigger than ETH_DATA_LEN.

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

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

Lorenzo Bianconi (2):
      net: stmmac: rework stmmac_rx to support XDP rx multi-buff
      net: stmmac: allow non-linear xdp_buff in XDP mode

 drivers/net/ethernet/stmicro/stmmac/stmmac.h      |   3 +-
 drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 401 ++++++++++++++--------
 drivers/net/ethernet/stmicro/stmmac/stmmac_xdp.c  |   8 -
 3 files changed, 253 insertions(+), 159 deletions(-)
---
base-commit: d8674294aefef02266c4d47ad10131f1bffbe534
change-id: 20260918-stmmac-rx-mb-16469a2714ea

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

[PATCH net-next v5 1/3] 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.

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.
Track whether a frame remains incomplete independently of the status
read from the next descriptor, so a DMA-owned descriptor does not erase
the continuation state.

Keep the running frame length when an XDP verdict ends a descriptor
early. The old loop returned to read_again for STMMAC_XDP_CONSUMED while
rx_not_ls was set, but the STMMAC_XDP_TX/STMMAC_XDP_REDIRECT path fell
through to the next iteration. Resetting the frame state there makes the
remaining descriptors of the same hardware frame get parsed as a new
frame; on cores with a second RX buffer this makes stmmac_rx_buf2_len()
report more bytes than the buffer holds and read past the page. Route
both verdicts back to read_again while rx_not_ls is set, clearing skb
first since it holds the XDP status rather than a packet.

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>
Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@oss.qualcomm.com>
---
 drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 91 +++++++++++++----------
 1 file changed, 52 insertions(+), 39 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index b8b4de4c5b1a..6128e6d9504c 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;
 
@@ -5620,12 +5623,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);
@@ -5636,23 +5639,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;
@@ -5691,6 +5696,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);
@@ -5705,6 +5712,7 @@ static int stmmac_rx_zc(struct stmmac_priv *priv, int limit, u32 queue)
 
 		if (unlikely(error && (status & rx_not_ls)))
 			goto read_again;
+
 		if (unlikely(error)) {
 			count++;
 			continue;
@@ -5763,7 +5771,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;
@@ -5805,9 +5813,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;
@@ -5827,25 +5836,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;
@@ -5874,6 +5886,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)) {
@@ -5886,11 +5900,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. */
@@ -5950,23 +5963,21 @@ static int stmmac_rx(struct stmmac_priv *priv, int limit, u32 queue)
 					buf->page = NULL;
 					rx_dropped++;
 
-					/* Clear skb as it was set as
-					 * status by XDP program.
-					 */
-					skb = NULL;
-
-					if (unlikely((status & rx_not_ls)))
+					if (unlikely((status & rx_not_ls))) {
+						skb = NULL;
 						goto read_again;
-
-					count++;
-					continue;
+					}
+					goto next;
 				} else if (xdp_res & (STMMAC_XDP_TX |
 						      STMMAC_XDP_REDIRECT)) {
 					xdp_status |= xdp_res;
 					buf->page = NULL;
-					skb = NULL;
-					count++;
-					continue;
+
+					if (unlikely((status & rx_not_ls))) {
+						skb = NULL;
+						goto read_again;
+					}
+					goto next;
 				}
 			}
 		}
@@ -6023,10 +6034,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);
@@ -6051,14 +6060,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;
@@ -8378,6 +8388,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

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

[netdev-bot+sashiko]

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

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

- [High] At this commit, stmmac_rx() can still treat the middle or last
  descriptor of one hardware frame as the start of a new frame.
- [High] bpf: packet injection via dropped jumbo frames in stmmac_rx_zc

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but it sits in the
  stmmac_rx_zc() continuation logic this patch changes, and it is still…
- [High] This problem wasn't introduced by this patch, but this patch's
  resume handling relies on it and the commit message describes the…
- [Medium] In stmmac_rx() at this commit, when napi_build_skb() fails on
  a descriptor that is not the last one, the code increments rx_dropped…

--

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 and save it again only if the frame is
still incomplete. It also drops the saved state in stmmac_reset_rx_queue()
and __free_dma_rx_desc_resources(). For non-PASS XDP verdicts on non-last
descriptors, both verdicts now go back to read_again and keep the running
frame length.

> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> index b8b4de4c5b1a7..6128e6d9504c8 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c

[ ... ]

> @@ -5691,6 +5696,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);
> @@ -5705,6 +5712,7 @@ static int stmmac_rx_zc(struct stmmac_priv *priv, int limit, u32 queue)
>  
>  		if (unlikely(error && (status & rx_not_ls)))
>  			goto read_again;
> +
>  		if (unlikely(error)) {
>  			count++;
>  			continue;

[Severity: High]
This is a pre-existing issue, but this patch reworks how
stmmac_rx_zc() carries error and len across descriptors and polls.
Should the dropped-head case be covered here too?

When a frame spans more than one XSK descriptor, the 1:1 mapping
check throws away the non-last descriptors:

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

That path never sets error. When the loop reaches the last descriptor
of the same frame, error is still 0, so both error checks are
skipped. The tail buffer then goes to __stmmac_xdp_run_prog() as if
it were a complete frame.

On XDP_PASS, stmmac_dispatch_skb_zc() builds an skb from it and
passes it up the stack. On XDP_REDIRECT, the AF_XDP socket receives
it.

The tail starts in the middle of the original payload, at an offset
the sender can predict from the RX buffer size. Can an on-link peer
send a jumbo frame (GMAC4 sets GMAC_CONFIG_JE) whose payload holds a
complete Ethernet/IP header at that offset?

The MAC address, VLAN and other hardware filters only check the outer
frame. The kernel would instead receive the crafted inner frame.

With this patch the same state can also cross a poll boundary. If the
budget runs out right after the head is dropped, in_progress is true
and error 0 is saved. The next poll then treats the tail the same
way.

Would setting error = 1 before the goto be enough? The error branches
only free buf->xdp and bump dirty when the current descriptor itself
reports discard_frame. Wouldn't the remaining descriptors of a
dropped frame then keep their XSK buffers and not be counted in
dirty? It looks like the drop path also has to release each
continuation buffer.

Later in the series stmmac_rx() is reworked to collect all
descriptors of a frame. stmmac_rx_zc() keeps this drop path, so the
problem is still present at the end of the series.

[Severity: High]
This is a pre-existing issue, but this patch reworks the continuation
state in stmmac_rx_zc(). Is the multi-descriptor case handled correctly
right after this point?

Non-last descriptors are dropped without setting error and without adding
their bytes to len:

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

So for an error-free frame that spans several XSK descriptors, the
continuation state is always error 0 and len 0. The last descriptor then
reaches:

		buf1_len = stmmac_rx_buf1_len(priv, p, status, len);
		...
		buf->xdp->data_end = buf->xdp->data + buf1_len;

With len 0 and SPH disabled, stmmac_rx_buf1_len() returns
min(dma_buf_sz, plen). Here plen is the total frame length, not the number
of bytes in the last buffer.

Doesn't that make data_end cover stale UMEM bytes past the real tail? If
the XSK rx frame size is smaller than dma_buf_sz, can it even point past
the chunk?

On XDP_PASS, stmmac_dispatch_skb_zc() copies this into an skb. When coe is
set and the ethertype is IP, it sets CHECKSUM_UNNECESSARY without checking
csum_none. On REDIRECT, the AF_XDP socket gets the same frame.

The head of the frame is dropped, and its tail plus stale data is
delivered as a standalone frame. An on-link peer can trigger this by
sending a frame longer than the XSK rx frame size, because GMAC4 always
sets GMAC_CONFIG_JE. The code is unchanged later in the series.

[ ... ]

> @@ -5950,23 +5963,21 @@ static int stmmac_rx(struct stmmac_priv *priv, int limit, u32 queue)
>  					buf->page = NULL;
>  					rx_dropped++;
>  
> -					/* Clear skb as it was set as
> -					 * status by XDP program.
> -					 */
> -					skb = NULL;
> -
> -					if (unlikely((status & rx_not_ls)))
> +					if (unlikely((status & rx_not_ls))) {
> +						skb = NULL;
>  						goto read_again;
> -
> -					count++;
> -					continue;
> +					}
> +					goto next;
>  				} else if (xdp_res & (STMMAC_XDP_TX |
>  						      STMMAC_XDP_REDIRECT)) {
>  					xdp_status |= xdp_res;
>  					buf->page = NULL;
> -					skb = NULL;
> -					count++;
> -					continue;
> +
> +					if (unlikely((status & rx_not_ls))) {
> +						skb = NULL;
> +						goto read_again;
> +					}

[Severity: High]
After jumping to read_again with skb == NULL, could the next descriptor of
the same hardware frame still be parsed as a new frame?

In stmmac_rx(), skb == NULL is the only marker for the start of a frame.
The next descriptor therefore enters the frame-start block again:

		if (!skb) {
			...
			/* Each frame starts here: reset the FCS handling */
			fcs_stripped = false;
			...
			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);

Because len is now kept, stmmac_rx_buf1_len() returns
min(dma_buf_sz, plen - len) for the last descriptor, and that can be 0 to
3 bytes. For example, on GMAC4 take a 3074-byte frame whose first
descriptor holds 3072 bytes in buf1 + buf2. The last descriptor then
holds 2 bytes.

Can buf1_len -= ETH_FCS_LEN then wrap to 0xFFFFFFFE, so that
xdp_prepare_buff() sets data_end = data - 2?

Suppose the XDP program returns XDP_PASS for that continuation. buf1_len
is recomputed as data_end - data and passed to skb_put(skb, buf1_len).
The tail wraps to below end, so skb_over_panic() does not fire. An skb
with len close to 4GB then goes to eth_type_trans() and the stack, with
no FCS trim because fcs_stripped is set.

If the continuation holds 4 or more bytes, the tail of a dropped or
redirected frame is instead shown to XDP as a standalone Ethernet frame,
and delivered on PASS.

The XDP_CONSUMED branch above already worked this way, and this change
puts XDP_TX and XDP_REDIRECT on the same path. The commit message says:

    Resetting the frame state there makes the
    remaining descriptors of the same hardware frame get parsed as a new
    frame

Isn't that still the case after this patch?

Later in the series, "net: stmmac: rework stmmac_rx to support XDP rx
multi-buff" collects the whole frame into an xdp_buff, runs XDP only at
the last descriptor, and strips the FCS with stmmac_xdp_shrink_tail().
That resolves this. Since this patch carries Fixes: tags, could it be
backported without that rework?

> +					goto next;
>  				}
>  			}
>  		}

[ ... ]

> @@ -6023,10 +6034,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: Medium]
This isn't a bug introduced by this patch, but what happens here if
napi_build_skb() failed on an earlier non-last descriptor of the same
frame?

That path leaves skb NULL and does not set error:

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

Because rx_not_ls is set, drain_data goes back to read_again. The next
descriptor then builds a new skb from the remaining mid-frame bytes.

This also works across a budget boundary. The saved state is in_progress
with skb NULL, error 0 and a nonzero len.

At the last descriptor, pskb_trim(skb, len) gets the full frame length.
That is larger than skb->len, so it returns 0 without trimming.

Doesn't that hand napi_gro_receive() a truncated frame that starts with
mid-frame payload and still carries the FCS? Doesn't it also over-count
rx_bytes?

This path goes away later in the series with "net: stmmac: rework
stmmac_rx to support XDP rx multi-buff". There, stmmac_build_skb() is
only called at the last descriptor, and a failure goes to error_free_frag.

[ ... ]

> @@ -8378,6 +8388,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 the commit message calls the resumed
ring freshly initialized:

    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.

Is the ring actually rebuilt on the stmmac_resume() path?

Suppose stmmac_rx_refill() hits a GFP_ATOMIC page_pool allocation failure
in the last poll before suspend. It breaks out and leaves the entries
between dirty_rx and cur_rx without pages:

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

On resume the sequence is then:

stmmac_resume()
  stmmac_reset_queues_param()
    stmmac_reset_rx_queue()
      frees state.skb, returning its pages to the page pool
      zeroes cur_rx and dirty_rx
  stmmac_clear_descriptors()
    init_rx_desc on every descriptor
  stmmac_hw_setup()
    stmmac_init_dma_engine()
      RX tail pointer set to buf_alloc_num

On GMAC4, dwmac4_rd_init_rx_desc() only sets OWN | BUF1V | IOC in des3.
It does not restore des0/des1, which write-back overwrote with status
data. On enhanced/normal descriptors, des2 still holds the old page's DMA
address.

Could the DMA then write to an address built from write-back status bits,
or into a page that was just released to the page pool?

When stmmac_rx() later reaches such an entry, buf->page is NULL. The call
napi_build_skb(page_address(buf->page), ...) then uses a bogus address.

Before this patch, the stale skb was resumed and then freed on its first
completion while the descriptor still referenced its page. So the
page-release part already existed.

The same structure is still there at the end of the series, where
stmmac_reset_rx_queue() calls stmmac_xdp_put_buff() on the saved
xdp_buff.

Does the resume path need to rebuild the missing RX buffers and
descriptor addresses, and handle allocation failure, before it restarts
RX DMA?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261009-stmmac-rx-mb-v5-0-c38fa4eaa138%40oss.qualcomm.com

[PATCH net-next v5 2/3] net: stmmac: rework stmmac_rx to support XDP rx multi-buff

[Lorenzo Bianconi]

Build the xdp_buff by accumulating all the descriptors that make up a
frame, so the XDP program runs on the full (possibly fragmented) packet
instead of just the first buffer. 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.

For those multi-descriptor frames the old code was buggy as well: it ran
the XDP program once per descriptor buffer, so for a multi-descriptor
frame the program was also invoked on continuation buffers holding
mid-frame payload. It also subtracted ETH_FCS_LEN from the first buffer
even when it was not the last descriptor (its second buffer empty),
dropping four bytes from the middle of the frame while marking the FCS as
already stripped and skipping the final pskb_trim(). Accumulating the
whole frame before running XDP fixes both.

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;
- drop multi-buffer frames when the attached program was not loaded with
  BPF_F_XDP_HAS_FRAGS, since a single-buffer program must not be handed a
  non-linear xdp_buff;
- 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;
- 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 | 346 ++++++++++++++--------
 2 files changed, 222 insertions(+), 127 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac.h b/drivers/net/ethernet/stmicro/stmmac/stmmac.h
index 9278378407e8..63020e8edd8a 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 6128e6d9504c..be2cba8fcbc6 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;
@@ -5460,8 +5465,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) {
@@ -5469,7 +5474,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);
 }
@@ -5798,6 +5804,118 @@ 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,
+					  num_frags * xdp->frame_sz,
+					  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--;
+		} 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
@@ -5811,21 +5929,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);
@@ -5838,23 +5955,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;
 		}
@@ -5891,19 +6010,24 @@ 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;
 			error = 1;
 			if (!priv->hwts_rx_en)
 				rx_errors++;
 		}
 
-		if (unlikely(error && (status & rx_not_ls)))
-			goto read_again;
-
 		if (unlikely(error)) {
-			dev_kfree_skb(skb);
-			goto next;
+			page_pool_put_page(rx_q->page_pool, buf->page, 0, true);
+			buf->page = NULL;
+			if (buf->sec_page) {
+				page_pool_put_page(rx_q->page_pool,
+						   buf->sec_page, 0, true);
+				buf->sec_page = NULL;
+			}
+
+			if (status & rx_not_ls)
+				goto read_again;
+
+			goto error_free_frag;
 		}
 
 		/* Buffer is good. Go on. */
@@ -5913,131 +6037,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;
 
-		/* 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++;
-
-					if (unlikely((status & rx_not_ls))) {
-						skb = NULL;
-						goto read_again;
-					}
-					goto next;
-				} else if (xdp_res & (STMMAC_XDP_TX |
-						      STMMAC_XDP_REDIRECT)) {
-					xdp_status |= xdp_res;
-					buf->page = NULL;
-
-					if (unlikely((status & rx_not_ls))) {
-						skb = NULL;
-						goto read_again;
-					}
-					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)
@@ -6064,15 +6146,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);
@@ -8068,6 +8158,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;
 
 	ret = stmmac_tc_init(priv, priv);
@@ -8388,9 +8479,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 v5 2/3] net: stmmac: rework stmmac_rx to support XDP rx multi-buff

[netdev-bot+sashiko]

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

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

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

- [High] net: stmmac: OOB read in stmmac_rx_zc() for multi-buffer frames
- [Medium] Real RX bug fixes are bundled into a net-next feature rework
  that has no Fixes: tag.
- [Medium] The patch now attaches XDP metadata to the skb in
  stmmac_build_skb() (skb_metadata_set(skb, metasize)).
- [Low] The new error branch in stmmac_rx() frees both buf->page and
  buf->sec_page for every descriptor seen while `error` is set.

Pre-existing issues:
- [Medium] This problem wasn't introduced by this patch, but the AF_XDP
  zero-copy XDP_PASS path still loses metadata contents.
- [Medium] This problem wasn't introduced by this patch, but RX
  descriptor reads have no DMA read ordering.

--

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

This reworks stmmac_rx() so all descriptors of a frame are collected into
an xdp_buff before the XDP program runs. The skb is then built from that
xdp_buff with napi_build_skb() and xdp_update_skb_frags_info(). The
in-progress xdp_buff is kept in rx_q->state across NAPI polls,
NETDEV_XDP_ACT_RX_SG is advertised, XDP metadata is carried into the skb,
and the FCS is stripped by a new stmmac_xdp_shrink_tail() helper.

> For those multi-descriptor frames the old code was buggy as well: it ran
> the XDP program once per descriptor buffer, so for a multi-descriptor
> frame the program was also invoked on continuation buffers holding
> mid-frame payload. It also subtracted ETH_FCS_LEN from the first buffer
> even when it was not the last descriptor (its second buffer empty),
> dropping four bytes from the middle of the frame while marking the FCS as
> already stripped and skipping the final pskb_trim(). Accumulating the
> whole frame before running XDP fixes both.

[ ... ]

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

[Severity: Medium]
The paragraph above describes two existing RX bugs in stmmac_rx():

- the XDP program runs once per descriptor buffer.
- ETH_FCS_LEN is subtracted from buffer1 of a non-last descriptor
  whenever stmmac_xdp_is_enabled(priv) && !buf2_len.

The old path also lost XDP metadata on XDP_PASS.
xdp_prepare_buff() was called with meta_valid=true, but
skb_metadata_set() was never called.

The FCS strip logic came from 88095728511e ("net: stmmac: fix rx
Scatter-Gather support"). That commit carries Fixes: 88ebe2cf7f3f, so it
is headed for stable. The per-descriptor XDP invocation goes back to
5fabb01207a2 and be8b38a722e6 from 2021.

GMAC4 sets GMAC_CONFIG_JE unconditionally. So even at MTU 1500 with XDP
loaded, a remote peer can send a frame that spans several descriptors.
The mid-frame FCS strip additionally needs stmmac_rx_check_buf2_cap() to
be false, which is the case on legacy cores.

Here these fixes are only side effects of a patch that also adds
NETDEV_XDP_ACT_RX_SG and multi-buffer support. The only trailer is
Signed-off-by. The first patch in this series ("net: stmmac: take
ownership of saved RX state at poll entry") does carry Fixes: tags.

Could the fixes go in a separate patch with Fixes: tags, so they can be
picked up for net and stable? As it stands, stable trees that take
88095728511e will keep the per-descriptor XDP run, the mid-frame FCS
strip and the dropped metadata.

> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> index 6128e6d9504c8..be2cba8fcbc6d 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c

[ ... ]

> @@ -5798,6 +5804,118 @@ static int stmmac_rx_zc(struct stmmac_priv *priv, int limit, u32 queue)
>  	return failure ? limit : (int)count;
>  }
>  

[Severity: High]
This isn't a bug introduced by this patch. The commit message calls
this work groundwork for XDP on frames that span several descriptors,
though, and the AF_XDP zero-copy receive path still mishandles such
frames. stmmac_rx_zc() releases every non-last descriptor of a frame
without adding its length to len:

drivers/net/ethernet/stmicro/stmmac/stmmac_main.c:stmmac_rx_zc() {
    ...
	/* 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;
	}
    ...
	buf1_len = stmmac_rx_buf1_len(priv, p, status, len);
    ...
}

When the last descriptor arrives, len is still 0, or whatever was
saved at the previous poll boundary. stmmac_rx_buf1_len() therefore
computes min(dma_buf_sz, plen - len) from the full frame length, not
from the bytes left for this descriptor. For a frame longer than
dma_buf_sz that gives dma_buf_sz. The DMA only wrote the last
plen - (n - 1) * dma_buf_sz bytes into this XSK buffer.

buf->xdp->data_end is then set from that value. The XDP program, an
AF_XDP socket on XDP_REDIRECT, and stmmac_construct_skb_zc() on
XDP_PASS all receive the tail of the frame plus whatever was left in
the UMEM chunk, presented as a complete packet. The head of the frame
is silently lost, and none of this is counted in rx_dropped.

As noted above, GMAC4 sets GMAC_CONFIG_JE unconditionally. A remote
peer can therefore already send such frames while an XSK pool is
bound, whatever the MTU check in stmmac_xdp_set_prog() allows.

Fixing only the len accounting would still hand userspace the tail of
the frame as if it were a whole packet. Should stmmac_rx_zc() instead
mark the frame as errored on the first rx_not_ls descriptor? The last
descriptor would then be released too, and the whole frame accounted
as dropped.

[ ... ]

> +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);

[Severity: Medium]
This isn't a bug introduced by this patch, but the AF_XDP zero-copy
XDP_PASS path still loses the metadata contents.
stmmac_construct_skb_zc() copies only the packet bytes into the new skb:

drivers/net/ethernet/stmicro/stmmac/stmmac_main.c: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);
    ...
}

skb_metadata_set() only records meta_len. The bytes in
[xdp->data_meta, xdp->data) are never copied.

Take a program that calls bpf_xdp_adjust_meta() and returns XDP_PASS,
on the path stmmac_rx_zc()->stmmac_dispatch_skb_zc()->
stmmac_construct_skb_zc(). Would a TC/BPF program then read
uninitialized skb headroom through data_meta?

Other drivers such as i40e copy from xdp->data_meta instead. The commit
message says metadata is now carried into the skb. Should the ZC path
get the same treatment?

> +
> +	if (unlikely(xdp_buff_has_frags(xdp)))
> +		xdp_update_skb_frags_info(skb, num_frags, sinfo->xdp_frags_size,
> +					  num_frags * xdp->frame_sz,
> +					  xdp_buff_get_skb_flags(xdp));
> +	return skb;
> +}

[ ... ]

> @@ -5891,19 +6010,24 @@ 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;
>  			error = 1;
>  			if (!priv->hwts_rx_en)
>  				rx_errors++;
>  		}

[Severity: Medium]
This is a pre-existing issue, but is a DMA read barrier missing between
the ownership check and the reads of the other descriptor words?
dwmac4_wrback_get_rx_status() loads des1 and des2 before it checks
RDES3_OWN:

drivers/net/ethernet/stmicro/stmmac/dwmac4_descs.c:dwmac4_wrback_get_rx_status() {
	u32 rdes1 = le32_to_cpu(p->des1);
	u32 rdes2 = le32_to_cpu(p->des2);
	u32 rdes3 = le32_to_cpu(p->des3);
    ...
	if (unlikely(rdes3 & RDES3_OWN))
		return dma_own;
    ...
}

stmmac_rx() has no dma_rmb() after its dma_own check either. On weakly
ordered CPUs such as arm64, could des1/des2 be read before the DMA
writeback and des3 after it?

If so, a stale rdes1 could be missing RDES1_IP_PAYLOAD_ERROR or
RDES1_IP_HDR_ERROR, and a frame with a bad checksum would be marked
CHECKSUM_UNNECESSARY. Stale rdes2 bits could also cause spurious
filter-fail drops.

>  
> -		if (unlikely(error && (status & rx_not_ls)))
> -			goto read_again;
> -
>  		if (unlikely(error)) {
> -			dev_kfree_skb(skb);
> -			goto next;
> +			page_pool_put_page(rx_q->page_pool, buf->page, 0, true);
> +			buf->page = NULL;
> +			if (buf->sec_page) {
> +				page_pool_put_page(rx_q->page_pool,
> +						   buf->sec_page, 0, true);
> +				buf->sec_page = NULL;
> +			}

[Severity: Low]
This frees buf->sec_page for every descriptor seen while error is set.
On GMAC4, dwmac4_wrback_get_rx_status() returns discard_frame for RX
timestamp context descriptors:

	if (unlikely(rdes3 & RDES3_CONTEXT_DESCRIPTOR))
		return discard_frame;

The DMA writes only descriptor words for those slots, so their pages are
still valid. With hwts_rx_en, every timestamped frame is followed by such
a slot.

Before this patch, discard_frame released only buf->page and sec_page
stayed attached. Continuation descriptors of an errored frame also kept
their pages.

Is it intended that each context slot now hands two DMA-mapped pages
back to the page_pool? stmmac_rx_refill() then has to allocate a new
sec_page with GFP_ATOMIC for each one. It stops at the first allocation
failure, so this adds one more way for ring re-arming to stall under
memory pressure.

> +
> +			if (status & rx_not_ls)
> +				goto read_again;
> +
> +			goto error_free_frag;
>  		}

[ ... ]

> @@ -5913,131 +6037,89 @@ static int stmmac_rx(struct stmmac_priv *priv, int limit, u32 queue)

[ ... ]

> +		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)

[Severity: Medium]
stmmac_build_skb() now calls skb_metadata_set(skb, metasize). That
assumes the metadata sits directly before the MAC header, because
skb_metadata_end() returns skb_mac_header(skb).

When hw_vlan_en is false, stmmac_rx_vlan() pops the tag by moving the
MAC addresses forward. This happens on dwmac100/dwmac1000 cores with
NETIF_F_HW_VLAN_CTAG_RX set, for example:

drivers/net/ethernet/stmicro/stmmac/stmmac_main.c:stmmac_rx_vlan() {
    ...
		memmove(skb->data + VLAN_HLEN, veth, ETH_ALEN * 2);
		skb_pull(skb, VLAN_HLEN);
    ...
}

The metadata stays where it is, and eth_type_trans() then sets the MAC
header VLAN_HLEN bytes further in. For VLAN-tagged frames, would TC/BPF
programs reading data_meta lose the first 4 metadata bytes and see 4
bytes of the original destination MAC in their place?

The core helper skb_reorder_vlan_header() moves the metadata along with
the header:

net/core/skbuff.c:skb_reorder_vlan_header() {
    ...
	meta_len = skb_metadata_len(skb);
	if (meta_len) {
		meta = skb_metadata_end(skb) - meta_len;
		memmove(meta + VLAN_HLEN, meta, meta_len);
	}
    ...
}

Before this patch the metadata was dropped (meta_len 0) rather than
misaligned.

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261009-stmmac-rx-mb-v5-0-c38fa4eaa138%40oss.qualcomm.com

[PATCH net-next v5 3/3] net: stmmac: allow non-linear xdp_buff in XDP mode

[Lorenzo Bianconi]

Since the RX path now builds multi-buffer xdp_buffs, XDP can operate on
frames bigger than ETH_DATA_LEN (jumbo frames and scatter-gather
frames). Drop the checks that rejected attaching an XDP program when
the MTU is larger than ETH_DATA_LEN, and that refused to increase the
MTU while XDP is enabled.

Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@oss.qualcomm.com>
---
 drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 6 ------
 drivers/net/ethernet/stmicro/stmmac/stmmac_xdp.c  | 8 --------
 2 files changed, 14 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index be2cba8fcbc6..cbeb43011c4b 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -6351,12 +6351,6 @@ static int stmmac_change_mtu(struct net_device *dev, int new_mtu)
 		txfifosz = priv->dma_cap.tx_fifo_size;
 
 	txfifosz /= priv->plat->tx_queues_to_use;
-
-	if (stmmac_xdp_is_enabled(priv) && new_mtu > ETH_DATA_LEN) {
-		netdev_dbg(priv->dev, "Jumbo frames not supported for XDP\n");
-		return -EINVAL;
-	}
-
 	new_mtu = STMMAC_ALIGN(new_mtu);
 
 	/* If condition true, FIFO is too small or MTU too large */
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_xdp.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_xdp.c
index 9af9e0cb366c..f6b3e8628a7c 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_xdp.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_xdp.c
@@ -109,14 +109,6 @@ int stmmac_xdp_set_prog(struct stmmac_priv *priv, struct bpf_prog *prog,
 
 	if_running = netif_running(dev);
 
-	if (prog && dev->mtu > ETH_DATA_LEN) {
-		/* For now, the driver doesn't support XDP functionality with
-		 * jumbo frames so we return error.
-		 */
-		NL_SET_ERR_MSG_MOD(extack, "Jumbo frames not supported");
-		return -EOPNOTSUPP;
-	}
-
 	if (!prog)
 		xdp_features_clear_redirect_target(dev);
 

-- 
2.56.0

Re: [PATCH net-next v5 3/3] net: stmmac: allow non-linear xdp_buff in XDP mode

[netdev-bot+sashiko]

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

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

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

- [Critical] Out-of-bounds read in the AF_XDP zero-copy RX path,
  triggerable by a remote peer.
- [High] XDP_TX and ndo_xdp_xmit frames longer than buffer1 of a legacy
  (normal or enhanced) TX descriptor are programmed wrongly.
- [High] On XDP_TX of the head buffer, stmmac_xdp_xmit_xdpf() computes
  the DMA address as `page_pool_get_dma_addr(virt_to_page(xdpf->data)) +…
- [Medium] On bugged_jumbo cores (snps,dwmac-3.40a, snps,dwmac-3.610,
  snps,dwmac-3.710 per stmmac_probe_config_dt()), TX checksum insertion…
- [Medium] net/stmmac: silent drop of jumbo frames with non-SG XDP

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but the patch
  makes it much easier to hit.
- [High] This problem wasn't introduced by this patch, but the patch
  exposes it to XDP and to XDP_TX/redirect.

--

net: stmmac: allow non-linear xdp_buff in XDP mode

This commit removes the check that rejected attaching an XDP program
when the MTU is above ETH_DATA_LEN. It also removes the check that
refused to raise the MTU above ETH_DATA_LEN while XDP is enabled. The
reason given is that the RX path now builds multi-buffer xdp_buffs.

> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> index be2cba8fcbc6d..cbeb43011c4bd 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> @@ -6351,12 +6351,6 @@ static int stmmac_change_mtu(struct net_device *dev, int new_mtu)
>  		txfifosz = priv->dma_cap.tx_fifo_size;
>  
>  	txfifosz /= priv->plat->tx_queues_to_use;
> -
> -	if (stmmac_xdp_is_enabled(priv) && new_mtu > ETH_DATA_LEN) {
> -		netdev_dbg(priv->dev, "Jumbo frames not supported for XDP\n");
> -		return -EINVAL;
> -	}
> -

[Severity: Critical]

Can this lead to an out-of-bounds read in the AF_XDP zero-copy RX path?

ZC RX only runs while an XDP program is attached. Before this patch,
that kept dma_conf.dma_buf_sz at 1536.

With XDP attached and, say, MTU 9000, stmmac_setup_dma_desc() now picks
BUF_SIZE_16KiB. For a ZC queue, stmmac_set_queue_rx_buf_size() programs
the hardware buffer size from the pool:

    buf_size = xsk_pool_get_rx_frame_size(rx_q->xsk_pool);

So a jumbo frame gets split across several descriptors. stmmac_rx_zc()
still assumes a 1:1 mapping. It drops the non-last buffers without
adding to len or setting error:

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

When the last descriptor arrives, len is still 0, so stmmac_rx_buf1_len()
returns about the full frame length:

    return min_t(unsigned int, priv->dma_conf.dma_buf_sz, plen - len);

stmmac_rx_zc() then sets:

    buf->xdp->data_end = buf->xdp->data + buf1_len;

This puts data_end about 9000 bytes past data. The XSK chunk only holds
xsk_pool_get_rx_frame_size() bytes.

Three consumers would then read past the chunk:

  - the BPF program
  - the memcpy() in stmmac_construct_skb_zc() on XDP_PASS
  - xdp_convert_zc_to_xdp_frame() on XDP_TX

They would read neighbouring UMEM chunks, or fault past the end of the
UMEM mapping in NAPI context.

Both orders seem to reach this state:

  - bind the pool at MTU 1500, then raise the MTU here
  - raise the MTU first, then attach the program

The mtu + ETH_PAD_LEN <= frame_size check in xp_assign_dev() only runs
at bind time. stmmac_xdp_enable_pool() only requires frame_size >=
ETH_FRAME_LEN + VLAN_HLEN * 2, and its comment says "XDP ZC does not
span multiple frame".

The commit message says "XDP can operate on frames bigger than
ETH_DATA_LEN". That doesn't seem to hold for the ZC path, and the driver
still advertises NETDEV_XDP_ACT_XSK_ZEROCOPY.

Should the MTU limit stay in place while an XSK pool is bound, or be
checked against xsk_pool_get_rx_frame_size()?

[Severity: High]

This is a pre-existing issue, but this patch exposes it to XDP programs
and to XDP_TX/redirect. In chain mode (forced for dwmac-sun8i, or with
chain_mode=1), does the RX length accounting match what the hardware
writes?

chain_mode_ops has no set_16kib_bfsize, so stmmac_set_bfsize() picks
dma_buf_sz = BUF_SIZE_16KiB for len > BUF_SIZE_8KiB. The descriptor
init caps hardware buffer1:

enh_desc_init_rx_desc():
    bfsize1 = min(bfsize, BUF_SIZE_8KiB);

ndesc_init_rx_desc() caps it at 2047. Chain mode has no buffer2,
because des3 is the next-descriptor pointer.

stmmac_rx_buf1_len() still returns the full software size for every
non-last descriptor:

    if (status & rx_not_ls)
        return priv->dma_conf.dma_buf_sz;

For the last descriptor, the unsigned plen - len wraps (for example
9022 - 16368), so min_t() returns dma_buf_sz again.

Wouldn't a 9022-byte frame then become a ~32 KiB xdp_buff that is
mostly stale data from recycled page_pool pages? The XDP program would
see that data, and XDP_TX would send it back out. The non-XDP skb path
builds from the same xdp_buff and looks affected in the same way.

>  	new_mtu = STMMAC_ALIGN(new_mtu);
>  
>  	/* If condition true, FIFO is too small or MTU too large */
> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_xdp.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_xdp.c
> index 9af9e0cb366c3..f6b3e8628a7cf 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_xdp.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_xdp.c
> @@ -109,14 +109,6 @@ int stmmac_xdp_set_prog(struct stmmac_priv *priv, struct bpf_prog *prog,
>  
>  	if_running = netif_running(dev);
>  
> -	if (prog && dev->mtu > ETH_DATA_LEN) {
> -		/* For now, the driver doesn't support XDP functionality with
> -		 * jumbo frames so we return error.
> -		 */
> -		NL_SET_ERR_MSG_MOD(extack, "Jumbo frames not supported");
> -		return -EOPNOTSUPP;
> -	}
> -

[Severity: Medium]
Removing both checks entirely means a single-buffer XDP program can now
be combined with a jumbo MTU. Neither stmmac_xdp_set_prog() nor
stmmac_change_mtu() looks at prog->aux->xdp_has_frags. That allows the
setup in either order:

  - attach a program without frags support at MTU 9000
  - raise the MTU while such a program is attached

After that, any frame that spans more than one RX buffer arrives as an
xdp_buff with frags. stmmac_xdp_run_prog() then skips the program:

    if (likely(!xdp_buff_has_frags(xdp) || prog->aux->xdp_has_frags))
        res = __stmmac_xdp_run_prog(priv, prog, xdp);

res stays at STMMAC_XDP_CONSUMED, so the frame is dropped. All traffic
larger than one RX buffer is blackholed. The user got no error when
setting this up that would explain why.

Other multi-buffer capable drivers reject this combination up front.
mvneta, for example, refuses to attach a program without xdp_has_frags
when the MTU exceeds its single-buffer size. It also refuses the
matching MTU change while such a program is loaded.

Should these two checks be kept, but applied only when the program
lacks xdp_has_frags? If so, is ETH_DATA_LEN still the right threshold,
or should it be the largest frame that fits in one RX buffer?

[Severity: High]

With XDP now allowed at jumbo MTU, can stmmac_xdp_xmit_xdpf() program
legacy (normal or enhanced) TX descriptors with buffers longer than
buffer1?

stmmac_xdp_xmit_xdpf() writes one descriptor per buffer. It sets only
des2 and passes the full length:

    stmmac_set_desc_addr(priv, desc, dma_addr);
    stmmac_prepare_tx_desc(priv, desc, !i, len, csum,
                           priv->descriptor_mode, !!i, last_frame,
                           xdp_get_frame_len(xdpf));

In ring mode, enh_set_tx_desc_len_on_ring() moves the excess into the
buffer2 size field when len > 4096:

    if (unlikely(len > buffer1_max_length)) {
        p->des1 |= cpu_to_le32(FIELD_PREP(ETDES1_BUFFER2_SIZE_MASK,
                               len - buffer1_max_length) |

norm_set_tx_desc_len_on_ring() does the same above 2047. The DMA engine
then reads buffer2 from des3, which the XDP path never sets. It is
usually 0, or a stale value.

The skb path handles this in stmmac_xmit() through
stmmac_is_jumbo_frm(), with jumbo_frm() in ring_mode.c doing:

    desc->des3 = cpu_to_le32(lower_32_bits(des2) + BUF_SIZE_4KiB);

In chain mode, des3 is the next-descriptor pointer and the length is
masked with ETDES1_BUFFER1_SIZE_MASK. A 9018-byte frame would go out
truncated.

Should stmmac_xdp_xmit_xdpf() split oversized buffers the same way the
skb path does?

[Severity: High]

Does stmmac_xdp_xmit_xdpf() still compute the right DMA address for the
head buffer, now that RX pages can be compound?

    page = frag ? skb_frag_page(frag)
                : virt_to_page(xdpf->data);
    dma_addr = page_pool_get_dma_addr(page);

The offset added afterwards (sizeof(*xdpf) + xdpf->headroom) is relative
to the head page. virt_to_page() returns the page that contains
xdpf->data.

At jumbo MTU, __alloc_dma_rx_desc_resources() now ends up with
pp_params.order = order_base_2(num_pages) > 0 while XDP is enabled.

Suppose a program moves data forward with bpf_xdp_adjust_head() by more
than about a page and returns XDP_TX. Then xdpf->data is in a tail page,
and the tail page doesn't hold the page_pool DMA address. Wouldn't
dma_sync_single_for_device() and the TX descriptor get a bogus bus
address?

stmmac_xdp_put_buff() in the same file already uses
virt_to_head_page(xdp->data) for the same buffer.

[Severity: Medium]

The bugged_jumbo cores are snps,dwmac-3.40a, snps,dwmac-3.610 and
snps,dwmac-3.710. On these, stmmac_fix_features() disables TX checksum
insertion at jumbo MTUs:

    if (priv->plat->bugged_jumbo && (dev->mtu > ETH_DATA_LEN))
        features &= ~NETIF_F_CSUM_MASK;

stmmac_xdp_xmit_xdpf() makes its own decision:

    bool csum = !priv->plat->tx_queues_cfg[queue].coe_unsupported;

enh_desc_prepare_tx_desc() then sets TX_CIC_FULL.

Now that XDP is allowed at MTU > ETH_DATA_LEN, wouldn't XDP_TX and
redirected jumbo frames on these cores go out with full checksum
insertion enabled? That is what the bugged_jumbo workaround is meant to
avoid.

[Severity: High]

This isn't a bug introduced by this patch, but the patch makes it much
easier to hit. Further down in stmmac_xdp_set_prog(), the result of the
reopen is ignored and the function returns 0:

    if (if_running && need_update)
        stmmac_xdp_open(dev);

If stmmac_xdp_open() fails, its error paths have already freed the DMA
resources:

init_error:
    free_dma_desc_resources(priv, &priv->dma_conf);

The NAPIs that stmmac_xdp_release() disabled stay disabled, and the
device stays IFF_UP.

The next ndo_stop or XDP toggle reaches __stmmac_disable_all_queues():

    if (queue < rx_queues_cnt)
        napi_disable(&ch->rx_napi);

napi_disable() on a NAPI that is already disabled loops forever in
napi_disable_locked() while holding RTNL.

Any path that gets past that calls free_dma_desc_resources() again.
__free_dma_rx_desc_resources() doesn't clear dma_rx, buf_pool or
page_pool, so that would be a double free.

With this patch, attaching or detaching XDP at jumbo MTU makes the
reopen fill every RX ring entry with order-2/3 page_pool pages. These
are allocated with GFP_ATOMIC and can fail on a fragmented system.

Should the stmmac_xdp_open() error be propagated, with the device state
cleaned up?

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261009-stmmac-rx-mb-v5-0-c38fa4eaa138%40oss.qualcomm.com

Re: [PATCH net-next v5 0/3] 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 v5 0/3] net: stmmac: introduce XDP rx multi-buff support

[Lorenzo Bianconi]

[-- Attachment #1: Type: text/plain, Size: 1338 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 were 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.

these are theoretical issues 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.

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