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

[Lorenzo Bianconi <lorenzo.bianconi@oss.qualcomm.com>]

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