Re: [PATCH 3/3] selftests: net: add ioam6 ECMP test (RFC 9486 C2)

[netdev-bot+sashiko@kernel.org]

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

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

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 compatibility probe in MAIN (lines 308-315) only checks
  that an inline-mode IOAM trace can be attached (`encap ioam6 mode…
- [Medium] In MAIN, `setup_topology || { log_test_skip "Setup failed.";
  exit ...; }` (line 305) runs before `trap cleanup EXIT` (line 306).
- [Low] The commit message and the ioam6_ecmp.sh header comment describe
  scenario 2 differently from what the script does.
- [Low] ioam6_ecmp.sh skips the whole test when
  /proc/sys/net/ipv4/fib_multipath_hash_seed is missing (lines 300-303).
- [Low] The comment in set_beta_tnl() (lines 139-143) says "ip6_tnl
  caches the outer dst per CPU for a few seconds".
- [Low] setup_topology() does not check the return value of the multipath
  route install, which is the core of the test (`ip -netns $ns_beta -6…
- [Low] setup_tunnel() (lines 274-287) decides whether the test loaded
  ip6_tunnel only from `lsmod | grep -q ip6_tunnel`.

--

selftests: net: add ioam6 ECMP test (RFC 9486 C2)

This adds ioam6_ecmp.sh. The test checks that, with hash policy 0, the
ECMP nexthop chosen for a packet carrying IOAM options is the same as
for the packet without them. It covers a transit node forwarding
inline-inserted IOAM Hop-by-Hop packets, and an encapsulating node
doing the ioam6 encap re-lookup with a plain ip6ip6 tunnel as the
baseline.

> diff --git a/tools/testing/selftests/net/ioam6_ecmp.sh b/tools/testing/selftests/net/ioam6_ecmp.sh
> new file mode 100755
> index 0000000000000..c5c82884c7579
> --- /dev/null
> +++ b/tools/testing/selftests/net/ioam6_ecmp.sh
> @@ -0,0 +1,349 @@
> +#!/bin/bash
> +# SPDX-License-Identifier: GPL-2.0+

[ ... ]

> +# 2) encapsulating node: beta itself encapsulates (ioam6 encap mode,
> +#    tunnel dst = original dst, tunnel src pinned) over a multipath
> +#    route. The ioam6 re-lookup hashed NEXTHDR_HOP instead of the
> +#    transformed packet's real nexthdr (IPPROTO_IPV6). The baseline is
> +#    the same encapsulation without the IOAM option: a plain ip6ip6
> +#    tunnel over the same multipath route, with the same outer
> +#    addresses.

[Severity: Low]
Is "tunnel dst = original dst" accurate here? The commit message says
the same thing:

  - encapsulating node re-lookup: the forwarder itself encapsulates
    (ioam6 encap mode, tunnel dst equal to the original dst) over a
    multipath route, with a plain ip6ip6 tunnel as the
    identical-packet-without-IOAM baseline.

The script sends to 2001:db8:3::2, but set_beta_ioam_encap() installs
this route:

  2001:db8:3::2/128 encap ioam6 mode encap ... tundst 2001:db8:2:1::2

So the tunnel dst is not the original dst. The topology comment further
down describes this correctly.

If the tunnel dst really were the original dst, the re-lookup would
resolve back to the same ioam6 route:

net/ipv6/ioam6_iptunnel.c:ioam6_output() {
    ...
		if (orig_dst->lwtstate == dst->lwtstate)
			dst_cache_set_ip6(&ilwt->cache,
					  &ilwt->null_rt.dst, &fl6.saddr);
    ...
}

In that case orig_output would be used and the multipath route would
never be consulted.

The ip6ip6 baseline is also not an identical packet on the wire.
ip6_tnl_xmit() pushes an encap-limit Destination Options header when
encap_limit >= 0, and it sets the flow label with ip6_make_flowlabel()
after the route lookup. Only the hash inputs at beta's lookup match.

Could the commit message and this header comment be updated to match
what the test actually does?

[ ... ]

> +set_beta_tnl()
> +{
> +	# Recreate the tunnel: ip6_tnl caches the outer dst per CPU for a
> +	# few seconds, and neither the route change below nor the
> +	# fib_multipath_hash_seed write resets it. A stale cache would
> +	# resolve the outer sibling under a previous seed (or on another
> +	# CPU), making the baseline flip between attempts of one seed.

[Severity: Low]
This isn't a bug, but is "for a few seconds" accurate? ip6_tnl_xmit()
reads the cache with dst_cache_get(&t->dst_cache), and
dst_cache_per_cpu_get() only drops an entry in two cases:

net/core/dst_cache.c:dst_cache_per_cpu_get() {
    ...
	if (unlikely(!time_after(idst->refresh_ts,
				 READ_ONCE(dst_cache->reset_ts)) ||
		     (READ_ONCE(dst->obsolete) && !dst->ops->check(dst, idst->cookie)))) {
    ...
}

That means an explicit dst_cache_reset(), or an obsolete dst that fails
->check(). There is no time-based expiry, so a cached outer dst can
stay indefinitely.

Recreating the tunnel for every seed is still the right workaround. The
comment, though, could lead someone to replace the recreation with a
sleep.

[ ... ]

> +set_beta_ioam_encap()
> +{
> +	ip -netns "$ns_beta" -6 route replace 2001:db8:3::2/128 \
> +		encap ioam6 mode encap tunsrc 2001:db8:2:1::1 tundst 2001:db8:2:1::2 \
> +		trace prealloc type 0x800000 ns 0 size 12 dev veth1
> +}

[Severity: Medium]
What happens on a kernel or iproute2 that supports ioam6 inline and
encap modes but not the tunsrc attribute? The compat check in MAIN
only probes inline mode, so it passes, and setup_tunnel() succeeds.

This route install would then fail on every seed, and run_scenario()
takes this path:

		if ! $ioam; then
			RET=$ksft_fail
			log_test "$desc (ioam route setup failed, seed $seed)"

The result is 12 hard FAILs instead of a SKIP. ioam6.sh already treats
tunsrc as a feature under test.

Is the tunsrc pin needed at all? Without it, ioam6_do_encap() calls
ipv6_dev_get_saddr() on the original dst device (veth1), which should
pick 2001:db8:2:1::1 anyway.

[ ... ]

> +	# The ECMP route under test (scenario 1 hashes its selection at
> +	# beta's input; scenario 2 at the encapsulating re-lookup).
> +	ip -netns "$ns_beta" -6 route add 2001:db8:2:1::2/128 \
> +		nexthop via 2001:db8:2:1::2 dev veth1 \
> +		nexthop via 2001:db8:2:2::2 dev veth2

[Severity: Low]
Should this route add check its return value? If it fails, beta still
has the connected 2001:db8:2:1::/64 prefix on veth1, so all traffic to
2001:db8:2:1::2 leaves through veth1 and arrives on gamma's veth0.

measure_link() would then return "A" for both variants on every seed.
run_scenario() would report PASS even though no ECMP route exists.

Adding "|| return 1" here would catch this.

[ ... ]

> +setup_tunnel()
> +{
> +	if ! lsmod 2>/dev/null | grep -q "ip6_tunnel"; then
> +		modprobe ip6_tunnel &>/dev/null
> +		lsmod 2>/dev/null | grep -q "ip6_tunnel" || \
> +			ip a 2>/dev/null | grep -q "ip6tnl0" || return 1
> +		ip6tnl_loaded=1
> +	fi

[Severity: Low]
Can this unload an ip6_tunnel module the test did not load? Consider
lsmod being unavailable or failing while modprobe works, with
ip6_tunnel already loaded:

  - the first grep fails;
  - modprobe ip6_tunnel succeeds as a no-op;
  - grep ip6tnl0 matches the existing fallback device in the init
    netns;
  - ip6tnl_loaded=1 is set.

cleanup() then runs:

		modprobe -r ip6_tunnel &>/dev/null

This removes the module if nothing else is using it.

When ip6_tunnel is built in, lsmod does not list it either, so
ip6tnl_loaded=1 is set in that case too. The later modprobe -r then
fails harmlessly. ioam6.sh uses the same heuristic.

[ ... ]

> +if [ ! -e /proc/sys/net/ipv4/fib_multipath_hash_seed ]; then
> +	log_test_skip "Kernel without CONFIG_IP_ROUTE_MULTIPATH."
> +	exit "${EXIT_STATUS}"
> +fi

[Severity: Low]
Should tools/testing/selftests/net/config be updated for this test?
fib_multipath_hash_seed only exists with CONFIG_IP_ROUTE_MULTIPATH.
That option depends on CONFIG_IP_ADVANCED_ROUTER, neither has a
default, and neither is in the net selftests config fragment.

On a kernel built from a minimal base plus that fragment, this test
would always SKIP.

CONFIG_IPV6_TUNNEL, which scenario 2 needs, is not listed explicitly
either. It only gets enabled because CONFIG_IPV6_GRE=m selects it.

> +
> +setup_topology || { log_test_skip "Setup failed."; exit "${EXIT_STATUS}"; }
> +trap cleanup EXIT

[Severity: Medium]
Can this leak ns_alpha, ns_beta and ns_gamma? setup_ns has already
created them by the time setup_topology() returns failure. The trap is
installed only after this exit, so cleanup_all_ns never runs and the
namespaces and any veths stay on the host.

setup_topology() can fail after setup_ns through the veth "|| return 1"
paths. Its exit status is also that of its last command:

	tc -n "$ns_alpha" filter add dev veth0 ingress pref 1 \
                protocol ipv6 flower skip_hw ip_proto icmpv6 type 1 action drop

That command fails on a kernel without cls_flower or act_gact.
setup_ns only cleans up when ip netns add itself fails, and lib.sh
does not install an EXIT trap.

As a side effect, a missing flower is reported as "Setup failed.", and
the later "Cannot install tc flower measurement filters" skip is never
reached for that cause.

Would installing the trap before calling setup_topology() fix this?

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261028-ipv6_ioam_ecmp-v1-0-9b6715b6f4c0%40kylinos.cn