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

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

> [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.
>
I will update the tests to verify the lladdr


> [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.
>
Will update the tests to cover one source address outside the prefix and 
one inside.

> [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?
>
Will change the tests to start the capture prior to bringing up the host 
side