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

[PATCH net 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.

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

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

[PATCH net 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 re-armed, and the first poll
resumes that stale skb with descriptors from the freshly initialized
ring. __free_dma_rx_desc_resources() releases it on teardown.

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

Fixes: ec222003bd94 ("net: stmmac: Prepare to add Split Header support")
Fixes: bba2556efad6 ("net: stmmac: Enable RX via AF_XDP zero-copy")
Signed-off-by: James Hilliard <james.hilliard1@gmail.com>
Reviewed-by: Nicolai Buchwitz <nb@tipi-net.de>
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 b20e5038eead..fba21599a5e1 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)
 			stmmac_rx_extended_status(priv, &priv->xstats,
 						  rx_q->dma_erx + entry);
@@ -5772,7 +5779,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;
@@ -5814,9 +5821,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;
@@ -5836,25 +5844,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;
@@ -5883,6 +5894,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)) {
@@ -5895,11 +5908,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. */
@@ -5973,9 +5985,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;
 				}
 			}
 		}
@@ -6032,10 +6042,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);
@@ -6060,14 +6068,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;
@@ -8412,6 +8421,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 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 1 potential
issue(s) to consider.

Critical: 0 · High: 1 · 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] The commit message says that on resume a saved partial frame
  would be resumed "with descriptors from the freshly initialized ring",…

Pre-existing issues:
- [Medium] In stmmac_rx()
  (drivers/net/ethernet/stmicro/stmmac/stmmac_main.c), the…

--

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

The saved partial-frame RX state is now taken and cleared at the start of
stmmac_rx() and stmmac_rx_zc(). It is saved again only if the frame is
still incomplete when the poll ends. The saved skb is also dropped in
stmmac_reset_rx_queue() and __free_dma_rx_desc_resources().

> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> index b20e5038eeadd..fba21599a5e15 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c

[ ... ]

> @@ -6032,10 +6042,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 can a multi-descriptor
frame reach the stack as a truncated fragment when napi_build_skb() fails
on a descriptor that is not the last one?

The allocation failure path in stmmac_rx() 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;
	}

When status & rx_not_ls is set, drain_data jumps back to read_again. At
that point skb == NULL and error == 0, and len still holds the bytes of
the dropped prefix.

With no XDP program attached, stmmac_xdp_run_prog() returns NULL. The next
descriptor of the same frame then builds a new skb from a mid-frame
buffer.

On the last descriptor, len still includes the dropped prefix. So
pskb_trim() does nothing, because len is not smaller than skb->len:

	return (len < skb->len) ? __pskb_trim(skb, len) : 0;

Wouldn't that pass a mid-payload fragment, with its FCS still attached,
to napi_gro_receive()? Its ip_summed would come from the status of the
whole frame, and rx_bytes would be counted with the full-frame len.

The same thing seems to happen across a poll boundary. In that case
in_progress saves skb == NULL, error == 0 and len.

The old status & rx_not_ls save condition behaved the same way. Later in
the series, "net: stmmac: drop multi-buffer frames in XDP mode" sets
error for rx_not_ls when XDP is enabled. The non-XDP path is unchanged.

[ ... ]

> @@ -8412,6 +8421,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]
The commit message says that without this drain, a partial frame saved
before suspend would be resumed "with descriptors from the freshly
initialized ring". Is the ring actually freshly initialized on the
stmmac_resume() path?

stmmac_resume() does:

	stmmac_reset_queues_param(priv);

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

	ret = stmmac_hw_setup(ndev);

stmmac_clear_rx_descriptors() calls stmmac_init_rx_desc() on every RX
descriptor, and that only sets ownership bits:

- dwmac4_rd_init_rx_desc() and dwxgmac2_init_rx_desc() OR OWN|BUF1V into
  des3.
- enh_desc_init_rx_desc() and ndesc_init_rx_desc() OR RDES0_OWN into
  des0.

Buffer addresses are never reprogrammed and empty slots are not refilled.
stmmac_init_dma_engine() then sets the RX tail pointer to buf_alloc_num,
which gives every descriptor back to the DMA.

Suppose page_pool_alloc_pages() failed in stmmac_rx_refill() for the
slots used by the saved partial frame. The loop then breaks before
stmmac_set_desc_addr():

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

Those descriptors keep the DMA addresses of pages that state.skb now
owns, and buf->page == NULL.

With this change, stmmac_reset_rx_queue() returns those pages to the
page_pool through dev_kfree_skb_any(). The same descriptors are then
re-armed and the DMA is restarted.

Can the device then DMA into pages that have already been recycled or
freed?

There is a separate problem on dwmac4 and xgmac. A descriptor that was
written back but not refilled before suspend still holds write-back data
in des0/des1. For example, dwmac4_wrback_get_rx_vlan_tci() reads the VLAN
tag from des0. Re-arming that descriptor would make the DMA treat the
write-back data as a buffer address.

The re-arm-without-refill behaviour predates this patch: an earlier
upstream re-init on resume was reverted. Before this patch, the stale skb
would also have been completed and freed on the first poll after resume,
while those descriptors were still armed. So this change frees the pages
earlier rather than creating the problem.

Since the resume handling depends on the ring being freshly initialized,
should the commit message be corrected? And should the resume path refill
and reprogram the RX descriptors before the DMA is restarted?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261023-stmmac-xdp-fixes-v1-0-efb37e19f5bd%40oss.qualcomm.com

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

[Lorenzo Bianconi]

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

> Thank you for your contribution! Sashiko AI review found 1 potential
> issue(s) to consider.
> 
> Critical: 0 · High: 1 · 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] The commit message says that on resume a saved partial frame
>   would be resumed "with descriptors from the freshly initialized ring",…
> 
> Pre-existing issues:
> - [Medium] In stmmac_rx()
>   (drivers/net/ethernet/stmicro/stmmac/stmmac_main.c), the…
> 
> --
> 
> net: stmmac: take ownership of saved RX state at poll entry
> 
> The saved partial-frame RX state is now taken and cleared at the start of
> stmmac_rx() and stmmac_rx_zc(). It is saved again only if the frame is
> still incomplete when the poll ends. The saved skb is also dropped in
> stmmac_reset_rx_queue() and __free_dma_rx_desc_resources().
> 
> > diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > index b20e5038eeadd..fba21599a5e15 100644
> > --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> 
> [ ... ]
> 
> > @@ -6032,10 +6042,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 can a multi-descriptor
> frame reach the stack as a truncated fragment when napi_build_skb() fails
> on a descriptor that is not the last one?

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

