Re: [PATCH net v2 5/6] net: stmmac: fix VLAN tag stripping on short and non-linear RX frames

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

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

> Thank you for your contribution! Sashiko AI review found 1 potential
> issue(s) to consider.
> 
> Critical: 0 · High: 0 · Medium: 1 · Low: 0 (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).
> 
> - [Medium] The commit message says the VLAN-pop code now moves the XDP
>   metadata written with bpf_xdp_adjust_meta() along with the MAC header,…
> 
> Pre-existing issues:
> - [Low] priv->xstats.rx_dropped is a single device-wide unsigned long
>   (struct stmmac_extra_stats, common.h:240).
> 
> --
> 
> net: stmmac: fix VLAN tag stripping on short and non-linear RX frames
> 
> stmmac_rx_vlan() now pulls ETH_HLEN, and then VLAN_ETH_HLEN, into the
> linear area before it reads the VLAN header. It returns an error so that
> stmmac_rx() and stmmac_dispatch_skb_zc() drop malformed frames. When the
> tag is popped, it also moves any XDP metadata together with the MAC
> addresses.
> 
> > diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > index d9fc2464a160a..fb3b7174599e0 100644
> > --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > @@ -5102,22 +5102,35 @@ static netdev_features_t stmmac_features_check(struct sk_buff *skb,
> 
> [ ... ]
> 
> > +		__be16 vlan_proto = veth->h_vlan_proto;
> > +		u8 meta_len = skb_metadata_len(skb);
> > +		u16 vlanid;
> > +
> >  		/* pop the vlan tag */
> > +		if (!pskb_may_pull(skb, VLAN_ETH_HLEN))
> > +			return -EINVAL;
> > +
> > +		veth = skb_vlan_eth_hdr(skb);
> >  		vlanid = ntohs(veth->h_vlan_TCI);
> > -		memmove(skb->data + VLAN_HLEN, veth, ETH_ALEN * 2);
> > +		memmove(skb->data - meta_len + VLAN_HLEN,
> > +			skb->data - meta_len, meta_len + ETH_ALEN * 2);
> 
> [Severity: Medium]
> The commit message says:
> 
>   Also move the XDP metadata written with bpf_xdp_adjust_meta() together
>   with the MAC header when the tag is popped, so data_meta still points at
>   the metadata after eth_type_trans() anchors mac_header
> 
> Does that hold for frames coming from stmmac_rx(), or only for the AF_XDP
> zero-copy path?
> 
> On the page_pool path, the XDP buffer is set up with meta_valid set to
> true. However, the XDP_PASS skb is built without calling
> skb_metadata_set():
> 
> stmmac_rx() {
>     ...
> 	xdp_prepare_buff(&ctx.xdp, page_address(buf->page),
> 			 buf->page_offset, buf1_len, true);
>     ...
> 	head_pad_len = ctx.xdp.data - ctx.xdp.data_hard_start;
> 	skb_reserve(skb, head_pad_len);
> 	skb_put(skb, buf1_len);
> 	skb_mark_for_recycle(skb);
>     ...
> }
> 
> napi_build_skb() clears skb_shared_info up to dataref, and meta_len sits
> before dataref. So skb_metadata_len(skb) is always 0 here.
> 
> On that path, this memmove() does the same MAC-address-only move as
> before. The metadata written by the XDP program stays where it was, and
> TC/BPF sees an empty data_meta range.
> 
> The only skb_metadata_set() call in the driver appears to be in
> stmmac_construct_skb_zc():
> 
> 	if (metasize)
> 		skb_metadata_set(skb, metasize);
> 
> The last patch of the series does not seem to add another one either.
> 
> The missing skb_metadata_set() in stmmac_rx() predates this patch. Still,
> the Fixes: tag points at 5fabb01207a2 ("net: stmmac: Add initial XDP
> support"), which is the page_pool path.
> 
> Would it make sense to add something like
> skb_metadata_set(skb, ctx.xdp.data - ctx.xdp.data_meta) in stmmac_rx()?
> Alternatively, the commit message could say that the metadata handling
> only applies to the zero-copy path.

I agree this is real issue and it needs to be fixed, but it has not been
introduced by this patch and it needs to be fixed with a dedicated patch.

> 
> >  		skb_pull(skb, VLAN_HLEN);
> >  		__vlan_hwaccel_put_tag(skb, vlan_proto, vlanid);
> >  	}
> 
> [ ... ]
> 
> > @@ -5546,12 +5559,15 @@ static void stmmac_dispatch_skb_zc(struct stmmac_priv *priv, u32 queue,
> >  	}
> >  
> >  	stmmac_get_rx_hwtstamp(priv, p, np, skb);
> > -	if (priv->hw->hw_vlan_en)
> > +	if (priv->hw->hw_vlan_en) {
> >  		/* MAC level stripping. */
> >  		stmmac_rx_hw_vlan(priv, priv->hw, p, skb);
> > -	else
> > -		/* Driver level stripping. */
> > -		stmmac_rx_vlan(priv->dev, skb);
> > +	} else if (stmmac_rx_vlan(priv->dev, skb)) {
> > +		dev_kfree_skb_any(skb);
> > +		priv->xstats.rx_dropped++;
> 
> [Severity: Low]
> This is a pre-existing issue, not one introduced by this patch. Can
> updates to priv->xstats.rx_dropped get lost here?

This issue has not been introduced by this patch and it needs to be fixed in a
dedicated patch.

Regards,
Lorenzo

> 
> The counter is a single unsigned long in struct stmmac_extra_stats, shared
> by all queues. Several per-queue NAPI contexts update it with a plain
> read-modify-write:
> 
> stmmac_dispatch_skb_zc():
> 	priv->xstats.rx_dropped++;
> 
> stmmac_rx_zc():
> 	priv->xstats.rx_dropped += rx_dropped;
> 
> stmmac_rx():
> 	priv->xstats.rx_dropped += rx_dropped;
> 
> With per-channel MSI or threaded NAPI, polls for different queues can run
> on different CPUs at the same time. stmmac_get_stats64() then reads the
> counter without any locking, so the reported rx_dropped value can be too
> low.
> 
> This patch adds another writer that follows the existing pattern, and the
> new VLAN drops in stmmac_rx() go through the same shared update.
> 
> > +		return;
> > +	}
> > +
> >  	skb->protocol = eth_type_trans(skb, priv->dev);
> 
> [ ... ]
> 
> -- 
> Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261027-stmmac-xdp-fixes-v2-0-10eae8a093a8%40oss.qualcomm.com

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