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

[sashiko-bot@kernel.org]

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