Thank you for your contribution! Sashiko AI review found 2 potential
issue(s) to consider.
Critical: 0 · High: 0 · Medium: 1 · Low: 1
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 new tun_vnet_udptnl_proto and tun_vnet_udptnl_proto_l3
tests do not work on big-endian hosts.
- [Low] The new comment in virtio_net_l4_start()
(include/linux/virtio_net.h) says the code does something it does not.
--
selftests: net: tun: test outer protocol of UDP tunnel GSO packets
This adds tun selftests that write single 4in4 Geneve GSO frames to tap
and IFF_TUN devices with UDP tunnel offloads enabled. They check that
frames with a matching outer IPv4 header are accepted. They also check
that a mismatched outer ethertype, tun_pi protocol or IP version is
rejected with EINVAL.
> diff --git a/tools/testing/selftests/net/tun.c
> b/tools/testing/selftests/net/tun.c
> index 34de2580e32ec..81410afbbcc8d 100644
> --- a/tools/testing/selftests/net/tun.c
> +++ b/tools/testing/selftests/net/tun.c
> @@ -683,6 +683,275 @@ TEST_F(tun_vnet_gso, vlan_tcpv4_gso_no_csum_64k)
[ ... ]
> +static int build_udptnl_frame(uint8_t *buf, bool vlan, uint16_t l3proto,
> + int payload_len)
> +{
> + struct virtio_net_hdr_v1_hash_tunnel *vh = (void *)buf;
> + int vlan_len = vlan ? 4 : 0;
> + int hlen = ETH_HLEN + vlan_len + UDP_TUNNEL_GENEVE_4IN4_HDRLEN;
> + uint8_t *outer_udph, *cur = buf;
> + int pktlen;
> +
> + /* The buffer is reused: do not leave bytes from a previous frame. */
> + memset(buf, 0, TUN_VNET_TNL_SIZE + hlen + payload_len);
> +
> + cur += build_virtio_net_hdr_v1_hash_tunnel(cur, true, hlen, 1400,
> + AF_INET, AF_INET);
> + vh->outer_th_offset += vlan_len;
> + vh->inner_nh_offset += vlan_len;
> + vh->hash_hdr.hdr.csum_start += vlan_len;
[Severity: Medium]
Do these new tests work on big-endian hosts? In struct
virtio_net_hdr_v1_hash_tunnel, outer_th_offset and inner_nh_offset are
__le16. Here they get native-endian arithmetic, applied on top of the
host-order values that build_virtio_net_hdr_v1_hash_tunnel() in
tuntap_helpers.h already wrote:
vh_tunnel->outer_th_offset = eth_hlen + outer_iphlen;
vh_tunnel->inner_nh_offset = vh_tunnel->outer_th_offset + ETH_HLEN +
GENEVE_HLEN + sizeof(struct udphdr);
The kernel always decodes the tunnel offsets as little-endian. csum_start,
on the other hand, follows the tun vnet endianness:
include/linux/virtio_net.h:virtio_net_hdr_tnl_to_skb() {
...
inner_th = __virtio16_to_cpu(little_endian, hdr->csum_start);
inner_nh = le16_to_cpu(vhdr->inner_nh_offset);
outer_th = le16_to_cpu(vhdr->outer_th_offset);
if (outer_th < outer_l3min ||
inner_nh < outer_th + sizeof(struct udphdr) ||
inner_th < inner_nh + inner_l3min)
return -EINVAL;
...
}
tun_open() never issues TUNSETVNETLE. On s390x or BE ppc64, csum_start is
therefore read in host order, but outer_th 34 is read as 0x2200 and
inner_nh 64 as 0x4000. The offset check above then returns -EINVAL before
virtio_net_tnl_offsets_ok() is reached.
Wouldn't outer_proto_ok then fail on BE? And wouldn't outer_proto_bad
pass there without ever reaching the new outer protocol validation?
build_udptnl_l3_frame() does the same:
vh->outer_th_offset += IPV4_OPT_LEN;
vh->inner_nh_offset += IPV4_OPT_LEN;
The helper's host-order writes predate this patch, and the existing
tun_vnet_udptnl tests share the problem. Could these fields be converted
with htole16()/le16toh() here and in the helper?
> +
> + if (vlan) {
> + uint16_t vlan_tag[2] = { htons(100), htons(l3proto) };
[ ... ]
The following is about the first patch in this series, commit
fca58b5faf90 ("net: virtio: validate outer protocol of UDP tunnel GSO
packets"), which adds the code that the outer_proto_bad tests exercise.
> diff --git a/include/linux/virtio_net.h b/include/linux/virtio_net.h
> index d6466f96cdd00..b9c8babb1fd73 100644
> --- a/include/linux/virtio_net.h
> +++ b/include/linux/virtio_net.h
[ ... ]
> + if (proto == htons(ETH_P_IPV6)) {
> + const struct ipv6hdr *iph;
> + int flags = IP6_FH_F_AUTH;
> +
> + if (!pskb_may_pull(skb, nhoff + sizeof(*iph)))
> + return false;
> + iph = (const void *)(skb->data + nhoff);
> + if (iph->version != 6)
> + return false;
> + *thoff = nhoff + sizeof(*iph);
> + *ip_proto = iph->nexthdr;
> + if (*ip_proto != IPPROTO_TCP && *ip_proto != IPPROTO_UDP) {
> + *thoff = nhoff;
> + /* Find the transport header behind any
> + * extension headers, AH included.
> + */
> + *ip_proto = ipv6_find_hdr(skb, thoff, -1, NULL,
> + &flags);
[Severity: Low]
This isn't a bug, but does this comment in virtio_net_l4_start() match
what ipv6_find_hdr() does with these arguments? With flags = IP6_FH_F_AUTH
and target = -1, it stops at the AH header instead of walking past it:
net/ipv6/exthdrs_core.c:ipv6_find_hdr() {
...
} else if (nexthdr == NEXTHDR_AUTH) {
if (flags && (*flags & IP6_FH_F_AUTH) && (target < 0))
break;
...
}
So for an outer IPv6 header followed by AH, ip_proto comes back as
NEXTHDR_AUTH and virtio_net_tnl_offsets_ok() rejects the frame. The commit
message says this is intended:
Unlike the original helpers, ipv6_find_hdr() stops at an AH header,
as Willem suggested in the review of that series.
Could the comment say that the walk stops at AH?
> + if (*ip_proto < 0 || flags & IP6_FH_F_FRAG)
> + return false;
> + }
> + return true;
> + }
[ ... ]
--
Sashiko AI review ·
https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261104050131.629967-1-edumazet%40kernel.org
