Re: [PATCH net-next 05/15] selftests: netfilter: nft_queue: check the scope of the hook drop flush

[netdev-bot+sashiko@kernel.org]

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

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

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 new test's failure does not reach the script's exit
  status.
- [Low] The 'keep' case cannot tell the two halves of nf_hook_cmp()
  apart.
- [Low] In hook_drop_case() the exit status of the command under test,
  `ip netns exec "$ns1" nft "$@"` (line 932), is never checked, and the…
- [Low] hook_drop_case() ignores the return value of `busywait
  "$BUSYWAIT_TIMEOUT" hook_drop_all_queued "$packets"` (line 930).

--

selftests: netfilter: nft_queue: check the scope of the hook drop flush

This adds test_hook_drop_scope to nft_queue.sh. The test queues 20 UDP
packets to the nf_queue helper, which holds the verdicts, and deletes a
base chain while the packets wait. It then checks whether the packets
were kept or dropped. It also adds a -H option to nf_queue that holds
verdicts until SIGUSR1 is received.

> diff --git a/tools/testing/selftests/net/netfilter/nft_queue.sh b/tools/testing/selftests/net/netfilter/nft_queue.sh
> index c8d1f2eb41339..a07652813a4b0 100755
> --- a/tools/testing/selftests/net/netfilter/nft_queue.sh
> +++ b/tools/testing/selftests/net/netfilter/nft_queue.sh

[ ... ]

> +hook_drop_case()
> +{

[ ... ]

> +	ip netns exec "$ns1" bash -c \
> +		"for i in \$(seq $packets); do echo x > /dev/udp/127.0.0.1/12345; done"
> +
> +	busywait "$BUSYWAIT_TIMEOUT" hook_drop_all_queued "$packets"

[Severity: Low]
What happens if this busywait times out? On timeout, loopy_wait() in
lib.sh just returns 1. It does not abort the caller:

tools/testing/selftests/net/lib.sh:loopy_wait() {
    ...
		if ((current_time - start_time > timeout_ms)); then
			echo -n "$out"
			return 1
		fi
    ...
}

The return value is ignored here. So the test still deletes the chain
and classifies the result when fewer than 20 packets were queued, for
example when some of the sends were lost before they reached the queue.

The drop classification further down only checks for zero:

	elif [ "$queued" = "0" ] && [ "$delivered" = "0" ]; then
		result="drop"

Could the drop case then report PASS with only part of the batch queued,
or with nothing queued at all? In the keep case the same situation
would be reported as inconsistent.

> +
> +	ip netns exec "$ns1" nft "$@"
> +	queued=$(hook_drop_queued)

[Severity: Low]
Should the exit status of this nft command be checked?

Suppose the deletion fails, or test_hook_drop_scope() passes the wrong
arguments. Then no hook is unregistered and the 20 packets stay queued.
SIGUSR1 releases them, the input counter reaches 20 and nf_queue prints
"20 packets total". The keep case would report PASS even though no
chain was deleted.

Other deletions in this file do check the result, for example in
test_udp_gro_ct():

	if ! ip netns exec "$nsrouter" nft delete table inet udpq; then
		echo "FAIL: Could not delete udpq table"
		ret=1

[ ... ]

> +test_hook_drop_scope()
> +{
> +	# the hook array of another hook point changes, queued packets stay
> +	hook_drop_case keep "base chain removed at another hook point" \
> +		delete chain ip6 unrelated prerouting

[Severity: Low]
Does this keep case test the hook number check and the family check in
nf_hook_cmp() separately? The queued packets are IPv4 (sent to
127.0.0.1) at NF_INET_LOCAL_OUT. The deleted chain differs from them in
both family (ip6) and hook (prerouting):

net/netfilter/nfnetlink_queue.c:nf_hook_cmp() {
    ...
	if (entry->state.hook != hooknum)
		return 0;

	if (pf == NFPROTO_INET)
		return entry->state.pf == NFPROTO_IPV4 ||
		       entry->state.pf == NFPROTO_IPV6;

	return entry->state.pf == pf;
}

If nf_hook_cmp() compared only the hook number, or only the family, it
would still keep the entries and this case would pass. The drop case
matches on both fields, so it doesn't isolate either check.

Would it make sense to add a chain at the same hook in another family
(for example ip6 output)? A chain at another hook in the same family
(for example ip prerouting) would cover the other check.

The testing notes of the previous patch, "netfilter: nf_queue: limit the
hook drop flush to the unregistered hook point", say that deleting an
ip6 table with an output base chain does not drop the packets. That
case isn't encoded in this test.

[ ... ]

> @@ -909,6 +1016,7 @@ test_queue_stress
>  # should be last, adds vrf device in ns1 and changes routes
>  test_icmp_vrf
>  test_queue_removal
> +test_hook_drop_scope
>  
>  # turns router into a bridge
>  test_queue_bridge

[Severity: Medium]
Does a failure in test_hook_drop_scope() reach the exit status of the
script? hook_drop_case() records a failure only by setting the global
ret=1.

test_queue_bridge runs afterwards and calls test_queue() twice.
test_queue() does this:

tools/testing/selftests/net/netfilter/nft_queue.sh:test_queue() {
    ...
	wait
	ret=$?
    ...
}

A bare wait returns 0, so ret is reset to 0 whenever the bridge tests
pass. The script then reports success with exit $ret.

The FAIL line still goes to stderr, but kselftest runners decide pass
or fail from the exit code. The same reset already affects the earlier
tests in this sequence.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261018005638.1776148-2-pablo%40netfilter.org