jhujhiti_adjectivism.org marked 6 inline comments as done. jhujhiti_adjectivism.org added a comment.
As I mentioned in the PR, this is my first attempt at kernel work, so I very much appreciate the comments. I'll go ahead and update the review summary at my next opportunity. The testcase that is difficult to write is unfortunately the one that tests the most complex code paths: configuring multiple prefixes and default routers from router advertisements in different FIBs. I'm not sure it's possible to write this with a single machine (sending RAs to itself)? For what it's worth, my test case while working has been to attach the machine's NIC (which is connected to a network sending RAs) to a bridge, and then connect several epair interfaces in different FIBs to the bridge. This test also proved that the code has no problems with overlapping subnets in different FIBs. INLINE COMMENTS > asomers wrote in route.c:2224 > This line is going to break vimage, and it's also the wrong place to check > the tunable. See rtinit1, line 2032. Please revert this chunk. You're right - this was mistakenly left over from my first (poor) attempt at the patch. I'll revert and replace with rtinit1() in the applicable places in netinet6/in6.c. Can I ask what you mean by this being the wrong place to check the tunable? Is this a stylistic thing, or is there a technical reason I'm not aware of? > asomers wrote in icmp6.c:2147 > It seems like this code selects the incoming interface's FIB for routing an > ICMPv6 response. Do I understand that correctly? If so, why is similar code > not needed in icmp_reflect? You do understand correctly, and trying to answer why equivalent code is not needed in icmp_reflect() caused me to realize that this logic is mostly redundant. icmp6_reflect() callers copy the FIB when creating the mbuf of the packet to reflect, and RT_DEFAULT_FIB will get passed to in6_selectsrc_addr() below in *almost* all of the same cases as it did before. The one case that seems worth thinking about more is the case where the packet is destined to an anycast address. The logic on lines 2125-2126 avoid selecting that address as a source. Since srcp will not be set in this case, we will end up selecting a source address for the response from the default FIB on line 2159 (assuming my change had been reverted). This seems like a case where we should attempt to reply from some other address in the same FIB. Does this logic make sense? If so, I'll rework the FIB selection logic above to be less redundant. Alternatively, I notice that a note in RFC 6724, Appendix B suggests that anycast source addresses may be valid (https://tools.ietf.org/html/rfc6724 page 30, section 3), so perhaps avoiding selecting the anycast address as a source for the reply is no longer necessary (although reworking source address selection in general is definitely not in scope for my change). > asomers wrote in in6_src.c:566 > I think this is a bug. If we have a socket, then the source address should > be based on the socket's FIB, not the interface's. Socket FIBs default to > the FIB of the process, but can be manually changed. I must be misunderstanding the inpcb structure -- does this not represent the socket (and therefore inp->inp_inc.inc_fibnum represent the socket FIB)? Even if my understanding is correct, I do think there's a bug here: it seems like we should prefer the socket's FIB, then the interface's FIB, and only the default as a last resort. > asomers wrote in nd6.c:1295 > This looks wrong to me. If we're trying to determine whether a given IPv6 > address is a neighbor, that shouldn't depend on the interface's default fib. > OTOH, the old code looks wrong too, because it only checks the default FIB, > not all FIBs. The right answer is probably to loop through all FIBs. I > would ask for @bz's review of this section I think this makes sense... I assume the old code used the default FIB because all interface addresses were added to all FIBs, so it didn't matter which FIB was searched. > asomers wrote in nd6.h:472 > This is a KPI change. We generally can't break existing KPIs. Your options > are: > > 1. Is `defrouter_select` new in FreeBSD 12? If so, go ahead and change it > 2. Has `defrouter_select`'s signature already changed since FreeBSD 11.0? If > so, go ahead and change it. > 3. Otherwise, leave it alone, and add a new function called > `defrouter_select_fib`. I was under the impression that KPIs were mutable on CURRENT - although I also understand that a KPI change would block an MFC. Are the rules around KPI changes on CURRENT more strict than I realized? I realized while writing this comment that I can simply make the old function loop over all FIBs, so maintaining KPI stability works :) > asomers wrote in nd6_rtr.c:735 > Please document the new parameter. Also, I'm not sure the code is correct > when `fibnum == RT_ADDR_ALLFIBS`. In that case, no interface will ever have > `if_fib == fibnum`. You should probably fall back to legacy behavior in that > case. I would expect a value RT_ADDR_ALLFIBS to either (1) be an invalid argument (assertion failure?) or (2) call ourselves recursively to loop over all FIBs rather than falling back to old behavior, which doesn't make a lot of sense -- old behavior wasn't FIB-aware and could trample over multiple selected routers. REPOSITORY rS FreeBSD src repository REVISION DETAIL https://reviews.freebsd.org/D9451 EMAIL PREFERENCES https://reviews.freebsd.org/settings/panel/emailpreferences/ To: jhujhiti_adjectivism.org, #network, asomers, bz Cc: bz, imp, ae, freebsd-net-list _______________________________________________ freebsd-net@freebsd.org mailing list https://lists.freebsd.org/mailman/listinfo/freebsd-net To unsubscribe, send any mail to "freebsd-net-unsubscr...@freebsd.org"
