Re: [PATCH net-next v8 4/5] net: stmmac: fix VLAN tag stripping on non-linear RX frames

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] net: stmmac: unconditional VLAN_ETH_HLEN pull in stmmac_rx_vlan()
--

commit 0a2d776ad3e3751ad20c9dd1144b0c000e67acc1
Author: Lorenzo Bianconi <lorenzo.bianconi@oss.qualcomm.com>

net: stmmac: fix VLAN tag stripping on non-linear RX frames

This commit updates stmmac_rx_vlan() to linearize the VLAN header with
pskb_may_pull(), preserve XDP metadata when popping the VLAN tag, and drop
the frame in stmmac_rx() and stmmac_dispatch_skb_zc() if pulling fails.

> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> index e929cc0f23125..04dc6dcaa435d 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> @@ -5095,22 +5095,30 @@ 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, VLAN_ETH_HLEN))
> +		return -EINVAL;
> +
> +	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)) {

[Severity: Medium]
Can calling pskb_may_pull(skb, VLAN_ETH_HLEN) unconditionally at the start
of stmmac_rx_vlan() cause valid untagged frames to be dropped, or force
unnecessary fragment pulls on non-linear untagged packets?

When priv->hw->hw_vlan_en is false, stmmac_rx() and stmmac_dispatch_skb_zc()
call stmmac_rx_vlan() on every received frame, even when VLAN RX stripping
offloads are disabled in dev->features or the packet is untagged.

XDP helpers such as bpf_xdp_adjust_head(), bpf_xdp_adjust_tail(), and
bpf_xdp_frags_shrink_tail() allow shrinking a frame down to ETH_HLEN (14
bytes):

net/core/filter.c:bpf_xdp_adjust_head() {
	...
	if (unlikely(data < data_start ||
		     data > xdp->data_end - ETH_HLEN))
		return -EINVAL;
	...
}

For an untagged frame of 14 to 17 bytes (where ETH_HLEN <= skb->len <
VLAN_ETH_HLEN), pskb_may_pull(skb, VLAN_ETH_HLEN) fails and returns -EINVAL,
causing both stmmac_rx() and stmmac_dispatch_skb_zc() to free and drop the
skb via dev_kfree_skb_any(skb).

Additionally, for non-linear untagged frames with skb->len >= VLAN_ETH_HLEN
whose linear head is ETH_HLEN bytes (for example, after bpf_xdp_adjust_head()
with BPF_F_XDP_HAS_FRAGS, or with GMAC4 split header mode splitting after the
14-byte L2 header), pulling VLAN_ETH_HLEN before checking h_vlan_proto forces
__pskb_pull_tail() on every untagged packet.

Since veth->h_vlan_proto sits within the first ETH_HLEN bytes of the header,
should stmmac_rx_vlan() only pull ETH_HLEN initially (or check dev->features
first), and defer pskb_may_pull(skb, VLAN_ETH_HLEN) and reloading veth until
after confirming that veth->h_vlan_proto matches a VLAN ethertype to be
stripped?

> +		__be16 vlan_proto = veth->h_vlan_proto;
> +		u16 vlanid = ntohs(veth->h_vlan_TCI);
> +		u8 meta_len = skb_metadata_len(skb);
> +
>  		/* pop the vlan tag */
> -		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);
>  		skb_pull(skb, VLAN_HLEN);
>  		__vlan_hwaccel_put_tag(skb, vlan_proto, vlanid);
>  	}
> +
> +	return 0;
>  }

[ ... ]

> @@ -5530,12 +5538,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++;
> +		return;
> +	}
> +
>  	skb->protocol = eth_type_trans(skb, priv->dev);
>  
>  	if (unlikely(!coe) || !stmmac_has_ip_ethertype(skb))
> @@ -6041,12 +6052,14 @@ static int stmmac_rx(struct stmmac_priv *priv, int limit, 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);
> +			rx_dropped++;
> +			goto next;
> +		}
>  
>  		skb->protocol = eth_type_trans(skb, priv->dev);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261018-stmmac-rx-mb-v8-0-5f58c0b25849@oss.qualcomm.com?part=4