Re: [PATCH net 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: 6923 bytes --]

> Thank you for your contribution! Sashiko AI review found more severe
> issue(s) in other patches of this series. This mail is only an FYI, in case
> you want to address the nit picks below when you respin.
> 
> Low priority / nit picks for your consideration:
> 
> - [Low] In stmmac_rx_vlan()
>   (drivers/net/ethernet/stmicro/stmmac/stmmac_main.c), the patch adds an…
> 
> Pre-existing issues:
> - [Medium] Only one of the two stmmac RX paths that call stmmac_rx_vlan()
>   records XDP metadata in the skb.
> - [Medium] The new VLAN-error drop branch in stmmac_dispatch_skb_zc()
>   does a plain `priv->xstats.rx_dropped++` (line 5567).
> 
> --
> 
> net: stmmac: fix VLAN tag stripping on short and non-linear RX frames
> 
> This patch changes stmmac_rx_vlan() so that it pulls ETH_HLEN before
> reading h_vlan_proto, and pulls VLAN_ETH_HLEN before reading the TCI. It
> returns an error so callers can drop frames that claim a VLAN tag but lack
> the full header. It also moves XDP metadata together with the MAC
> addresses when the tag is popped.
> 
> > diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > index 8a2faf50d6957..32b2c5834c340 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,
> >  	return vlan_features_check(skb, features);
> >  }
> >  
> > -static void stmmac_rx_vlan(struct net_device *dev, struct sk_buff *skb)
> > +static int stmmac_rx_vlan(struct net_device *dev, struct sk_buff *skb)
> >  {
> > -	struct vlan_ethhdr *veth = skb_vlan_eth_hdr(skb);
> > -	__be16 vlan_proto = veth->h_vlan_proto;
> > -	u16 vlanid;
> > +	struct vlan_ethhdr *veth;
> >  
> > -	if ((vlan_proto == htons(ETH_P_8021Q) &&
> > +	if (!pskb_may_pull(skb, ETH_HLEN))
> > +		return -EINVAL;
> 
> [Severity: Low]
> This pull happens before the h_vlan_proto and dev->features checks. A frame
> shorter than ETH_HLEN is now freed and counted in rx_dropped by both
> stmmac_rx() and stmmac_dispatch_skb_zc(), even with CTAG/STAG RX stripping
> disabled. The hw_vlan_en branch in those callers has no matching check.
> 
> Is that intended? The commit message says:
> 
>     untagged frames, including short ones, are left untouched.
> 
> That holds only for frames between ETH_HLEN and VLAN_ETH_HLEN in length.
> 
> XDP doesn't seem able to reach this case, because bpf_xdp_adjust_head()
> and bpf_xdp_adjust_tail() both refuse to go below ETH_HLEN. Dropping such
> a runt is also arguably safer than handing it to eth_type_trans().
> 
> Could the commit message mention that frames shorter than ETH_HLEN are now
> dropped?

ack, I will fix the commit message in v2.

> 
> > +
> > +	veth = skb_vlan_eth_hdr(skb);
> > +	if ((veth->h_vlan_proto == htons(ETH_P_8021Q) &&
> >  	     dev->features & NETIF_F_HW_VLAN_CTAG_RX) ||
> > -	    (vlan_proto == htons(ETH_P_8021AD) &&
> > +	    (veth->h_vlan_proto == htons(ETH_P_8021AD) &&
> >  	     dev->features & NETIF_F_HW_VLAN_STAG_RX)) {
> > +		__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]
> This isn't a bug introduced by this patch, but skb_metadata_len() can only
> be non-zero for skbs built by stmmac_construct_skb_zc() on the AF_XDP
> zero-copy path:

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

> 
> stmmac_construct_skb_zc() {
>     ...
> 	memcpy(skb->data - metasize, xdp->data_meta, datasize + metasize);
> 	if (metasize)
> 		skb_metadata_set(skb, metasize);
>     ...
> }
> 
> On the page_pool path, stmmac_rx() calls xdp_prepare_buff() with
> meta_valid set to true, so bpf_xdp_adjust_meta() succeeds there. The
> XDP_PASS skb is then built without recording the metadata length:
> 
> stmmac_rx() {
>     ...
> 			/* XDP program may adjust header */
> 			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);
>     ...
> }
> 
> So skb_shinfo(skb)->meta_len stays 0 for these skbs. Doesn't that mean
> metadata written by a non-ZC XDP program is lost before tc BPF can see it?
> It would also mean this memmove only moves metadata for zero-copy skbs.
> 
> Should stmmac_rx() also call skb_metadata_set() with
> ctx.xdp.data - ctx.xdp.data_meta?
> 
> This patch's own claim still holds on that path, since meta_len is 0
> there and data_meta cannot end up pointing at the wrong bytes.
> 
> >  		skb_pull(skb, VLAN_HLEN);
> >  		__vlan_hwaccel_put_tag(skb, vlan_proto, vlanid);
> >  	}
> > +
> > +	return 0;
> >  }
> 
> [ ... ]
> 
> > @@ -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: Medium]
> This is a pre-existing issue, but priv->xstats.rx_dropped is one
> device-wide unsigned long in struct stmmac_extra_stats. Every channel's
> NAPI poll updates it with a plain read-modify-write:

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

Regards,
Lorenzo

> 
> stmmac_dispatch_skb_zc()
>     priv->xstats.rx_dropped++;
> 
> stmmac_rx_zc() and stmmac_rx()
>     priv->xstats.rx_dropped += rx_dropped;
> 
> stmmac_napi_add() registers a separate rx_napi/rxtx_napi for each channel,
> so these polls can run at the same time on different CPUs. No lock, atomic
> or u64_stats_sync protects the counter.
> 
> Can drop increments be lost this way? stmmac_get_stats64() also reads the
> field without READ_ONCE().
> 
> This patch only adds one more site that follows the existing pattern.
> 
> > +		return;
> > +	}
> > +
> >  	skb->protocol = eth_type_trans(skb, priv->dev);
> 
> [ ... ]
> 
> -- 
> Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261023-stmmac-xdp-fixes-v1-0-efb37e19f5bd%40oss.qualcomm.com

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