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

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

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