Re: [PATCH net-next v3 3/3] selftests: net: cover IPv6 uncached route device mismatch

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] test_ipv6_uncached_mismatch() mutates the shared topology built
  once by setup() and never restores it: it installs a root netem qdisc…
- [Low] The test's regression signal depends on bucket separation that
  the implementation does not guarantee.
- [Low] test_ipv6_uncached_mismatch() replaces the VRF root qdisc with
  `netem limit 1 delay 10s` (line 397) but neither checks nor logs the…
- [Low] The test verifies 'route retained in the VRF qdisc' with a one-
  shot `tc -s qdisc show` snapshot (line 408) and then performs the…

--

selftests: net: cover IPv6 uncached route device mismatch

This adds test_ipv6_uncached_mismatch() to vrf-xfrm-tests.sh, which installs
a delayed netem qdisc on the VRF, adds a local IPv6 route on the VRF member
interface, sends raw-header packets with msg_zerocopy, and then deletes the
member interface while the uncached route is still referenced by the qdisc
backlog.

> diff --git a/tools/testing/selftests/net/vrf-xfrm-tests.sh b/tools/testing/selftests/net/vrf-xfrm-tests.sh
> index b64dd891699d3..4f409d135a99a 100755
> --- a/tools/testing/selftests/net/vrf-xfrm-tests.sh
> +++ b/tools/testing/selftests/net/vrf-xfrm-tests.sh
> @@ -385,6 +385,37 @@ run_tests()
>  	cleanup_xfrm_dev
>  }
>  
> +test_ipv6_uncached_mismatch()
> +{
> +	local sender_pid
> +	local backlog
> +	local rc
> +
> +	# A local route through a VRF uses the VRF as dst.dev while retaining
> +	# the VRF member interface in rt6i_idev. Raw header sends create uncached
> +	# routes, and netem keeps them referenced while the interface is deleted.
> +	run_cmd_host1 tc qdisc replace dev ${VRF} root netem limit 1 delay 10s

[Severity: Low]
Should the result of this qdisc replacement be checked or logged like the
other setup commands in this script?  The script does not use set -e, so the
status is discarded here.

The main flow has already installed a different qdisc on the same device
before calling this function:

	run_cmd_host1 tc qdisc add dev ${VRF} root netem delay 100ms
	echo
	echo "netem qdisc on VRF device"
	run_tests

If the replace fails, the 100ms qdisc stays in place.  msg_zerocopy with
-t 0 runs for about 200ms (parse_opts() computes
cfg_runtime_ms = 200 + strtoul(optarg, NULL, 10) * 1000), so packets
enqueued in the last 100ms are still in the backlog when tc -s qdisc show is
sampled and the grep for a non-empty backlog still matches.  That backlog
then drains in about 100ms, well within the 2s deletion timeout, so every
added check can pass without ever holding a route across the device
deletion.  Would checking the replace status here close that gap?

> +	ip -6 -netns "$host1" route add local ${HOST1_6}/128 dev eth0
> +	ip netns exec "$host1" ./msg_zerocopy -6 \
> +		-S ${HOST1_6} -D ${HOST1_6} -s 1200 -t 0 raw_hdrincl \
> +		>/dev/null 2>&1 &
> +	sender_pid=$!
> +	wait "$sender_pid"
> +	rc=$?
> +	log_test $rc 0 "Create uncached IPv6 routes with mismatched devices"
> +	[ $rc -ne 0 ] && return
> +
> +	backlog=$(ip netns exec "$host1" tc -s qdisc show dev ${VRF})
> +	if ! echo "$backlog" | grep -Eq 'backlog .* [1-9][0-9]*p'; then
> +		log_test 1 0 "Retain uncached IPv6 routes in VRF qdisc"
> +		return
> +	fi
> +	log_test 0 0 "Retain uncached IPv6 routes in VRF qdisc"
> +

[Severity: Low]
Is the retention observed by this snapshot guaranteed to still hold when the
delete runs a few statements later?

The only thing keeping the skb, and therefore the uncached route reference,
in the qdisc is the netem 10s timer, and that timer is armed at enqueue time
during the msg_zerocopy run rather than at the moment of this check.  With
-t 0 the sender runs about 200ms, so roughly 9.8s of slack remain between
the tc -s qdisc show sample and run_cmd_host1 timeout 2 ip link del eth0.

If that slack is consumed by shell forks, grep, printf, or scheduling stalls
on a loaded single-CPU or nested VM, netem has already dequeued the packet
and dropped the dst/idev references, so the delete completes promptly
regardless of whether the kernel bug is present, and the test reports a
pass.  Would a longer netem delay, or re-sampling the backlog immediately
before the delete, make the assertion self-checking?

> +	run_cmd_host1 timeout 2 ip link del eth0
> +	log_test $? 0 "Flush uncached IPv6 routes with mismatched devices"
> +}

