Re: [PATCH v2 net 2/2] selftests: net: tun: add test for VLAN-tagged GSO without NEEDS_CSUM

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] The 'Truncated TCP header without NEEDS_CSUM' sub-case in
  TEST_F(tun_vnet_gso, vlan_tcpv4_gso_no_csum)…

--

selftests: net: tun: add test for VLAN-tagged GSO without NEEDS_CSUM

This adds a selftest in tun.c that writes a VLAN-tagged TCPv4 GSO packet
without VIRTIO_NET_HDR_F_NEEDS_CSUM to a TAP device with IFF_VNET_HDR. It
checks that the packet is accepted with flags = 0 and with
flags = VIRTIO_NET_HDR_F_DATA_VALID. It also checks that a mismatched GSO
type and a truncated header are rejected with -EINVAL.

> diff --git a/tools/testing/selftests/net/tun.c b/tools/testing/selftests/net/tun.c
> index abe488bac50b..c6afafb7b957 100644
> --- a/tools/testing/selftests/net/tun.c
> +++ b/tools/testing/selftests/net/tun.c

[ ... ]

> +TEST_F(tun_vnet_gso, vlan_tcpv4_gso_no_csum)
> +{

[ ... ]

> +	/* Truncated TCP header without NEEDS_CSUM */
> +	vh.gso_type = VIRTIO_NET_HDR_GSO_TCPV4;
> +	vh.hdr_len = ETH_HLEN + 4 + sizeof(struct iphdr);
> +	memcpy(pkt, &vh, sizeof(vh));
> +	ret = write(self->fd, pkt, sizeof(vh) + vh.hdr_len);
> +	ASSERT_EQ(ret, -1);
> +	ASSERT_EQ(errno, EINVAL);

[Severity: Low]
Would this sub-case still pass on a kernel without commit 67fbe1f8a962
("net: always dissect GSO packets in __virtio_net_hdr_to_skb()")?

The frame written here is ETH_HLEN + 4 + sizeof(struct iphdr) = 38 bytes
after the vnet header.

Before that fix, tun_get_user() reached the unvalidated gso_type fallback
with skb->network_header still at 0. That fallback did
pskb_may_pull(skb, nh_min_len + thlen), which is 40 bytes, so a 38-byte
frame already failed with -EINVAL.

After the fix, __virtio_net_hdr_to_skb() dissects the frame. thoff is
14 + 4 + 20 = 38:

include/linux/virtio_net.h:__virtio_net_hdr_to_skb() {
    ...
		p_off = keys.control.thoff + thlen;
		if (!pskb_may_pull(skb, p_off) ||
    ...
			return -EINVAL;
    ...
}

With p_off = 58 this also fails with -EINVAL. Both kernels therefore give
the same result, and the ASSERTs never exercise the thoff-based pull.

Only frame lengths from 40 to 57 bytes separate the two kernels for this
VLAN packet. Take hdr_len = ETH_HLEN + 4 + sizeof(struct iphdr) + 10 = 48.
The old kernel pulls 40 bytes and succeeds. Since skb->len - nh_off (8)
is not greater than gso_size, no GSO is set and write() returns the full
length. The fixed kernel needs 58 bytes and returns -EINVAL.

Could the truncated length be picked from that range, so this sub-case
catches a regression of the thoff-based pull? The TCPV6-on-IPv4 sub-case
above already separates the two kernels through
virtio_net_hdr_match_proto().

> +}
> +
>  FIXTURE(tun_vnet_udptnl)
>  {
>  	char ifname[IFNAMSIZ];

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