Thank you for your contribution! Sashiko AI review found 5 potential
issue(s) to consider:
Critical: 0 · High: 1 · Medium: 2 · Low: 2
- [High] test_ipv6_uncached_mismatch() (tools/testing/selftests/net/vrf-
xfrm-tests.sh:388-417) asserts that `timeout 2 ip link del eth0`…
- [Medium] Both setup steps of the new test are unchecked.
- [Medium] `run_cmd_host1 timeout 2 ip link del eth0`
(tools/testing/selftests/net/vrf-xfrm-tests.sh:415) is used as the…
- [Low] test_ipv6_uncached_mismatch() mutates and then destroys the
shared fixture without any teardown: it replaces the VRF root qdisc…
- [Low] The retention of the uncached route depends solely on a 10 s
netem timer armed at enqueue, and nothing re-verifies that the packet…
--
selftests: net: cover IPv6 uncached route device mismatch
This adds a new test case to vrf-xfrm-tests.sh that creates a local IPv6
route through a VRF so that dst.dev is the VRF while rt6i_idev stays on the
member interface, holds the resulting uncached route in a netem queue, and
then deletes the member interface. A few questions below about the new test
function and about the commit message.
> 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
> + ip -6 -netns "$host1" route add local ${HOST1_6}/128 dev eth0
[Severity: Medium]
Should the exit status of these two setup steps be checked?
The tc qdisc replace status is discarded, and the route add is not run
through run_cmd_host1 at all, so it is invisible under -v and has no
log_test either.
If CONFIG_NET_SCH_NETEM is not built, the qdisc is never installed, but
msg_zerocopy still exits 0 because rawv6_send_hdrinc() in net/ipv6/raw.c
masks -ENOBUFS:
err = ip6_push_pending_frames(sk);
...
if (err == -ENOBUFS && !inet6_test_bit(RECVERR6, sk))
err = 0;
So the first log_test passes and the failure instead shows up later as
"Retain uncached IPv6 routes in VRF qdisc" FAIL, which reads as a kernel
route-retention regression rather than a missing scheduler. Would a check
plus a skip be clearer here?
The same applies to the route add: it is the precondition that creates the
dst.dev(VRF) versus rt6i_idev(eth0) mismatch the test is named after. If it
fails (EEXIST, table/rule differences, older iproute2), the test either
fails at the sender assertion or silently exercises an unrelated path. The
later "[ $rc -ne 0 ] && return" guard cannot help, since rc only carries
msg_zerocopy's status.
Related to this, the commit message says:
verify route creation, qdisc retention, and prompt interface
deletion.
Is "verify route creation" accurate? No assertion covers the route add; the
first log_test only reports the sender program's exit status.
> + 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 window here deterministic?
The only thing holding the uncached route is the 10s netem timer armed at
enqueue, and the backlog is sampled once and then assumed to still hold at
the ip link del below. Between enqueue and the delete the script runs
msg_zerocopy for its full runtime (cfg_runtime_ms is 200 + 0 * 1000 with
-t 0, see tools/testing/selftests/net/msg_zerocopy.c), one ip netns exec tc
-s qdisc show, and two more process spawns.
If the timer fires before the grep, the result is a spurious FAIL. If it
fires after the grep but before the delete, all three checks pass while
nothing is actually exercised. Would a much longer netem delay, or a
re-check of the backlog immediately before the delete, make this
deterministic?
> +
> + run_cmd_host1 timeout 2 ip link del eth0
> + log_test $? 0 "Flush uncached IPv6 routes with mismatched devices"
[Severity: High]
Can this assertion pass on the tree the patch applies to?
The uncached dst created via ip6_pol_route() -> ip6_rt_cache_alloc() ->
rt6_uncached_list_add() holds a reference on the local route's fib6_info
through ip6_rt_copy_init() -> rt6_set_from(). That fib6_info's nexthop holds
a tracked netdev reference on eth0:
net/ipv6/route.c:fib6_nh_init() {
...
dev = netdev_get_by_index(net, cfg->fc_ifindex,
dev_tracker, gfp_flags);
...
}
and that reference is only dropped by fib6_nh_release(), which runs from
fib6_info_destroy_rcu(), i.e. after the last fib6_info reference goes away.
The teardown path does not break that chain:
net/ipv6/route.c:rt6_disable_ip() {
...
rt6_sync_down_dev(dev, event);
rt6_uncached_list_flush_dev(dev);
neigh_ifdown(nd_table(net), dev);
...
}
and rt6_uncached_list_flush_dev() only swaps rt6i_idev/dst.dev to
blackhole_netdev:
if (rt_idev && rt_idev->dev == dev) {
rt->rt6i_idev = in6_dev_get(blackhole_netdev);
in6_dev_put(rt_idev);
handled = true;
}
It never touches rt->from. The xchg(&rt->from, NULL) in ip6_dst_ifdown() is
only reached through dst->ops->ifdown from dst_dev_put(), which is not
called for this dst because ip6_rt_get_dev_rcu() gives the VRF master as
dst.dev for an RTF_LOCAL route on an enslaved device - the very mismatch
described in the commit message.
So eth0's refcount stays elevated until netem dequeues the skb roughly 10s
later, ip link del blocks in netdev_wait_allrefs_any() for that long,
timeout 2 returns non-zero, and this log_test reports FAIL. The commit
message does not name a kernel fix this test accompanies and does not
mention an expected failure. Is a corresponding kernel change meant to be
part of this series?
[Severity: Medium]
Separately, can timeout bound this operation at all?
RTM_DELLINK finishes unregistration synchronously in the caller's syscall
context via rtnl_unlock() -> netdev_run_todo() -> netdev_wait_allrefs_any(),
whose loop is:
net/core/dev.c:netdev_wait_allrefs_any() {
...
} else {
msleep(wait);
wait = min(wait << 1, WAIT_REFS_MAX_MSECS);
}
...
}
There is no signal check there, so the SIGTERM that timeout 2 queues cannot
be delivered until the syscall returns, and killing ip would not roll back
the in-kernel unregistration either. During that window the half-removed
device also emits the periodic "unregister_netdevice: waiting for eth0 to
become free" messages.
That makes the verdict of this log_test depend on signal delivery timing
rather than on the outcome of the delete, and if a reference were genuinely
never released the script would block indefinitely and never reach the
summary printf and exit $ret.
The test also never removes the retaining netem qdisc, which is the one
action that would release the pinning skb immediately. Would deleting the
qdisc before the link delete, and checking device refcounts instead of
wrapping ip in timeout, be a more reliable detector?
> +}
> +
[Severity: Low]
Does test_ipv6_uncached_mismatch() need to restore what it changes?
It replaces the VRF root qdisc with netem limit 1 delay 10s, adds
local ${HOST1_6}/128 dev eth0, and then deletes eth0, which is the veth
carrying the whole host1/host2 topology built once by setup().
Neither early return ("[ $rc -ne 0 ] && return" and the backlog-grep
return) removes the qdisc or the added route, and the success path also
leaves the 10s qdisc on the VRF.
Because the shared fixture is consumed, the file also gains an implicit
ordering constraint: the test only works while it is the last thing invoked.
Adding anything after it, or repeating run_tests, would fail.
> @@ -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
> +
Continuing the point above: the earlier phase leaves netem delay 100ms on the
same VRF device, so if the replace in the new test fails the leftover qdisc
can still produce a non-zero backlog and satisfy the grep without holding the
route past the two-second timeout.
The script also has no final cleanup and no EXIT trap. The existing
"cleanup 2>/dev/null" runs before setup() while $host1/$host2 are still
unset, so cleanup_ns gets no arguments. That part is pre-existing and not
introduced by this patch, but with eth0 now deleted and the qdisc and route
left in place the namespaces are leaked in a modified state.
> printf "\nTests passed: %3d\n" ${nsuccess}
> printf "Tests failed: %3d\n" ${nfail}
>
--
Sashiko AI review ·
https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260914-hash-bucket-route-lists-v2-0-29f6297d8a5a%40cloudflare.com
