BPF List
 help / color / mirror / Atom feed
* [PATCH net v2] net: stmmac: fix rx Scatter-Gather support
@ 2026-09-21 14:47 Lorenzo Bianconi
  2026-09-22 14:48 ` sashiko-bot
                   ` (2 more replies)
  0 siblings, 3 replies; 5+ messages in thread
From: Lorenzo Bianconi @ 2026-09-21 14:47 UTC (permalink / raw)
  To: Maxime Chevallier, Andrew Lunn, David S. Miller, Eric Dumazet,
	Jakub Kicinski, Paolo Abeni, Maxime Coquelin, Alexandre Torgue,
	Jose Abreu, Alexei Starovoitov, Daniel Borkmann,
	Jesper Dangaard Brouer, John Fastabend, Stanislav Fomichev
  Cc: netdev, linux-stm32, linux-arm-kernel, bpf, Lorenzo Bianconi

When a received frame is larger than dma_buf_sz, the DMA scatters it
across multiple RX descriptors (rx Scatter-Gather). The secondary RX
buffer (sec_page) was only allocated and programmed when split-header
(SPH) was active, so for regular frames buffer2 was neither allocated
nor backed by a valid mapping. As soon as an incoming frame overflowed
buffer1, the DMA wrote the overflow into the unmapped secondary-buffer
address, triggering an SMMU translation fault on IOMMU-based platforms:

arm-smmu 15000000.iommu: Unhandled context fault: fsr=0x402, iova=0x00000000, fsynr=0x7f0011, cbfrsynra=0x1c90, cb=11
arm-smmu 15000000.iommu: FSR    = 00000402 [Format=2 TF], SID=0x1c90
arm-smmu 15000000.iommu: FSYNR0 = 007f0011 [S1CBNDX=127 WNR PLVL=1]

Enable scatter-gather for non-SPH frames on cores that can program an
independent secondary RX buffer (GMAC4/XGMAC): allocate and mark buffer2
as valid in stmmac_init_rx_buffers() and stmmac_rx_refill(), and account
for it in the buffer length computation. Legacy cores have no set_sec_addr
op, so they keep buffer2 disabled.

Since buffer2 is handed to the DMA at page offset 0, the page pool sync
window is widened to cover both buffers in every mode: offset is set to 0
and max_len to dma_buf_sz + stmmac_rx_offset().

The FCS can straddle the buffer1/buffer2 boundary and a descriptor
boundary, so it is now stripped from the tail of the assembled frame with
skb_trim() instead of from a single buffer, avoiding an unsigned underflow
for frames that overflow a buffer by 1..3 bytes. For single-buffer frames
the XDP program must not see the FCS, so it is removed from the XDP buffer
before the program runs.

Native XDP currently only supports single-buffer (linear) frames. An
oversized frame accepted by the MAC (jumbo enabled) is received via
buffer2; the XDP program only sees buffer1, so on a TX/REDIRECT verdict
the frame is forwarded truncated. XDP multi-buffer support to handle
this case is planned as a follow-up.

AF_XDP zero-copy RX is not covered by this change: a ZC queue still
programs buffer2 at DMA address 0 (and XGMAC has no buffer2-valid bit),
so an oversized frame overflowing buffer1 can still trigger the same
SMMU translation fault. Handling is planned as a follow-up.

Fixes: 88ebe2cf7f3f ("net: stmmac: Rework stmmac_rx()")
Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@oss.qualcomm.com>
---
Changes in v2:
- Gate buf2 allocation by stmmac_rx_check_buf2_cap() routine.
- Always set pp_params.offset to 0 and pp_params.max_len to dma_buf_sz +
  rx_offset.
- Do not remove fcs_len from buf1_len or buf2_len, just from len
  counter.
- Link to v1: https://lore.kernel.org/r/20260916-stmmac-rx-sg-fix-v1-1-b49b7b8f725f@oss.qualcomm.com
---
 drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 84 ++++++++++++++---------
 1 file changed, 50 insertions(+), 34 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index 11035b7d944e..d41892b56c5a 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -1629,6 +1629,16 @@ static void stmmac_clear_descriptors(struct stmmac_priv *priv,
 		stmmac_clear_tx_descriptors(priv, dma_conf, queue);
 }
 
+static bool stmmac_rx_check_buf2_cap(struct stmmac_priv *priv)
+{
+	/* Only cores that can program an independent secondary RX buffer
+	 * (used for scatter-gather overflow or split-header payload) back
+	 * buffer2. Legacy cores have no set_sec_addr op, so buffer2 is
+	 * never handed to the hardware there.
+	 */
+	return priv->hw->desc && priv->hw->desc->set_sec_addr;
+}
+
 /**
  * stmmac_init_rx_buffers - init the RX descriptor buffer.
  * @priv: driver private structure
@@ -1659,16 +1669,13 @@ static int stmmac_init_rx_buffers(struct stmmac_priv *priv,
 		buf->page_offset = stmmac_rx_offset(priv);
 	}
 
-	if (priv->sph_active && !buf->sec_page) {
+	if (stmmac_rx_check_buf2_cap(priv) && !buf->sec_page) {
 		buf->sec_page = page_pool_alloc_pages(rx_q->page_pool, gfp);
 		if (!buf->sec_page)
 			return -ENOMEM;
 
 		buf->sec_addr = page_pool_get_dma_addr(buf->sec_page);
 		stmmac_set_desc_sec_addr(priv, p, buf->sec_addr, true);
-	} else {
-		buf->sec_page = NULL;
-		stmmac_set_desc_sec_addr(priv, p, buf->sec_addr, false);
 	}
 
 	buf->addr = page_pool_get_dma_addr(buf->page) + buf->page_offset;
@@ -2256,13 +2263,8 @@ static int __alloc_dma_rx_desc_resources(struct stmmac_priv *priv,
 	pp_params.nid = dev_to_node(priv->device);
 	pp_params.dev = priv->device;
 	pp_params.dma_dir = xdp_prog ? DMA_BIDIRECTIONAL : DMA_FROM_DEVICE;
-	pp_params.offset = stmmac_rx_offset(priv);
-	pp_params.max_len = dma_conf->dma_buf_sz;
-
-	if (priv->sph_active) {
-		pp_params.offset = 0;
-		pp_params.max_len += stmmac_rx_offset(priv);
-	}
+	pp_params.offset = 0;
+	pp_params.max_len = dma_conf->dma_buf_sz + stmmac_rx_offset(priv);
 
 	rx_q->page_pool = page_pool_create(&pp_params);
 	if (IS_ERR(rx_q->page_pool)) {
@@ -5097,7 +5099,7 @@ static inline void stmmac_rx_refill(struct stmmac_priv *priv, u32 queue)
 				break;
 		}
 
-		if (priv->sph_active && !buf->sec_page) {
+		if (stmmac_rx_check_buf2_cap(priv) && !buf->sec_page) {
 			buf->sec_page = page_pool_alloc_pages(rx_q->page_pool, gfp);
 			if (!buf->sec_page)
 				break;
@@ -5108,10 +5110,8 @@ static inline void stmmac_rx_refill(struct stmmac_priv *priv, u32 queue)
 		buf->addr = page_pool_get_dma_addr(buf->page) + buf->page_offset;
 
 		stmmac_set_desc_addr(priv, p, buf->addr);
-		if (priv->sph_active)
-			stmmac_set_desc_sec_addr(priv, p, buf->sec_addr, true);
-		else
-			stmmac_set_desc_sec_addr(priv, p, buf->sec_addr, false);
+		stmmac_set_desc_sec_addr(priv, p, buf->sec_addr,
+					 stmmac_rx_check_buf2_cap(priv));
 		stmmac_refill_desc3(priv, rx_q, p);
 
 		rx_q->rx_count_frames++;
@@ -5142,7 +5142,9 @@ static unsigned int stmmac_rx_buf1_len(struct stmmac_priv *priv,
 	unsigned int plen = 0, hlen = 0;
 	int coe = priv->hw->rx_csum;
 
-	/* Not first descriptor, buffer is always zero */
+	/* Not first descriptor, SPH enabled: buffer1 only carries the
+	 * split header of the first descriptor, so it is zero here.
+	 */
 	if (priv->sph_active && len)
 		return 0;
 
