[PATCH net v3] net: stmmac: fix rx Scatter-Gather support

[PATCH net v3] net: stmmac: fix rx Scatter-Gather support

[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
pskb_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 v3:
- Run pskb_trim() to remove the ETH FCS from skb tail to take into
  account non-linear skb.
- Link to v2: https://lore.kernel.org/r/20260921-stmmac-rx-sg-fix-v2-1-b6d88c5ac2d7@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 | 83 +++++++++++++----------
 1 file changed, 49 insertions(+), 34 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index 3f34d491c959..ec62fa7418f4 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)) {
@@ -5099,7 +5101,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;
@@ -5110,10 +5112,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++;
@@ -5144,7 +5144,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;
 
@@ -5155,14 +5157,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,
@@ -5172,8 +5176,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
@@ -5190,14 +5193,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,
@@ -5720,6 +5724,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;
 
@@ -5809,24 +5814,25 @@ 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);
 
+			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);
@@ -5927,6 +5933,15 @@ 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 && pskb_trim(skb, len)) {
+			dev_kfree_skb_any(skb);
+			skb = NULL;
+			rx_dropped++;
+			count++;
+			continue;
+		}
+
 		stmmac_get_rx_hwtstamp(priv, p, np, skb);
 
 		if (priv->hw->hw_vlan_en)

---
base-commit: 4d56986fe713d91889fec7e155131fcc2c99a2e3
change-id: 20260916-stmmac-rx-sg-fix-a8d2d8a3ba01

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

Re: [PATCH net v3] net: stmmac: fix rx Scatter-Gather support

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] net: stmmac: data corruption in multi-descriptor frames with XDP on legacy MACs
--

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

net: stmmac: fix rx Scatter-Gather support

This commit enables scatter-gather for non-SPH frames on GMAC4/XGMAC cores to
address an SMMU translation fault on oversized frames overflowing the buffer. It
also updates FCS trimming to use pskb_trim() on the assembled skb instead of
subtracting it from individual buffers.

> 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

[ ... ]

> @@ -5809,24 +5814,25 @@ 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);
>  
> +			if (stmmac_xdp_is_enabled(priv) && !buf2_len) {
> +				buf1_len -= ETH_FCS_LEN;
> +				fcs_stripped = true;
> +			}

[Severity: High]
Does this incorrectly strip the FCS from the first descriptor of a
multi-descriptor frame on legacy MACs?

On legacy MACs, buffer2 is not used, so buf2_len is always 0. When a scattered
(multi-descriptor) frame is received with XDP enabled, this condition evaluates
to true on the first descriptor.

Because there is no check for whether this is the last descriptor
(!(status & rx_not_ls)), it incorrectly subtracts 4 bytes (ETH_FCS_LEN) from the
payload in the first descriptor, corrupting the packet data.

Additionally, because fcs_stripped is set to true, the actual FCS at the end of
the frame will not be removed when the final descriptor is processed.

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

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

Re: [PATCH net v3] net: stmmac: fix rx Scatter-Gather support

[Lorenzo Bianconi]

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

On Sep 24, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [High] net: stmmac: data corruption in multi-descriptor frames with XDP on legacy MACs
> --
> 
> commit bffa481d7cb386a04b24759408cee45add543fcd
> Author: Lorenzo Bianconi <lorenzo.bianconi@oss.qualcomm.com>
> 
> net: stmmac: fix rx Scatter-Gather support
> 
> This commit enables scatter-gather for non-SPH frames on GMAC4/XGMAC cores to
> address an SMMU translation fault on oversized frames overflowing the buffer. It
> also updates FCS trimming to use pskb_trim() on the assembled skb instead of
> subtracting it from individual buffers.
> 
> > 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
> 
> [ ... ]
> 
> > @@ -5809,24 +5814,25 @@ 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);
> >  
> > +			if (stmmac_xdp_is_enabled(priv) && !buf2_len) {
> > +				buf1_len -= ETH_FCS_LEN;
> > +				fcs_stripped = true;
> > +			}
> 
> [Severity: High]
> Does this incorrectly strip the FCS from the first descriptor of a
> multi-descriptor frame on legacy MACs?
> 
> On legacy MACs, buffer2 is not used, so buf2_len is always 0. When a scattered
> (multi-descriptor) frame is received with XDP enabled, this condition evaluates
> to true on the first descriptor.
> 
> Because there is no check for whether this is the last descriptor
> (!(status & rx_not_ls)), it incorrectly subtracts 4 bytes (ETH_FCS_LEN) from the
> payload in the first descriptor, corrupting the packet data.
> 
> Additionally, because fcs_stripped is set to true, the actual FCS at the end of
> the frame will not be removed when the final descriptor is processed.

