Re: [PATCH net 2/2] selftests: netfilter: cover IPv6 FIB rule suppression

[netdev-bot+sashiko@kernel.org]

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

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

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 ICMPv6 neighbour discovery replies that this test itself
  triggers can make the existing logging RPF rule fire, and…
- [Low] The new reject-route checks in nft_fib.sh only create legacy
  reject route types, via `ip -6 route replace $type default table 128`…

--

selftests: netfilter: cover IPv6 FIB rule suppression

This commit extends nft_fib.sh to exercise suppress_prefixlength and
suppress_ifgroup for IPv6 FIB rules. It checks that default routes and
routes through a matching interface group are suppressed. It also checks
that non-default routes and nonmatching groups stay usable, and that
blackhole, unreachable and prohibit routes remain terminal.

> diff --git a/tools/testing/selftests/net/netfilter/nft_fib.sh b/tools/testing/selftests/net/netfilter/nft_fib.sh
> index 04544905c2164..02b6257946694 100755
> --- a/tools/testing/selftests/net/netfilter/nft_fib.sh
> +++ b/tools/testing/selftests/net/netfilter/nft_fib.sh
> @@ -202,6 +202,38 @@ check_fib_counter() {
>  	return 0
>  }
>  
> +# Count and drop probes before the existing RPF rules can log expected failures.
> +test_fib_suppress() {
> +	local label="$1" expected="$2"
> +	local result="${3:-oifname}" err=0
> +
> +	ip netns exec "$nsrouter" nft -f /dev/stdin <<EOF || return 1
> +flush table ip6 filter
> +table ip6 filter {
> +	chain prerouting {
> +		type filter hook prerouting priority -1;
> +		ip6 saddr dead:1::99 icmpv6 type echo-request \
> +			fib saddr . mark $result "$expected" counter drop
> +	}
> +}
> +EOF

[Severity: Medium]

The comment above test_fib_suppress() says probes are dropped before the
existing RPF rules can log expected failures. This chain only matches
echo-request, though.

Can other ICMPv6 packets from dead:1::99 still reach the logging rule that
load_ruleset() installed on nsrouter?

load_ruleset() {
    ...
	        fib saddr . iif oif missing counter log prefix "$netns nft_rpfilter: " drop
    ...
}

In most of the states this block sets up, dead:1::99 does not resolve to
veth0. In "retain non-default route" and "retain nonmatching interface
group" the lookup picks dead:1::/64 dev veth1. In the six reject loop
states, nft_fib6_eval() returns with no oif.

The block itself looks like it produces such a packet:

1. The first ping makes ns1 send an NS from dead:1::99. ndisc_recv_ns() on
   nsrouter creates a NUD_STALE neighbour entry. Sending the NA reply moves
   it to NUD_DELAY with a DELAY_PROBE_TIME (5s) timer.

2. All echo-requests are dropped, so nothing confirms the entry. About 5s
   later neigh_timer_handler() moves it to NUD_PROBE, and nsrouter sends a
   unicast NS to dead:1::99.

3. ns1 answers through ndisc_send_na(). The target is its own address, so
   the NA is sourced from dead:1::99:

   net/ipv6/ndisc.c:ndisc_send_na() {
       ...
	ifp = ipv6_get_ifaddr(dev_net(dev), solicited_addr, dev, 1);
	if (ifp) {
		src_addr = solicited_addr;
       ...
   }

4. The NA does not match the echo-request chain. The RPF rule checks it
   under the current pref 100/101 rules, then logs and drops it. With
   ucast_probes=3, nsrouter retries at about +1s and +2s.

ns1's own NUD probes do not seem to cause this. In the NUD_PROBE state,
ndisc_send_ns() uses the link-local source, and the "to dead:1::99" rules
do not match that. The trigger is nsrouter's NUD_DELAY to NUD_PROBE
transition.

Right after the block, check_drops() greps the global dmesg:

    test_ping 10.0.2.1 dead:2::1 || exit 1
    check_drops

Would this print "FAIL: rpfilter did drop packets" and set ret=1?

That would also hide the later "PASS: fib expression did not cause
unwanted packet drops" message, even when every suppression check passed.
It only happens if the block runs longer than about 5s after the first
probe. That seems plausible on slow or debug kernels such as KASAN or
lockdep builds.

Some possible ways to avoid it:

- Install permanent neighbour entries on both sides while the block runs.
- Temporarily flush or replace the logging inet prerouting rule.
- Also drop nd-neighbor-advert from dead:1::99 in the priority -1 chain.

An accept in the priority -1 chain would not help, because the priority 0
base chain still runs.

[ ... ]

> @@ -724,6 +756,42 @@ test_fib_vrf()

[ ... ]

> +# Reject routes use loopback (group 0) and must remain terminal.
> +ip -net "$nsrouter" -6 route flush table 128
> +for suppress in suppress_prefixlength suppress_ifgroup; do
> +	ip -net "$nsrouter" -6 rule del pref 100
> +	ip -net "$nsrouter" -6 rule add pref 100 to dead:1::99 table 128 "$suppress" 0 || exit 1
> +	for type in blackhole unreachable prohibit; do
> +		ip -net "$nsrouter" -6 route replace "$type" default table 128 || exit 1
> +		test_fib_suppress "retain $type ($suppress)" "$type" type || ret=1
> +	done
> +done

[Severity: Low]

This loop only creates legacy reject route types. Should it also cover a
route that uses a blackhole nexthop object?

The companion commit "ipv6: honor rule suppression in raw FIB lookups"
reads the result flags in fib6_rule_suppress(). Its commit message says
this is so blackhole nexthop objects are covered too:

net/ipv6/fib6_rules.c:fib6_rule_suppress() {
    ...
	if (arg->lookup_ptr == fib6_table_lookup) {
		/* Raw lookups report reject routes without an error. */
		if (res->fib6_flags & RTF_REJECT)
			return false;
    ...
}

Blackhole, unreachable and prohibit routes have RTF_REJECT both in
f6i->fib6_flags and in the result. A blackhole nexthop sets it only in the
result, in __find_rr_leaf():

net/ipv6/route.c:__find_rr_leaf() {
    ...
			if (nexthop_is_blackhole(f6i->nh)) {
				res->fib6_flags = RTF_REJECT;
				res->fib6_type = RTN_BLACKHOLE;
    ...
}

This means the new checks cannot tell res->fib6_flags apart from
res->f6i->fib6_flags.

Suppose the guard were changed to read the FIB entry flags. A blackhole
nexthop default route in table 128 would then be suppressed, and the lookup
would fall through to table 129 (veth0). The tests here would still pass.

Something like this inside the suppress loop might cover it:

    ip -net "$nsrouter" nexthop add id N blackhole
    ip -net "$nsrouter" -6 route replace default nhid N table 128
    test_fib_suppress "retain blackhole nexthop ($suppress)" blackhole type

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261110131638.15004-3-leonard%40lausen.nl