Re: CVS commit: src/sys (and netbsd-8 stalled pullup request #390)
On 28/11/2017 16:36, Roy Marples wrote: On 27/11/2017 20:27, Robert Elz wrote: 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 src/sys/netinet6/in6_src.c r1.84 addresses this. Roy
Re: CVS commit: src/sys (and netbsd-8 stalled pullup request #390)
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 c
Re: CVS commit: src/sys (and netbsd-8 stalled pullup request #390)
Date:Mon, 27 Nov 2017 09:42:47 + From:Roy Marples Message-ID: | I don't understand the change to ip_output.c (ie why yours works and | mine doesn't) but as long as it works then I'm happy. That one is easy, ifa_ifwithaddr() needs a struct sockaddr * as its parameter, you were passing it a (cast to struct sockaddr *) struct in6_addr * instead, which is an entirely different beast. That's why the sin6 variable existed in ip6_ifaddrvalid() in the first place, and was applied to the src addr (which was the only one checked before) - fortunately the needs don't overlap, so the same sin6 var could be used for both tests (which saves most of the init that would be needed if a new one was required) - the exceedingly ugly way I made the change was just so the copy of the dest addr into sin6 would be done only when absolutely required. Rewriting the code more would probably allow that to be cleaned up. This type error isn't detected by the compiler because of the use of the sin6tosa() macro - which converts struct sockaddr_in6 * pointers into struct sockaddr * pointers (ie: it adds a cast) ... I considered changing that to be a static inline function, so the compiler could do arg type verification, but at the time going and checking every use (to make sure it is never used with some other sockaddr * type - like a sockaddr_storage perhaps) was too much... (The right fix would be to have a static inline function for each sockaddr pointer type that we want to be able to treat as a generic struct sockaddr pointer, and then always use the appropriate one, but that means lots of code churn I suspect.) | > 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.) 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 no