This is a real issue, but it is confined to multi-descriptor frames in XDP mode,
which is explicitly unsupported. Within the supported scope (single-buffer XDP),
buf2_len == 0 on the frame-start descriptor implies a single-buffer frame,
so the FCS strip is correct. The proper fix is to handle multi-buffer frames in XDP
(including FCS placement) as part of XDP multi-buffer support, which will supersede
this logic. XDP-mb support is already posted on the mailing list for review.

Regards,
Lorenzo

> 
> > +
> >  			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);
> 
> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/20260923-stmmac-rx-sg-fix-v3-1-ed26fea7180d@oss.qualcomm.com?part=1

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

Re: [PATCH net v3] net: stmmac: fix rx Scatter-Gather support

[netdev-bot+sashiko]

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

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

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

- [High] The new XDP FCS strip in stmmac_rx() (`if
  (stmmac_xdp_is_enabled(priv) && !buf2_len) { buf1_len -= ETH_FCS_LEN;…
- [High] The page_pool sync length no longer matches its offset base on
  the XDP_CONSUMED path.
- [High] Out-of-bounds buffer2 fragment after an XDP_TX/REDIRECT verdict
  in the middle of a frame.
- [Medium] stmmac_rx() removes the FCS with the wrong length when XDP has
  changed the frame.
- [Low] Some new or kept comments do not match the code.

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but the same
  unbacked buffer2 DMA destination is still reachable on AF_XDP zero-…
- [High] This problem wasn't introduced by this patch, but in
  stmmac_rx(), when napi_build_skb() fails, buf->page is returned to the…

--

net: stmmac: fix rx Scatter-Gather support

This patch enables the secondary RX buffer (buffer2) for non-SPH frames on
GMAC4/XGMAC cores. Frames larger than dma_buf_sz should then no longer spill
into an unmapped address. It also moves the page_pool sync window to page
offset 0, and strips the FCS by trimming the assembled skb with pskb_trim()
(or from the XDP buffer for single-buffer frames).

> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> index d5a984ad864f2..36ecd32921c8e 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.
> +	 */

[Severity: Low]
This isn't a bug, but is "buffer2 is never handed to the hardware there"
accurate? In ring mode with dma_buf_sz == BUF_SIZE_16KiB, legacy cores do
program DES3 as a second buffer in ring_mode.c:

    static void init_desc3(struct dma_desc *p)
    {
            p->des3 = cpu_to_le32(le32_to_cpu(p->des2) + BUF_SIZE_8KiB);
    }

refill_desc3() does the same thing.

The comments in stmmac_rx_buf2_len() have the same problem. The
"/* GMAC4 or last descriptor */" label no longer matches the new condition,
because the code after it now runs for GMAC4 with SPH active, or for the
last descriptor. The block comment above it still says "'plen - len' always
gives the correct length of buf2", but the patch adds a clamp there.

> +	return priv->hw->desc && priv->hw->desc->set_sec_addr;
> +}

[ ... ]

> @@ -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: High]
Does the XDP_CONSUMED recycle path still sync the right range after this
change?

With XDP loaded, stmmac_xdp_set_prog() forces SPH off:

    priv->sph_active = priv->sph_capable && !stmmac_xdp_is_enabled(priv);

So before this patch, pp_params.offset in XDP mode was always equal to
buf->page_offset.

stmmac_rx() still computes sync_len 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);

__page_pool_dma_sync_for_device() then syncs starting at the pool offset:

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

With offset now 0, wouldn't this sync [0, sync_len) instead of
[page_offset, page_offset + sync_len)? The last page_offset bytes the CPU
touched would not be cleaned for the device. That is up to 258 bytes, and it
includes the Ethernet header that an XDP_TX program may have rewritten
before the TX ring filled up.

On non-coherent platforms, could a dirty line left there be written back
over the next frame DMA'd into the recycled page? One case is
dma_sync_single_for_cpu() on the unaligned buf->addr, which cleans and
invalidates the first line.

This can happen for XDP_DROP or XDP_ABORTED after the program wrote the
packet, for a failed XDP_REDIRECT, or for XDP_TX with a full TX ring.

[ ... ]

> @@ -5099,7 +5101,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;

[Severity: High]
This isn't a bug introduced by this patch, but it concerns how refill
reuses buf->page. In stmmac_rx(), when napi_build_skb() fails, the page is
recycled but buf->page still points at it:

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

stmmac_rx_refill() only allocates when !buf->page, so it re-arms the
descriptor with the recycled page. The same page also sits in the pool's
direct cache, and the next page_pool_alloc_pages() can hand it out again.

Refill now also allocates sec_page for every consumed secondary buffer on
GMAC4/XGMAC, which makes that reuse more likely. Can two descriptors end up
DMAing into the same page, with both owners later recycling it?

Should buf->page be set to NULL after page_pool_recycle_direct()?

> @@ -5110,10 +5112,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));

[Severity: High]
This is a pre-existing issue, and the commit message already says AF_XDP
zero-copy is not covered. Still, the same unbacked buffer2 can be reached
on XGMAC ZC queues. stmmac_rx_refill_zc() does:

    stmmac_set_desc_sec_addr(priv, rx_desc, 0, false);

and dwxgmac2_set_sec_addr() ignores is_valid:

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

stmmac_set_queue_rx_buf_size() gives a ZC queue a buffer size of
xsk_pool_get_rx_frame_size(), which is about 3840 bytes with 4 KiB chunks.
XGMAC_CORE_INIT_RX accepts frames up to 16368 bytes.

On a ZC queue, would a frame that overflows buffer1 be written to DMA
address 0? That would give the same SMMU fault this patch fixes, or a write
to physical address 0 on systems without an IOMMU.

[ ... ]

> @@ -5190,14 +5193,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: High]
buffer2 is now enabled in non-SPH mode, which is always the case with XDP
loaded. Can this return more than dma_buf_sz?

When the XDP program returns TX or REDIRECT on a non-last descriptor,
stmmac_rx() does:

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

The top of the loop then resets len to 0 even though the frame isn't
finished. The next descriptor is handled as a new frame, and on the last
descriptor plen - len is no longer limited to one descriptor.

For example, take GMAC4 with dma_buf_sz 1536 and a 6000 byte frame. desc0
is transmitted, and desc1 (the last one) gets buf1_len 1536 and buf2_len
4464.

If XDP then returns PASS for that chunk, stmmac_rx() does:

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

sec_page is an order-0 page here. Wouldn't the fragment run past the end of
the page, so that adjacent memory and stale pool contents reach the stack?
GMAC4 accepts frames up to about 9018 bytes with JE set, and XGMAC up to
16368.

Before this patch, buf2_len was always 0 without SPH.

[ ... ]

> @@ -5809,24 +5814,25 @@ 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);
>  
> +			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 here, and is this always
the last descriptor?

On GMAC4/XGMAC, an XDP_CONSUMED verdict on a non-last descriptor does goto
read_again with skb = NULL, and len keeps the bytes counted so far. The next
descriptor then enters this block as if it were a new frame.

Take a frame of n * 2 * dma_buf_sz + r bytes with r = 1..3, for example 3073
to 3075 bytes with dma_buf_sz 1536. The last descriptor gets
buf1_len = min(dma_buf_sz, plen - len) = r from stmmac_rx_buf1_len() and
buf2_len = 0, so this subtraction wraps:

    buf1_len -= ETH_FCS_LEN;

xdp_prepare_buff() then gets data_end < data, and on XDP_PASS this passes a
value around 0xfffffffd to skb_put():

    buf1_len = ctx.xdp.data_end - ctx.xdp.data;
    ...
    skb_put(skb, buf1_len);

Could this send an skb with len around 4 GiB to eth_type_trans() and the IP
stack, where later trims can put the tail past the napi frag?

On legacy cores without set_sec_addr, stmmac_rx_buf2_len() returns 0 for
every descriptor, so !buf2_len doesn't mean last descriptor there either.
Wouldn't the first descriptor of a multi-descriptor frame lose 4 bytes of
payload, and the final pskb_trim() then be skipped so the real FCS stays at
the tail?

Before this patch the strip only happened on the last descriptor. buf1_len
there was min(dma_buf_sz, plen), so it could not be 1..3.

> +
>  			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);
> @@ -5926,6 +5932,15 @@ 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 && pskb_trim(skb, len)) {

[Severity: Medium]
Does len still match the skb here when an XDP program has changed the size
of the frame?

For a frame that uses buffer2 on the first descriptor, fcs_stripped stays
false. On XDP_PASS the skb head is built from the adjusted XDP boundaries:

    buf1_len = ctx.xdp.data_end - ctx.xdp.data;
    ...
    skb_reserve(skb, head_pad_len);
    skb_put(skb, buf1_len);

len, however, comes only from the descriptor lengths minus ETH_FCS_LEN. It
never includes the bpf_xdp_adjust_head() or bpf_xdp_adjust_tail() delta.

pskb_trim() takes an absolute length and does nothing when len >= skb->len:

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

So wouldn't a program that pushes N bytes lose N bytes of payload from the
tail? And wouldn't one that pulls or shrinks by 4 or more bytes leave the
FCS in place?

Would pskb_trim(skb, skb->len - ETH_FCS_LEN) be more robust here?

> +			dev_kfree_skb_any(skb);
> +			skb = NULL;
> +			rx_dropped++;
> +			count++;
> +			continue;
> +		}

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

Re: [PATCH net v3] net: stmmac: fix rx Scatter-Gather support

[Lorenzo Bianconi]

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

> Thank you for your contribution! Sashiko AI review found 5 potential
> issue(s) to consider.
> 
> Critical: 0 · High: 3 (2) · Medium: 1 · Low: 1
> 
> If you believe none of the issues are legitimate, please reply to each
> issue of Medium severity or higher and explain why the AI is wrong.
> If any of them are legitimate and you plan to respin, please let patchwork
> know by including "pw-bot: cr" as a separate line at the end of your reply
> (one such reply per series is enough).
> 
> - [High] The new XDP FCS strip in stmmac_rx() (`if
>   (stmmac_xdp_is_enabled(priv) && !buf2_len) { buf1_len -= ETH_FCS_LEN;…
> - [High] The page_pool sync length no longer matches its offset base on
>   the XDP_CONSUMED path.
> - [High] Out-of-bounds buffer2 fragment after an XDP_TX/REDIRECT verdict
>   in the middle of a frame.
> - [Medium] stmmac_rx() removes the FCS with the wrong length when XDP has
>   changed the frame.
> - [Low] Some new or kept comments do not match the code.
> 
> Pre-existing issues:
> - [High] This problem wasn't introduced by this patch, but the same
>   unbacked buffer2 DMA destination is still reachable on AF_XDP zero-…
> - [High] This problem wasn't introduced by this patch, but in
>   stmmac_rx(), when napi_build_skb() fails, buf->page is returned to the…
> 
> --
> 
[...]

> [Severity: High]
> Does the XDP_CONSUMED recycle path still sync the right range after this
> change?
> 
> With XDP loaded, stmmac_xdp_set_prog() forces SPH off:
> 
>     priv->sph_active = priv->sph_capable && !stmmac_xdp_is_enabled(priv);
> 
> So before this patch, pp_params.offset in XDP mode was always equal to
> buf->page_offset.
> 
> stmmac_rx() still computes sync_len relative to buf->page_offset:
> 
>     sync_len = ctx.xdp.data_end - ctx.xdp.data_hard_start -
>                buf->page_offset;

I think the reported issue is real it is already present before this patch.
In particulat it is not correct to subtract buf->page_offset from the sync_len
since the program can push/pop headers in the headroom. I guess we should fix
this issue with a dedicated patch defining sync_len as below:

	sync_len = ctx.xdp.data_end - ctx.xdp.data_hard_start;

>     ...
>     page_pool_put_page(rx_q->page_pool,
>                        virt_to_head_page(ctx.xdp.data),
>                        sync_len, true);
> 
> __page_pool_dma_sync_for_device() then syncs starting at the pool offset:
> 
>     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);
> 
> With offset now 0, wouldn't this sync [0, sync_len) instead of
> [page_offset, page_offset + sync_len)? The last page_offset bytes the CPU
> touched would not be cleaned for the device. That is up to 258 bytes, and it
> includes the Ethernet header that an XDP_TX program may have rewritten
> before the TX ring filled up.
> 
> On non-coherent platforms, could a dirty line left there be written back
> over the next frame DMA'd into the recycled page? One case is
> dma_sync_single_for_cpu() on the unaligned buf->addr, which cleans and
> invalidates the first line.
> 
> This can happen for XDP_DROP or XDP_ABORTED after the program wrote the
> packet, for a failed XDP_REDIRECT, or for XDP_TX with a full TX ring.
> 
> [ ... ]
> 
> > @@ -5099,7 +5101,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;
> 
> [Severity: High]
> This isn't a bug introduced by this patch, but it concerns how refill
> reuses buf->page. In stmmac_rx(), when napi_build_skb() fails, the page is
> recycled but buf->page still points at it:
> 
>     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;
>     }
> 
> stmmac_rx_refill() only allocates when !buf->page, so it re-arms the
> descriptor with the recycled page. The same page also sits in the pool's
> direct cache, and the next page_pool_alloc_pages() can hand it out again.
> 
> Refill now also allocates sec_page for every consumed secondary buffer on
> GMAC4/XGMAC, which makes that reuse more likely. Can two descriptors end up
> DMAing into the same page, with both owners later recycling it?
> 
> Should buf->page be set to NULL after page_pool_recycle_direct()?

