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/[email protected] 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
