[Differential] D9451: Constrain IPv6 interface routes to each FIB

[asomers (Alan Somers)]

asomers added a comment.


  In https://reviews.freebsd.org/D9451#196364, @jhujhiti_adjectivism.org wrote:
  
  > 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.
  
  
  I think we can make the testcase a little simpler.  We can create two tap devices in different fibs and bridge them with socat.  Then we can configure one to broadcast router advertisements, and check that the other one sets up its routes correctly.  We could create a second pair of tap interfaces too, to test having multiple simultaneous subnets configured with SLAAC.  I can write the testcase, if you tell me what to check and how to configure an interface to broadcast router advertisements.

INLINE COMMENTS

> jhujhiti_adjectivism.org wrote in route.c:2224
> 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?

Just three lines below this point, `rtinit` calls `rtinit1`, and `rtinit1` checks `V_rt_add_addr_allfibs` as almost the first thing that it does.  So `rtinit` shouldn't also check it.

> jhujhiti_adjectivism.org wrote in icmp6.c:2147
> 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).

At line 2159, do we know the fib of the receiving interface?  If so, we should probably use it.  The interface fib is the fib to use when forwarding packets, so it makes sense to use it for ICMP replies, too.

Your observation about RFC 6724 is interesting, but I don't think we should act on it as part of this commit.  It should go in separately.

> jhujhiti_adjectivism.org wrote in in6_src.c:566
> 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.

You're correct; I was confused about inpcb.  However, there is no need to consider the interface fib.  The interface fib really only matters for forwarding packets and for automatically adding routes to routing tables.  Also, I think that the `inp != NULL` check is superfluous.  AFAICT, there's no way for `inp` to be `NULL` right here.

> jhujhiti_adjectivism.org wrote in nd6.h:472
> 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 :)

Definitely better to preserver the old `defrouter_select` as a wrapper around the new function.

> jhujhiti_adjectivism.org wrote in nd6_rtr.c:735
> 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.

Looping sounds like a good idea.

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: jch, bz, imp, ae, freebsd-net-list