[PATCH v2 net] vlan: do not insert the vlan header in vlan_dev_hard_header()

[PATCH v2 net] vlan: do not insert the vlan header in vlan_dev_hard_header()

[Eric Dumazet]

Commit 447cbe95ebb9 ("vlan: fix skb_under_panic and races when toggling
HW VLAN offload") replaced vlan_passthru_header_ops with vlan_header_ops
unconditionally to avoid lockless data races when modifying
hard_header_len dynamically under RTNL.

Prior to that commit, vlan devices on top of a real device capable of
HW VLAN TX offload used vlan_passthru_header_ops, which never inserted
the 802.1Q header in the frame. vlan_header_ops was only used when the
real device had no such offload.

However, vlan_dev_hard_header() retained its legacy path:

	if (!(vlan->flags & VLAN_FLAG_REORDER_HDR)) {
		...
		vhdr = skb_push(skb, VLAN_HLEN);
		...
	}

With vlan_header_ops used unconditionally, creating a VLAN with
reorder_hdr=off (as done by Home Assistant OS / NetworkManager when
flags are omitted over D-Bus) causes vlan_dev_hard_header() to push
an in-band 802.1Q header in software, even if the real device supports
HW VLAN TX offload.

This is a regression for r8169 users (Realtek RTL8168h), who report
transmit queue timeouts (NETDEV WATCHDOG): this NIC stalls when asked
to offload checksum and/or TSO for frames carrying an in-band tag.

Reverting the blamed commit, disabling TX checksum offload (which
also disables TSO), or setting reorder_hdr=on were all reported to
work around the issue.

This insertion is also inconsistent with dev->hard_header_len, which
does not account for VLAN_HLEN.

The in-band insertion is not needed: validate_xmit_vlan() inserts the
tag in software right before ndo_start_xmit() when the real device
lacks HW VLAN TX offload.

Remove it, and make vlan_dev_hard_start_xmit() always set the hwaccel
tag. The (veth->h_vlan_proto != vlan->vlan_proto) test it used to skip
tagging with reorder_hdr=off only made sense together with the
insertion in vlan_dev_hard_header(). Without it, stacked VLANs (Q-in-Q)
and AF_PACKET SOCK_DGRAM frames using ETH_P_8021Q would lose our tag.

The TX path now behaves as it always did with reorder_hdr=on (the
default). VLAN_FLAG_REORDER_HDR only controls whether vlan_do_receive()
reinserts the tag on RX.

Note that AF_PACKET SOCK_RAW users of a reorder_hdr=off VLAN building
frames that already carry our tag will now send them double tagged,
as they would with reorder_hdr=on.

Fixes: 447cbe95ebb9 ("vlan: fix skb_under_panic and races when toggling HW VLAN offload")
Reported-by: sairon <sairon@users.noreply.github.com>
Reported-by: hitchin999 <hitchin999@users.noreply.github.com>
Reported-by: Stefan Agner <stefan@agner.ch>
Closes: https://github.com/home-assistant/operating-system/issues/5019
Closes: https://lore.kernel.org/netdev/CAPa5EdCj3v17tB-SF2JNecq5Q8s1Pranr-XzAFWNpNTBPgPG7w@mail.gmail.com/
Assisted-by: LLM
Signed-off-by: Eric Dumazet <edumazet@kernel.org>
Cc: Heiner Kallweit <hkallweit1@gmail.com>
---
v2: Always set the hwaccel tag in vlan_dev_hard_start_xmit(): with the
    (veth->h_vlan_proto != vlan->vlan_proto) test kept, the outer tag was
    lost for Q-in-Q and SOCK_DGRAM ETH_P_8021Q with reorder_hdr=off
    (reported by sashiko).
v1: https://lore.kernel.org/netdev/20261007073027.459868-1-edumazet@kernel.org/

 net/8021q/vlan_dev.c | 60 ++++++++------------------------------------
 1 file changed, 11 insertions(+), 49 deletions(-)

diff --git a/net/8021q/vlan_dev.c b/net/8021q/vlan_dev.c
index c949c6a829456c2f75d6c514b35a85ff64c493d8..bd98c191a66f96acc958f52e6fae2337b410ef3b 100644
--- a/net/8021q/vlan_dev.c
+++ b/net/8021q/vlan_dev.c
@@ -35,11 +35,15 @@
 #include <linux/netpoll.h>
 
 /*
- *	Create the VLAN header for an arbitrary protocol layer
+ *	Create the hard header for an arbitrary protocol layer
  *
  *	saddr=NULL	means use device source address
  *	daddr=NULL	means leave destination address (eg unresolved arp)
  *
+ *  The VLAN tag is not inserted here: vlan_dev_hard_start_xmit() sets
+ *  the hwaccel tag, and the core inserts it in software if the real
+ *  device can not.
+ *
  *  This is called when the SKB is moving down the stack towards the
  *  physical devices.
  */
@@ -49,47 +53,14 @@ static int vlan_dev_hard_header(struct sk_buff *skb, struct net_device *dev,
 				unsigned int len)
 {
 	struct vlan_dev_priv *vlan = vlan_dev_priv(dev);
-	struct vlan_hdr *vhdr;
-	unsigned int vhdrlen = 0;
-	u16 vlan_tci = 0;
-	int rc;
-
-	if (!(vlan->flags & VLAN_FLAG_REORDER_HDR)) {
-		unsigned int hlen = READ_ONCE(dev->hard_header_len) +
-				    READ_ONCE(dev->needed_headroom);
-
-		if (skb_cow_head(skb, hlen) < 0)
-			return -ENOMEM;
-		vhdr = skb_push(skb, VLAN_HLEN);
-
-		vlan_tci = vlan->vlan_id;
-		vlan_tci |= vlan_dev_get_egress_qos_mask(dev, skb->priority);
-		vhdr->h_vlan_TCI = htons(vlan_tci);
-
-		/*
-		 *  Set the protocol type. For a packet of type ETH_P_802_3/2 we
-		 *  put the length in here instead.
-		 */
-		if (type != ETH_P_802_3 && type != ETH_P_802_2)
-			vhdr->h_vlan_encapsulated_proto = htons(type);
-		else
-			vhdr->h_vlan_encapsulated_proto = htons(len);
-
-		skb->protocol = vlan->vlan_proto;
-		type = ntohs(vlan->vlan_proto);
-		vhdrlen = VLAN_HLEN;
-	}
+	struct net_device *real_dev = vlan->real_dev;
 
 	/* Before delegating work to the lower layer, enter our MAC-address */
 	if (saddr == NULL)
 		saddr = dev->dev_addr;
 
 	/* Now make the underlying real hard header */
-	dev = vlan->real_dev;
-	rc = dev_hard_header(skb, dev, type, daddr, saddr, len + vhdrlen);
-	if (rc > 0)
-		rc += vhdrlen;
-	return rc;
+	return dev_hard_header(skb, real_dev, type, daddr, saddr, len);
 }
 
 static inline netdev_tx_t vlan_netpoll_send_skb(struct vlan_dev_priv *vlan, struct sk_buff *skb)
@@ -106,22 +77,13 @@ static netdev_tx_t vlan_dev_hard_start_xmit(struct sk_buff *skb,
 					    struct net_device *dev)
 {
 	struct vlan_dev_priv *vlan = vlan_dev_priv(dev);
-	struct vlan_ethhdr *veth = (struct vlan_ethhdr *)(skb->data);
 	unsigned int len;
+	u16 vlan_tci;
 	int ret;
 
-	/* Handle non-VLAN frames if they are sent to us, for example by DHCP.
-	 *
-	 * NOTE: THIS ASSUMES DIX ETHERNET, SPECIFICALLY NOT SUPPORTING
-	 * OTHER THINGS LIKE FDDI/TokenRing/802.3 SNAPs...
-	 */
-	if (vlan->flags & VLAN_FLAG_REORDER_HDR ||
-	    veth->h_vlan_proto != vlan->vlan_proto) {
-		u16 vlan_tci;
-		vlan_tci = vlan->vlan_id;
-		vlan_tci |= vlan_dev_get_egress_qos_mask(dev, skb->priority);
-		__vlan_hwaccel_put_tag(skb, vlan->vlan_proto, vlan_tci);
-	}
+	vlan_tci = vlan->vlan_id;
+	vlan_tci |= vlan_dev_get_egress_qos_mask(dev, skb->priority);
+	__vlan_hwaccel_put_tag(skb, vlan->vlan_proto, vlan_tci);
 
 	skb->dev = vlan->real_dev;
 	len = skb->len;
-- 
2.53.0

Re: [PATCH v2 net] vlan: do not insert the vlan header in vlan_dev_hard_header()

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 1 · Low: 0

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 patch changes what VLAN_FLAG_REORDER_HDR (reorder_hdr)
  does on TX.

--

vlan: do not insert the vlan header in vlan_dev_hard_header()

This patch removes the in-band 802.1Q insertion from
vlan_dev_hard_header() for reorder_hdr=off VLANs. It also makes
vlan_dev_hard_start_xmit() always set the hwaccel tag, and leaves software
insertion to validate_xmit_vlan(). The goal is to fix r8169 transmit
timeouts seen since 447cbe95ebb9.

Closes: https://github.com/home-assistant/operating-system/issues/5019
Closes: https://lore.kernel.org/netdev/CAPa5EdCj3v17tB-SF2JNecq5Q8s1Pranr-XzAFWNpNTBPgPG7w@mail.gmail.com/

> diff --git a/net/8021q/vlan_dev.c b/net/8021q/vlan_dev.c
> index c949c6a829456..bd98c191a66f9 100644
> --- a/net/8021q/vlan_dev.c
> +++ b/net/8021q/vlan_dev.c

[ ... ]

> @@ -49,47 +53,14 @@ static int vlan_dev_hard_header(struct sk_buff *skb, struct net_device *dev,
>  				unsigned int len)
>  {
>  	struct vlan_dev_priv *vlan = vlan_dev_priv(dev);
> -	struct vlan_hdr *vhdr;
> -	unsigned int vhdrlen = 0;
> -	u16 vlan_tci = 0;
> -	int rc;
> -
> -	if (!(vlan->flags & VLAN_FLAG_REORDER_HDR)) {

[ ... ]

> -		vhdr = skb_push(skb, VLAN_HLEN);

[ ... ]

> -		skb->protocol = vlan->vlan_proto;
> -		type = ntohs(vlan->vlan_proto);
> -		vhdrlen = VLAN_HLEN;
> -	}
> +	struct net_device *real_dev = vlan->real_dev;

[ ... ]

> @@ -106,22 +77,13 @@ static netdev_tx_t vlan_dev_hard_start_xmit(struct sk_buff *skb,
>  					    struct net_device *dev)
>  {
>  	struct vlan_dev_priv *vlan = vlan_dev_priv(dev);
> -	struct vlan_ethhdr *veth = (struct vlan_ethhdr *)(skb->data);
>  	unsigned int len;
> +	u16 vlan_tci;
>  	int ret;
>  
> -	/* Handle non-VLAN frames if they are sent to us, for example by DHCP.
> -	 *
> -	 * NOTE: THIS ASSUMES DIX ETHERNET, SPECIFICALLY NOT SUPPORTING
> -	 * OTHER THINGS LIKE FDDI/TokenRing/802.3 SNAPs...
> -	 */
> -	if (vlan->flags & VLAN_FLAG_REORDER_HDR ||
> -	    veth->h_vlan_proto != vlan->vlan_proto) {
> -		u16 vlan_tci;
> -		vlan_tci = vlan->vlan_id;
> -		vlan_tci |= vlan_dev_get_egress_qos_mask(dev, skb->priority);
> -		__vlan_hwaccel_put_tag(skb, vlan->vlan_proto, vlan_tci);
> -	}
> +	vlan_tci = vlan->vlan_id;
> +	vlan_tci |= vlan_dev_get_egress_qos_mask(dev, skb->priority);
> +	__vlan_hwaccel_put_tag(skb, vlan->vlan_proto, vlan_tci);

[Severity: Medium]
Does this change more of the reorder_hdr=off TX behaviour than the commit
message describes?

The changelog says the (veth->h_vlan_proto != vlan->vlan_proto) test "only
made sense together with the insertion in vlan_dev_hard_header()". Before
447cbe95ebb9, offload-capable real devices used vlan_passthru_header_ops.
That never inserted a tag, yet this test still applied to them.

The test compares only the TPID, not the VID. Without it, take any frame
that reaches vlan_dev_hard_start_xmit() on a reorder_hdr=off VLAN and
already carries an in-band tag with the same TPID (any VID). It now goes
out as ETH|VLAN(ours)|VLAN(theirs). Before this patch it kept only its
original tag.

AF_PACKET SOCK_RAW is not the only source of such frames. Others include
tc mirred or BPF redirect from another reorder_hdr=off VLAN's ingress, OVS
ports, and userspace reflectors.

The RX side, which this patch does not change, still produces that in-band
layout:

net/8021q/vlan_core.c:vlan_do_receive() {
    ...
	if (!(vlan_dev_priv(vlan_dev)->flags & VLAN_FLAG_REORDER_HDR) &&
	    !netif_is_macvlan_port(vlan_dev) &&
	    !netif_is_bridge_port(vlan_dev)) {
		...
		skb = *skbp = vlan_insert_inner_tag(skb, skb->vlan_proto,
						    skb->vlan_tci, skb->mac_len);
    ...
	__vlan_hwaccel_clear_tag(skb);
    ...
}

vlan_parse_protocol() also still expects SOCK_RAW frames to carry an
in-band tag, and skips it:

static __be16 vlan_parse_protocol(const struct sk_buff *skb)
{
	struct vlan_ethhdr *veth = (struct vlan_ethhdr *)(skb->data);

	return __vlan_get_protocol(skb, veth->h_vlan_proto, NULL);
}

So a frame received on a reorder_hdr=off VLAN and sent back out through
one is now double tagged. Before, RX and TX agreed on the layout. Is that
intended?

There is also a change on real devices without HW VLAN TX offload. Those
always used vlan_header_ops. For reorder_hdr=off, vlan_dev_hard_header()
has pushed the 802.1Q header and set skb->protocol = vlan_proto since
2.6.12.

After this patch, anything that sees frames at the VLAN device's egress
gets untagged frames, with skb->protocol set to the L3 protocol. That
includes dev_queue_xmit_nit taps, tc/BPF egress and nft netdev egress.
Egress rules written for the tagged layout would stop matching.

That goes beyond undoing what 447cbe95ebb9 changed on offload-capable
NICs. The commit message does not mention it.

Locally generated traffic still leaves with a single tag, the same as
reorder_hdr=on. But 447cbe95ebb9 is marked for stable back to 3.19, so
this change would be backported widely.

Could the changelog describe these changes? Alternatively, TX could skip
adding a tag when the frame already carries our tag in-band, so that it
matches RX.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261008123812.554729-1-edumazet%40kernel.org