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

[netdev-bot+sashiko@kernel.org]

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?

> +
> +	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:

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:

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