Re: [PATCH net-next v2 2/2] selftests: net: big_tcp_tunnels: Cover bareudp

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 3 potential
issue(s) to consider.

Critical: 0 · High: 1 · Medium: 0 · Low: 2

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).

- [High] Seven of the twelve new bareudp test cases cannot pass, so
  tools/testing/selftests/net/big_tcp_tunnels.sh now always fails.
- [Low] Comments in tools/testing/selftests/net/big_tcp_tunnels.sh do not
  match the new coverage.
- [Low] The return status of the new bareudp setup commands is never
  checked.

--

selftests: net: big_tcp_tunnels: Cover bareudp

This patch adds bareudp to the tunnel types tested by big_tcp_tunnels.sh.
The bareudp devices run in external mode with ethertype ipv4 multiproto. A
tc tunnel_key action on egress supplies the tunnel metadata, and the device
MTU is lowered to leave room for the IPv6 and UDP headers.

> diff --git a/tools/testing/selftests/net/big_tcp_tunnels.sh b/tools/testing/selftests/net/big_tcp_tunnels.sh
> index cc0875e52fb9..cf38377f576b 100755
> --- a/tools/testing/selftests/net/big_tcp_tunnels.sh
> +++ b/tools/testing/selftests/net/big_tcp_tunnels.sh
> @@ -53,6 +53,16 @@ setup() {
>  	DEFAULT_TCP_MIN_TSO_SEGS=$(ip netns exec "$CLIENT_NS" sysctl -n net.ipv4.tcp_min_tso_segs)
>  }
>  
> +# bareudp only works in external mode: attach the tunnel metadata to all
> +# packets sent through the device. Its MTU does not account for the
> +# encapsulation either, so leave room for IPv6 and UDP headers.
> +setup_bareudp() {
> +	ip -netns "$1" link set "$2" mtu 1452
> +	tc -netns "$1" qdisc add dev "$2" clsact
> +	tc -netns "$1" filter add dev "$2" egress matchall \
> +		action tunnel_key set src_ip "$3" dst_ip "$4" id 0 ttl 64
> +}

[Severity: Low]
Should failures from these commands be passed back to the caller?
setup_bareudp() ignores the exit status of ip link set, tc qdisc add and tc
filter add. The same is true of the ip link add ... type bareudp ...
multiproto calls in setup_tunnel().

The exit status of setup_tunnel() comes from its last command:

    ip netns exec "$SERVER_NS" ethtool -K link1 tx-checksumming "$3" > /dev/null

As a result, setup_tunnel ... || exit "$?" never sees these failures. The
new tc -V check only shows that the tc binary exists. It does not show that
matchall or tunnel_key work.

Without the tunnel_key filter, bareudp_xmit() drops every packet:

drivers/net/bareudp.c:bareudp_xmit() {
    ...
	info = skb_tunnel_info(skb);
	if (unlikely(!info || !(info->mode & IP_TUNNEL_INFO_TX))) {
		err = -EINVAL;
		goto tx_error;
	}
    ...
}

The test then reports a packet-count FAIL rather than a SKIP or a clear
setup error. The vxlan and geneve setup ignores errors in the same way.

> @@ -64,26 +74,42 @@ setup_tunnel() {

[ ... ]

> +	bareudp)
> +		ip -netns "$CLIENT_NS" link add tun0 type bareudp \
> +			dstport 6635 ethertype ipv4 multiproto
> +		setup_bareudp "$CLIENT_NS" tun0 "$CLIENT_IP" "$SERVER_IP"
> +		;;
> +	esac

[Severity: High]
Can the bareudp cases that use IPv6, inner or outer, with csum offload off
pass in this configuration?

With tx-checksumming off on link0, every encapsulated GSO skb is segmented
in software by skb_udp_tunnel_segment(). That function chooses the inner
handler based on skb->inner_protocol:

net/ipv4/udp_offload.c:skb_udp_tunnel_segment() {
    ...
	case ENCAP_TYPE_ETHER:
		protocol = skb->inner_protocol;
		gso_inner_segment = skb_mac_gso_segment;
    ...
}

On the IPv4 underlay, bareudp_xmit_skb() always stores the configured
ethertype. It does this even for IPv6 inner traffic in multiproto mode:

drivers/net/bareudp.c:bareudp_xmit_skb() {
    ...
	skb_set_inner_protocol(skb, bareudp->ethertype);
    ...
}

The IPv6 header is then handed to inet_gso_segment(). There, iph->ihl * 4
reads the low nibble of the first IPv6 byte, and the ihl < sizeof(*iph)
check fails.

On the IPv6 underlay, bareudp6_xmit_skb() never sets inner_protocol. The
field stays 0, so skb_mac_gso_segment() fails:

net/core/gso.c:skb_mac_gso_segment() {
    ...
	__be16 type = skb_network_protocol(skb, &vlan_depth);

	if (unlikely(!type))
		return ERR_PTR(-EINVAL);
    ...
}

In both cases validate_xmit_skb() drops the GSO skbs. The bigtcp iptables
counter on tun1 stays below packets_threshold, and do_test ... || exit "$?"
stops the run at the first such case: bareudp, tun_family 4,
traffic_family 6, csum off.

vxlan and geneve are not affected because they set inner_protocol to
ETH_P_TEB.

This driver behaviour is older than this series. The previous patch,
"bareudp: Enable BIG TCP packets", only raises tso_max_size and does not
change it. As a result, the commit message goal to "run the same checks
over bareudp as for vxlan and geneve" cannot be met as written.

Could the bareudp inner_protocol handling be fixed first? That would mean
setting it from the actual inner protocol, and also setting it on the IPv6
path. Alternatively, the bareudp combinations could be limited to the
cases that work.

[ ... ]

> @@ -205,7 +236,7 @@ fi
>  WORKDIR=$(mktemp -d)
>  trap cleanup EXIT
>  setup
> -for tunnel in vxlan geneve; do
> +for tunnel in vxlan geneve bareudp; do

[Severity: Low]
This isn't a bug, but the comment at the top of the file still reads:

# Testing for IPv4 and IPv6 BIG TCP over VXLAN and GENEVE tunnels.

Should it mention bareudp now?

The comment in do_test() is also out of date:

	# packet to veth. Check BIG TCP packets inside the VXLAN tunnel to verify

That branch runs for every tunnel type. The wording was already wrong once
geneve was added, and adding bareudp makes it more misleading.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261003100939.468181-1-yanhaishuang%40cmss.chinamobile.com