> 
> The allocation failure path in stmmac_rx() 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;
> 	}
> 
> When status & rx_not_ls is set, drain_data jumps back to read_again. At
> that point skb == NULL and error == 0, and len still holds the bytes of
> the dropped prefix.
> 
> With no XDP program attached, stmmac_xdp_run_prog() returns NULL. The next
> descriptor of the same frame then builds a new skb from a mid-frame
> buffer.
> 
> On the last descriptor, len still includes the dropped prefix. So
> pskb_trim() does nothing, because len is not smaller than skb->len:
> 
> 	return (len < skb->len) ? __pskb_trim(skb, len) : 0;
> 
> Wouldn't that pass a mid-payload fragment, with its FCS still attached,
> to napi_gro_receive()? Its ip_summed would come from the status of the
> whole frame, and rx_bytes would be counted with the full-frame len.
> 
> The same thing seems to happen across a poll boundary. In that case
> in_progress saves skb == NULL, error == 0 and len.
> 
> The old status & rx_not_ls save condition behaved the same way. Later in
> the series, "net: stmmac: drop multi-buffer frames in XDP mode" sets
> error for rx_not_ls when XDP is enabled. The non-XDP path is unchanged.
> 
> [ ... ]
> 
> > @@ -8412,6 +8421,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]
> The commit message says that without this drain, a partial frame saved
> before suspend would be resumed "with descriptors from the freshly
> initialized ring". Is the ring actually freshly initialized on the
> stmmac_resume() path?
> 
> stmmac_resume() does:
> 
> 	stmmac_reset_queues_param(priv);
> 
> 	stmmac_free_tx_skbufs(priv);
> 	stmmac_clear_descriptors(priv, &priv->dma_conf);
> 
> 	ret = stmmac_hw_setup(ndev);
> 
> stmmac_clear_rx_descriptors() calls stmmac_init_rx_desc() on every RX
> descriptor, and that only sets ownership bits:
> 
> - dwmac4_rd_init_rx_desc() and dwxgmac2_init_rx_desc() OR OWN|BUF1V into
>   des3.
> - enh_desc_init_rx_desc() and ndesc_init_rx_desc() OR RDES0_OWN into
>   des0.
> 
> Buffer addresses are never reprogrammed and empty slots are not refilled.
> stmmac_init_dma_engine() then sets the RX tail pointer to buf_alloc_num,
> which gives every descriptor back to the DMA.
> 
> Suppose page_pool_alloc_pages() failed in stmmac_rx_refill() for the
> slots used by the saved partial frame. The loop then breaks before
> stmmac_set_desc_addr():
> 
> 	if (!buf->page) {
> 		buf->page = page_pool_alloc_pages(rx_q->page_pool, gfp);
> 		if (!buf->page)
> 			break;
> 	}
> 
> Those descriptors keep the DMA addresses of pages that state.skb now
> owns, and buf->page == NULL.
> 
> With this change, stmmac_reset_rx_queue() returns those pages to the
> page_pool through dev_kfree_skb_any(). The same descriptors are then
> re-armed and the DMA is restarted.
> 
> Can the device then DMA into pages that have already been recycled or
> freed?
> 
> There is a separate problem on dwmac4 and xgmac. A descriptor that was
> written back but not refilled before suspend still holds write-back data
> in des0/des1. For example, dwmac4_wrback_get_rx_vlan_tci() reads the VLAN
> tag from des0. Re-arming that descriptor would make the DMA treat the
> write-back data as a buffer address.
> 
> The re-arm-without-refill behaviour predates this patch: an earlier
> upstream re-init on resume was reverted. Before this patch, the stale skb
> would also have been completed and freed on the first poll after resume,
> while those descriptors were still armed. So this change frees the pages
> earlier rather than creating the problem.
> 
> Since the resume handling depends on the ring being freshly initialized,
> should the commit message be corrected? And should the resume path refill
> and reprogram the RX descriptors before the DMA is restarted?

I will rework the commit message in v2 but the reported issue has not been
introduced by this patch and it needs to be fixed with a dedicated patch.

Regards,
Lorenzo

> 
> -- 
> Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261023-stmmac-xdp-fixes-v1-0-efb37e19f5bd%40oss.qualcomm.com

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

