Re: [PATCH v2 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 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