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

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 1 · Medium: 0 · Low: 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).

- [High] Stacked VLANs of the same protocol lose their outer tag when
  reorder_hdr=off.
- [Low] On non-offload real devices, the patch removes the transmit-side
  meaning of reorder_hdr=off.

--

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

This removes the in-band 802.1Q push that vlan_dev_hard_header() did for
reorder_hdr=off. Tagging is left to vlan_dev_hard_start_xmit(), which sets
the hwaccel tag, and to validate_xmit_vlan(), which inserts the tag in
software when the device can't. The goal is to fix NETDEV WATCHDOG stalls
on r8169 that started after commit 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..c3db1ac23e6a8 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)) {
> -		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);

[Severity: High]
With this push gone, can stacked VLANs of the same protocol drop their
outer tag when reorder_hdr=off? Take eth0.200.100 on top of eth0.200, both
802.1Q with reorder_hdr=off. The same applies to 802.1ad over 802.1ad.

Here is the transmit path after this patch:

eth0.200.100: vlan_dev_hard_header()
  dev_hard_header() -> eth_header() writes the payload ethertype (0x0800)

eth0.200.100: vlan_dev_hard_start_xmit()
  0x0800 != vlan_proto, so __vlan_hwaccel_put_tag(skb, 0x8100, 100)
  dev_queue_xmit() on eth0.200

eth0.200: __dev_queue_xmit()->validate_xmit_skb()->validate_xmit_vlan()
  __vlan_hwaccel_push_inside() puts tag 100 in-band

VLAN devices never advertise NETIF_F_HW_VLAN_CTAG_TX or STAG_TX.
vlan_dev_init() leaves them out of hw_features, and vlan_dev_fix_features()
only intersects. So the core always inserts the tag in software here.

Then in vlan_dev_hard_start_xmit() for eth0.200:

	if (vlan->flags & VLAN_FLAG_REORDER_HDR ||
	    veth->h_vlan_proto != vlan->vlan_proto) {

REORDER_HDR is clear and h_vlan_proto is now 0x8100, so eth0.200 treats
the upper device's tag as its own. It never adds tag 200, and eth0 sends a
frame tagged only with VLAN 100.

Before this patch, both hard_header calls pushed their tags, so the wire
carried 8100/200 8100/100.

The commit message says:

  "Frames sent on the wire are unchanged for real devices without
  HW VLAN TX offload."

Isn't the lower VLAN device itself a real device without HW VLAN TX
offload in this setup? The result also doesn't depend on what eth0
supports.

Could this also bring back the r8169 stall for stacked setups? Before,
eth0 received a double-tagged frame, and vlan_features_check() masks
csum/TSO for multi-tagged frames. Now eth0 gets a frame with a single
in-band tag, so csum/TSO stay enabled through vlan_features.

A smaller variant seems to exist with AF_PACKET SOCK_DGRAM and protocol ==
vlan_proto on a single reorder_hdr=off VLAN. Before, the device's own tag
was pushed in front of the frame. Now the same h_vlan_proto test skips it.

The new comment above vlan_dev_hard_header() says that
vlan_dev_hard_start_xmit() sets the hwaccel tag. That only happens when the
outer ethertype differs from vlan_proto. The h_vlan_proto test was the
counterpart of the in-band insertion removed here. Does the lower VLAN need
some way to still tag frames whose in-band header came from an upper VLAN's
hwaccel push?

> -
> -		vlan_tci = vlan->vlan_id;
> -		vlan_tci |= vlan_dev_get_egress_qos_mask(dev, skb->priority);
> -		vhdr->h_vlan_TCI = htons(vlan_tci);
> -

[ ... ]

> -		skb->protocol = vlan->vlan_proto;

[Severity: Low]
This isn't about the wire format, but removing this also changes what
observers on the VLAN device itself see when reorder_hdr=off. ip-link(8)
says that with reorder_hdr off, the VLAN header is inserted immediately.

Before, the tagged frame with skb->protocol = vlan_proto was visible on the
VLAN device to egress tc/clsact, netfilter egress, an attached qdisc and
packet taps (for example tcpdump -i eth0.100).

Now the tag only exists as hwaccel metadata, set later inside
vlan_dev_hard_start_xmit(). Those observers see an untagged frame with
skb->protocol set to the payload protocol. A filter such as "protocol
802.1q" on the VLAN device would stop matching.

Should the commit message mention this user-visible change? It currently
only covers frames on the wire.

> -		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);
>  }

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