[PATCH net 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.

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 051253601225..297e2428a8f9 100644
--- a/drivers/net/ethernet/stmicro/stmmac/enh_desc.c
+++ b/drivers/net/ethernet/stmicro/stmmac/enh_desc.c
@@ -184,7 +184,7 @@ static int enh_desc_get_rx_status(struct stmmac_extra_stats *x,
 
 	if (unlikely(!(rdes0 & RDES0_LAST_DESCRIPTOR))) {
 		x->rx_length++;
-		return discard_frame;
+		return discard_frame | rx_not_ls;
 	}
 
 	if (unlikely(rdes0 & RDES0_ERROR_SUMMARY)) {
diff --git a/drivers/net/ethernet/stmicro/stmmac/norm_desc.c b/drivers/net/ethernet/stmicro/stmmac/norm_desc.c
index c4b613564f87..d19bf7ca4db3 100644
--- a/drivers/net/ethernet/stmicro/stmmac/norm_desc.c
+++ b/drivers/net/ethernet/stmicro/stmmac/norm_desc.c
@@ -70,7 +70,7 @@ static int ndesc_get_rx_status(struct stmmac_extra_stats *x,
 
 	if (unlikely(!(rdes0 & RDES0_LAST_DESCRIPTOR))) {
 		x->rx_length++;
-		return discard_frame;
+		return discard_frame | rx_not_ls;
 	}
 
 	if (unlikely(rdes0 & RDES0_ERROR_SUMMARY)) {
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index fba21599a5e1..3fa2c15222d8 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -5710,18 +5710,21 @@ static int stmmac_rx_zc(struct stmmac_priv *priv, int limit, u32 queue)
 		if (priv->extend_desc)
 			stmmac_rx_extended_status(priv, &priv->xstats,
 						  rx_q->dma_erx + entry);
-		if (unlikely(status == discard_frame)) {
-			xsk_buff_free(buf->xdp);
-			buf->xdp = NULL;
-			dirty++;
+
+		if (unlikely(!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;
 		}
@@ -5730,8 +5733,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;
 		}
 
@@ -5898,7 +5902,8 @@ static int stmmac_rx(struct stmmac_priv *priv, int limit, u32 queue)
 
 		if (priv->extend_desc)
 			stmmac_rx_extended_status(priv, &priv->xstats, rx_q->dma_erx + entry);
-		if (unlikely(status == discard_frame)) {
+
+		if (unlikely(status & discard_frame)) {
 			page_pool_put_page(rx_q->page_pool, buf->page, 0, true);
 			buf->page = NULL;
 			error = 1;

-- 
2.56.0

Re: [PATCH net 2/6] net: stmmac: drop split frames on 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 (2) · Medium: 1 (2) · Low: 0

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

- [Medium] The commit message does not describe one behaviour change the
  patch makes, and nothing explains the second Fixes tag.

Pre-existing issues:
- [High] On GMAC4/XGMAC, stmmac_rx() in native (non-ZC) XDP mode still
  handles split RX frames wrongly at this commit.
- [High] On AF_XDP zero-copy XDP_PASS, stmmac_dispatch_skb_zc() ignores
  the csum_none bit from the descriptor status.
- [Medium] In stmmac_rx(), if napi_build_skb() fails on the first
  descriptor of a multi-descriptor frame, the code does `rx_dropped++;…
- [Medium] priv->xstats.rx_dropped and priv->xstats.rx_errors are device-
  wide `unsigned long` counters.

--

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

When stmmac_rx_zc() drops a frame that spans more than one RX descriptor,
it now sets error, so the remaining descriptors are freed and the tail is
not handled as a new frame. The dwmac100/dwmac1000 descriptor callbacks now
return rx_not_ls together with discard_frame for non-last descriptors. The
discard_frame checks in stmmac_rx_zc() and stmmac_rx() change from == to &.

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

[Severity: High]
This is a pre-existing issue, but with this test, a dwmac4 frame that only
has csum_none set still reaches the XDP program. That covers an L4 payload
checksum error without RDES3 ERROR_SUMMARY. On XDP_PASS the frame goes to
stmmac_dispatch_skb_zc(), and that function never receives status:

stmmac_dispatch_skb_zc() {
    ...
	if (unlikely(!coe) || !stmmac_has_ip_ethertype(skb))
		skb_checksum_none_assert(skb);
	else
		skb->ip_summed = CHECKSUM_UNNECESSARY;
    ...
}

dwmac4_dma_rx_chan_op_mode() always sets MTL_OP_MODE_DIS_TCP_EF, so the
hardware does not drop these frames. dwmac4_wrback_get_rx_status() reports
them with no discard_frame:

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

Can a packet with a bad TCP/UDP checksum get marked CHECKSUM_UNNECESSARY
on the AF_XDP zero-copy XDP_PASS path and be delivered to sockets?

stmmac_rx() checks (status & csum_none) before it sets
CHECKSUM_UNNECESSARY. Should stmmac_dispatch_skb_zc() do the same check?
The gap is still there at the end of the series.

[ ... ]

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

[Severity: Medium]
This is a pre-existing issue, but the rx_dropped count added here is added
to the device-wide counter at the end of stmmac_rx_zc():

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

stmmac_rx() does the same thing, and stmmac_dispatch_skb_zc() does
priv->xstats.rx_dropped++. These are plain unsigned long read-modify-writes
with no lock, no atomic and no per-queue split.

NAPI polls for different RX queues can finish at the same time on
different CPUs. Can they lose increments here, so that stmmac_get_stats64()
under-reports rx_dropped and rx_errors?

[ ... ]

> @@ -5898,7 +5902,8 @@ static int stmmac_rx(struct stmmac_priv *priv, int limit, u32 queue)
>  
>  		if (priv->extend_desc)
>  			stmmac_rx_extended_status(priv, &priv->xstats, rx_q->dma_erx + entry);
> -		if (unlikely(status == discard_frame)) {
> +
> +		if (unlikely(status & discard_frame)) {
>  			page_pool_put_page(rx_q->page_pool, buf->page, 0, true);
>  			buf->page = NULL;
>  			error = 1;

[Severity: Medium]
The commit message does not seem to describe this change or explain the
second Fixes tag:

    Fixes: 644b8437ccef9e ("net: stmmac: dwmac4: report Rx checksum errors in status")

Is this change from == to & the actual fix for 644b8437ccef9e?
dwmac4_wrback_get_rx_status() sets ret = discard_frame on
RDES3_ERROR_SUMMARY. It then ORs 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. Frames with CRC,
overflow, watchdog or giant errors that also failed the IP checksum were
passed to the stack in stmmac_rx(), or to XDP/XSK in stmmac_rx_zc().

The fix looks correct. However, the body only covers the AF_XDP zero-copy
1:1 drop and the dwmac100/dwmac1000 descriptor change, and this hunk
changes the regular non-XDP RX path on every GMAC4/XGMAC system.

Could the commit message describe this change and the 644b8437ccef9e
regression it fixes?

[Severity: High]
This is a pre-existing issue, but does stmmac_rx() in native XDP mode
still mishandle split frames on GMAC4/XGMAC? For a non-last descriptor,
dwmac4_wrback_get_rx_status() returns rx_not_ls without discard_frame:

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

So the discard_frame drain above never starts, and the XDP program sees
the first fragment as a complete frame. On XDP_DROP:

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

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

skb is NULL and error is still 0. The tail then goes through XDP again
and can be passed to the stack as a new packet.

On XDP_TX or XDP_REDIRECT the code does goto next, which clears
in_progress. The next iteration resets len to 0, so stmmac_rx_buf1_len()
reports the full dma_buf_sz for the tail. Can that expose stale page_pool
contents that DMA did not write for this frame?

This is the same kind of bug the patch fixes for stmmac_rx_zc(), but here
stmmac_rx() is only covered for dwmac100/dwmac1000. The next patch in the
series, "net: stmmac: drop multi-buffer frames in XDP mode", appears to
fix it. That patch sets error on rx_not_ls in XDP mode, drops frames that
use buf2_len, and makes the XDP_CONSUMED path goto next.

[Severity: Medium]
This is a pre-existing issue, but stmmac_rx() has a similar problem when
napi_build_skb() fails on the first descriptor of a multi-descriptor
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 is not set, so drain_data jumps to read_again with skb == NULL and
error == 0. If allocation succeeds on the next descriptor of the same
frame, the if (!skb) branch builds a new skb from only the tail fragment.

At the last descriptor, pskb_trim(skb, len) does nothing. len still
includes the dropped first fragment, so it is larger than skb->len.

Does this pass the tail to the stack as a standalone packet with its FCS
still attached, and count the frame against the budget twice? Should this
path set error so the rest of the frame is drained?

At the end of the series this only happens in non-XDP mode, because XDP
mode now sets error on rx_not_ls.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261023-stmmac-xdp-fixes-v1-0-efb37e19f5bd%40oss.qualcomm.com

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

[Lorenzo Bianconi]

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

> Thank you for your contribution! Sashiko AI review found 1 potential
> issue(s) to consider.
> 
> Critical: 0 · High: 0 (2) · Medium: 1 (2) · Low: 0
> 
> If you believe none of the issues are legitimate, please reply to each
> issue of Medium severity or higher and explain why the AI is wrong.
> If any of them are legitimate and you plan to respin, please let patchwork
> know by including "pw-bot: cr" as a separate line at the end of your reply
> (one such reply per series is enough).
> 
> - [Medium] The commit message does not describe one behaviour change the
>   patch makes, and nothing explains the second Fixes tag.
> 
> Pre-existing issues:
> - [High] On GMAC4/XGMAC, stmmac_rx() in native (non-ZC) XDP mode still
>   handles split RX frames wrongly at this commit.
> - [High] On AF_XDP zero-copy XDP_PASS, stmmac_dispatch_skb_zc() ignores
>   the csum_none bit from the descriptor status.
> - [Medium] In stmmac_rx(), if napi_build_skb() fails on the first
>   descriptor of a multi-descriptor frame, the code does `rx_dropped++;…
> - [Medium] priv->xstats.rx_dropped and priv->xstats.rx_errors are device-
>   wide `unsigned long` counters.
> 
> --
> 
> net: stmmac: drop split frames on AF_XDP zero-copy RX
> 
> When stmmac_rx_zc() drops a frame that spans more than one RX descriptor,
> it now sets error, so the remaining descriptors are freed and the tail is
> not handled as a new frame. The dwmac100/dwmac1000 descriptor callbacks now
> return rx_not_ls together with discard_frame for non-last descriptors. The
> discard_frame checks in stmmac_rx_zc() and stmmac_rx() change from == to &.
> 
> > diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > index fba21599a5e15..3fa2c15222d88 100644
> > --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > @@ -5710,18 +5710,21 @@ static int stmmac_rx_zc(struct stmmac_priv *priv, int limit, u32 queue)
> >  		if (priv->extend_desc)
> >  			stmmac_rx_extended_status(priv, &priv->xstats,
> >  						  rx_q->dma_erx + entry);
> > -		if (unlikely(status == discard_frame)) {
> > -			xsk_buff_free(buf->xdp);
> > -			buf->xdp = NULL;
> > -			dirty++;
> > +
> > +		if (unlikely(!error && (status & discard_frame))) {
> >  			error = 1;
> >  			if (!priv->hwts_rx_en)
> >  				rx_errors++;
> >  		}
> 
> [Severity: High]
> This is a pre-existing issue, but with this test, a dwmac4 frame that only
> has csum_none set still reaches the XDP program. That covers an L4 payload
> checksum error without RDES3 ERROR_SUMMARY. On XDP_PASS the frame goes to
> stmmac_dispatch_skb_zc(), and that function never receives status:

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

> 
> stmmac_dispatch_skb_zc() {
>     ...
> 	if (unlikely(!coe) || !stmmac_has_ip_ethertype(skb))
> 		skb_checksum_none_assert(skb);
> 	else
> 		skb->ip_summed = CHECKSUM_UNNECESSARY;
>     ...
> }
> 
> dwmac4_dma_rx_chan_op_mode() always sets MTL_OP_MODE_DIS_TCP_EF, so the
> hardware does not drop these frames. dwmac4_wrback_get_rx_status() reports
> them with no discard_frame:
> 
> 	if (rdes1 & RDES1_IP_PAYLOAD_ERROR) {
> 		x->ip_payload_err++;
> 		ret |= csum_none;
> 	}
> 
> Can a packet with a bad TCP/UDP checksum get marked CHECKSUM_UNNECESSARY
> on the AF_XDP zero-copy XDP_PASS path and be delivered to sockets?
> 
> stmmac_rx() checks (status & csum_none) before it sets
> CHECKSUM_UNNECESSARY. Should stmmac_dispatch_skb_zc() do the same check?
> The gap is still there at the end of the series.
> 
> [ ... ]
> 
> > @@ -5730,8 +5733,9 @@ static int stmmac_rx_zc(struct stmmac_priv *priv, int limit, u32 queue)
> >  		if (likely(status & rx_not_ls)) {
> >  			xsk_buff_free(buf->xdp);
> >  			buf->xdp = NULL;
> > +			rx_dropped++;
> > +			error = 1;
> >  			dirty++;
> > -			count++;
> >  			goto read_again;
> >  		}
> 
> [Severity: Medium]
> This is a pre-existing issue, but the rx_dropped count added here is added
> to the device-wide counter at the end of stmmac_rx_zc():
> 
> 	priv->xstats.rx_dropped += rx_dropped;
> 	priv->xstats.rx_errors += rx_errors;
> 
> stmmac_rx() does the same thing, and stmmac_dispatch_skb_zc() does
> priv->xstats.rx_dropped++. These are plain unsigned long read-modify-writes
> with no lock, no atomic and no per-queue split.
> 
> NAPI polls for different RX queues can finish at the same time on
> different CPUs. Can they lose increments here, so that stmmac_get_stats64()
> under-reports rx_dropped and rx_errors?

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

> 
> [ ... ]
> 
> > @@ -5898,7 +5902,8 @@ static int stmmac_rx(struct stmmac_priv *priv, int limit, u32 queue)
> >  
> >  		if (priv->extend_desc)
> >  			stmmac_rx_extended_status(priv, &priv->xstats, rx_q->dma_erx + entry);
> > -		if (unlikely(status == discard_frame)) {
> > +
> > +		if (unlikely(status & discard_frame)) {
> >  			page_pool_put_page(rx_q->page_pool, buf->page, 0, true);
> >  			buf->page = NULL;
> >  			error = 1;
> 
> [Severity: Medium]
> The commit message does not seem to describe this change or explain the
> second Fixes tag:
> 
>     Fixes: 644b8437ccef9e ("net: stmmac: dwmac4: report Rx checksum errors in status")
> 
> Is this change from == to & the actual fix for 644b8437ccef9e?
> dwmac4_wrback_get_rx_status() sets ret = discard_frame on
> RDES3_ERROR_SUMMARY. It then ORs 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. Frames with CRC,
> overflow, watchdog or giant errors that also failed the IP checksum were
> passed to the stack in stmmac_rx(), or to XDP/XSK in stmmac_rx_zc().
> 
> The fix looks correct. However, the body only covers the AF_XDP zero-copy
> 1:1 drop and the dwmac100/dwmac1000 descriptor change, and this hunk
> changes the regular non-XDP RX path on every GMAC4/XGMAC system.
> 
> Could the commit message describe this change and the 644b8437ccef9e
> regression it fixes?
> 
> [Severity: High]
> This is a pre-existing issue, but does stmmac_rx() in native XDP mode
> still mishandle split frames on GMAC4/XGMAC? For a non-last descriptor,
> dwmac4_wrback_get_rx_status() returns rx_not_ls without discard_frame:
> 
> 	if (likely(!(rdes3 & RDES3_LAST_DESCRIPTOR)))
> 		return rx_not_ls;
> 
> So the discard_frame drain above never starts, and the XDP program sees
> the first fragment as a complete frame. On XDP_DROP:
> 
> 				if (xdp_res & STMMAC_XDP_CONSUMED) {
> 					...
> 					skb = NULL;
> 
> 					if (unlikely((status & rx_not_ls)))
> 						goto read_again;
> 
> skb is NULL and error is still 0. The tail then goes through XDP again
> and can be passed to the stack as a new packet.
> 
> On XDP_TX or XDP_REDIRECT the code does goto next, which clears
> in_progress. The next iteration resets len to 0, so stmmac_rx_buf1_len()
> reports the full dma_buf_sz for the tail. Can that expose stale page_pool
> contents that DMA did not write for this frame?
> 
> This is the same kind of bug the patch fixes for stmmac_rx_zc(), but here
> stmmac_rx() is only covered for dwmac100/dwmac1000. The next patch in the
> series, "net: stmmac: drop multi-buffer frames in XDP mode", appears to
> fix it. That patch sets error on rx_not_ls in XDP mode, drops frames that
> use buf2_len, and makes the XDP_CONSUMED path goto next.

I will fix the commit message in v2.

> 
> [Severity: Medium]
> This is a pre-existing issue, but stmmac_rx() has a similar problem when
> napi_build_skb() fails on the first descriptor of a multi-descriptor
> 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 is not set, so drain_data jumps to read_again with skb == NULL and
> error == 0. If allocation succeeds on the next descriptor of the same
> frame, the if (!skb) branch builds a new skb from only the tail fragment.
> 
> At the last descriptor, pskb_trim(skb, len) does nothing. len still
> includes the dropped first fragment, so it is larger than skb->len.
> 
> Does this pass the tail to the stack as a standalone packet with its FCS
> still attached, and count the frame against the budget twice? Should this
> path set error so the rest of the frame is drained?
> 
> At the end of the series this only happens in non-XDP mode, because XDP
> mode now sets error on rx_not_ls.

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

Regards,
Lorenzo

> 
> -- 
> Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261023-stmmac-xdp-fixes-v1-0-efb37e19f5bd%40oss.qualcomm.com

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

[PATCH net 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:
GMAC4/XGMAC always provide a second data buffer per descriptor, so a
frame up to 2 * dma_buf_sz can land on a single descriptor while the XDP
program still only sees the first buffer. XDP_TX then transmits a
truncated frame, and XDP_PASS appends the second buffer only after the
program has already run over the first one.

XDP does not support scatter-gather frames until the multi-buffer
rework, so drop the whole frame instead. 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")
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 3fa2c15222d8..30a2774d722b 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -5903,18 +5903,23 @@ static int stmmac_rx(struct stmmac_priv *priv, int limit, u32 queue)
 		if (priv->extend_desc)
 			stmmac_rx_extended_status(priv, &priv->xstats, rx_q->dma_erx + entry);
 
-		if (unlikely(status & discard_frame)) {
-			page_pool_put_page(rx_q->page_pool, buf->page, 0, true);
-			buf->page = NULL;
+		if (unlikely(!error && (status & discard_frame))) {
 			error = 1;
 			if (!priv->hwts_rx_en)
 				rx_errors++;
 		}
 
-		if (unlikely(error && (status & rx_not_ls)))
-			goto read_again;
+		/* We do not support SG in XDP mode */
+		if (unlikely(!error && (status & rx_not_ls) &&
+			     stmmac_xdp_is_enabled(priv))) {
+			rx_dropped++;
+			error = 1;
+		}
 
 		if (unlikely(error)) {
+			if (status & rx_not_ls)
+				goto read_again;
+
 			dev_kfree_skb(skb);
 			goto next;
 		}
@@ -5926,6 +5931,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;
@@ -5975,17 +5988,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

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

[netdev-bot+sashiko]

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

Low priority / nit picks for your consideration:

- [Low] The patch cites only `Fixes: 5fabb01207a2` ("net: stmmac: Add
  initial XDP support").

Pre-existing issues:
- [High] Without an XDP program, stmmac_rx() still re-parses the rest of
  a frame as a new frame when napi_build_skb() fails on a non-last…
- [Medium] stmmac_rx() adds its per-poll local counters to the device-
  wide `priv->xstats.rx_dropped` and `priv->xstats.rx_errors` (plain…

--

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

This patch makes stmmac_rx() drop frames that span more than one
descriptor, or that use the second buffer of a descriptor, while an XDP
program is attached. Discarded buffers are no longer returned to the page
pool, so stmmac_rx_refill() can reuse them in place. The re-parse of the
remaining descriptors after an XDP_CONSUMED verdict is also removed.

> Fixes: 5fabb01207a2 ("net: stmmac: Add initial XDP support")

[Severity: Low]
Should this also carry a Fixes tag for 88095728511e ("net: stmmac: fix rx
Scatter-Gather support")? Two of the failures described in the commit
message seem to come from that commit, not from the initial XDP support.

First, the buf1_len underflow. Before 88095728511e, stmmac_rx_buf1_len()
returned min_t(unsigned int, dma_buf_sz, plen) for the last descriptor. A
tail re-parsed after XDP_CONSUMED therefore got dma_buf_sz bytes.

88095728511e changed the return to:

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

It also moved the XDP-mode FCS strip into the if (!skb) block:

	if (stmmac_xdp_is_enabled(priv) && !buf2_len) {
		buf1_len -= ETH_FCS_LEN;

Together, these two changes let a 0-3 byte buf1_len underflow.

Second, the second-buffer case. Before 88095728511e, stmmac_rx_buf2_len()
started with:

	if (!priv->sph_active)
		return 0;

stmmac_xdp_set_prog() clears sph_active while XDP is enabled, so buf2_len
was always 0 in XDP mode. That check is now:

	if (!stmmac_rx_check_buf2_cap(priv))
		return 0;

So "GMAC4/XGMAC always provide a second data buffer per descriptor" has
only been true since 88095728511e. The new buf2_len drop fixes a condition
that commit introduced.

The last patch in this series ("net: stmmac: fix XDP RX buffer DMA sync
length") already carries Fixes: 88095728511e.

> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> index 3fa2c15222d88..30a2774d722b7 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> @@ -5903,18 +5903,23 @@ static int stmmac_rx(struct stmmac_priv *priv, int limit, u32 queue)

[ ... ]

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

[Severity: High]
This is a pre-existing issue, and this patch did not introduce it. The new
error-and-drain handling only runs when stmmac_xdp_is_enabled() is true.
Without an XDP program, can the same re-parse still happen when
napi_build_skb() fails on a non-last descriptor?

The failure branch in stmmac_rx() leaves skb == NULL and error == 0:

	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;
	}
	...
drain_data:
	if (likely(status & rx_not_ls))
		goto read_again;

The next descriptor of the same hardware frame then passes the error check
and is handled as the start of a new frame.

SPH is active by default on SPH-capable GMAC4/XGMAC when no XDP program is
attached. In that case stmmac_rx_buf1_len() returns 0 for this descriptor:

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

napi_build_skb() then builds an skb with an empty linear area, and the
payload is only in a buf2 frag. pskb_trim(skb, len) does nothing, because
the stale len is larger than skb->len.

With the default hw_vlan_en, nothing pulls the header into the linear area
before this call chain:

	eth_type_trans()
	  eth_skb_pull_mac()
	    skb_pull_inline(skb, ETH_HLEN)
	      __skb_pull()
	        BUG()

Under memory pressure, could an on-link peer panic the machine this way by
sending a frame larger than one buffer? GMAC_CONFIG_JE is set in
GMAC_CORE_INIT, so such frames are accepted even at MTU 1500.

Without SPH, the stack instead gets a frame whose Ethernet header comes
from the middle of the peer's payload.

This path is still unchanged at the end of the series. The VLAN rework in
"net: stmmac: fix VLAN tag stripping on short and non-linear RX frames"
adds pskb_may_pull() only to the software stmmac_rx_vlan() path.

Would it drain the frame correctly to set error = 1 in the
napi_build_skb() failure branch, as this patch does for XDP mode?

> @@ -5926,6 +5931,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;
> +		}

[Severity: Medium]
This isn't a bug introduced by this patch. The new drop paths add to
rx_dropped, which stmmac_rx() folds into a device-wide counter at the end:

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

These are plain unsigned long fields in struct stmmac_extra_stats. They
are updated with a non-atomic read-modify-write, and no shared lock is
held.

Each RX queue has its own NAPI instance. Can two queues polled on
different CPUs lose each other's increments here? The napi_syncp section
just before this only protects the per-queue rxq_stats.

The race goes back to 133466c3bbe171 ("net: stmmac: use per-queue 64 bit
statistics where necessary").

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261023-stmmac-xdp-fixes-v1-0-efb37e19f5bd%40oss.qualcomm.com

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

[Lorenzo Bianconi]

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

On Oct 27, netdev-bot+sashiko@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found more severe
> issue(s) in other patches of this series. This mail is only an FYI, in case
> you want to address the nit picks below when you respin.
> 
> Low priority / nit picks for your consideration:
> 
> - [Low] The patch cites only `Fixes: 5fabb01207a2` ("net: stmmac: Add
>   initial XDP support").
> 
> Pre-existing issues:
> - [High] Without an XDP program, stmmac_rx() still re-parses the rest of
>   a frame as a new frame when napi_build_skb() fails on a non-last…
> - [Medium] stmmac_rx() adds its per-poll local counters to the device-
>   wide `priv->xstats.rx_dropped` and `priv->xstats.rx_errors` (plain…
> 
> --
> 
> net: stmmac: drop multi-buffer frames in XDP mode
> 
> This patch makes stmmac_rx() drop frames that span more than one
> descriptor, or that use the second buffer of a descriptor, while an XDP
> program is attached. Discarded buffers are no longer returned to the page
> pool, so stmmac_rx_refill() can reuse them in place. The re-parse of the
> remaining descriptors after an XDP_CONSUMED verdict is also removed.
> 
> > Fixes: 5fabb01207a2 ("net: stmmac: Add initial XDP support")
> 
> [Severity: Low]
> Should this also carry a Fixes tag for 88095728511e ("net: stmmac: fix rx
> Scatter-Gather support")? Two of the failures described in the commit
> message seem to come from that commit, not from the initial XDP support.
> 
> First, the buf1_len underflow. Before 88095728511e, stmmac_rx_buf1_len()
> returned min_t(unsigned int, dma_buf_sz, plen) for the last descriptor. A
> tail re-parsed after XDP_CONSUMED therefore got dma_buf_sz bytes.
> 
> 88095728511e changed the return to:
> 
> 	return min_t(unsigned int, priv->dma_conf.dma_buf_sz, plen - len);
> 
> It also moved the XDP-mode FCS strip into the if (!skb) block:
> 
> 	if (stmmac_xdp_is_enabled(priv) && !buf2_len) {
> 		buf1_len -= ETH_FCS_LEN;
> 
> Together, these two changes let a 0-3 byte buf1_len underflow.
> 
> Second, the second-buffer case. Before 88095728511e, stmmac_rx_buf2_len()
> started with:
> 
> 	if (!priv->sph_active)
> 		return 0;
> 
> stmmac_xdp_set_prog() clears sph_active while XDP is enabled, so buf2_len
> was always 0 in XDP mode. That check is now:
> 
> 	if (!stmmac_rx_check_buf2_cap(priv))
> 		return 0;
> 
> So "GMAC4/XGMAC always provide a second data buffer per descriptor" has
> only been true since 88095728511e. The new buf2_len drop fixes a condition
> that commit introduced.
> 
> The last patch in this series ("net: stmmac: fix XDP RX buffer DMA sync
> length") already carries Fixes: 88095728511e.

ack, I will fix the commit message in v2

> 
> > diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > index 3fa2c15222d88..30a2774d722b7 100644
> > --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > @@ -5903,18 +5903,23 @@ static int stmmac_rx(struct stmmac_priv *priv, int limit, u32 queue)
> 
> [ ... ]
> 
> > -		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;
> >  		}
> 
> [Severity: High]
> This is a pre-existing issue, and this patch did not introduce it. The new
> error-and-drain handling only runs when stmmac_xdp_is_enabled() is true.
> Without an XDP program, can the same re-parse still happen when
> napi_build_skb() fails on a non-last descriptor?

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

> 
> The failure branch in stmmac_rx() leaves skb == NULL and error == 0:
> 
> 	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;
> 	}
> 	...
> drain_data:
> 	if (likely(status & rx_not_ls))
> 		goto read_again;
> 
> The next descriptor of the same hardware frame then passes the error check
> and is handled as the start of a new frame.
> 
> SPH is active by default on SPH-capable GMAC4/XGMAC when no XDP program is
> attached. In that case stmmac_rx_buf1_len() returns 0 for this descriptor:
> 
> 	if (priv->sph_active && len)
> 		return 0;
> 
> napi_build_skb() then builds an skb with an empty linear area, and the
> payload is only in a buf2 frag. pskb_trim(skb, len) does nothing, because
> the stale len is larger than skb->len.
> 
> With the default hw_vlan_en, nothing pulls the header into the linear area
> before this call chain:
> 
> 	eth_type_trans()
> 	  eth_skb_pull_mac()
> 	    skb_pull_inline(skb, ETH_HLEN)
> 	      __skb_pull()
> 	        BUG()
> 
> Under memory pressure, could an on-link peer panic the machine this way by
> sending a frame larger than one buffer? GMAC_CONFIG_JE is set in
> GMAC_CORE_INIT, so such frames are accepted even at MTU 1500.
> 
> Without SPH, the stack instead gets a frame whose Ethernet header comes
> from the middle of the peer's payload.
> 
> This path is still unchanged at the end of the series. The VLAN rework in
> "net: stmmac: fix VLAN tag stripping on short and non-linear RX frames"
> adds pskb_may_pull() only to the software stmmac_rx_vlan() path.
> 
> Would it drain the frame correctly to set error = 1 in the
> napi_build_skb() failure branch, as this patch does for XDP mode?
> 
> > @@ -5926,6 +5931,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;
> > +		}
> 
> [Severity: Medium]
> This isn't a bug introduced by this patch. The new drop paths add to
> rx_dropped, which stmmac_rx() folds into a device-wide counter at the end:
> 
> 	priv->xstats.rx_dropped += rx_dropped;
> 	priv->xstats.rx_errors += rx_errors;

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

Regards,
Lorenzo

> 
> These are plain unsigned long fields in struct stmmac_extra_stats. They
> are updated with a non-atomic read-modify-write, and no shared lock is
> held.
> 
> Each RX queue has its own NAPI instance. Can two queues polled on
> different CPUs lose each other's increments here? The napi_syncp section
> just before this only protects the per-queue rxq_stats.
> 
> The race goes back to 133466c3bbe171 ("net: stmmac: use per-queue 64 bit
> statistics where necessary").
> 
> [ ... ]
> 
> -- 
> Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261023-stmmac-xdp-fixes-v1-0-efb37e19f5bd%40oss.qualcomm.com

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

[PATCH net 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, and keeps the metadata aligned
when stmmac_rx_vlan() relocates it with the MAC header while stripping a
tag.

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 30a2774d722b..8a2faf50d695 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

Re: [PATCH net 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] At this commit, the commit message says
  stmmac_construct_skb_zc() 'keeps the metadata aligned when…

--

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

This commit changes stmmac_construct_skb_zc() so that it copies the XDP
metadata in [xdp->data_meta, xdp->data) into the new skb together with
the packet data. The metadata ends up right before skb->data, and
meta_len 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 30a2774d722b7..8a2faf50d6957 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]
The commit message says this change:

  "keeps the metadata aligned when stmmac_rx_vlan() relocates it with the
  MAC header while stripping a tag."

Is that true at this commit? Here, stmmac_rx_vlan() still moves only the
two MAC addresses:

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

The software pop path runs when hw_vlan_en is false, CTAG or STAG RX
offload is enabled, and a tagged frame arrives. In that case,
stmmac_dispatch_skb_zc() calls stmmac_rx_vlan() before eth_type_trans():

    stmmac_rx_vlan(priv->dev, skb);
    skb->protocol = eth_type_trans(skb, priv->dev);

The metadata stays at [old_data - metasize, old_data), but mac_header is
set to old_data + VLAN_HLEN. skb_metadata_end() returns skb_mac_header(),
so the metadata window seen by TC BPF and the stack is shifted by 4
bytes.

Wouldn't that drop the first 4 bytes the XDP program wrote, and return
the first 4 bytes of the destination MAC as the last 4 bytes of
metadata?

The next patch in the 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. That
fixes this by the end of the series.

Until then, this commit is a bisection window. A stable backport that
takes this patch without the next one would also have the problem.
Would it make sense to reorder the two patches? Another option is to
reword the commit message so it doesn't depend on a change that hasn't
landed yet.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261023-stmmac-xdp-fixes-v1-0-efb37e19f5bd%40oss.qualcomm.com

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

[Lorenzo Bianconi]

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

On Oct 27, 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] At this commit, the commit message says
>   stmmac_construct_skb_zc() 'keeps the metadata aligned when…
> 
> --
> 
> net: stmmac: copy XDP metadata in AF_XDP zero-copy RX
> 
> This commit changes stmmac_construct_skb_zc() so that it copies the XDP
> metadata in [xdp->data_meta, xdp->data) into the new skb together with
> the packet data. The metadata ends up right before skb->data, and
> meta_len 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 30a2774d722b7..8a2faf50d6957 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]
> The commit message says this change:
> 
>   "keeps the metadata aligned when stmmac_rx_vlan() relocates it with the
>   MAC header while stripping a tag."
> 
> Is that true at this commit? Here, stmmac_rx_vlan() still moves only the
> two MAC addresses:
> 
> drivers/net/ethernet/stmicro/stmmac/stmmac_main.c:stmmac_rx_vlan() {
>     ...
> 		vlanid = ntohs(veth->h_vlan_TCI);
> 		memmove(skb->data + VLAN_HLEN, veth, ETH_ALEN * 2);
> 		skb_pull(skb, VLAN_HLEN);
>     ...
> }
> 
> The software pop path runs when hw_vlan_en is false, CTAG or STAG RX
> offload is enabled, and a tagged frame arrives. In that case,
> stmmac_dispatch_skb_zc() calls stmmac_rx_vlan() before eth_type_trans():
> 
>     stmmac_rx_vlan(priv->dev, skb);
>     skb->protocol = eth_type_trans(skb, priv->dev);
> 
> The metadata stays at [old_data - metasize, old_data), but mac_header is
> set to old_data + VLAN_HLEN. skb_metadata_end() returns skb_mac_header(),
> so the metadata window seen by TC BPF and the stack is shifted by 4
> bytes.
> 
> Wouldn't that drop the first 4 bytes the XDP program wrote, and return
> the first 4 bytes of the destination MAC as the last 4 bytes of
> metadata?
> 
> The next patch in the 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. That
> fixes this by the end of the series.
> 
> Until then, this commit is a bisection window. A stable backport that
> takes this patch without the next one would also have the problem.
> Would it make sense to reorder the two patches? Another option is to
> reword the commit message so it doesn't depend on a change that hasn't
> landed yet.

I agree, I will reword the commit message in v2.

Regards,
Lorenzo

> 
> -- 
> Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261023-stmmac-xdp-fixes-v1-0-efb37e19f5bd%40oss.qualcomm.com

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

[PATCH net 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; untagged frames, including short ones, are left
untouched.

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 8a2faf50d695..32b2c5834c34 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))
@@ -6057,12 +6073,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

Re: [PATCH net 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 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] In stmmac_rx_vlan()
  (drivers/net/ethernet/stmicro/stmmac/stmmac_main.c), the patch adds an…

Pre-existing issues:
- [Medium] Only one of the two stmmac RX paths that call stmmac_rx_vlan()
  records XDP metadata in the skb.
- [Medium] The new VLAN-error drop branch in stmmac_dispatch_skb_zc()
  does a plain `priv->xstats.rx_dropped++` (line 5567).

--

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

This patch changes stmmac_rx_vlan() so that it pulls ETH_HLEN before
reading h_vlan_proto, and pulls VLAN_ETH_HLEN before reading the TCI. It
returns an error so callers can drop frames that claim a VLAN tag but lack
the full header. It also moves XDP metadata together with the MAC
addresses when the tag is popped.

> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> index 8a2faf50d6957..32b2c5834c340 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;

[Severity: Low]
This pull happens before the h_vlan_proto and dev->features checks. A frame
shorter than ETH_HLEN is now freed and counted in rx_dropped by both
stmmac_rx() and stmmac_dispatch_skb_zc(), even with CTAG/STAG RX stripping
disabled. The hw_vlan_en branch in those callers has no matching check.

Is that intended? The commit message says:

    untagged frames, including short ones, are left untouched.

That holds only for frames between ETH_HLEN and VLAN_ETH_HLEN in length.

XDP doesn't seem able to reach this case, because bpf_xdp_adjust_head()
and bpf_xdp_adjust_tail() both refuse to go below ETH_HLEN. Dropping such
a runt is also arguably safer than handing it to eth_type_trans().

Could the commit message mention that frames shorter than ETH_HLEN are now
dropped?

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

[Severity: Medium]
This isn't a bug introduced by this patch, but skb_metadata_len() can only
be non-zero for skbs built by stmmac_construct_skb_zc() on the AF_XDP
zero-copy path:

stmmac_construct_skb_zc() {
    ...
	memcpy(skb->data - metasize, xdp->data_meta, datasize + metasize);
	if (metasize)
		skb_metadata_set(skb, metasize);
    ...
}

On the page_pool path, stmmac_rx() calls xdp_prepare_buff() with
meta_valid set to true, so bpf_xdp_adjust_meta() succeeds there. The
XDP_PASS skb is then built without recording the metadata length:

stmmac_rx() {
    ...
			/* 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);
    ...
}

So skb_shinfo(skb)->meta_len stays 0 for these skbs. Doesn't that mean
metadata written by a non-ZC XDP program is lost before tc BPF can see it?
It would also mean this memmove only moves metadata for zero-copy skbs.

Should stmmac_rx() also call skb_metadata_set() with
ctx.xdp.data - ctx.xdp.data_meta?

This patch's own claim still holds on that path, since meta_len is 0
there and data_meta cannot end up pointing at the wrong bytes.

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

[ ... ]

> @@ -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: Medium]
This is a pre-existing issue, but priv->xstats.rx_dropped is one
device-wide unsigned long in struct stmmac_extra_stats. Every channel's
NAPI poll updates it with a plain read-modify-write:

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

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

stmmac_napi_add() registers a separate rx_napi/rxtx_napi for each channel,
so these polls can run at the same time on different CPUs. No lock, atomic
or u64_stats_sync protects the counter.

Can drop increments be lost this way? stmmac_get_stats64() also reads the
field without READ_ONCE().

This patch only adds one more site that follows the existing pattern.

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

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261023-stmmac-xdp-fixes-v1-0-efb37e19f5bd%40oss.qualcomm.com

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

[Lorenzo Bianconi]

[-- Attachment #1: Type: text/plain, Size: 6923 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] In stmmac_rx_vlan()
>   (drivers/net/ethernet/stmicro/stmmac/stmmac_main.c), the patch adds an…
> 
> Pre-existing issues:
> - [Medium] Only one of the two stmmac RX paths that call stmmac_rx_vlan()
>   records XDP metadata in the skb.
> - [Medium] The new VLAN-error drop branch in stmmac_dispatch_skb_zc()
>   does a plain `priv->xstats.rx_dropped++` (line 5567).
> 
> --
> 
> net: stmmac: fix VLAN tag stripping on short and non-linear RX frames
> 
> This patch changes stmmac_rx_vlan() so that it pulls ETH_HLEN before
> reading h_vlan_proto, and pulls VLAN_ETH_HLEN before reading the TCI. It
> returns an error so callers can drop frames that claim a VLAN tag but lack
> the full header. It also moves XDP metadata together with the MAC
> addresses when the tag is popped.
> 
> > diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > index 8a2faf50d6957..32b2c5834c340 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;
> 
> [Severity: Low]
> This pull happens before the h_vlan_proto and dev->features checks. A frame
> shorter than ETH_HLEN is now freed and counted in rx_dropped by both
> stmmac_rx() and stmmac_dispatch_skb_zc(), even with CTAG/STAG RX stripping
> disabled. The hw_vlan_en branch in those callers has no matching check.
> 
> Is that intended? The commit message says:
> 
>     untagged frames, including short ones, are left untouched.
> 
> That holds only for frames between ETH_HLEN and VLAN_ETH_HLEN in length.
> 
> XDP doesn't seem able to reach this case, because bpf_xdp_adjust_head()
> and bpf_xdp_adjust_tail() both refuse to go below ETH_HLEN. Dropping such
> a runt is also arguably safer than handing it to eth_type_trans().
> 
> Could the commit message mention that frames shorter than ETH_HLEN are now
> dropped?

ack, I will fix the commit message in v2.

> 
> > +
> > +	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);
> 
> [Severity: Medium]
> This isn't a bug introduced by this patch, but skb_metadata_len() can only
> be non-zero for skbs built by stmmac_construct_skb_zc() on the AF_XDP
> zero-copy path:

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

> 
> stmmac_construct_skb_zc() {
>     ...
> 	memcpy(skb->data - metasize, xdp->data_meta, datasize + metasize);
> 	if (metasize)
> 		skb_metadata_set(skb, metasize);
>     ...
> }
> 
> On the page_pool path, stmmac_rx() calls xdp_prepare_buff() with
> meta_valid set to true, so bpf_xdp_adjust_meta() succeeds there. The
> XDP_PASS skb is then built without recording the metadata length:
> 
> stmmac_rx() {
>     ...
> 			/* 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);
>     ...
> }
> 
> So skb_shinfo(skb)->meta_len stays 0 for these skbs. Doesn't that mean
> metadata written by a non-ZC XDP program is lost before tc BPF can see it?
> It would also mean this memmove only moves metadata for zero-copy skbs.
> 
> Should stmmac_rx() also call skb_metadata_set() with
> ctx.xdp.data - ctx.xdp.data_meta?
> 
> This patch's own claim still holds on that path, since meta_len is 0
> there and data_meta cannot end up pointing at the wrong bytes.
> 
> >  		skb_pull(skb, VLAN_HLEN);
> >  		__vlan_hwaccel_put_tag(skb, vlan_proto, vlanid);
> >  	}
> > +
> > +	return 0;
> >  }
> 
> [ ... ]
> 
> > @@ -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: Medium]
> This is a pre-existing issue, but priv->xstats.rx_dropped is one
> device-wide unsigned long in struct stmmac_extra_stats. Every channel's
> NAPI poll updates it with a plain read-modify-write:

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

Regards,
Lorenzo

> 
> stmmac_dispatch_skb_zc()
>     priv->xstats.rx_dropped++;
> 
> stmmac_rx_zc() and stmmac_rx()
>     priv->xstats.rx_dropped += rx_dropped;
> 
> stmmac_napi_add() registers a separate rx_napi/rxtx_napi for each channel,
> so these polls can run at the same time on different CPUs. No lock, atomic
> or u64_stats_sync protects the counter.
> 
> Can drop increments be lost this way? stmmac_get_stats64() also reads the
> field without READ_ONCE().
> 
> This patch only adds one more site that follows the existing pattern.
> 
> > +		return;
> > +	}
> > +
> >  	skb->protocol = eth_type_trans(skb, priv->dev);
> 
> [ ... ]
> 
> -- 
> Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261023-stmmac-xdp-fixes-v1-0-efb37e19f5bd%40oss.qualcomm.com

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

[PATCH net 6/6] net: stmmac: fix XDP RX buffer DMA sync length

[Lorenzo Bianconi]

Since commit 88095728511e ("net: stmmac: fix rx Scatter-Gather support")
moved the page_pool DMA window to offset 0, the sync range no longer
starts at the XDP headroom. The XDP path still computed sync_len as

        data_end - data_hard_start - buf->page_offset

so page_pool_dma_sync_for_device() synced [base, base + len) instead of
[base + page_offset, base + page_offset + len).

On a non-coherent platform an XDP program can write packet data, write
metadata or grow the tail. If the program then returns XDP_DROP/ABORTED,
or a redirect fails, the driver recycles the head page with an incomplete
sync. Dirty cache lines in the tail of the frame can be written back
after the device has DMA'd a new frame into that page.

Include buf->page_offset in the sync length so the whole CPU-touched
window is synced before the page is reused.

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 | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index 32b2c5834c34..c51358c51fae 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -5980,8 +5980,7 @@ 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;
+			pre_len = ctx.xdp.data_end - ctx.xdp.data_hard_start;
 
 			ctx.priv = priv;
 			ctx.desc = p;
@@ -5991,8 +5990,7 @@ static int stmmac_rx(struct stmmac_priv *priv, int limit, u32 queue)
 			/* 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 = ctx.xdp.data_end - ctx.xdp.data_hard_start;
 			sync_len = max(sync_len, pre_len);
 
 			/* For Not XDP_PASS verdict */

-- 
2.56.0

Re: [PATCH net 6/6] net: stmmac: fix XDP RX buffer DMA sync length

[netdev-bot+sashiko]

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

Critical: 0 · High: 1 · Medium: 0 · 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] In stmmac_rx()
  (drivers/net/ethernet/stmicro/stmmac/stmmac_main.c), the length passed…

--

net: stmmac: fix XDP RX buffer DMA sync length

This commit changes the XDP path in stmmac_rx() so that pre_len and
sync_len are measured from data_hard_start, and buf->page_offset is no
longer subtracted. That matches the page_pool DMA window, which now
starts at offset 0. The goal is for page_pool_put_page() on the
consumed XDP path to sync the whole region the CPU touched.

> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> index 32b2c5834c340..c51358c51fae7 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> @@ -5980,8 +5980,7 @@ 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;
> +			pre_len = ctx.xdp.data_end - ctx.xdp.data_hard_start;
>  
>  			ctx.priv = priv;
>  			ctx.desc = p;
> @@ -5991,8 +5990,7 @@ static int stmmac_rx(struct stmmac_priv *priv, int limit, u32 queue)
>  			/* 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 = ctx.xdp.data_end - ctx.xdp.data_hard_start;
>  			sync_len = max(sync_len, pre_len);

[Severity: High]

Does this cover the furthest point the CPU touched if the XDP program
grows the tail and then shrinks it again before it returns?

The comment says the sync covers the "max len CPU touch". However,
max(sync_len, pre_len) only looks at data_end before the program runs
and after it returns. bpf_xdp_adjust_tail() writes to the grown area
when the tail grows:

net/core/filter.c:bpf_xdp_adjust_tail() {
    ...
	/* Clear memory area on grow, can contain uninit kernel memory */
	if (offset > 0)
		memset(xdp->data_end, 0, offset);

	xdp->data_end = data_end;
    ...
}

A later negative adjust only moves data_end back.

Take a 64 byte frame at page_offset 256 and a program that does
adjust_tail(+512), then adjust_tail(-512), then returns XDP_DROP. Both
pre_len and sync_len end up at the original end, around 316. The memset
dirtied the cache lines in [316, 828), but this call does not clean
them:

    page_pool_put_page(rx_q->page_pool,
                       virt_to_head_page(ctx.xdp.data),
                       sync_len, true);

__page_pool_dma_sync_for_device() caps the length at p.max_len but
never extends it. The direct recycle cache does no further sync.
stmmac_rx_refill() gives the buffer back to the device after dma_wmb(),
with no more cache maintenance.

max_len is dma_buf_sz + stmmac_rx_offset(priv), which is 1792 with
dma_buf_sz = 1536. So that range lies inside the window the device DMAs
into.

On a non-coherent platform, could one of those dirty lines be evicted
after the device has written the next frame into the recycled page, and
corrupt that frame? It looks like the same failure the commit message
describes:

    Dirty cache lines in the tail of the frame can be written back
    after the device has DMA'd a new frame into that page.

The commit message names "grow the tail" as a trigger and says "the
whole CPU-touched window is synced". The max(pre, post) approach was
already there before this patch.

Would it be better for the STMMAC_XDP_CONSUMED path to sync the full
pool window instead of checking only the two endpoints? Passing
p.max_len, or using page_pool_recycle_direct(), would do that.

>  
>  			/* For Not XDP_PASS verdict */

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261023-stmmac-xdp-fixes-v1-0-efb37e19f5bd%40oss.qualcomm.com

Re: [PATCH net 6/6] net: stmmac: fix XDP RX buffer DMA sync length

[Lorenzo Bianconi]

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

On Oct 27, netdev-bot+sashiko@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential
> issue(s) to consider.
> 
> Critical: 0 · High: 1 · Medium: 0 · 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] In stmmac_rx()
>   (drivers/net/ethernet/stmicro/stmmac/stmmac_main.c), the length passed…
> 
> --
> 
> net: stmmac: fix XDP RX buffer DMA sync length
> 
> This commit changes the XDP path in stmmac_rx() so that pre_len and
> sync_len are measured from data_hard_start, and buf->page_offset is no
> longer subtracted. That matches the page_pool DMA window, which now
> starts at offset 0. The goal is for page_pool_put_page() on the
> consumed XDP path to sync the whole region the CPU touched.
> 
> > diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > index 32b2c5834c340..c51358c51fae7 100644
> > --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > @@ -5980,8 +5980,7 @@ 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;
> > +			pre_len = ctx.xdp.data_end - ctx.xdp.data_hard_start;
> >  
> >  			ctx.priv = priv;
> >  			ctx.desc = p;
> > @@ -5991,8 +5990,7 @@ static int stmmac_rx(struct stmmac_priv *priv, int limit, u32 queue)
> >  			/* 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 = ctx.xdp.data_end - ctx.xdp.data_hard_start;
> >  			sync_len = max(sync_len, pre_len);
> 
> [Severity: High]
> 
> Does this cover the furthest point the CPU touched if the XDP program
> grows the tail and then shrinks it again before it returns?

ack, I agree this is a real problem. I will fix it in v2.

Regards,
Lorenzo

> 
> The comment says the sync covers the "max len CPU touch". However,
> max(sync_len, pre_len) only looks at data_end before the program runs
> and after it returns. bpf_xdp_adjust_tail() writes to the grown area
> when the tail grows:
> 
> net/core/filter.c:bpf_xdp_adjust_tail() {
>     ...
> 	/* Clear memory area on grow, can contain uninit kernel memory */
> 	if (offset > 0)
> 		memset(xdp->data_end, 0, offset);
> 
> 	xdp->data_end = data_end;
>     ...
> }
> 
> A later negative adjust only moves data_end back.
> 
> Take a 64 byte frame at page_offset 256 and a program that does
> adjust_tail(+512), then adjust_tail(-512), then returns XDP_DROP. Both
> pre_len and sync_len end up at the original end, around 316. The memset
> dirtied the cache lines in [316, 828), but this call does not clean
> them:
> 
>     page_pool_put_page(rx_q->page_pool,
>                        virt_to_head_page(ctx.xdp.data),
>                        sync_len, true);
> 
> __page_pool_dma_sync_for_device() caps the length at p.max_len but
> never extends it. The direct recycle cache does no further sync.
> stmmac_rx_refill() gives the buffer back to the device after dma_wmb(),
> with no more cache maintenance.
> 
> max_len is dma_buf_sz + stmmac_rx_offset(priv), which is 1792 with
> dma_buf_sz = 1536. So that range lies inside the window the device DMAs
> into.
> 
> On a non-coherent platform, could one of those dirty lines be evicted
> after the device has written the next frame into the recycled page, and
> corrupt that frame? It looks like the same failure the commit message
> describes:
> 
>     Dirty cache lines in the tail of the frame can be written back
>     after the device has DMA'd a new frame into that page.
> 
> The commit message names "grow the tail" as a trigger and says "the
> whole CPU-touched window is synced". The max(pre, post) approach was
> already there before this patch.
> 
> Would it be better for the STMMAC_XDP_CONSUMED path to sync the full
> pool window instead of checking only the two endpoints? Passing
> p.max_len, or using page_pool_recycle_direct(), would do that.
> 
> >  
> >  			/* For Not XDP_PASS verdict */
> 
> -- 
> Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261023-stmmac-xdp-fixes-v1-0-efb37e19f5bd%40oss.qualcomm.com

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

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

[Lorenzo Bianconi]

[-- Attachment #1: Type: text/plain, Size: 1380 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 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 by 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 --]