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

[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 (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] Undisclosed fix of a pre-existing false-PASS defect, without a
  Fixes: tag.
- [Low] No `trap cleanup EXIT INT TERM` is registered, while the patch
  converts tcpdump from a foreground command into a backgrounded child…

Pre-existing issues:
- [Low] Claimed issue (validated as a pre-existing Low-severity
  robustness nit): in verify_ndisc(), the absent-entry branch (lines…

--

selftests: net: test untracked NA recovery of FAILED neighbors

This extends the accept_untracked_na selftest to seed a FAILED neighbour
entry before triggering an unsolicited NA, and checks the resulting state,
link-layer address and extern_learn marker for modes 1 and 2. It also
reworks the tcpdump handling so the capture is armed before the host link
comes up and is waited for before the neighbour table is inspected.

Link: https://lore.kernel.org/r/20260813233344.445265-1-lfqlee314@gmail.com

A few questions below.

> diff --git a/tools/testing/selftests/net/ndisc_unsolicited_na_test.sh b/tools/testing/selftests/net/ndisc_unsolicited_na_test.sh
> index 5db69dad0cfc..ba9e670b5149 100755
> --- a/tools/testing/selftests/net/ndisc_unsolicited_na_test.sh
> +++ b/tools/testing/selftests/net/ndisc_unsolicited_na_test.sh

[ ... ]

> @@ -102,24 +115,39 @@ setup()
>  }
>  
>  start_tcpdump() {
> -	set -e
> -	tcpdump_stdout=`mktemp`
> -	tcpdump_stderr=`mktemp`
> +	tcpdump_stdout=$(mktemp) || return 1
> +	tcpdump_stderr=$(mktemp) || return 1
>  	${IP_ROUTER_EXEC} timeout 15s \
>                  tcpdump --immediate-mode -tpni ${ROUTER_INTF} -c 1 \
>                  "icmp6 && icmp6[0] == 136 && src ${HOST_ADDR}" \
> -                > ${tcpdump_stdout} 2> /dev/null
> -	set +e
> +                > "${tcpdump_stdout}" 2> "${tcpdump_stderr}" &
> +	tcpdump_pid=$!
> +
> +	slowwait 5 grep -q "listening on ${ROUTER_INTF}" "${tcpdump_stderr}"
> +}
> +
> +wait_tcpdump()
> +{
> +	local rc
> +
> +	wait "${tcpdump_pid}"
> +	rc=$?
> +	tcpdump_pid=
> +
> +	return "${rc}"
>  }
>  
>  cleanup_tcpdump()
>  {
> -	set -e
> -	[[ ! -z  ${tcpdump_stdout} ]] && rm -f ${tcpdump_stdout}
> -	[[ ! -z  ${tcpdump_stderr} ]] && rm -f ${tcpdump_stderr}
> +	if [ -n "${tcpdump_pid}" ]; then
> +		kill "${tcpdump_pid}" 2> /dev/null
> +		wait "${tcpdump_pid}" 2> /dev/null
> +	fi

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

> +	[ -n "${tcpdump_stdout}" ] && rm -f "${tcpdump_stdout}"
> +	[ -n "${tcpdump_stderr}" ] && rm -f "${tcpdump_stderr}"
>  	tcpdump_stdout=
>  	tcpdump_stderr=
> -	set +e
> +	tcpdump_pid=
>  }
>  
>  cleanup()

[ ... ]

> @@ -129,58 +157,145 @@ cleanup()

[ ... ]

> +	if [ -n "${expected_state}" ]; then
> +		neigh_show_output=$(${IP_ROUTER} neigh show \
> +			to "${HOST_ADDR}" dev "${ROUTER_INTF}")

[ ... ]

>  	else
> -		# Neighbour entry expected to be absent for all other cases
> +		neigh_show_output=$(${IP_ROUTER} neigh show \
> +			to "${HOST_ADDR}" dev "${ROUTER_INTF}")
>  		[[ -z ${neigh_show_output} ]]
>  	fi
>  }

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

>  
>  test_unsolicited_na_common()
>  {

[ ... ]

> -	# Bring the link up, wait for the NA,
> -	# and add a delay to ensure neighbour processing is done.
> -	link_up
> -	start_tcpdump
> +	# Arm the capture before bringing up the host and starting DAD.
> +	router_link_up || return 1
> +	start_tcpdump || return 1
> +	host_link_up || return 1
> +
> +	# Closing tcpdump's packet socket calls synchronize_net(), so waiting
> +	# for it also waits for receive processing of the captured NA.
> +	wait_tcpdump || return 1
>  
>  	# Verify the neighbour table
> -	verify_ndisc $1 $2 $3
> +	verify_ndisc "$1" "$2" "$3" "$4" "${same_subnet}"
>  
>  }
>  
>  test_unsolicited_na_combination() {
> -	test_unsolicited_na_common $1 $2 $3
> +	local initial_state=${4:-absent}
> +	local same_subnet=${5:-1}
> +	local rc
> +
> +	test_unsolicited_na_common "$1" "$2" "$3" "${initial_state}" \
> +		"${same_subnet}"
> +	rc=$?
>  	test_msg=("test_unsolicited_na: "
>  		"drop_unsolicited_na=$1 "
>  		"accept_untracked_na=$2 "
>  		"forwarding=$3")
> -	log_test $? 0 "${test_msg[*]}"
> +	if [ "${initial_state}" = "failed" ]; then
> +		test_msg+=("initial_state=failed")
> +	fi
> +	if [ "$2" -eq 2 ]; then
> +		test_msg+=("same_subnet=${same_subnet}")
> +	fi
> +	log_test "${rc}" 0 "${test_msg[*]}"
>  	cleanup
>  }

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

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/cover.1789734885.git.lfqlee314%40gmail.com