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

[netdev-bot+sashiko]

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

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

- [Medium] The new FAILED-seed cases assert an in-place neighbour state
  transition but the script has no synchronization guaranteeing that the…
- [Low] The two new `accept_untracked_na=2` cases
  (`test_unsolicited_na_combination 0 2 1 failed 1` at line 275 and `...…
- [Low] The new FAILED-neighbor recovery cases verify only the resulting
  NUD state and the `extern_learn` marker, not that the link-layer…

--

selftests: net: test untracked NA recovery of FAILED neighbors

This extends ndisc_unsolicited_na_test.sh with a seeded NUD_FAILED
neighbour entry and six new matrix rows covering accept_untracked_na modes
1 and 2 plus the drop_unsolicited_na and forwarding gates. The seed is
marked extern_learn and ndisc_evict_nocarrier is disabled so the entry
survives until the advertisement arrives.

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

A few questions below about the synchronization and the coverage of the new
rows.

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

[ ... ]

> @@ -20,7 +26,9 @@ HOST_INTF="veth-host"
>  ROUTER_INTF="veth-router"
>  
>  ROUTER_ADDR="2000:20::1"
> -HOST_ADDR="2000:20::2"
> +HOST_ADDR_IN_NETWORK="2000:20::2"
> +HOST_ADDR_OUT_OF_NETWORK="2000:21::2"
> +HOST_ADDR="${HOST_ADDR_IN_NETWORK}"
>  SUBNET_WIDTH=64
>  ROUTER_ADDR_WITH_MASK="${ROUTER_ADDR}/${SUBNET_WIDTH}"
>  HOST_ADDR_WITH_MASK="${HOST_ADDR}/${SUBNET_WIDTH}"

[ ... ]

> @@ -140,24 +150,77 @@ verify_ndisc() {

[ ... ]

> +	if [ -n "${expected_state}" ]; then
> +		neigh_show_output=$(${IP_ROUTER} neigh show \
> +			to "${HOST_ADDR}" dev "${ROUTER_INTF}")
> +		if [[ " ${neigh_show_output} " != \
> +				*" ${expected_state} "* ]]; then
> +			return 1
> +		fi
> +		if [ "${initial_state}" = "failed" ]; then
> +			[[ "${neigh_show_output}" == *"extern_learn"* ]]
> +		fi

[Severity: Low]
Should verify_ndisc() also check the link-layer address that the
advertisement carried?

The seed in test_unsolicited_na_common() deliberately creates the entry
without one:

	${IP_ROUTER} neigh replace "${HOST_ADDR}" dev "${ROUTER_INTF}" \
		nud failed extern_learn

while the recovery path in net/ipv6/ndisc.c:ndisc_recv_na() pulls the target
link-layer address out of the options and passes it down:

	lladdr = ndisc_opt_addr_data(ndopts.nd_opts_tgt_lladdr, dev);
	...
	__ndisc_update(dev, neigh, lladdr,
		       new_state, update_flags, neigh_failed,
		       NDISC_NEIGHBOUR_ADVERTISEMENT, &ndopts);

Since the only assertions here are the state substring and extern_learn,
would a change that moves FAILED -> STALE but installs no lladdr, or an
incorrect one, still report OK?  Comparing the resulting lladdr against the
host veth MAC would close that gap.

>  	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
>  }
>  
>  test_unsolicited_na_common()
>  {
> +	local same_subnet=${5:-1}
> +	local neigh_show_output
> +
> +	if [ "${same_subnet}" -eq 1 ]; then
> +		HOST_ADDR="${HOST_ADDR_IN_NETWORK}"
> +	else
> +		HOST_ADDR="${HOST_ADDR_OUT_OF_NETWORK}"
> +	fi
> +	HOST_ADDR_WITH_MASK="${HOST_ADDR}/${SUBNET_WIDTH}"

[Severity: Low]
Can this switch actually isolate the mode 2 in-prefix gate?

Moving HOST_ADDR from 2000:20::2 to 2000:21::2 changes the source address of
the advertisement, its target address, and the key of the seeded neighbour
entry all at once, while the router keeps 2000:20::1/64.

The advertisement under test comes from addrconf_dad_completed(), and
ndisc_send_na() uses the same address for both fields when it is local:

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

so saddr == target for every packet this test generates.  The kernel gate is
source based:

net/ipv6/ndisc.c:accept_untracked_na() {
	case 2:
		return !!ipv6_chk_prefix(saddr, idev->dev);
}

and Documentation/networking/ip-sysctl.rst describes mode 2 in terms of "the
source IP address".  Would a change that tested msg->target instead of saddr
leave both new mode 2 rows green?  Keeping HOST_ADDR fixed and instead
varying the prefix configured on the router side would exercise the gate
without also moving the target and the entry key.

>  
>  	# Setup the test bed, but keep links down
> -	setup $1 $2 $3
> +	setup "$1" "$2" "$3"
> +
> +	if [ "${4:-absent}" = "failed" ]; then
> +		if ! ${IP_ROUTER} neigh replace "${HOST_ADDR}" \
> +				dev "${ROUTER_INTF}" \
> +				nud failed extern_learn; then
> +			echo "Unable to create NUD_FAILED neighbor entry"
> +			return 1
> +		fi

[ ... ]

> @@ -165,22 +228,35 @@ 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
>  
>  	# Verify the neighbour table
> -	verify_ndisc $1 $2 $3
> +	verify_ndisc "$1" "$2" "$3" "$4" "${same_subnet}"

[Severity: Medium]
The comment says "add a delay to ensure neighbour processing is done", but
link_up, start_tcpdump and verify_ndisc run back to back with no sleep and no
retry.  Where is the delay?

start_tcpdump() blocks on:

	${IP_ROUTER_EXEC} timeout 15s \
		tcpdump --immediate-mode -tpni ${ROUTER_INTF} -c 1 \
		"icmp6 && icmp6[0] == 136 && src ${HOST_ADDR}"

but the AF_PACKET tap that satisfies -c 1 is fed from ptype_all delivery in
__netif_receive_skb_core(), which runs ahead of the state change the test
looks at:

ipv6_rcv() -> icmpv6_rcv() -> ndisc_recv_na() -> __ndisc_update() ->
neigh_update()

Does tcpdump exiting really order against that update, given verify_ndisc()
takes a single un-retried snapshot of ip neigh show?

There is a second effect for the new rows.  In the four expect-FAILED cases:

	test_unsolicited_na_combination 0 0 1 failed
	test_unsolicited_na_combination 0 1 0 failed
	test_unsolicited_na_combination 1 1 1 failed
	test_unsolicited_na_combination 0 2 1 failed 0

the expected value FAILED is exactly the state the entry was seeded with, so
"still FAILED" and "the advertisement was never processed" look identical.
Would a bounded poll for the expected state, or an explicit settle, make
these rows able to distinguish the two?

Related question on ordering of the capture: tcpdump is started after
link_up, so can the single unsolicited advertisement emitted at DAD
completion be missed if the fork and exec are slow?  In that case timeout
exits 124 while set -e is in effect inside start_tcpdump, which aborts the
whole script rather than failing one row.  Would arming the capture before
link_up avoid that?

>  
>  }

[ ... ]

> @@ -193,6 +269,16 @@ test_unsolicited_na_combinations() {
>  	test_unsolicited_na_combination 1 0 1
>  	test_unsolicited_na_combination 1 1 0
>  	test_unsolicited_na_combination 1 1 1
> +
> +	# Expect FAILED entry to become STALE
> +	test_unsolicited_na_combination 0 1 1 failed
> +	test_unsolicited_na_combination 0 2 1 failed 1
> +
> +	# Expect FAILED entry to remain FAILED
> +	test_unsolicited_na_combination 0 0 1 failed
> +	test_unsolicited_na_combination 0 1 0 failed
> +	test_unsolicited_na_combination 1 1 1 failed
> +	test_unsolicited_na_combination 0 2 1 failed 0
>  }

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