Re: CVS commit: src/sys (and netbsd-8 stalled pullup request #390)

[Roy Marples <roy%marples.name@localhost>]

On 27/11/2017 20:27, Robert Elz wrote:
>    | > I still don't believe that the way it works is the way it should,
>    | > but that's a topic for another day.
>    |
>    | Are you talking about the functional change or the code itself?
>
> Perhaps both.   There are 3 changes I would suggest to what is there now,
> one of which I suspect that you will agree with (and might be considered
> just a change to the code itself, though it does make a functional difference,)
> one which I thought you agreed with, as it was the way that the code was
> written before Friday, and one which I am fairly confident that you do not
> agree with... (yet anyway!)
>
> Easiest first, you changed the rules for source addr selection,
> in6_select_best_ia() is/was written to follow the rules in RFCmumble
> almost exactly.
>
> First, it eliminates any candidate addresses that are simply invalid (that's
> still being done) then it has a very particular order in which it
> compares to potential addresses to see which is best.
>
> You changed detached/tentative from "simply invalid" to "valid but not
> to be preferred" but left the test in the part of the code that is to
> be dealing with invalid addresses, rejecting them - that's why you needed
> the test for ia_best != NULL embedded in the new test - none of the later
> tests have that, as if you're comparing two potentially valid addresses,
> then you must have two of them, there's no need to test for this being the
> first one (there is a separate test that says "if this is the first valid
> addr we have seen it must be the best one so far" that deals with that case.
>
> Further, by leaving it there, you're preferring a non detached/tentative
> addr with the wrong scope to a detached/tentative addr with the correct
> score (ie: a fully valid link-local wins over a detached/tentative global)
> - I doubt that you really intended to do that.
>
> The new test should be moved down to rule 3, probably just before the
> deprecated addr test, that is, a deprecated addr which is still valid
> should be preferred over a detached (or tentative) addr  - detached I
> will come back to in the next section, but for tentative this means that
> if an old addr has become deprecated, and we're in the process of generating
> a new one, but DaD has not completed yet, we keep using the old addr
> until the new one is no longer tentative, and then switch (the way your
> code is now would also do that - but because the check is much too violent.)
> This is why you might consider this just to be a coding change (with functional
> side effects.)

I don't see a problem with moving the detached/tentative test there 
other than what rule do we show when the debug code for it is enabled?
I certainly don't want tentative/detached outright denied as it stops 
userland binding to those addresses, so the intent of the change is a 
good thing still.

> But there are also arguments for doing those tests the other way, and
> preferring a tentative addr over a deprecated addr for new connections
> (the new connection might last beyond the period that the deprecated addr
> remains valid, whereas the tentative addr is very unlikely to fail DaD and
> should remain valid considerably longer.)   I might even make that choice
> depend upon the upper level protocol - prefer deprecated for icmp/udp sending,
> and prefer tentative for tcp (and for ucp connecting - but the kernel doesn't
> make the choice in the latter case, the app does, so we can ignore that.)
>
> Second - the one I thought you agreed with - is the treatment of deprecated
> addresses.  I preferred the way the code was last week (before Friday),
> that is, exactly the opposite to the complaint from Robert Swindells.
> Addresses belong to interfaces, if the interface is non-functional, its
> address is (or should be) non-functional as well - including for
> applications on the local host.
>
> The "route to lo0" that he kept bringing up is just an implementation
> artifact, it makes it easier to handle packets addressed to the local host,
> as they all end up at lo0 just using the routing code, we don't need extra
> tests all over the place with "or if this address is mine..." to handle that,
> and aside from that (internal) use, is (or should be) irrelevant to
> everything else.
>
> But if another host cannot connect to the addr I have assigned to
> interface xx0 then I should not be able to either - it makes local testing
> that all my interfaces are working much easier (just ping all of my
> addresses and make sure they're all replying, exactly the same way I
> would test them from a remote host - which means I can write an application
> that validates the network, and not need to special case testing for
> addresses that happen to belong to the host running the tests.)
>
> So, I would return all the detached address tests to the way they were
> early last week, it is simply better that way.

This was my initial reaction as well, but I do understand Robert's 
position that this is also change in behavior from prior NetBSD 
versions. As it's impossible to have both conditions satisfied this 
could be controlled via sysctl.

> Third - the one I think you do not agree with - tentative addresses are
> not invalid, they're more like deprecated addresses than detached ones,
> and should be treated just like deprecated addresses (as an address we
> would prefer not to use if possible, but not in any way invalid.)

Correct, I do not agree with that :)

> As above, for some uses a tentative addr is even better to use than a
> deprecated one (whereas for other uses the opposite applies) and of course
> an addr that is neither deprecated nor tentative is better than both.
>
> So, while it is entirely reasonable to forbid sending or receiving
> packets from (or to) detached addresses, packets to/from tentative
> addrs should simply be allowed always (we know we *must* allow sending
> from a tentative addr to implement DaD correctly - we must defend the
> tentative addr from other hosts' attempts to also DaD the same addr
> at the same time, and that means replying to an NDP probe claimimg
> that we own the addr already (as they will reply to our probe saying they
> own the addr already) and that means sending from the tentative addr.
> [Binding to a tentative addr should be allowed, always, as well.]

DaD probes are sent from the unspecified address, not the tentative 
address. They are received on the all-nodes multicast address.
So no, we don't have to send or receive *anything* on it for DaD to work.

> But it is more than that, a tentative addr is 99.9% (perhaps much higher
> in the v6 world) to be validated in a second or so (maybe 2 or 3 if the
> host admin is paranoid - and which should probably be done to all manually
> assigned addrs - humans make far more mistakes than RNGs or mac addr
> assignments) and delaying their use just creates complications.
>
> I would allow treating them just like we treat deprecated addresses, with
> a sysctl to say whether or not we should ever use them (which I would just
> turn on in sysctl.conf and you can leave turned off if you like) - provided
> that we make sure we have not broken DaD - including DaD implemented in a
> userland daemon rather than in the kernel, regardless of the state of the
> sysctl -- and I certainly support use of "ifconfig -w" to wait for the
> addresses to have been validated before using them, if that is what the app
> needs.

Aside from the issue of having too many knobs anyway, another one would 
not hurt to allow using a tentative address.
The last remaining question is do we want this one knob to cover IPv6 
and IPv6 or per protocol?

> Aside: is there a way with that to wait for a particular addr to become valid?
> The (ifconfig) man page reads like there isn't - not even to wait for addrs
> on a particular interface, but the code might be different - if not, then I
> think we need to allow the -i ifconfig option to accompany with -w (and -W)
> to apply to just one interface, and perhaps also an addr option, to wait for
> just that addr - I'd have thought that would be useful when adding a new
> addr just assigned by a dhcp server, we don't really care about any other
> addresses that might, already, or while we're waiting, enter DaD, just
> the one being assigned right now.

The -w/-W flag takes a list of all addresses from all interfaces and 
just waits for the tentative/detached flag to disappear from them.
You can't single out an interface or address.

This also brings up a question - the original rc.d/network script has 
always slept until DaD finishes, probably because you could never bind 
to a tentative address.
Now that we can, do we still need to?

To be clear, I'm considering removing the DaD wait check from 
rc.d/network entirely.

> Lastly, I certainly do not think that any of these changes should be pulled
> up to -8 until all of this is sorted out.   If ever.

martin@ has already stalled the ticket.

Roy