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] In TEST_F(tun_vnet_udptnl_proto, outer_proto_link_type) in
tools/testing/selftests/net/tun.c, the second write sends a VLAN-…
--
selftests: net: tun: test UDP tunnel GSO on retyped devices
This commit adds a tun_set_link_type() helper that uses TUNSETLINK to
retype tun/tap devices. It also adds two tests that check the outer
headers of UDP tunnel GSO frames are validated by the tun/tap framing
rather than by dev->type.
> diff --git a/tools/testing/selftests/net/tun.c
> b/tools/testing/selftests/net/tun.c
> index 81410afbbcc8d..cfeca53863ce2 100644
> --- a/tools/testing/selftests/net/tun.c
> +++ b/tools/testing/selftests/net/tun.c
> @@ -816,6 +816,59 @@ TEST_F(tun_vnet_udptnl_proto, outer_proto_bad)
[ ... ]
> +TEST_F(tun_vnet_udptnl_proto, outer_proto_link_type)
> +{
> + uint8_t *pkt = self->pkt;
> + int len, ret;
> +
> + /* The frames still start with an Ethernet header, but the device
> + * type no longer says so: the Ethernet header must still be
> + * checked.
> + */
> + ASSERT_EQ(tun_set_link_type(self->fd, self->ifname, ARPHRD_NONE), 0);
> +
> + len = build_udptnl_frame(pkt, false, ETH_P_NSH, 2800);
> + ret = write(self->fd, pkt, len);
> + ASSERT_EQ(ret, -1);
> + ASSERT_EQ(errno, EINVAL);
> +
> + len = build_udptnl_frame(pkt, true, ETH_P_NSH, 2800);
> + ret = write(self->fd, pkt, len);
> + ASSERT_EQ(ret, -1);
> + ASSERT_EQ(errno, EINVAL);
[Severity: Low]
Does this VLAN-tagged NSH write fail on a kernel that is missing the fix?
The commit message says:
Without the patch "net: virtio: do not use dev->type to find the outer
L3 header", the frames expected to be rejected are accepted.
Without that fix, dev->type is ARPHRD_NONE here, so virtio_net_l3_start()
takes the non-Ethernet branch. For IFF_TAP, tun_get_user() has already
done:
skb_set_network_header(skb, ETH_HLEN);
so the IP version is read at frame offset 14. In a tagged frame, that
offset holds the 802.1Q TCI that build_udptnl_frame() writes:
uint16_t vlan_tag[2] = { htons(100), htons(l3proto) };
The first byte is 0x00, so the old code sees version 0:
if (iph->version == 4)
*proto = htons(ETH_P_IP);
else if (iph->version == 6)
*proto = htons(ETH_P_IPV6);
else
return false;
and the write returns -EINVAL. The old code rejects every VLAN-tagged
frame on a retyped tap, NSH or not. With the fix, this frame is still
rejected, because the protocol after the tag is NSH. So this assertion
passes with or without the fix.
The untagged ETH_P_IP accept case below also passes on both kernels,
because the old path finds a valid IPv4 header at offset 14. Only the
untagged NSH write tells the two kernels apart.
Could a VLAN-tagged ETH_P_IP frame that must be accepted be added here?
The unfixed kernel rejects it (version 0 at offset 14), and the fixed
kernel accepts it after walking the tag. The case is still missing at the
end of the series, after "selftests: net: tuntap: write UDP tunnel offsets
in little endian".
Should the commit message also be narrowed, since the tagged NSH frame is
rejected without the fix as well?
> +
> + len = build_udptnl_frame(pkt, false, ETH_P_IP, 2800);
> + ret = write(self->fd, pkt, len);
> + ASSERT_EQ(ret, len);
> +}
[ ... ]
--
Sashiko AI review ·
https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261105115715.1466924-1-edumazet%40kernel.org