This patch is already fixed in the following commit:

commit 0a7822e34a0bfde31b194ac3da3253e5032b44cc
Author: Lorenzo Bianconi <lorenzo.bianconi@oss.qualcomm.com>
Date:   Mon Sep 21 16:46:18 2026 +0200

    net: stmmac: clear stale buf->page after recycling on skb build failure
    
    In stmmac_rx(), when napi_build_skb() fails the descriptor page is
    recycled back to the page pool with page_pool_recycle_direct(), but
    buf->page is left pointing at the recycled page, unlike every other
    consumption site in the function which clears the pointer after handing
    the page away.
    
    With the stale pointer stmmac_rx_refill() skips the replacement
    allocation and programs the already-recycled page back into the RX
    descriptor.
    
    Clear buf->page on the napi_build_skb() failure path to keep the buffer
    lifecycle consistent with the other consumption sites.
    
    Fixes: df542f669307 ("net: stmmac: Switch to zero-copy in non-XDP RX path")
    Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@oss.qualcomm.com>
    Reviewed-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
    Link: https://patch.msgid.link/20260921-stmmac-fix-napi-build-skb-error-v1-1-3d54bf6d9bb6@oss.qualcomm.com
    Signed-off-by: Jakub Kicinski <kuba@kernel.org>

> 
> > @@ -5110,10 +5112,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));
> 
> [Severity: High]
> This is a pre-existing issue, and the commit message already says AF_XDP
> zero-copy is not covered. Still, the same unbacked buffer2 can be reached
> on XGMAC ZC queues. stmmac_rx_refill_zc() does:
> 
>     stmmac_set_desc_sec_addr(priv, rx_desc, 0, false);
> 
> and dwxgmac2_set_sec_addr() ignores is_valid:
> 
>     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));
>     }
> 
> stmmac_set_queue_rx_buf_size() gives a ZC queue a buffer size of
> xsk_pool_get_rx_frame_size(), which is about 3840 bytes with 4 KiB chunks.
> XGMAC_CORE_INIT_RX accepts frames up to 16368 bytes.
> 
> On a ZC queue, would a frame that overflows buffer1 be written to DMA
> address 0? That would give the same SMMU fault this patch fixes, or a write
> to physical address 0 on systems without an IOMMU.

