Re: [PATCH net v3 8/8] selftests: net: tuntap: write UDP tunnel offsets in little endian

[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 · 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] selftests: net: tun: UDP tunnel GSO tests still fail on big-
  endian hosts

--

> diff --git a/tools/testing/selftests/net/tuntap_helpers.h b/tools/testing/selftests/net/tuntap_helpers.h
> --- a/tools/testing/selftests/net/tuntap_helpers.h
> +++ b/tools/testing/selftests/net/tuntap_helpers.h
> @@ -375,11 +377,14 @@ static inline int build_virtio_net_hdr_v1_hash_tunnel(uint8_t *buf, bool is_tap,
>  	inner_iphlen = (inner_family == AF_INET) ? sizeof(struct iphdr) :
>  						   sizeof(struct ipv6hdr);
>  
> -	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);
> +	outer_th = eth_hlen + outer_iphlen;
> +	inner_nh = outer_th + ETH_HLEN + GENEVE_HLEN + sizeof(struct udphdr);
>  
> -	vh->csum_start = vh_tunnel->inner_nh_offset + inner_iphlen;
> +	/* The tunnel offsets are always little endian. */
> +	vh_tunnel->outer_th_offset = htole16(outer_th);
> +	vh_tunnel->inner_nh_offset = htole16(inner_nh);
> +
> +	vh->csum_start = inner_nh + inner_iphlen;
>  	vh->csum_offset = __builtin_offsetof(struct udphdr, check);
>  	vh->flags = VIRTIO_NET_HDR_F_NEEDS_CSUM;
>  	vh->hdr_len = hdr_len;

[Severity: Medium]
Does this fully fix the big-endian case? The commit message says
csum_start follows the tun vnet header endianness, but that holds for
only part of the kernel path.

tun_open() never issues TUNSETVNETLE, so on a big-endian host
tun_vnet_is_little_endian() is false. virtio_net_hdr_tnl_to_skb() does
read csum_start with __virtio16_to_cpu(little_endian, ...) for the
initial offset ordering check. It then calls
__virtio_net_hdr_to_skb(skb, hdr, true, ...) with little_endian
hardcoded to true.

Inside __virtio_net_hdr_to_skb(), csum_start, csum_offset and
gso_size get byte swapped. build_virtio_net_hdr_v1_hash_tunnel()
still writes all three in host order, so:

  csum_start 84 becomes 21504
  csum_offset 6 becomes 1536
  gso_size 1400 becomes 30725

The 2800 byte frames used by tun_vnet_udptnl_proto and
tun_vnet_udptnl_proto_l3 are much smaller than that. The
pskb_may_pull() for the checksum location therefore fails, and
write() returns -EINVAL before virtio_net_tnl_offsets_ok() is ever
reached. The outer_proto_bad* cases would still see EINVAL, but for
the wrong reason, and the positive cases would still fail.

The receive side has the same problem.
virtio_net_hdr_tnl_from_skb() calls
virtio_net_hdr_from_skb(skb, hdr, true, false, vlan_hlen), so
gso_size comes back little endian. recv_gso_packet() then compares
vh->gso_size directly against the host order variant->gso_size.

So the UDP tunnel tests in tun.c would still fail on big-endian
hosts after this patch. Which way do you want to fix it?

One option is for the tests to switch the device to little endian
with TUNSETVNETLE and write the base header fields with htole16() too.

The other is to treat the hardcoded true in
virtio_net_hdr_tnl_to_skb() and virtio_net_hdr_tnl_from_skb() as the
real bug. The same function already decodes csum_start with
little_endian a few lines earlier.

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