Re: [PATCH net v3 7/8] selftests: net: tun: test more outer header mismatches of UDP tunnel GSO

[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] The new helper build_udptnl6_frame() in
  tools/testing/selftests/net/tun.c (lines 873-908) gets the header…

--

selftests: net: tun: test more outer header mismatches of UDP tunnel GSO

This commit adds tap tests for outer header checks on UDP tunnel GSO
packets. It adds a new 4in6 frame builder, build_udptnl6_frame(), with a
positive control and a family mismatch test. It also adds VLAN and L4
protocol mismatch tests that reuse the existing 4in4 builder.

Closes: https://lore.kernel.org/netdev/179386753718.488856.13433005125725728519@kernel.org/

> diff --git a/tools/testing/selftests/net/tun.c b/tools/testing/selftests/net/tun.c
> index cfeca53863ce2..a135d3aa10d18 100644
> --- a/tools/testing/selftests/net/tun.c
> +++ b/tools/testing/selftests/net/tun.c
> @@ -869,6 +869,103 @@ TEST_F(tun_vnet_udptnl_proto, outer_proto_link_type)
>  	ASSERT_EQ(ret, len);
>  }
>  
> +/* Build a 4in6 Geneve GSO frame whose outer ethertype is @l3proto. */
> +static int build_udptnl6_frame(uint8_t *buf, uint16_t l3proto,
> +			       int payload_len)
> +{
> +	int hlen = UDP_TUNNEL_GENEVE_4IN6_HDRLEN;
> +	uint8_t *outer_udph, *cur = buf;
> +	int pktlen;
> +
> +	memset(buf, 0, TUN_VNET_TNL_SIZE + hlen + payload_len);
> +
> +	cur += build_virtio_net_hdr_v1_hash_tunnel(cur, true, hlen, 1400,
> +						   AF_INET6, AF_INET);
> +	cur += build_eth(cur, l3proto, param_hwaddr_outer_dst,
> +			 param_hwaddr_outer_src);
> +
> +	pktlen = UDP_TUNNEL_GENEVE_4IN6_HDRLEN - ETH_HLEN -
> +		 sizeof(struct ipv6hdr) + payload_len;

[Severity: Medium]
Are hlen and pktlen both 14 bytes too small here?

UDP_TUNNEL_GENEVE_4IN6_HDRLEN counts only the inner Ethernet header:

#define UDP_TUNNEL_GENEVE_4IN6_HDRLEN \
	(ETH_HLEN + sizeof(struct iphdr) + sizeof(struct ipv6hdr) + \
	 GENEVE_HLEN + 2 * sizeof(struct udphdr))

The 4in4 sibling build_udptnl_frame() adds the outer Ethernet header
separately. It subtracts only the outer L3 header:

	int hlen = ETH_HLEN + vlan_len + UDP_TUNNEL_GENEVE_4IN4_HDRLEN;
	...
	pktlen = UDP_TUNNEL_GENEVE_4IN4_HDRLEN - sizeof(struct iphdr) +
		 payload_len;

build_gso_packet_into_tun() follows the same convention for 4in6 on tap.

In build_udptnl6_frame(), the virtio header's hdr_len is set to 98. The
real header stack is 112 bytes. The memset() also clears 14 bytes less
than the frame that gets written.

pktlen works out to 44 + payload_len. The data after the outer IPv6 header
is outer UDP + Geneve + inner Ethernet + inner IPv4 + inner UDP + payload,
which is 58 + payload_len.

So ip6h->payload_len and the outer udph->len are both 14 bytes short.
build_udp_packet_csum() sums only ntohs(udph->len) bytes, so the outer UDP
checksum is also wrong.

outer_proto_ok_ipv6 passes today only because the tun write path does not
cross-check those lengths. Would it start failing if outer header
validation became stricter? And could outer_proto_bad_family then get
EINVAL from the length mismatch instead of the family check it is meant
to cover?

The later patch in the series, "selftests: net: tuntap: write UDP tunnel
offsets in little endian", leaves build_udptnl6_frame() unchanged.

Something like this would match build_udptnl_frame():

	int hlen = ETH_HLEN + UDP_TUNNEL_GENEVE_4IN6_HDRLEN;
	...
	pktlen = UDP_TUNNEL_GENEVE_4IN6_HDRLEN - sizeof(struct ipv6hdr) +
		 payload_len;

> +	cur += build_ipv6_header(cur, IPPROTO_UDP, 0, pktlen,
> +				 &param_ipaddr6_outer_dst,
> +				 &param_ipaddr6_outer_src);
> +
> +	outer_udph = cur;
> +	cur += build_udp_header(cur, UDP_SRC_PORT, VN_PORT,
> +				pktlen - sizeof(struct udphdr));

[ ... ]

> +	build_udp_packet_csum(outer_udph, AF_INET6, false);
> +
> +	return cur - buf;
> +}
> +
> +TEST_F(tun_vnet_udptnl_proto, outer_proto_ok_ipv6)
> +{
> +	int len, ret;
> +
> +	len = build_udptnl6_frame(self->pkt, ETH_P_IPV6, 2800);
> +	ret = write(self->fd, self->pkt, len);
> +	ASSERT_EQ(ret, len);
> +}
> +
> +/* An IPv4 ethertype on a declared IPv6 tunnel. */
> +TEST_F(tun_vnet_udptnl_proto, outer_proto_bad_family)
> +{
> +	int len, ret;
> +
> +	len = build_udptnl6_frame(self->pkt, ETH_P_IP, 2800);
> +	ret = write(self->fd, self->pkt, len);
> +	ASSERT_EQ(ret, -1);
> +	ASSERT_EQ(errno, EINVAL);
> +}

[ ... ]

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