As pointed out by sashiko, this is a pre-existing issue and it needs to be
fixed with a dedicated patch.

> 
> [ ... ]
> 
> > @@ -5190,14 +5193,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: High]
> buffer2 is now enabled in non-SPH mode, which is always the case with XDP
> loaded. Can this return more than dma_buf_sz?
> 
> When the XDP program returns TX or REDIRECT on a non-last descriptor,
> stmmac_rx() does:
> 
>     } else if (xdp_res & (STMMAC_XDP_TX |
>                           STMMAC_XDP_REDIRECT)) {
>             xdp_status |= xdp_res;
>             buf->page = NULL;
>             skb = NULL;
>             count++;
>             continue;
>     }
> 
> The top of the loop then resets len to 0 even though the frame isn't
> finished. The next descriptor is handled as a new frame, and on the last
> descriptor plen - len is no longer limited to one descriptor.
> 
> For example, take GMAC4 with dma_buf_sz 1536 and a 6000 byte frame. desc0
> is transmitted, and desc1 (the last one) gets buf1_len 1536 and buf2_len
> 4464.
> 
> If XDP then returns PASS for that chunk, stmmac_rx() does:
> 
>     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);
> 
> sec_page is an order-0 page here. Wouldn't the fragment run past the end of
> the page, so that adjacent memory and stale pool contents reach the stack?
> GMAC4 accepts frames up to about 9018 bytes with JE set, and XGMAC up to
> 16368.
> 
> Before this patch, buf2_len was always 0 without SPH.

