Re: [PATCH net v2 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 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.

>  		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?

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