[Severity: Low]
Can a hash bucket collision make this check pass on a kernel where the
insertion key regressed?

rt6_uncached_list_add() in net/ipv6/route.c reduces the keying device
pointer to one of 64 buckets:

	ul = &table->buckets[hash_ptr(dev, RT6_UNCACHED_HASH_BITS)];

and teardown of a non-master device scans only that one bucket:

	ul = &table->buckets[hash_ptr(dev,
				      RT6_UNCACHED_HASH_BITS)];
	rt6_uncached_list_flush(ul, dev);

With RT6_UNCACHED_HASH_BITS == 6, if a regression reintroduced keying by
dst_dev() (the VRF) and hash_ptr(vrf) happens to land in the same bucket as
hash_ptr(eth0), the eth0 scan still reaches the wrongly indexed route,
rt6_uncached_list_flush() matches on rt6i_idev->dev == eth0 and releases the
reference, so the delete succeeds and the test passes.  The pointers differ
per boot, so that is roughly a 1-in-64 chance per run.  Would creating
several VRF and member pairs make the signal independent of bucket layout?

[Severity: Low]
Does this function need to undo the state it installs?

test_ipv6_uncached_mismatch() mutates the topology built once by setup() and
never restores it:

  - a root netem qdisc with limit 1 delay 10s on the VRF
  - a local IPv6 route on eth0
  - deletion of eth0, which is the VRF's only member and the only veth to
    host2

There is no matching tc qdisc del or ip -6 route del on any exit path, and
both early returns (the [ $rc -ne 0 ] && return after the sender, and the
backlog-check failure path) skip even the eth0 deletion, leaving the 10s
netem qdisc behind.

That makes the function correct only as the final statement of the script,
and that constraint isn't documented anywhere, so anything appended after it
runs against a namespace with no eth0 and a limit 1 / 10s-delay qdisc.

Related, and not introduced by this patch: the script has no trap cleanup
EXIT, and the cleanup 2>/dev/null in the main flow runs before setup while
$host1/$host2 are still unset, so the namespaces and this state are simply
abandoned at exit.  setup_ns() in tools/testing/selftests/net/lib.sh only
records names in NS_LIST and does not arrange teardown.  Since setup_ns()
picks a fresh randomized name per invocation, a re-run does not inherit the
broken topology, so what remains is leaked state plus the undocumented
ordering requirement.

>  
>  ################################################################################
>  # usage
>  
> @@ -425,6 +456,10 @@ echo
>  echo "netem qdisc on VRF device"
>  run_tests
>  
> +echo
> +echo "Uncached IPv6 route with mismatched devices"
> +test_ipv6_uncached_mismatch
> +
>  printf "\nTests passed: %3d\n" ${nsuccess}
>  printf "Tests failed: %3d\n"   ${nfail}
>  

Would a short comment here noting that this test must stay last, or a
cleanup/restore inside the function, help whoever extends the script next?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260917-hash-bucket-route-lists-v3-0-30493a37b6eb%40cloudflare.com