This issue (as even the ones reported below) is due to the missing XDP
multi-buff support. The reported problem will be cleanly fixed adding
XDP multi-buff support (I have already posted the related patches on
the mailing list).

> 
> [ ... ]
> 
> > @@ -5809,24 +5814,25 @@ 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);
> >  
> > +			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 here, and is this always
> the last descriptor?
> 
> On GMAC4/XGMAC, an XDP_CONSUMED verdict on a non-last descriptor does goto
> read_again with skb = NULL, and len keeps the bytes counted so far. The next
> descriptor then enters this block as if it were a new frame.
> 
> Take a frame of n * 2 * dma_buf_sz + r bytes with r = 1..3, for example 3073
> to 3075 bytes with dma_buf_sz 1536. The last descriptor gets
> buf1_len = min(dma_buf_sz, plen - len) = r from stmmac_rx_buf1_len() and
> buf2_len = 0, so this subtraction wraps:
> 
>     buf1_len -= ETH_FCS_LEN;
> 
> xdp_prepare_buff() then gets data_end < data, and on XDP_PASS this passes a
> value around 0xfffffffd to skb_put():
> 
>     buf1_len = ctx.xdp.data_end - ctx.xdp.data;
>     ...
>     skb_put(skb, buf1_len);
> 
> Could this send an skb with len around 4 GiB to eth_type_trans() and the IP
> stack, where later trims can put the tail past the napi frag?
> 
> On legacy cores without set_sec_addr, stmmac_rx_buf2_len() returns 0 for
> every descriptor, so !buf2_len doesn't mean last descriptor there either.
> Wouldn't the first descriptor of a multi-descriptor frame lose 4 bytes of
> payload, and the final pskb_trim() then be skipped so the real FCS stays at
> the tail?
> 
> Before this patch the strip only happened on the last descriptor. buf1_len
> there was min(dma_buf_sz, plen), so it could not be 1..3.

