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

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

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

> Thank you for your contribution! Sashiko AI review found 9 potential
> issue(s) to consider.
> 
> Critical: 1 · High: 1 (1) · Medium: 6 · 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).
> 
> - [Critical] stmmac_rx() strips the FCS with an unchecked unsigned
>   subtraction (`buf2_len -= ETH_FCS_LEN` at stmmac_main.c:5803,…
> - [High] The patch unconditionally allocates a secondary RX page and
>   unconditionally reports a non-zero buffer2 length for every MAC core,…
> - [Medium] The RX page pool is created with `pp_params.offset =
>   stmmac_rx_offset(priv)` and `max_len = dma_conf->dma_buf_sz`, and only…
> - [Medium] For GMAC4 without SPH, stmmac_rx_buf2_len() now assumes every
>   intermediate descriptor's buffer2 is completely filled (returns…
> - [Medium] The patch makes the secondary page-pool page mandatory for
>   every RX descriptor on every stmmac device and configuration…
> - [Medium] With a native XDP program attached, stmmac_xdp_set_prog()
>   explicitly clears priv->sph_active, and this patch now arms buffer2…
> - [Medium] The AF_XDP zero-copy refill path is left untouched:
>   stmmac_rx_refill_zc() still calls `stmmac_set_desc_sec_addr(priv,…
> - [Medium] The new length arithmetic depends on `priv->hw->rx_csum`,
>   which stmmac_set_features() (stmmac_main.c:6210-6214) mutates from…
> - [Low] The descriptor-position comments in the changed RX length helpers
>   no longer describe the code.

I will address sashiko's comments in v2.

Regards,
Lorenzo

> 
> Pre-existing issues:
> - [High] In stmmac_rx(), when napi_build_skb() fails the code calls
>   page_pool_recycle_direct(rx_q->page_pool, buf->page) and jumps to…
> 
> --
> 
> net: stmmac: fix rx Scatter-Gather support
> 
> The patch always allocates and programs the secondary RX buffer (sec_page)
> instead of doing so only when split-header is active, so that a frame
> overflowing buffer1 does not make the DMA write to an unmapped address.
> stmmac_rx_buf1_len() and stmmac_rx_buf2_len() are adjusted to account for
> buffer2 in the non-SPH case.
> 
> A few questions about the buffer2 accounting and the cores/paths that are
> now affected.
> 
> > diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > index 1fb5f804ea234..be7cb0cafeb50 100644
> > --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > @@ -1659,17 +1659,14 @@ 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 (!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);
> >  	}
> > +	stmmac_set_desc_sec_addr(priv, p, buf->sec_addr, true);
> 
> [Severity: High]
> Does the "always mark buffer2 as valid" part hold on the legacy cores?
> Only dwmac4_descs.c and dwxgmac2_descs.c provide a set_sec_addr callback;
> enh_desc_ops (dwmac1000) and ndesc_ops (dwmac100) do not, and the
> dispatch macro silently skips an absent callback:
> 
> drivers/net/ethernet/stmicro/stmmac/hwif.h:stmmac_do_void_callback() {
> 	int __result = -EINVAL; \
> 	if ((__priv)->hw->__module && (__priv)->hw->__module->__cname) { \
> 		(__priv)->hw->__module->__cname((__arg0), ##__args); \
> 	...
> }
> 
> So on those cores buf->sec_addr is never handed to the hardware, and in
> ring mode with 16 KiB buffers the hardware buffer2 is inside the primary
> page anyway:
> 
> drivers/net/ethernet/stmicro/stmmac/ring_mode.c:refill_desc3() {
> 	if (priv->dma_conf.dma_buf_sz == BUF_SIZE_16KiB)
> 		p->des3 = cpu_to_le32(le32_to_cpu(p->des2) + BUF_SIZE_8KiB);
> }
> 
> With the "!priv->sph_active" early return gone from stmmac_rx_buf2_len(),
> buf2_len becomes non-zero on those cores too, and stmmac_rx() then does:
> 
> 	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);
> 
> Can this publish the contents of a page the DMA never wrote to the network
> stack?  On dwmac1000/dwmac100 the intermediate descriptors of a scattered
> frame are discarded by enh_desc_get_rx_status()/ndesc_get_rx_status(), so
> the last descriptor is processed with len = 0 and its FL field is the total
> frame length; a 3000-byte frame with dma_buf_sz = 2048 would give
> buf1_len = 2048 and buf2_len = 952 out of the untouched sec_page.  For
> larger jumbo frames buf2_len (plen - dma_buf_sz) can also exceed the
> order-0 sec_page.  Should buffer2 be gated on the cores that can actually
> program an independently mapped buffer2?
> 
> [Severity: Medium]
> The secondary buffer is handed to the DMA at page offset 0, but the page
> pool is only configured with offset 0 when SPH is active:
> 
> drivers/net/ethernet/stmicro/stmmac/stmmac_main.c:__alloc_dma_rx_desc_resources() {
> 	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);
> 	}
> }
> 
> 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);
> }
> 
> With !sph_active the pool syncs from dma_addr + NET_SKB_PAD + NET_IP_ALIGN,
> so the first bytes of every secondary page are given to the device without
> a sync-for-device, both on first map and on recycle.  On a non-coherent
> platform, can a dirty cache line in that prefix be written back over data
> the device has already placed there?  The SPH path sets offset = 0 for
> exactly this reason.
> 
> > @@ -5097,7 +5094,7 @@ static inline void stmmac_rx_refill(struct stmmac_priv *priv, u32 queue)
> >  				break;
> >  		}
> >  
> > -		if (priv->sph_active && !buf->sec_page) {
> > +		if (!buf->sec_page) {
> >  			buf->sec_page = page_pool_alloc_pages(rx_q->page_pool, gfp);
> >  			if (!buf->sec_page)
> >  				break;
> 
> [Severity: Medium]
> This makes a second page-pool page mandatory for every RX descriptor on
> every stmmac device, doubling the RX buffer footprint (order-2/order-3
> pages for the 8/16 KiB bfsize cases), while pp_params.pool_size stays at
> dma_conf->dma_rx_size, i.e. the recycle ring is now sized for half the
> in-flight pages.
> 
> For MTU-conformant traffic dma_buf_sz is always above the MTU:
> 
> drivers/net/ethernet/stmicro/stmmac/stmmac_main.c:stmmac_set_bfsize() {
> 	if (mtu >= BUF_SIZE_8KiB)
> 		ret = BUF_SIZE_16KiB;
> 	...
> 	else
> 		ret = DEFAULT_BUFSIZE;
> }
> 
> so buffer1 alone holds the frame and the second page is unused.  Could the
> commit message state in which configuration an in-spec frame exceeds
> dma_buf_sz, and mention the extra page per descriptor plus the pool_size
> mismatch?
> 
> > @@ -5108,10 +5105,7 @@ 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, true);
> >  		stmmac_refill_desc3(priv, rx_q, p);
> 
> [Severity: Medium]
> Native XDP always runs in the non-SPH mode that this patch now arms
> buffer2 for:
> 
> drivers/net/ethernet/stmicro/stmmac/stmmac_xdp.c:stmmac_xdp_set_prog() {
> 	priv->sph_active = priv->sph_capable && !stmmac_xdp_is_enabled(priv);
> }
> 
> In stmmac_rx() the program only ever sees buffer1:
> 
> 	xdp_prepare_buff(&ctx.xdp, page_address(buf->page),
> 			 buf->page_offset, buf1_len, true);
> 
> and on a TX/redirect verdict the loop leaves before the buf2_len block:
> 
> 	} else if (xdp_res & (STMMAC_XDP_TX |
> 			      STMMAC_XDP_REDIRECT)) {
> 		xdp_status |= xdp_res;
> 		buf->page = NULL;
> 		skb = NULL;
> 		count++;
> 		continue;
> 	}
> 
> XDP caps the configured MTU at ETH_DATA_LEN, but GMAC_CORE_INIT in
> dwmac4.h contains GMAC_CONFIG_JE and XGMAC_CORE_INIT_RX programs
> GPSL = XGMAC_JUMBO_LEN, so an oversized frame is still accepted and split.
> Does this silently forward a truncated frame, and should the multi-buffer
> case be rejected instead?
> 
> [Severity: Medium]
> The AF_XDP zero-copy refill path is left as it was:
> 
> drivers/net/ethernet/stmicro/stmmac/stmmac_main.c:stmmac_rx_refill_zc() {
> 	stmmac_set_desc_addr(priv, rx_desc, dma_addr);
> 	stmmac_set_desc_sec_addr(priv, rx_desc, 0, false);
> }
> 
> but on XGMAC the validity argument is ignored and there is no BUF2V bit in
> the RX read descriptor:
> 
> drivers/net/ethernet/stmicro/stmmac/dwxgmac2_descs.c:dwxgmac2_set_sec_addr() {
> 	p->des2 = cpu_to_le32(lower_32_bits(addr));
> 	p->des3 = cpu_to_le32(upper_32_bits(addr));
> }
> 
> so buffer2 stays armed at DMA address 0 while RBSZ for a ZC queue is
> xsk_pool_get_rx_frame_size() and GPSL is the jumbo limit.  Is the SMMU
> translation fault quoted in the commit message still reachable on this
> path?  The multi-descriptor rejection in stmmac_rx_zc() happens after the
> DMA write.
> 
> > @@ -5160,7 +5154,7 @@ static unsigned int stmmac_rx_buf1_len(struct stmmac_priv *priv,
> >  	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);
> > +	return min_t(unsigned int, priv->dma_conf.dma_buf_sz, plen - len);
> >  }
> 
> [Severity: Low]
> This isn't a bug, but the descriptor-position comments here no longer
> match the code.  The non-first-descriptor early return at the top of
> stmmac_rx_buf1_len() is gated on priv->sph_active, so with non-SPH
> scatter-gather a continuation descriptor (len != 0) falls through to the
> branches commented "First descriptor, not last descriptor and not split
> header" and "First descriptor and last descriptor and not split header",
> while plen - len only makes sense for a continuation descriptor.
> 
> > @@ -5170,10 +5164,6 @@ 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)
> > -		return 0;
> > -
> 
> [Severity: Critical]
> With this return gone, the last-descriptor path returns the exact
> remainder plen - len, which can be 1, 2 or 3.  stmmac_rx() then strips the
> FCS with an unchecked unsigned subtraction:
> 
> 	/* 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;
> 		}
> 	}
> 
> and passes the result straight on:
> 
> 	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);
> 
> Can a frame that overflows a buffer boundary by 1..3 bytes wrap this?
> GMAC_CORE_INIT in dwmac4.h sets GMAC_CONFIG_JE unconditionally, so with
> MTU 1500 and dma_buf_sz = DEFAULT_BUFSIZE = 1536 a 1537-byte frame is
> accepted and split as 1536 + 1.  On that first-and-last descriptor
> stmmac_rx_buf1_len() returns min(1536, 1537 - 0) = 1536, then
> stmmac_rx_buf2_len() returns 1537 - 1536 = 1, and 1 - 4 gives 0xfffffffd
> being handed to dma_sync_single_for_cpu() and skb_add_rx_frag().  The
> buf1_len branch is reachable the same way with frames of
> 2 * n * dma_buf_sz + 1..3 bytes, and stmmac_rx_zc() also subtracts
> ETH_FCS_LEN from stmmac_rx_buf1_len() unconditionally.  Before this patch
> the non-SPH path returned 0 here, and in SPH mode buffer1 ends at the
> split header so the whole FCS always landed in buffer2.
> 
> >  	/* For GMAC4, when split header is enabled, in some rare cases, the
> >  	 * hardware does not fill buf2 of the first descriptor with payload.
> >  	 * Thus we cannot assume buf2 is always fully filled if it is not
> > @@ -5188,8 +5178,9 @@ 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 */
> 
> [Severity: Medium]
> For GMAC4 without SPH this now assumes every intermediate descriptor's
> buffer2 is completely filled, discarding the accumulated-PL value that the
> comment just above describes as always giving the correct length of buf2.
> 
> The fall-through has no len <= plen guard:
> 
> 	plen = stmmac_get_rx_frame_len(priv, p, coe);
> 
> 	return plen - len;
> 
> and stmmac_rx_buf1_len() clamps the same wrapped value with
> min_t(unsigned int, dma_buf_sz, plen - len) rather than detecting the
> inconsistency.  One way the assumption can break is the RBSZ field itself:
> DMA_RBSZ_MASK is GENMASK(14, 1) in dwmac4_dma.h and cannot represent
> BUF_SIZE_16KiB, so dwmac4_set_dma_bfsize() writes a truncated RBSZ while
> the driver keeps assuming dma_buf_sz per buffer.  If an intermediate
> buffer2 is short by even one byte, does the accumulated len exceed plen and
> turn buf2_len into a multi-gigabyte fragment length?  Would a
> plen > len ? plen - len : 0 form be safer here?
> 
> The commit message says the GMAC4 plus split-header path keeps the
> accumulated payload semantics "since there an intermediate descriptor's
> buffer2 can be only partially filled" - is there a databook statement that
> this cannot happen for GMAC4 outside SPH mode?
> 
> [Severity: Medium]
> Both helpers read priv->hw->rx_csum independently for the same descriptor
> (int coe = priv->hw->rx_csum;) and feed it to stmmac_get_rx_frame_len(),
> whose COE_TYPE1 path subtracts 2 from the descriptor frame length.  The
> writer runs from process context with no synchronization against the RX
> NAPI poll:
> 
> drivers/net/ethernet/stmicro/stmmac/stmmac_main.c:stmmac_set_features() {
> 	if (features & NETIF_F_RXCSUM)
> 		priv->hw->rx_csum = priv->plat->rx_coe;
> 	else
> 		priv->hw->rx_csum = 0;
> 	...
> 	stmmac_rx_ipc(priv, priv->hw);
> }
> 
> If ethtool -K ethX rx off/on lands between the stmmac_rx_buf1_len() and
> stmmac_rx_buf2_len() reads on type-1-COE hardware, plen in the second call
> is 2 bytes smaller than the one used to accumulate len, so plen - len
> wraps.  Before this patch the non-SPH path returned 0 here and buf1_len did
> no subtraction, so the worst case was a 2-byte length inaccuracy.  Could
> coe/plen be read once per descriptor?
> 
> [Severity: High]
> This isn't a bug introduced by this patch, but while looking at the RX
> buffer lifecycle the patch reworks: in stmmac_rx() the napi_build_skb()
> failure path releases 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 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 descriptor:
> 
> 	if (!buf->page) {
> 		buf->page = page_pool_alloc_pages(rx_q->page_pool, gfp);
> 
> and at teardown stmmac_free_rx_buffer() puts it a second time:
> 
> 	page_pool_put_full_page(rx_q->page_pool, buf->page, false);
> 	buf->page = NULL;
> 
> Can this alias one page into two descriptors and underflow the page-pool
> refcount?
> 
> -- 
> Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260916-stmmac-rx-sg-fix-v1-1-b49b7b8f725f%40oss.qualcomm.com

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