@@ -5153,14 +5155,16 @@ static unsigned int stmmac_rx_buf1_len(struct stmmac_priv *priv,
 		return hlen;
 	}
 
-	/* First descriptor, not last descriptor and not split header */
+	/* Not last descriptor and not split header: buffer1 is fully filled */
 	if (status & rx_not_ls)
 		return priv->dma_conf.dma_buf_sz;
 
 	plen = stmmac_get_rx_frame_len(priv, p, coe);
 
-	/* First descriptor and last descriptor and not split header */
-	return min_t(unsigned int, priv->dma_conf.dma_buf_sz, plen);
+	/* Last descriptor and not split header: buffer1 holds the remaining
+	 * bytes of the frame, up to dma_buf_sz
+	 */
+	return min_t(unsigned int, priv->dma_conf.dma_buf_sz, plen - len);
 }
 
 static unsigned int stmmac_rx_buf2_len(struct stmmac_priv *priv,
@@ -5170,8 +5174,7 @@ static unsigned int stmmac_rx_buf2_len(struct stmmac_priv *priv,
 	int coe = priv->hw->rx_csum;
 	unsigned int plen = 0;
 
-	/* Not split header, buffer is not available */
-	if (!priv->sph_active)
+	if (!stmmac_rx_check_buf2_cap(priv))
 		return 0;
 
 	/* For GMAC4, when split header is enabled, in some rare cases, the
@@ -5188,14 +5191,15 @@ static unsigned int stmmac_rx_buf2_len(struct stmmac_priv *priv,
 	 * Thus 'plen - len' always gives the correct length of buf2.
 	 */
 
-	/* Not GMAC4 and not last descriptor */
-	if (priv->plat->core_type != DWMAC_CORE_GMAC4 && (status & rx_not_ls))
+	/* Not GMAC4, or non-SPH and not last descriptor */
+	if ((priv->plat->core_type != DWMAC_CORE_GMAC4 || !priv->sph_active) &&
+	    (status & rx_not_ls))
 		return priv->dma_conf.dma_buf_sz;
 
 	/* GMAC4 or last descriptor */
 	plen = stmmac_get_rx_frame_len(priv, p, coe);
 
-	return plen - len;
+	return plen > len ? plen - len : 0;
 }
 
 static int stmmac_xdp_xmit_xdpf(struct stmmac_priv *priv, int queue,
@@ -5718,6 +5722,7 @@ static int stmmac_rx(struct stmmac_priv *priv, int limit, u32 queue)
 	unsigned int desc_size;
 	struct sk_buff *skb = NULL;
 	struct stmmac_xdp_buff ctx;
+	bool fcs_stripped = false;
 	int xdp_status = 0;
 	int bufsz;
 
@@ -5807,24 +5812,31 @@ static int stmmac_rx(struct stmmac_priv *priv, int limit, u32 queue)
 		len += buf2_len;
 
 		/* ACS is disabled; strip manually. */
-		if (likely(!(status & rx_not_ls))) {
-			if (buf2_len) {
-				buf2_len -= ETH_FCS_LEN;
-				len -= ETH_FCS_LEN;
-			} else if (buf1_len) {
-				buf1_len -= ETH_FCS_LEN;
-				len -= ETH_FCS_LEN;
-			}
-		}
+		if (likely(!(status & rx_not_ls)))
+			len -= ETH_FCS_LEN;
 
 		if (!skb) {
 			unsigned int pre_len, sync_len;
 
+			/* Each frame starts here: reset the FCS handling */
+			fcs_stripped = false;
+
 			dma_sync_single_for_cpu(priv->device, buf->addr,
 						buf1_len, dma_dir);
 			net_prefetch(page_address(buf->page) +
 				     buf->page_offset);
 
+			/* The XDP program must not see the FCS. This only
+			 * applies to a single-buffer frame (buf2_len == 0),
+			 * where the whole frame and its FCS sit in buffer1;
+			 * otherwise the FCS is stripped from the assembled
+			 * frame with skb_trim().
+			 */
+			if (stmmac_xdp_is_enabled(priv) && !buf2_len) {
+				buf1_len -= ETH_FCS_LEN;
+				fcs_stripped = true;
+			}
+
 			xdp_init_buff(&ctx.xdp, bufsz, &rx_q->xdp_rxq);
 			xdp_prepare_buff(&ctx.xdp, page_address(buf->page),
 					 buf->page_offset, buf1_len, true);
@@ -5925,6 +5937,10 @@ static int stmmac_rx(struct stmmac_priv *priv, int limit, u32 queue)
 
 		/* Got entire packet into SKB. Finish it. */
 
+		/* Remove FCS if needed */
+		if (!fcs_stripped)
+			skb_trim(skb, len);
+
 		stmmac_get_rx_hwtstamp(priv, p, np, skb);
 
 		if (priv->hw->hw_vlan_en)

---
base-commit: 9bca72a2dd0663d4b39a5d4ed252ad78a8569b5d
change-id: 20260916-stmmac-rx-sg-fix-a8d2d8a3ba01

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


^ permalink raw reply related	[flat|nested] 5+ messages in thread

* Re: [PATCH net v2] net: stmmac: fix rx Scatter-Gather support
  2026-09-21 14:47 [PATCH net v2] net: stmmac: fix rx Scatter-Gather support Lorenzo Bianconi
@ 2026-09-22 14:48 ` sashiko-bot
  2026-09-23  7:11   ` Lorenzo Bianconi
  2026-09-24 14:48 ` netdev-bot+sashiko
  2026-09-26 21:00 ` Maxime Chevallier
  2 siblings, 1 reply; 5+ messages in thread
From: sashiko-bot @ 2026-09-22 14:48 UTC (permalink / raw)
  To: Lorenzo Bianconi; +Cc: bpf

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] stmmac: FCS stripping check corrupts payload on legacy cores
- [High] net: stmmac: skb_trim() on non-linear SKB triggers WARN_ON and fails
--

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

net: stmmac: fix rx Scatter-Gather support

This commit addresses SMMU translation faults on oversized incoming frames by
allocating and enabling a secondary RX buffer (buffer2) for non-SPH
scatter-gather frames on supported cores. Legacy cores keep buffer2 disabled,
and the FCS stripping logic is adjusted to handle both linear and assembled
frames correctly.

Link: https://lore.kernel.org/r/20260916-stmmac-rx-sg-fix-v1-1-b49b7b8f725f@oss.qualcomm.com

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

[ ... ]

> @@ -5807,24 +5812,31 @@ static int stmmac_rx(struct stmmac_priv *priv, int limit, u32 queue)
>  		len += buf2_len;
>  
>  		/* ACS is disabled; strip manually. */
> -		if (likely(!(status & rx_not_ls))) {
> -			if (buf2_len) {
> -				buf2_len -= ETH_FCS_LEN;
> -				len -= ETH_FCS_LEN;
> -			} else if (buf1_len) {
> -				buf1_len -= ETH_FCS_LEN;
> -				len -= ETH_FCS_LEN;
> -			}
> -		}
> +		if (likely(!(status & rx_not_ls)))
> +			len -= ETH_FCS_LEN;
>  
>  		if (!skb) {
>  			unsigned int pre_len, sync_len;
>  
> +			/* Each frame starts here: reset the FCS handling */
> +			fcs_stripped = false;
> +
>  			dma_sync_single_for_cpu(priv->device, buf->addr,
>  						buf1_len, dma_dir);
>  			net_prefetch(page_address(buf->page) +
>  				     buf->page_offset);
>  
> +			/* The XDP program must not see the FCS. This only
> +			 * applies to a single-buffer frame (buf2_len == 0),
> +			 * where the whole frame and its FCS sit in buffer1;
> +			 * otherwise the FCS is stripped from the assembled
> +			 * frame with skb_trim().
> +			 */
> +			if (stmmac_xdp_is_enabled(priv) && !buf2_len) {
> +				buf1_len -= ETH_FCS_LEN;
> +				fcs_stripped = true;
> +			}

[Severity: High]
Does this logic incorrectly strip the payload on legacy cores for
scatter-gather frames?

On legacy hardware without set_sec_addr support, stmmac_rx_check_buf2_cap()
returns false, meaning buf2_len is always 0. When a multi-descriptor (jumbo)
frame arrives on a legacy core with XDP enabled, the first descriptor enters
this if (!skb) block. Since buf2_len is 0, this check evaluates to true,
improperly subtracting ETH_FCS_LEN (4 bytes) from buf1_len and truncating the
payload. It also sets fcs_stripped = true, which means the actual FCS at the
end of the frame won't be trimmed later, leaving a corrupted frame.

[Severity: Critical]
This is a pre-existing issue, but can this underflow buf1_len and cause an
out-of-bounds access in the BPF program?

If a multi-descriptor frame is received and the XDP program drops the first
fragment, the driver frees the buffer, sets skb = NULL, and loops to read
the next descriptor via goto read_again. On the final descriptor,
stmmac_rx_buf2_len() returns 0. Since skb is NULL, the code re-enters this
if (!skb) block.

This check evaluates to true, executing buf1_len -= ETH_FCS_LEN. If the final
fragment only contains 1 to 3 bytes of data, this unsigned subtraction
underflows to a massive integer. This corrupted length is then passed to
xdp_prepare_buff(), setting a huge data_end and allowing the BPF program to
read and write out of bounds.

[ ... ]

> @@ -5924,6 +5936,10 @@ static int stmmac_rx(struct stmmac_priv *priv, int limit, u32 queue)
>  
>  		/* Got entire packet into SKB. Finish it. */
>  
> +		/* Remove FCS if needed */
> +		if (!fcs_stripped)
> +			skb_trim(skb, len);
> +

[Severity: High]
Can this trigger a WARN_ON and fail to strip the FCS for scatter-gather
frames?

For multi-buffer frames, data is appended using skb_add_rx_frag(), which
creates a non-linear SKB (skb->data_len > 0). However, skb_trim() requires a
linear SKB. Calling it on a non-linear SKB evaluates
WARN_ON(skb_is_nonlinear(skb)) and returns immediately without modifying the
SKB, leaving the FCS attached.

Should this use pskb_trim(skb, len) instead to correctly handle paged SKBs?

>  		stmmac_get_rx_hwtstamp(priv, p, np, skb);
>  
>  		if (priv->hw->hw_vlan_en)

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260921-stmmac-rx-sg-fix-v2-1-b6d88c5ac2d7@oss.qualcomm.com?part=1

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH net v2] net: stmmac: fix rx Scatter-Gather support
  2026-09-22 14:48 ` sashiko-bot
@ 2026-09-23  7:11   ` Lorenzo Bianconi
  0 siblings, 0 replies; 5+ messages in thread
From: Lorenzo Bianconi @ 2026-09-23  7:11 UTC (permalink / raw)
  To: sashiko-reviews; +Cc: bpf

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

> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> - [High] stmmac: FCS stripping check corrupts payload on legacy cores
> - [High] net: stmmac: skb_trim() on non-linear SKB triggers WARN_ON and fails
> --
> 
> commit a400f52631710a5add56c0a39b6ab0128eae6a53
> Author: Lorenzo Bianconi <lorenzo.bianconi@oss.qualcomm.com>
> 
> net: stmmac: fix rx Scatter-Gather support
> 
> This commit addresses SMMU translation faults on oversized incoming frames by
> allocating and enabling a secondary RX buffer (buffer2) for non-SPH
> scatter-gather frames on supported cores. Legacy cores keep buffer2 disabled,
> and the FCS stripping logic is adjusted to handle both linear and assembled
> frames correctly.
> 
> Link: https://lore.kernel.org/r/20260916-stmmac-rx-sg-fix-v1-1-b49b7b8f725f@oss.qualcomm.com
> 
> > diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> 
> [ ... ]
> 
> > @@ -5807,24 +5812,31 @@ static int stmmac_rx(struct stmmac_priv *priv, int limit, u32 queue)
> >  		len += buf2_len;
> >  
> >  		/* ACS is disabled; strip manually. */
> > -		if (likely(!(status & rx_not_ls))) {
> > -			if (buf2_len) {
> > -				buf2_len -= ETH_FCS_LEN;
> > -				len -= ETH_FCS_LEN;
> > -			} else if (buf1_len) {
> > -				buf1_len -= ETH_FCS_LEN;
> > -				len -= ETH_FCS_LEN;
> > -			}
> > -		}
> > +		if (likely(!(status & rx_not_ls)))
> > +			len -= ETH_FCS_LEN;
> >  
> >  		if (!skb) {
> >  			unsigned int pre_len, sync_len;
> >  
> > +			/* Each frame starts here: reset the FCS handling */
> > +			fcs_stripped = false;
> > +
> >  			dma_sync_single_for_cpu(priv->device, buf->addr,
> >  						buf1_len, dma_dir);
> >  			net_prefetch(page_address(buf->page) +
> >  				     buf->page_offset);
> >  
> > +			/* The XDP program must not see the FCS. This only
> > +			 * applies to a single-buffer frame (buf2_len == 0),
> > +			 * where the whole frame and its FCS sit in buffer1;
> > +			 * otherwise the FCS is stripped from the assembled
> > +			 * frame with skb_trim().
> > +			 */
> > +			if (stmmac_xdp_is_enabled(priv) && !buf2_len) {
> > +				buf1_len -= ETH_FCS_LEN;
> > +				fcs_stripped = true;
> > +			}
> 
> [Severity: High]
> Does this logic incorrectly strip the payload on legacy cores for
> scatter-gather frames?

I do think this issue can occur since the legacy cores never set rx_not_ls, so
the packet is always contained in a single buffer.

> 
> On legacy hardware without set_sec_addr support, stmmac_rx_check_buf2_cap()
> returns false, meaning buf2_len is always 0. When a multi-descriptor (jumbo)
> frame arrives on a legacy core with XDP enabled, the first descriptor enters
> this if (!skb) block. Since buf2_len is 0, this check evaluates to true,
> improperly subtracting ETH_FCS_LEN (4 bytes) from buf1_len and truncating the
> payload. It also sets fcs_stripped = true, which means the actual FCS at the
> end of the frame won't be trimmed later, leaving a corrupted frame.
> 
> [Severity: Critical]
> This is a pre-existing issue, but can this underflow buf1_len and cause an
> out-of-bounds access in the BPF program?
> 
> If a multi-descriptor frame is received and the XDP program drops the first
> fragment, the driver frees the buffer, sets skb = NULL, and loops to read
> the next descriptor via goto read_again. On the final descriptor,
> stmmac_rx_buf2_len() returns 0. Since skb is NULL, the code re-enters this
> if (!skb) block.
> 
> This check evaluates to true, executing buf1_len -= ETH_FCS_LEN. If the final
> fragment only contains 1 to 3 bytes of data, this unsigned subtraction
> underflows to a massive integer. This corrupted length is then passed to
> xdp_prepare_buff(), setting a huge data_end and allowing the BPF program to
> read and write out of bounds.

I think the main issue here is stmmac XDP implementation does not properly
support multi-buff. I am currently working on it.

> 
> [ ... ]
> 
> > @@ -5924,6 +5936,10 @@ static int stmmac_rx(struct stmmac_priv *priv, int limit, u32 queue)
> >  
> >  		/* Got entire packet into SKB. Finish it. */
> >  
> > +		/* Remove FCS if needed */
> > +		if (!fcs_stripped)
> > +			skb_trim(skb, len);
> > +
> 
> [Severity: High]
> Can this trigger a WARN_ON and fail to strip the FCS for scatter-gather
> frames?
> 
> For multi-buffer frames, data is appended using skb_add_rx_frag(), which
> creates a non-linear SKB (skb->data_len > 0). However, skb_trim() requires a
> linear SKB. Calling it on a non-linear SKB evaluates
> WARN_ON(skb_is_nonlinear(skb)) and returns immediately without modifying the
> SKB, leaving the FCS attached.
> 
> Should this use pskb_trim(skb, len) instead to correctly handle paged SKBs?

corret, I will fix it in v3.

Regards,
Lorenzo

> 
> >  		stmmac_get_rx_hwtstamp(priv, p, np, skb);
> >  
> >  		if (priv->hw->hw_vlan_en)
> 
> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/20260921-stmmac-rx-sg-fix-v2-1-b6d88c5ac2d7@oss.qualcomm.com?part=1

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

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH net v2] net: stmmac: fix rx Scatter-Gather support
  2026-09-21 14:47 [PATCH net v2] net: stmmac: fix rx Scatter-Gather support Lorenzo Bianconi
  2026-09-22 14:48 ` sashiko-bot
@ 2026-09-24 14:48 ` netdev-bot+sashiko
  2026-09-26 21:00 ` Maxime Chevallier
  2 siblings, 0 replies; 5+ messages in thread
From: netdev-bot+sashiko @ 2026-09-24 14:48 UTC (permalink / raw)
  To: lorenzo.bianconi
  Cc: maxime.chevallier, andrew+netdev, davem, edumazet, kuba, pabeni,
	mcoquelin.stm32, alexandre.torgue, Jose.Abreu, ast, daniel, hawk,
	john.fastabend, sdf, netdev, linux-stm32, linux-arm-kernel, bpf

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

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

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

- [Critical] stmmac_rx() strips the FCS from multi-buffer frames with
  skb_trim(skb, len), but skb_trim() -> __skb_trim() ->…
- [Critical] With buffer2 now armed for non-SPH RX on GMAC4/XGMAC, an
  XDP_TX/XDP_REDIRECT verdict taken on a non-final descriptor of a…
- [High] The new XDP FCS strip in stmmac_rx() performs `buf1_len -=
  ETH_FCS_LEN` (line 5836) with no check that buf1_len >= ETH_FCS_LEN;…
- [High] stmmac_rx_check_buf2_cap() equates 'has a set_sec_addr op' with
  'can program (and disable) an independent secondary RX buffer', but…
- [Medium] The patch hard-codes pp_params.offset = 0 for every mode while
  stmmac_rx() still computes the partial device-sync length relative to…
- [Medium] The commit message attributes the SMMU fault to buffer2 never
  being backed for non-SPH frames, but dwmac4_set_sec_addr() gates DMA…
- [Medium] An oversized frame can now carry data in buffer2 (or in
  following descriptors) that is invisible to the attached XDP program…
- [Medium] The final `skb_trim(skb, len)` uses the accumulated hardware
  DMA length, which is fixed before the XDP program runs, while the…
- [Low] stmmac_rx_buf2_len() keeps the comment block asserting "Thus
  'plen - len' always gives the correct length of buf2" and scoping its…
- [Low] Concern that stmmac_rx_buf2_len()'s new condition lets GMAC4 with
  SPH disabled take the 'buffer2 is completely full' shortcut (return…

Pre-existing issues:
- [High] On the napi_build_skb() failure path in stmmac_rx() (line
  5897-5905) the page is returned to the page pool with…

--

net: stmmac: fix rx Scatter-Gather support

The patch allocates and programs the secondary RX buffer (buffer2) for
non-split-header frames on cores that expose a set_sec_addr descriptor op,
widens the page pool sync window to cover both buffers in every mode, and
replaces the per-buffer FCS removal with a skb_trim() on the assembled
frame.

Link: https://lore.kernel.org/r/20260916-stmmac-rx-sg-fix-v1-1-b49b7b8f725f@oss.qualcomm.com

A few questions below.

> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> index 1fb5f804ea234..299355d6bf5fc 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> @@ -1629,6 +1629,16 @@ static void stmmac_clear_descriptors(struct stmmac_priv *priv,
>  		stmmac_clear_tx_descriptors(priv, dma_conf, queue);
>  }
>  
> +static bool stmmac_rx_check_buf2_cap(struct stmmac_priv *priv)
> +{
> +	/* Only cores that can program an independent secondary RX buffer
> +	 * (used for scatter-gather overflow or split-header payload) back
> +	 * buffer2. Legacy cores have no set_sec_addr op, so buffer2 is
> +	 * never handed to the hardware there.
> +	 */
> +	return priv->hw->desc && priv->hw->desc->set_sec_addr;
> +}

[Severity: High]

Does the presence of a set_sec_addr op really imply the core can also
disable buffer2?  dwmac4_set_sec_addr() honours the flag:

drivers/net/ethernet/stmicro/stmmac/dwmac4_descs.c:
	if (buf2_valid)
		p->des3 |= cpu_to_le32(RDES3_BUFFER2_VALID_ADDR);
	else
		p->des3 &= cpu_to_le32(~RDES3_BUFFER2_VALID_ADDR);

while dwxgmac2_set_sec_addr() ignores its is_valid argument entirely:

drivers/net/ethernet/stmicro/stmmac/dwxgmac2_descs.c:
static void dwxgmac2_set_sec_addr(struct dma_desc *p, dma_addr_t addr, bool is_valid)
{
	p->des2 = cpu_to_le32(lower_32_bits(addr));
	p->des3 = cpu_to_le32(upper_32_bits(addr));
}

The AF_XDP zero-copy refill path still does

	stmmac_set_desc_sec_addr(priv, rx_desc, 0, false);

in stmmac_rx_refill_zc() and then hands the descriptor to the DMA, and
stmmac_alloc_rx_buffers_zc() never programs a secondary address at all.
On XGMAC the false is discarded, so the descriptor stays armed with
buffer2 at DMA address 0 - the same unmapped-secondary-buffer condition
(iova=0x00000000) this patch is fixing for the non-ZC path.  XGMAC_CORE_INIT_RX
programs GPSL to XGMAC_JUMBO_LEN, so the MAC accepts frames far larger
than the XSK frame size used as dma_buf_sz.

The commit message acknowledges the gap:

  AF_XDP zero-copy RX is not covered by this change: a ZC queue still
  programs buffer2 at DMA address 0 (and XGMAC has no buffer2-valid bit),
  so an oversized frame overflowing buffer1 can still trigger the same
  SMMU translation fault. Handling is planned as a follow-up.

Since the fault class the patch claims to close stays reachable, would it
make sense to program a mapped buffer2 on ZC queues (or refuse ZC on cores
without a valid bit) here rather than relying on a flag the XGMAC
descriptor writer discards?  The new helper's comment also documents
semantics XGMAC does not implement.

>  
>  /**
>   * stmmac_init_rx_buffers - init the RX descriptor buffer.
>   * @priv: driver private structure
> @@ -1659,16 +1669,13 @@ static int stmmac_init_rx_buffers(struct stmmac_priv *priv,
>  		buf->page_offset = stmmac_rx_offset(priv);
>  	}
>  
> -	if (priv->sph_active && !buf->sec_page) {
> +	if (stmmac_rx_check_buf2_cap(priv) && !buf->sec_page) {
>  		buf->sec_page = page_pool_alloc_pages(rx_q->page_pool, gfp);
>  		if (!buf->sec_page)
>  			return -ENOMEM;
>  
>  		buf->sec_addr = page_pool_get_dma_addr(buf->sec_page);
>  		stmmac_set_desc_sec_addr(priv, p, buf->sec_addr, true);
> -	} else {
> -		buf->sec_page = NULL;
> -		stmmac_set_desc_sec_addr(priv, p, buf->sec_addr, false);
>  	}

[Severity: Medium]

Should the commit message mention that this doubles the RX ring memory
footprint?  On GMAC4/XGMAC configurations where SPH is not active
(STMMAC_FLAG_SPH_DISABLE, !dma_cap.sphen, or any attached XDP program,
which forces sph_active to false) every descriptor now pins two page pool
pages instead of one.

pp_params.pool_size stays at dma_conf->dma_rx_size while twice as many
pages are in flight, so a share of recycles now falls back to the page
allocator on the RX hot path.

Also, the commit message says buffer2 "was neither allocated nor backed by
a valid mapping".  For GMAC4 the removed else branch cleared
RDES3_BUFFER2_VALID_ADDR, which gates DMA writes to buffer2, whereas
dwxgmac2_set_sec_addr() has no such bit at all.  Which core produced the
quoted fault, and could the description distinguish the two cases?

>  
>  	buf->addr = page_pool_get_dma_addr(buf->page) + buf->page_offset;
> @@ -2256,13 +2263,8 @@ static int __alloc_dma_rx_desc_resources(struct stmmac_priv *priv,
>  	pp_params.nid = dev_to_node(priv->device);
>  	pp_params.dev = priv->device;
>  	pp_params.dma_dir = xdp_prog ? DMA_BIDIRECTIONAL : DMA_FROM_DEVICE;
> -	pp_params.offset = stmmac_rx_offset(priv);
> -	pp_params.max_len = dma_conf->dma_buf_sz;
> -
> -	if (priv->sph_active) {
> -		pp_params.offset = 0;
> -		pp_params.max_len += stmmac_rx_offset(priv);
> -	}
> +	pp_params.offset = 0;
> +	pp_params.max_len = dma_conf->dma_buf_sz + stmmac_rx_offset(priv);

[Severity: Medium]

With offset forced to 0 in every mode, does the partial device sync on the
XDP consumed path still cover the region the CPU touched?  stmmac_rx()
still computes the length relative to buf->page_offset:

			sync_len = ctx.xdp.data_end - ctx.xdp.data_hard_start -
				   buf->page_offset;
			...
					page_pool_put_page(rx_q->page_pool,
							   virt_to_head_page(ctx.xdp.data),
							   sync_len, true);

and page_pool starts the sync at pool->p.offset, capping but never
extending the supplied length:

net/core/page_pool.c:__page_pool_dma_sync_for_device() {
	dma_sync_size = min(dma_sync_size, pool->p.max_len);
	__dma_sync_single_for_device(pool->p.dev, dma_addr + pool->p.offset,
				     dma_sync_size, pool->p.dma_dir);
}

So the window is shifted down by page_offset: the headroom gets synced and
the last page_offset bytes (258 with XDP headroom) of the touched region do
not, before the page is recycled to the RX DMA.  Previously XDP always ran
with offset = stmmac_rx_offset(), because sph_active is forced false while a
program is attached, so the base matched the subtraction.

Would adding buf->page_offset back into sync_len (or keeping offset at
stmmac_rx_offset() and only widening max_len) be the intent here?

>  
>  	rx_q->page_pool = page_pool_create(&pp_params);
>  	if (IS_ERR(rx_q->page_pool)) {

[ ... ]

> @@ -5142,7 +5142,9 @@ static unsigned int stmmac_rx_buf1_len(struct stmmac_priv *priv,
>  	unsigned int plen = 0, hlen = 0;
>  	int coe = priv->hw->rx_csum;
>  
> -	/* Not first descriptor, buffer is always zero */
> +	/* Not first descriptor, SPH enabled: buffer1 only carries the
> +	 * split header of the first descriptor, so it is zero here.
> +	 */
>  	if (priv->sph_active && len)
>  		return 0;
>  
> @@ -5153,14 +5155,16 @@ static unsigned int stmmac_rx_buf1_len(struct stmmac_priv *priv,
>  		return hlen;
>  	}
>  
> -	/* First descriptor, not last descriptor and not split header */
> +	/* Not last descriptor and not split header: buffer1 is fully filled */
>  	if (status & rx_not_ls)
>  		return priv->dma_conf.dma_buf_sz;
>  
>  	plen = stmmac_get_rx_frame_len(priv, p, coe);
>  
> -	/* First descriptor and last descriptor and not split header */
> -	return min_t(unsigned int, priv->dma_conf.dma_buf_sz, plen);
> +	/* Last descriptor and not split header: buffer1 holds the remaining
> +	 * bytes of the frame, up to dma_buf_sz
> +	 */
> +	return min_t(unsigned int, priv->dma_conf.dma_buf_sz, plen - len);
>  }
>  
>  static unsigned int stmmac_rx_buf2_len(struct stmmac_priv *priv,
> @@ -5170,8 +5174,7 @@ static unsigned int stmmac_rx_buf2_len(struct stmmac_priv *priv,
>  	int coe = priv->hw->rx_csum;
>  	unsigned int plen = 0;
>  
> -	/* Not split header, buffer is not available */
> -	if (!priv->sph_active)
> +	if (!stmmac_rx_check_buf2_cap(priv))
>  		return 0;
>  
>  	/* For GMAC4, when split header is enabled, in some rare cases, the
> @@ -5188,14 +5191,15 @@ static unsigned int stmmac_rx_buf2_len(struct stmmac_priv *priv,
>  	 * Thus 'plen - len' always gives the correct length of buf2.
>  	 */
>  
> -	/* Not GMAC4 and not last descriptor */
> -	if (priv->plat->core_type != DWMAC_CORE_GMAC4 && (status & rx_not_ls))
> +	/* Not GMAC4, or non-SPH and not last descriptor */
> +	if ((priv->plat->core_type != DWMAC_CORE_GMAC4 || !priv->sph_active) &&
> +	    (status & rx_not_ls))
>  		return priv->dma_conf.dma_buf_sz;
>  
>  	/* GMAC4 or last descriptor */
>  	plen = stmmac_get_rx_frame_len(priv, p, coe);
>  
> -	return plen - len;
> +	return plen > len ? plen - len : 0;
>  }

[Severity: Critical]

The new clamp guards plen < len, but there is no upper bound on the
result.  Can buf2_len end up larger than the page backing sec_page?

In stmmac_rx() an XDP_TX/XDP_REDIRECT verdict abandons the frame with
continue rather than goto read_again, even when rx_not_ls is set:

				} else if (xdp_res & (STMMAC_XDP_TX |
						      STMMAC_XDP_REDIRECT)) {
					xdp_status |= xdp_res;
					buf->page = NULL;
					skb = NULL;
					count++;
					continue;

count has been incremented, so at the loop head the saved-state branch is
not taken and len is reset while the remaining descriptors of the same
frame are still in the ring:

		} else {
			rx_q->state_saved = false;
			skb = NULL;
			error = 0;
			len = 0;
		}

Those descriptors are then parsed as a fresh frame with len starting at 0,
while plen on the last descriptor is the full frame length.  For a 9000
byte frame with dma_buf_sz 1536: buf1_len = min(1536, 9000 - 3072) = 1536,
then buf2_len = 9000 - 4608 = 4392, which exceeds both dma_buf_sz and the
order-0 page holding sec_page.  That value is used unchecked:

		if (buf2_len) {
			dma_sync_single_for_cpu(priv->device, buf->sec_addr,
						buf2_len, dma_dir);
			skb_add_rx_frag(skb, skb_shinfo(skb)->nr_frags,
					buf->sec_page, 0, buf2_len,
					priv->dma_conf.dma_buf_sz);

sph_active is forced false whenever an XDP program is attached
(stmmac_xdp_set_prog()), so the configuration where buffer2 is newly armed
is exactly the XDP one, and GMAC_CORE_INIT sets GMAC_CONFIG_JE so the MAC
accepts oversized frames even at MTU 1500.  Before this patch buf2_len was
hard zero outside SPH, so this was unreachable.

Should buf2_len be clamped to dma_buf_sz, and should that verdict path use
goto read_again while rx_not_ls is set?

[Severity: Low]

This isn't a bug, but the retained comment block still asserts "Thus
'plen - len' always gives the correct length of buf2" while the return is
now guarded with plen > len ? plen - len : 0.

The block also scopes its reasoning to "For GMAC4, when split header is
enabled", although the function now serves the non-SPH scatter-gather case
too, including XGMAC.  The trailing "/* GMAC4 or last descriptor */" label
no longer matches the condition above it either.

>  
>  static int stmmac_xdp_xmit_xdpf(struct stmmac_priv *priv, int queue,

[ ... ]

> @@ -5807,24 +5812,31 @@ static int stmmac_rx(struct stmmac_priv *priv, int limit, u32 queue)
>  		len += buf2_len;
>  
>  		/* ACS is disabled; strip manually. */
> -		if (likely(!(status & rx_not_ls))) {
> -			if (buf2_len) {
> -				buf2_len -= ETH_FCS_LEN;
> -				len -= ETH_FCS_LEN;
> -			} else if (buf1_len) {
> -				buf1_len -= ETH_FCS_LEN;
> -				len -= ETH_FCS_LEN;
> -			}
> -		}
> +		if (likely(!(status & rx_not_ls)))
> +			len -= ETH_FCS_LEN;
>  
>  		if (!skb) {
>  			unsigned int pre_len, sync_len;
>  
> +			/* Each frame starts here: reset the FCS handling */
> +			fcs_stripped = false;
> +
>  			dma_sync_single_for_cpu(priv->device, buf->addr,
>  						buf1_len, dma_dir);
>  			net_prefetch(page_address(buf->page) +
>  				     buf->page_offset);
>  
> +			/* The XDP program must not see the FCS. This only
> +			 * applies to a single-buffer frame (buf2_len == 0),
> +			 * where the whole frame and its FCS sit in buffer1;
> +			 * otherwise the FCS is stripped from the assembled
> +			 * frame with skb_trim().
> +			 */
> +			if (stmmac_xdp_is_enabled(priv) && !buf2_len) {
> +				buf1_len -= ETH_FCS_LEN;
> +				fcs_stripped = true;
> +			}
> +

[Severity: High]

Is buf1_len guaranteed to be at least ETH_FCS_LEN at this point?  The old
code guarded the equivalent subtraction with else if (buf1_len).

stmmac_rx_buf1_len() now returns min_t(unsigned int, dma_buf_sz,
plen - len), which legitimately yields 1..3 on a last descriptor reached
while skb is still NULL and len is non-zero.  Two ways to get there:

  - an XDP CONSUMED verdict on an earlier descriptor does goto read_again
    with len already accumulated
  - napi_build_skb() fails, goto drain_data falls through to read_again

buf2_len is 0 in exactly that case thanks to the new
plen > len ? plen - len : 0 clamp, so this block runs and buf1_len wraps
to roughly 4 GiB.  The wrapped value becomes the data_len argument of
xdp_prepare_buff(), producing an xdp_buff with data_end below data for the
program, and on XDP_PASS it is recomputed as

			buf1_len = ctx.xdp.data_end - ctx.xdp.data;

and passed to skb_put(), which would hit skb_over_panic().

Separately, is !buf2_len a sound test for "single-buffer frame"?  On cores
without set_sec_addr, stmmac_rx_buf2_len() always returns 0, so this would
strip 4 payload bytes from buffer1 of a non-last descriptor and set
fcs_stripped so the real FCS is never removed.

Would testing !(status & rx_not_ls) and buf1_len >= ETH_FCS_LEN cover both?
The len -= ETH_FCS_LEN above is unguarded in the same way.

>  			xdp_init_buff(&ctx.xdp, bufsz, &rx_q->xdp_rxq);
>  			xdp_prepare_buff(&ctx.xdp, page_address(buf->page),
>  					 buf->page_offset, buf1_len, true);

[Severity: Medium]

Now that buffer2 is populated for non-SPH RX, the xdp_buff is built from
buffer1 only and the has-frags flag is never set, yet sec_page is appended
to the skb with skb_add_rx_frag() afterwards and delivered on XDP_PASS.

Can an XDP filter be bypassed by an oversized frame whose tail lands in
buffer2, since the program inspects a shorter packet than the stack
receives?  On TX/REDIRECT the tail is dropped instead.

The commit message documents the limitation:

  Native XDP currently only supports single-buffer (linear) frames. An
  oversized frame accepted by the MAC (jumbo enabled) is received via
  buffer2; the XDP program only sees buffer1, so on a TX/REDIRECT verdict
  the frame is forwarded truncated.

stmmac_xdp_set_prog() caps the MTU at 1500, but GMAC_CONFIG_JE in
GMAC_CORE_INIT and GPSL = XGMAC_JUMBO_LEN mean the MAC still accepts
oversized frames from the wire.  Would dropping frames that span buffers
while a native XDP program is attached be preferable until multi-buffer
support lands?

> @@ -5924,6 +5936,10 @@ static int stmmac_rx(struct stmmac_priv *priv, int limit, u32 queue)
>  
>  		/* Got entire packet into SKB. Finish it. */
>  
> +		/* Remove FCS if needed */
> +		if (!fcs_stripped)
> +			skb_trim(skb, len);
> +

[Severity: Critical]

Does this strip the FCS for frames assembled from more than one buffer?
skb_trim() refuses to touch a non-linear skb:

include/linux/skbuff.h:__skb_set_length() {
	if (WARN_ON(skb_is_nonlinear(skb)))
		return;
	...
}

net/core/skbuff.c:skb_trim() {
	if (skb->len > len)
		__skb_trim(skb, len);
}

Any frame where buffer2 or a following descriptor was attached with
skb_add_rx_frag() has skb->data_len != 0, so skb_is_nonlinear() is true.
For those frames this call emits a WARN_ON() splat and leaves the four FCS
bytes in skb->len, while rx_bytes is accounted with the shorter len.

With SPH active on GMAC4/XGMAC, buffer1 carries only the split header and
the payload always lands in buffer2, so this would fire for essentially
every received IP packet, once per packet.  Should this be pskb_trim() or
__pskb_trim() instead?

[Severity: Medium]

len is finalized from the descriptor lengths before the program runs,
while the skb head is built from the XDP-adjusted length:

			/* XDP program may expand or reduce tail */
			buf1_len = ctx.xdp.data_end - ctx.xdp.data;

If an XDP_PASS program calls bpf_xdp_adjust_tail() or
bpf_xdp_adjust_head() on a frame that spans buffer1 and buffer2, does
skb->len still correspond to len?

A shrink larger than ETH_FCS_LEN makes skb->len < len, so the trim does
nothing and the FCS stays in the frame; a smaller adjustment is partially
undone by the trim.  rx_bytes is accounted from len as well.

This is masked today by the non-linear skb_trim() behaviour above, but
becomes a live length error once that call is converted to pskb_trim().

[Severity: High]

This isn't a bug introduced by this patch, but the napi_build_skb()
failure path earlier in stmmac_rx() recycles the page without clearing
buf->page:

			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);
				rx_dropped++;
				count++;
				goto drain_data;
			}

Every other consumption site in the function sets buf->page = NULL.
stmmac_rx_refill() only allocates when buf->page is NULL:

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

so the descriptor is re-armed with the DMA address of a page already
returned to the pool, which page_pool_alloc_pages() may have handed to
another entry or skb.

Does this patch make the interleaving easier to hit, given that two pool
pages per descriptor are now in flight while pp_params.pool_size stays at
dma_rx_size?

>  		stmmac_get_rx_hwtstamp(priv, p, np, skb);
>  
>  		if (priv->hw->hw_vlan_en)

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260921-stmmac-rx-sg-fix-v2-1-b6d88c5ac2d7%40oss.qualcomm.com

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH net v2] net: stmmac: fix rx Scatter-Gather support
  2026-09-21 14:47 [PATCH net v2] net: stmmac: fix rx Scatter-Gather support Lorenzo Bianconi
  2026-09-22 14:48 ` sashiko-bot
  2026-09-24 14:48 ` netdev-bot+sashiko
@ 2026-09-26 21:00 ` Maxime Chevallier
  2 siblings, 0 replies; 5+ messages in thread
From: Maxime Chevallier @ 2026-09-26 21:00 UTC (permalink / raw)
  To: Lorenzo Bianconi, Andrew Lunn, David S. Miller, Eric Dumazet,
	Jakub Kicinski, Paolo Abeni, Maxime Coquelin, Alexandre Torgue,
	Jose Abreu, Alexei Starovoitov, Daniel Borkmann,
	Jesper Dangaard Brouer, John Fastabend, Stanislav Fomichev
  Cc: netdev, linux-stm32, linux-arm-kernel, bpf

Hi Lorenzo,

On 9/21/26 16:47, Lorenzo Bianconi wrote:
> When a received frame is larger than dma_buf_sz, the DMA scatters it
> across multiple RX descriptors (rx Scatter-Gather). The secondary RX
> buffer (sec_page) was only allocated and programmed when split-header
> (SPH) was active, so for regular frames buffer2 was neither allocated
> nor backed by a valid mapping. As soon as an incoming frame overflowed
> buffer1, the DMA wrote the overflow into the unmapped secondary-buffer
> address, triggering an SMMU translation fault on IOMMU-based platforms:
> 
> arm-smmu 15000000.iommu: Unhandled context fault: fsr=0x402, iova=0x00000000, fsynr=0x7f0011, cbfrsynra=0x1c90, cb=11
> arm-smmu 15000000.iommu: FSR    = 00000402 [Format=2 TF], SID=0x1c90
> arm-smmu 15000000.iommu: FSYNR0 = 007f0011 [S1CBNDX=127 WNR PLVL=1]
> 
> Enable scatter-gather for non-SPH frames on cores that can program an
> independent secondary RX buffer (GMAC4/XGMAC): allocate and mark buffer2
> as valid in stmmac_init_rx_buffers() and stmmac_rx_refill(), and account
> for it in the buffer length computation. Legacy cores have no set_sec_addr
> op, so they keep buffer2 disabled.
> 
> Since buffer2 is handed to the DMA at page offset 0, the page pool sync
> window is widened to cover both buffers in every mode: offset is set to 0
> and max_len to dma_buf_sz + stmmac_rx_offset().
> 
> The FCS can straddle the buffer1/buffer2 boundary and a descriptor
> boundary, so it is now stripped from the tail of the assembled frame with
> skb_trim() instead of from a single buffer, avoiding an unsigned underflow
> for frames that overflow a buffer by 1..3 bytes. For single-buffer frames
> the XDP program must not see the FCS, so it is removed from the XDP buffer
> before the program runs.
> 
> Native XDP currently only supports single-buffer (linear) frames. An
> oversized frame accepted by the MAC (jumbo enabled) is received via
> buffer2; the XDP program only sees buffer1, so on a TX/REDIRECT verdict
> the frame is forwarded truncated. XDP multi-buffer support to handle
> this case is planned as a follow-up.
> 
> AF_XDP zero-copy RX is not covered by this change: a ZC queue still
> programs buffer2 at DMA address 0 (and XGMAC has no buffer2-valid bit),
> so an oversized frame overflowing buffer1 can still trigger the same
> SMMU translation fault. Handling is planned as a follow-up.
> 
> Fixes: 88ebe2cf7f3f ("net: stmmac: Rework stmmac_rx()")
> Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@oss.qualcomm.com>

I was able to test that on DWMAC4 (stm32mp157) sending oversized frames,
and they correctly spill over the next descriptor, no crashes no stall,
the only limitation being the RX fifo size now.

Tested-by: Maxime Chevallier <maxime.chevallier@bootlin.com>

Maxime


^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2026-09-26 21:01 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-09-21 14:47 [PATCH net v2] net: stmmac: fix rx Scatter-Gather support Lorenzo Bianconi
2026-09-22 14:48 ` sashiko-bot
2026-09-23  7:11   ` Lorenzo Bianconi
2026-09-24 14:48 ` netdev-bot+sashiko
2026-09-26 21:00 ` Maxime Chevallier

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox