Le jeu. 5 nov. 2026 à 11:08, <[email protected]> a écrit :
>
> 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?
Right. build_virtio_net_hdr_v1_hash_tunnel() writes the tunnel offsets
in host order since commit a942fcd72e97 ("selftest: tun: Introduce
tuntap_helpers.h header for TUN/TAP testing"), so all UDP tunnel tests
in tun.c are broken on big-endian hosts, not only the new ones.
v3 adds a patch converting the offsets with htole16() in the helper,
and le16toh()/htole16() where the tests adjust them.
>
> > +
> > + 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?
Same as for patch 1: the comment is wrong, I will fix it in v3.
>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
