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

[Florian Westphal <fw@strlen.de>]

netdev-bot+sashiko@kernel.org <netdev-bot+sashiko@kernel.org> wrote:
> - [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).

I'll do a full quote here, despite not really responding: Antonio, if
you have to send a v2 for the nfnetlink_drop change, then please
consider sending a v2 for this one too. Otherwise I'm not sure its worth
following up with this one.

> 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
>