[PATCH linux next v2] net: stmmac: remove software VLAN tag stripping

[Maxime Chevallier]

Hi,

On 9/1/26 04:28, he.peilin at zte.com.cn wrote:
> Problem
> =======
> When the stmmac driver has NETIF_F_HW_VLAN_STAG_RX enabled by default,
> but the hardware does not support hardware stripping of ETH_P_8021AD
> (VLAN 802.1ad) tags, the driver falls back to software stripping in
> stmmac_rx_vlan(). If the received VLAN packet is fragmented and the
> VLAN header resides in the non-linear part of the skb, the driver
> attempts to pull the header without first ensuring it is linearized.
> This leads to a kernel BUG in __skb_pull() due to invalid header
> access.
> 
> Crash log
> =========
> [   72.212903] kernel BUG at include/linux/skbuff.h:2700!
> [   72.212908] Kernel BUG [#1]
> ..
> [   72.212958] [<ffffffff846919d2>] eth_type_trans+0xe2/0x168
> [   72.212962] [<ffffffff844672e2>] stmmac_rx+0x602/0xc58
> [   72.212966] [<ffffffff84467984>] stmmac_napi_poll_rx+0x4c/0xb8
> [   72.212970] [<ffffffff84635246>] __napi_poll+0x2e/0x1e0
> [   72.212975] [<ffffffff8463596e>] net_rx_action+0x31e/0x388
> [   72.212979] [<ffffffff83c329b0>] handle_softirqs+0x170/0x358
> [   72.212983] [<ffffffff83c32cc6>] __irq_exit_rcu+0xd6/0x100
> [   72.212986] [<ffffffff83c32ee0>] irq_exit_rcu+0x18/0x28
> [   72.212989] [<ffffffff84895b1e>] handle_riscv_irq+0x66/0x78
> [   72.212994] [<ffffffff84896728>] do_irq+0x60/0xa0
> 
> Root cause
> ==========
> In the software VLAN stripping path, stmmac_rx_vlan() does not call
> pskb_may_pull() to ensure that the Ethernet header plus VLAN header
> are in the linear area. As a result, __skb_pull() operates on an
> skb with insufficient linear data, triggering the BUG check.
> 
> Solution
> ========
> The software VLAN stripping logic in stmmac_rx_vlan() was originally
> introduced in 2014 by commit b93819854d6e ("stmmac: Add vlan rx for
> better GRO performance.") as a workaround to improve GRO performance,
> since at that time GRO could not handle frames with VLAN tags. However,
> this limitation was resolved in 2015 by commit 66e5133f19e9 ("vlan: Add
> GRO support for non hardware accelerated vlan"), which added GRO support
> for non-hardware-accelerated VLAN frames. Keeping a software fallback
> path for VLAN stripping is no longer necessary and only adds complexity.

I'm not convinced this is the proper explanation to use here. How did you
trigger it in the first place ? Just getting rid of unnecessary code
is good enough of an explanation :)

> 
> Rather than fixing the issue by adding pskb_may_pull() checks to
> stmmac_rx_vlan(), remove the function entirely. Additionally, ensure
> that hardware VLAN stripping features are only reported via
> dev->hw_features when the hardware truly supports them.
> 
> Fixes: b93819854d6e ("stmmac: Add vlan rx for better GRO performance.")
> Signed-off-by: Peilin He <he.peilin at zte.com.cn>
> Reviewed-by: xu xin <xu.xin16 at zte.com.cn>
> Reviewed-by: Jiang Kun <jiang.kun2 at zte.com.cn>
> ---
>  .../net/ethernet/stmicro/stmmac/stmmac_main.c | 27 ++-----------------
>  1 file changed, 2 insertions(+), 25 deletions(-)
> 
> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> index b2b7d0242dd3..790b7362048e 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> @@ -5021,24 +5021,6 @@ 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)
> -{
> -	struct vlan_ethhdr *veth = skb_vlan_eth_hdr(skb);
> -	__be16 vlan_proto = veth->h_vlan_proto;
> -	u16 vlanid;
> -
> -	if ((vlan_proto == htons(ETH_P_8021Q) &&
> -	     dev->features & NETIF_F_HW_VLAN_CTAG_RX) ||
> -	    (vlan_proto == htons(ETH_P_8021AD) &&
> -	     dev->features & NETIF_F_HW_VLAN_STAG_RX)) {
> -		/* pop the vlan tag */
> -		vlanid = ntohs(veth->h_vlan_TCI);
> -		memmove(skb->data + VLAN_HLEN, veth, ETH_ALEN * 2);
> -		skb_pull(skb, VLAN_HLEN);
> -		__vlan_hwaccel_put_tag(skb, vlan_proto, vlanid);
> -	}
> -}
> -
>  /**
>   * stmmac_rx_refill - refill used skb preallocated buffers
>   * @priv: driver private structure
> @@ -5407,9 +5389,7 @@ static void stmmac_dispatch_skb_zc(struct stmmac_priv *priv, u32 queue,
>  	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);
> +
>  	skb->protocol = eth_type_trans(skb, priv->dev);
> 
>  	if (unlikely(!coe) || !stmmac_has_ip_ethertype(skb))
> @@ -5901,9 +5881,6 @@ static int stmmac_rx(struct stmmac_priv *priv, int limit, u32 queue)
>  		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);
> 
>  		skb->protocol = eth_type_trans(skb, priv->dev);
> 
> @@ -7965,7 +7942,7 @@ static int __stmmac_dvr_probe(struct device *device,
>  	ndev->watchdog_timeo = msecs_to_jiffies(watchdog);
>  #ifdef STMMAC_VLAN_TAG_USED
>  	/* Both mac100 and gmac support receive VLAN tag detection */
> -	ndev->features |= NETIF_F_HW_VLAN_CTAG_RX | NETIF_F_HW_VLAN_STAG_RX;
> +	ndev->features |= NETIF_F_HW_VLAN_CTAG_RX;
>  	if (dwmac_is_xmac(priv->plat->core_type)) {
>  		ndev->hw_features |= NETIF_F_HW_VLAN_CTAG_RX;
>  		priv->hw->hw_vlan_en = true;

Now that we don't have software fallback, tag stripping is only going to work
on GMAC4 and later, so we mustn't unconditionally set the NETIF_F_HW_VLAN_CTAG_RX
flag anymore. It should be something like :

// No more dev->features |= NETIF_F_HW_VLAN_CTAG_RX;

if (dwmac_is_xgmac(priv->plat->core_type)) {
	ndev->features |= NETIF_F_HW_VLAN_CTAG_RX;
	ndev->hw_features |= NETIF_F_HW_VLAN_CTAG_RX;
	priv->hw->hw_vlan_en = true;
}

Maxime