Re: [PATCH net v5 1/3] net: virtio: account for the VLAN tag in UDP tunnel GSO offsets

[netdev-bot+sashiko@kernel.org]

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

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

- [High] virtio_net_hdr_tnl_from_skb() (include/linux/virtio_net.h)
  chooses the outer-family GSO bit with `if (skb->protocol ==…

--

net: virtio: account for the VLAN tag in UDP tunnel GSO offsets

This commit adds vlan_hlen to inner_nh_offset and outer_th_offset in
virtio_net_hdr_tnl_from_skb(), the same way virtio_net_hdr_from_skb()
already adjusts csum_start. It targets the case where tun_put_user()
inserts a hwaccel VLAN tag into the frame, which leaves both tunnel
offsets 4 bytes short.

Closes: https://lore.kernel.org/netdev/179421814766.994134.3669228181251158303@kernel.org/

> diff --git a/include/linux/virtio_net.h b/include/linux/virtio_net.h
> index d6466f96cdd00..ab44342cf09f7 100644
> --- a/include/linux/virtio_net.h
> +++ b/include/linux/virtio_net.h
> @@ -461,8 +461,11 @@ virtio_net_hdr_tnl_from_skb(const struct sk_buff *skb,
>  	if (skb_shinfo(skb)->gso_type & SKB_GSO_UDP_TUNNEL_CSUM)
>  		hdr->flags |= VIRTIO_NET_HDR_F_UDP_TUNNEL_CSUM;
>  
> -	inner_nh = skb->inner_network_header - skb_headroom(skb);
> -	outer_th = skb->transport_header - skb_headroom(skb);
> +	/* Like csum_start, the offsets must account for a VLAN tag
> +	 * the caller inserts in the frame.
> +	 */
> +	inner_nh = skb->inner_network_header - skb_headroom(skb) + vlan_hlen;
> +	outer_th = skb->transport_header - skb_headroom(skb) + vlan_hlen;

[Severity: High]
This fixes the hwaccel case. There, vlan_hlen is VLAN_HLEN and
skb->protocol still holds the real L3 protocol. What happens when the
skb carries its VLAN tag inside the packet data?

Just above this hunk, the outer family comes from skb->protocol:

include/linux/virtio_net.h:virtio_net_hdr_tnl_from_skb() {
    ...
	if (skb->protocol == htons(ETH_P_IPV6))
		hdr->gso_type |= VIRTIO_NET_HDR_GSO_UDP_TUNNEL_IPV6;
	else
		hdr->gso_type |= VIRTIO_NET_HDR_GSO_UDP_TUNNEL_IPV4;
    ...
}

For an in-band tagged frame, vlan_insert_tag_set_proto() has already set
skb->protocol to ETH_P_8021Q or ETH_P_8021AD. Wouldn't an IPv6 outer
header then be reported as VIRTIO_NET_HDR_GSO_UDP_TUNNEL_IPV4?

The commit message says:

    virtio_net passes vlan_hlen = 0 and is not affected.

virtio_net has no NETIF_F_HW_VLAN_CTAG_TX, so validate_xmit_vlan() pushes
every tag into the data:

net/core/dev.c:validate_xmit_vlan() {
	if (skb_vlan_tag_present(skb) &&
	    !vlan_hw_offload_capable(features, skb->vlan_proto))
		skb = __vlan_hwaccel_push_inside(skb);
    ...
}

Its vlan_features and hw_enc_features still include
NETIF_F_GSO_UDP_TUNNEL. Take a VXLAN or Geneve device over an IPv6
underlay on a VLAN subinterface: it sends unsegmented skbs with
skb->protocol == ETH_P_8021Q into
xmit_skb()->virtio_net_hdr_tnl_from_skb(). Is virtio_net really
unaffected in that case?

tun seems to reach the same path in three cases: ethtool -K tapX txvlan
off, QinQ (outer tag in hwaccel, inner tag in the data), and a vlan
device created with reorder_hdr off.

Before this series, a Linux receiver used this bit only to compute a
looser outer_l3min, so the wrong value did no harm. Later in the series,
"net: virtio: validate outer protocol of UDP tunnel GSO packets" adds
virtio_net_tnl_offsets_ok(). That function walks the VLAN tags and
rejects the mismatch:

	if (!virtio_net_l3_start(skb, has_eth, &nhoff, &proto))
		return false;
	if (outer_isv6 != (proto == htons(ETH_P_IPV6)))
		return false;

Once that check is in place, will tun_get_user(), tun_xdp_one() and
virtnet_receive_done() drop every VLAN tagged UDP tunnel GSO packet with
an IPv6 outer header from these senders?

Could the outer family be taken from vlan_get_protocol(skb) here instead
of skb->protocol?

>  	vhdr->inner_nh_offset = cpu_to_le16(inner_nh);
>  	vhdr->outer_th_offset = cpu_to_le16(outer_th);
>  	return 0;

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