Re: kern/55164 (net/ndp/t_ndp:ndp_rtm test case fails again)

[kre%NetBSD.org@localhost]

Synopsis: net/ndp/t_ndp:ndp_rtm test case fails again

State-Changed-From-To: open->analyzed
State-Changed-By: kre%NetBSD.org@localhost
State-Changed-When: Sun, 12 Apr 2020 07:31:02 +0000
State-Changed-Why:
I said:

  I am still testing to confirm that the info is actually in the RTM_MISS
  packet when it is sent to the route command, and is just not being
  printed (rather than being missing completely in the msg from the kernel).

Well, it turns out that it isn't.   The issue was caused by this commit
to src/sys/netinet6/nd6.c (cut/paste from cvsweb, with irrelevant stuff
deleted)

  Revision 1.268 / (download) - annotate - [select for diffs],
	Fri Apr 3 14:04:27 2020 UTC (8 days, 16 hours ago) by christos
  Diff to previous 1.267 (colored)

  PR/55030: Avoid locking against myself panic by moving the icmp error outside
  the lock. Thanks ozaki-r!

It turns out that the icmp error call (icmp_error2()) is what set the
address to be used in the RTM_MISS message, that is, the sequence of the
relevant address now is (from nd6_llinfo_timer() in netinet6/nd6.c):

        struct in6_addr mdaddr6 = zeroin6_addr;

                if (!IN6_IS_ADDR_UNSPECIFIED(&mdaddr6)) {
                        sockaddr_in6_init(&dsin6, &mdaddr6, 0, 0, 0);
                        sa = sin6tosa(&dsin6);
                } else
                        sa = NULL;
                rt_clonedmsg(RTM_MISS, sa, sin6tosa(&tsin6), NULL, ifp);

	if (m) {        
                icmp6_error2(m, ICMP6_DST_UNREACH,
                    ICMP6_DST_UNREACH_ADDR, 0, ifp, &mdaddr6);
        }       

It is easy to see that mdaddr6 is still zeroin6_addr which is
IN6_IS_ADDR_UNSPECIFIED() and hence sa == NULL when rt_clonedmsg()
is called.   That address is the "author" address for the RTM_MISS
message sent to the routing socket, and is omitted when it is NULL.

The icmp6_error2() call used to come before the rt_clonedmsg() sequence,
set mdaddr6 to be the source addr of the packet, which usually not have
been an unspecified address (and certainly not in the case in the t_ndp
ndp_rtm test) sa would thus not be null, and would be included in the
RTM_MISS message.

The easy for for the test (if the objective is simply to make the test
succeed) would be to do:

--- t_ndp.sh    15 Mar 2020 21:15:25 -0000      1.38
+++ t_ndp.sh    12 Apr 2020 07:03:15 -0000
@@ -471,8 +471,8 @@
        $DEBUG && cat $file
 
        hdr="RTM_MISS.+<DONE>"
-       what="<DST,GATEWAY,AUTHOR>"
-       addr="$IP6DST_FAIL1 link#2 $IP6SRC"
+       what="<DST,GATEWAY>"
+       addr="$IP6DST_FAIL1 link#2"
        atf_check -s exit:0 -o match:"$hdr" -o match:"$what" -o match:"$addr" \
                cat $file


and simply not expect the AUTHOR flag, or its address, to be present
(they certainly will not be with the current kernel).

I won't commit that, as I suspect that's not the correct resolution,
as at the very least netinet6/nd6.c needs more work - it is 100% pointless
to test an address we know is 0 to see if it is unspecified (it is).
So, if the eventual decision is to drop the author info from this particular
RTM_MISS (an ndp lookup failure) then the rt_clonedmsg() call should
simply have NULL instead of sa as its src parameter, and all the stuff
calculating sa can go away.

Otherwise, a reasonable method needs to be found to get the src addr
and set it in mdaddr6 without calling icmp_error2() and hence doing
the "locking against myself".

One way to do that might be to make

	struct mbuf *
	icmp6_error3(/*same params as icmp_error2)
		/* or some more intelligent name than that! */
	{
		/* everything that is in icmp6_error2()
		   except the final icmp6_error() call */

		return m;

	 out:
		m_freem(m);
		return NULL;
	}

and make icmp6_error2() become

	void
	icmp6_error2(/*same params as it has now*/)
	{
		if (m = icmp6_error3(/* pass down all params *.))
			icmp6_error(m, type, code, param);
	}

and then in nd6_llinfo_timer() where the rt_clonedmsg(RTM_MISS,...)
call is made, separately call icmp6_error3() early (without the
icmp6_error() call there is no locking issue, I believe) and then
later, icmp6_error() directly.   There's probably more work required
for any other cases where icmp6_error2() ends up being now called that
might need to be changed if that call turns into icmp6_error() instead.

But I am going to leave that to Roy, Christos, and Ozaki-R to sort
out, as they're the ones involved in this.   For now, I will return
ownership of this PR to kern-bug-people as I have no idea whether
it is possible to set it to 3 different developers at once (if that's
OK, someone can assign this PR to roy christos and ozaki-r

Rather than inventing a bool, the new function could just return m
(mbuf *) when it hasn't been freed, and NULL when it has - that would
save inventing a new variable to save he bool (the updated icmp6_error2()
wouldn't care either way).