Re: [PATCH net-next v3 2/2] selftests: net: test untracked NA recovery of FAILED neighbors

["Lawrence Lee" <lfqlee314@gmail.com>]

On Tue Sep 22, 2026 at 10:38 PM UTC,  wrote:
> [Severity: Low]
> tcpdump is now a background child, and cleanup_tcpdump() is the only place
> that kills and reaps it. Can that child be left running when the script
> leaves through a path that never reaches cleanup()?
>
> cleanup_tcpdump() is reached only from cleanup(), which runs at the tail of
> test_unsolicited_na_combination() after log_test() returns:
>
> 	log_test "${rc}" 0 "${test_msg[*]}"
> 	cleanup
>
> log_test() can exit first when the script is run with -p or -P and the
> operator answers 'q':
>
> 	if [ "${PAUSE}" = "yes" ]; then
> 		echo
> 		echo "hit enter to continue, 'q' to quit"
> 		read a
> 		[ "$a" = "q" ] && exit 1
> 	fi
>
> setup() also runs under set -e, so a failing sysctl there (for example
> accept_untracked_na=2 on a kernel without mode 2) exits the shell directly.
> There is no trap cleanup EXIT INT TERM anywhere in the file, so in those
> cases the two namespaces from setup_ns and both mktemp files are left
> behind, and if the failure happens between start_tcpdump() and
> wait_tcpdump() (for instance the new slowwait 5 grep for "listening on"
> timing out, or host_link_up failing) a live timeout 15s tcpdump keeps a
> packet socket and a netns reference until its own timeout expires.
>
> Would adding a trap that calls cleanup on EXIT, INT and TERM be reasonable
> here?
>

I would lean towards no, since tcpdump already times out on its own and 
any orphaned tempfiles or netns are a pre-existing issue. No change 
planned.

> [Severity: Low]
> This isn't a problem introduced by this patch, but the absent-entry branch
> of verify_ndisc() still discards the exit status of ip neigh show and only
> tests that the output is empty. If the command itself fails, is the empty
> output then reported as a pass without the neighbour table ever having been
> read? The same pattern is present in the baseline version of the function,
> so this is pre-existing; the new expected_state branch fails closed. Would
> checking the exit status of the query in the else branch be worth folding
> in while this function is being touched?
>

Pre-existing issue, will leave as-is

> [Severity: Medium]
> The new rc=$? capture also repairs a reporting bug that existed before this
> patch. In the baseline the sequence was:
>
> 	test_unsolicited_na_common $1 $2 $3
> 	test_msg=("test_unsolicited_na: "
> 		"drop_unsolicited_na=$1 "
> 		"accept_untracked_na=$2 "
> 		"forwarding=$3")
> 	log_test $? 0 "${test_msg[*]}"
>
> The array assignment between the helper call and log_test sets $? to 0
> (none of the assigned words contain a command substitution), so log_test
> always received 0 and all eight existing combinations reported OK no matter
> what verify_ndisc returned. Capturing rc immediately after
> test_unsolicited_na_common() changes the pass/fail semantics of those eight
> pre-existing cases so they can now actually fail.
>
> The changelog describes only the new FAILED-neighbour coverage and does not
> mention that the existing reporting was broken, and there is no Fixes: tag
> (git blame points the log_test $? line at f9a2fb73318eb).
>
> Would it make sense to split this into its own patch with a Fixes: tag, so
> it can be applied and backported independently of the new mode 2 and
> FAILED-state coverage, which needs kernel features not present in older
> trees?
>

An entirely new patch submission seems like overkill for this. I'll 
update the commit message to mention this change and add the Fixes tag.

pw-bot: changes-requested