Re: [PATCH net v3 2/8] net: virtio: do not use dev->type to find the outer L3 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 (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] net: virtio: outer_l3min includes ETH_HLEN in
  virtio_net_hdr_tnl_to_skb()

Pre-existing issues:
- [Medium] This patch makes Ethernet framing an explicit argument
  (`has_eth`), but virtio_net_hdr_tnl_to_skb() in…

--

net: virtio: do not use dev->type to find the outer L3 header

virtio_net_l3_start() used skb->dev->type to decide whether a frame
starts with an Ethernet header. tun can change that type with
TUNSETLINK, so this patch adds a has_eth argument to
virtio_net_hdr_tnl_to_skb() instead. Callers now pass true for
virtio_net and tun_xdp_one(), and pass whether the device is IFF_TAP
in tun_get_user().

Link: https://lore.kernel.org/netdev/179387332014.488856.9111956413610107951@kernel.org/

> diff --git a/drivers/net/tun.c b/drivers/net/tun.c
> index dcec7311e6011..3920b3dff1e84 100644
> --- a/drivers/net/tun.c
> +++ b/drivers/net/tun.c
> @@ -1944,7 +1944,8 @@ static ssize_t tun_get_user(struct tun_struct *tun, struct tun_file *tfile,
>  		break;
>  	}
>  
> -	if (tun_vnet_hdr_tnl_to_skb(tun->flags, features, skb, &hdr)) {
> +	if (tun_vnet_hdr_tnl_to_skb(tun->flags, features, skb, &hdr,
> +				    (tun->flags & TUN_TYPE_MASK) == IFF_TAP)) {
>  		atomic_long_inc(&tun->rx_frame_errors);
>  		err = -EINVAL;
>  		goto free_skb;

[ ... ]

> diff --git a/include/linux/virtio_net.h b/include/linux/virtio_net.h
> index 357116c2a1489..b7b6b542f4763 100644
> --- a/include/linux/virtio_net.h
> +++ b/include/linux/virtio_net.h

[ ... ]

> @@ -452,6 +450,7 @@ virtio_net_tnl_offsets_ok(struct sk_buff *skb, bool outer_isv6,
>  static inline int
>  virtio_net_hdr_tnl_to_skb(struct sk_buff *skb,
>  			  const struct virtio_net_hdr_v1_hash_tunnel *vhdr,
> +			  bool has_eth,
>  			  bool tnl_hdr_negotiated,
>  			  bool tnl_csum_negotiated,
>  			  bool little_endian)
> @@ -505,7 +504,7 @@ virtio_net_hdr_tnl_to_skb(struct sk_buff *skb,
>  	if (ret)
>  		return ret;
>  
> -	if (!virtio_net_tnl_offsets_ok(skb, outer_isv6, outer_th))
> +	if (!virtio_net_tnl_offsets_ok(skb, has_eth, outer_isv6, outer_th))
>  		return -EINVAL;

[Severity: Medium]
Now that the framing is passed in as has_eth, should the minimum outer
transport offset check earlier in virtio_net_hdr_tnl_to_skb() depend
on it as well? outer_l3min is still computed as ETH_HLEN +
virtio_l3min(outer_isv6), so outer_th must be at least 34 for IPv4 or
54 for IPv6, whatever has_eth says.

On an IFF_TUN device, tun_get_user() calls skb_reset_network_header()
and passes has_eth=false. virtio_net_l3_start() then returns nhoff = 0,
and virtio_net_l4_start() computes thoff = 20 for a plain IPv4 outer
header, or 40 for a plain IPv6 one. virtio_net_tnl_offsets_ok() then
requires outer_th == thoff, but the floor above has already rejected
both values.

As a result, the has_eth=false path only accepts frames whose outer L3
header is at least ETH_HLEN bytes longer than the minimum, such as IPv4
with options or IPv6 with extension headers. Would ordinary UDP tunnel
GSO frames written to an IFF_TUN fd get -EINVAL and be counted in
rx_frame_errors?

The transmit side of the same ABI produces exactly those offsets: in
TUN mode virtio_net_hdr_tnl_from_skb() fills outer_th_offset from
skb->transport_header - skb_headroom(skb), which is 20 for an L3-first
IPv4 frame. set_offload() also enables NETIF_F_GSO_UDP_TUNNEL without
looking at IFF_TAP or IFF_TUN, so IFF_TUN devices reach this path.

This isn't introduced by this patch, but since has_eth is now available
here, would something like this work?

	outer_l3min = (has_eth ? ETH_HLEN : 0) +
		      virtio_l3min(outer_isv6);

[Severity: Medium]
This isn't a bug introduced by this patch, but now that the framing is
passed in as has_eth, should the minimum outer transport offset in
virtio_net_hdr_tnl_to_skb() depend on it too? That check comes earlier
in the same function and still assumes an Ethernet header:

include/linux/virtio_net.h:virtio_net_hdr_tnl_to_skb() {
    ...
	outer_l3min = ETH_HLEN + virtio_l3min(outer_isv6);
    ...
	if (outer_th < outer_l3min ||
	    inner_nh < outer_th + sizeof(struct udphdr) ||
	    inner_th < inner_nh + inner_l3min)
		return -EINVAL;
    ...
}

On an IFF_TUN device, tun_get_user() resets the network header and
passes has_eth=false. virtio_net_l3_start() then returns nhoff = 0, and
virtio_net_tnl_offsets_ok() requires outer_th == thoff. For a plain
outer header, thoff is 20 for IPv4 and 40 for IPv6. The floor above has
already rejected both values, since it is 34 or 54.

As a result, the has_eth=false path only accepts frames whose outer L3
header is at least ETH_HLEN bytes longer than the minimum, such as IPv4
with options or IPv6 with extension headers. Would ordinary UDP tunnel
GSO frames written to an IFF_TUN fd get -EINVAL and be counted in
rx_frame_errors?

The transmit side of the same ABI seems to produce exactly those
offsets. In TUN mode, tun_put_user() ends up in
virtio_net_hdr_tnl_from_skb(), which writes:

	outer_th = skb->transport_header - skb_headroom(skb);
	vhdr->inner_nh_offset = cpu_to_le16(inner_nh);
	vhdr->outer_th_offset = cpu_to_le16(outer_th);

For an L3-first frame, that gives 20 for IPv4. Also, set_offload() in
tun.c enables NETIF_F_GSO_UDP_TUNNEL without checking IFF_TAP or
IFF_TUN, so IFF_TUN devices can reach this receive path.

The selftest commit in this series also mentions the limitation: "The
outer IPv4 header carries options, as virtio_net_hdr_tnl_to_skb()
expects room for an Ethernet header before the outer transport header."

Would something like this work here?

	outer_l3min = (has_eth ? ETH_HLEN : 0) + virtio_l3min(outer_isv6);

>  
>  	/* In case of USO, the inner protocol is still unknown and

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