This issue is due to the missing XDP multi-buff support. The reported problem
will be cleanly fixed adding XDP multi-buff support.
> 
> > +
> >  			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);
> > @@ -5926,6 +5932,15 @@ 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 && pskb_trim(skb, len)) {
> 
> [Severity: Medium]
> Does len still match the skb here when an XDP program has changed the size
> of the frame?
> 
> For a frame that uses buffer2 on the first descriptor, fcs_stripped stays
> false. On XDP_PASS the skb head is built from the adjusted XDP boundaries:
> 
>     buf1_len = ctx.xdp.data_end - ctx.xdp.data;
>     ...
>     skb_reserve(skb, head_pad_len);
>     skb_put(skb, buf1_len);
> 
> len, however, comes only from the descriptor lengths minus ETH_FCS_LEN. It
> never includes the bpf_xdp_adjust_head() or bpf_xdp_adjust_tail() delta.
> 
> pskb_trim() takes an absolute length and does nothing when len >= skb->len:
> 
>     return (len < skb->len) ? __pskb_trim(skb, len) : 0;
> 
> So wouldn't a program that pushes N bytes lose N bytes of payload from the
> tail? And wouldn't one that pulls or shrinks by 4 or more bytes leave the
> FCS in place?
> 
> Would pskb_trim(skb, skb->len - ETH_FCS_LEN) be more robust here?

This issue is due to the missing XDP multi-buff support. The reported problem
will be cleanly fixed adding XDP multi-buff support.

Regards,
Lorenzo

> 
> > +			dev_kfree_skb_any(skb);
> > +			skb = NULL;
> > +			rx_dropped++;
> > +			count++;
> > +			continue;
> > +		}
> 
> -- 
> Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260923-stmmac-rx-sg-fix-v3-1-ed26fea7180d%40oss.qualcomm.com

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

Re: [PATCH net v3] net: stmmac: fix rx Scatter-Gather support

[Maxime Chevallier]

Hi Lorenzo,

On 9/23/26 11:14, 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
> pskb_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>

Meh I replied to the previous iteration... I did test V3 actually, so
here's the blurb I said on V2 + tag :

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

Re: [PATCH net v3] net: stmmac: fix rx Scatter-Gather support

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net.git (main)
by Jakub Kicinski <kuba@kernel.org>:

On Wed, 23 Sep 2026 11:14:28 +0200 you 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:
> 
> [...]

Here is the summary with links:
  - [net,v3] net: stmmac: fix rx Scatter-Gather support
    https://git.kernel.org/netdev/net/c/88095728511e

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html