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

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@gmail.com (local)

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

[ ... ]

[ ... ]

[ ... ]

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

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

[ ... ]

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

 
 }

[ ... ]

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