Re: [PATCH v3 net] vlan: fix skb_under_panic and races when toggling HW VLAN offload

[Paolo Abeni <pabeni@redhat.com>]

On 8/11/26 10:52 AM, Eric Dumazet wrote:
> Toggling hardware VLAN TX offload (NETIF_F_HW_VLAN_CTAG_TX or
> NETIF_F_HW_VLAN_STAG_TX) on a lower device invokes vlan_transfer_features(),
> which dynamically changed vlandev->hard_header_len.
> 
> This causes two issues:
> 1. Lockless TX paths (e.g. packet_snd in af_packet.c, ip6_finish_output2)
>    read dev->hard_header_len without holding RTNL lock. Mutating
>    hard_header_len dynamically under RTNL creates a data race where upper
>    layers reserve insufficient headroom based on a stale hard_header_len,
>    resulting in skb_under_panic when vlan_dev_hard_header() is called.
> 2. In addition, vlan_transfer_features() updated hard_header_len without
>    updating header_ops, causing a mismatch between allocated headroom
>    and header creation.
> 
> Always setting dev->hard_header_len = real_dev->hard_header_len and
> dev->needed_headroom = real_dev->needed_headroom + VLAN_HLEN unconditionally
> ensures:
> - dev->hard_header_len remains 100% static and immutable at real_dev->hard_header_len,
>   eliminating all dynamic runtime updates and data races on hard_header_len.
> - Upper layers allocating skbs via LL_RESERVED_SPACE() will always reserve
>   sufficient headroom for software VLAN tag insertion (real_dev->hard_header_len +
>   real_dev->needed_headroom + VLAN_HLEN).
> - vlandev inherits real_dev->needed_tailroom so underlying trailer/padding/ICV
>   requirements are honored.
> - AF_PACKET SOCK_RAW network header offsets remain correctly aligned at
>   real_dev->hard_header_len.
> - vlan_header_ops is used unconditionally.
> 
> Note to stable teams: Make sure to backport these commits:
> 
> e16e960d55a4 ("ipvlan: inherit needed_headroom and needed_tailroom from phy_dev")
> cef51860becd ("macvlan: inherit needed_headroom and needed_tailroom from lowerdev")
> 
> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> Reported-by: Tangxin Xie <xietangxin@h-partners.com>
> Closes: https://lore.kernel.org/netdev/99d678ae-c7b2-4b44-b534-b8320679deb3@h-partners.com/
> Cc: <stable@vger.kernel.org> # 3.19: e16e960d55a4: ipvlan: inherit needed_headroom and needed_tailroom from phy_dev
> Cc: <stable@vger.kernel.org> # 3.19: cef51860becd: macvlan: inherit needed_headroom and needed_tailroom from lowerdev
> Cc: <stable@vger.kernel.org> # 3.19
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> ---
> v3: propagate needed_tailroom, add stable tags.
> v2: https://lore.kernel.org/netdev/20260724035419.1473174-1-edumazet@google.com/
> 
>  net/8021q/vlan_dev.c | 39 +++++++--------------------------------
>  1 file changed, 7 insertions(+), 32 deletions(-)
> 
> diff --git a/net/8021q/vlan_dev.c b/net/8021q/vlan_dev.c
> index ec2569b3f8dac629027b4344bc89402decf026d1..2859cbac3f266b7c4e3f44f41280d33ab69c5270 100644
> --- a/net/8021q/vlan_dev.c
> +++ b/net/8021q/vlan_dev.c
> @@ -502,26 +502,6 @@ static const struct header_ops vlan_header_ops = {
>  	.parse_protocol = vlan_parse_protocol,
>  };
>  
> -static int vlan_passthru_hard_header(struct sk_buff *skb, struct net_device *dev,
> -				     unsigned short type,
> -				     const void *daddr, const void *saddr,
> -				     unsigned int len)
> -{
> -	struct vlan_dev_priv *vlan = vlan_dev_priv(dev);
> -	struct net_device *real_dev = vlan->real_dev;
> -
> -	if (saddr == NULL)
> -		saddr = dev->dev_addr;
> -
> -	return dev_hard_header(skb, real_dev, type, daddr, saddr, len);
> -}
> -
> -static const struct header_ops vlan_passthru_header_ops = {
> -	.create	 = vlan_passthru_hard_header,
> -	.parse	 = eth_header_parse,
> -	.parse_protocol = vlan_parse_protocol,
> -};
> -
>  static const struct device_type vlan_type = {
>  	.name	= "vlan",
>  };
> @@ -580,14 +560,10 @@ static int vlan_dev_init(struct net_device *dev)
>  	dev->fcoe_ddp_xid = real_dev->fcoe_ddp_xid;
>  #endif
>  
> -	dev->needed_headroom = real_dev->needed_headroom;
> -	if (vlan_hw_offload_capable(real_dev->features, vlan->vlan_proto)) {
> -		dev->header_ops      = &vlan_passthru_header_ops;
> -		dev->hard_header_len = real_dev->hard_header_len;
> -	} else {
> -		dev->header_ops      = &vlan_header_ops;
> -		dev->hard_header_len = real_dev->hard_header_len + VLAN_HLEN;

Sashiko nipa points to a possible functional and performance regression
with unusual config (reorder_hdr off):

https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260811085246.2267779-1-edumazet%40google.com

Arguably the current behavior is incorrect, but it's also the
established one. WDYT?

/P