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

[asomers (Alan Somers)]

asomers added a comment.


  This review is starting to look pretty good.  But in addition to the few things I mentioned inline, there's one other change that you need to make: you get to clear the `atf_expect_fail` statements from tests/sys/netinet/fibs_test.sh.

INLINE COMMENTS

> jhujhiti_adjectivism.org wrote in nd6.c:1295
> I'm rethinking this a little bit. While I think it's true that the most correct way to answer the question "Is this address a neighbor?" is to consider addresses in any FIB, I'm not sure it's necessary. Since the caller is specifying which interface this address should be a neighbor on, it's safe to consider only that interface's FIB because interface routes (ie., those routes with neighbors) will always be added there.

Remember, the interface fib only matters for forwarding packets.  It's totally valid for an interface to have multiple addresses assigned, each of which is on a different fib.  So, to correctly determine whether `addr` is a neighbor of `ifp`, we must either

1. Loop over all fibs, and check whether `addr` is a neighbor of `ifp` on any of them, or
2. Loop over all addresses assigned to `ifp`, and check whether `addr` is a neighbor of `ifp` on that address's fib.  I'm guessing that this will be the slower option, because an interface can have arbitrarily many addresses

> jhujhiti_adjectivism.org wrote in nd6.c:1353
> Should we pass the ifp->if_fib here? We compare ifps on line 1355, so searching all FIBs is usually fine, but if we have two identical addresses in two different FIBs, we could find the wrong one here and end up returning false.

The original code seems too complicated.  I think it should go a little like this (locks elided):

  if (ifp->if_flags & IFF_POINTOPOINT) {
          TAILQ_FOREACH(ifa, &ifp->if_addrhead, ifa_link) {
  			if (ifa->ifa_addr->sa_family != addr->sa_family)
  				continue;
  			if (ifa->ifa_dstaddr != NULL &&
  			    sa_equal(addr, ifa->ifa_dstaddr)) {
  				return (1);
  			}
  		}
  }

No unnecessary looping over either fibs or interfaces.

> nd6.c:1310
>  		if ((pr->ndpr_stateflags & NDPRF_ONLINK) == 0) {
>  			/* Always use the default FIB here. */
>  			dst6 = (const struct sockaddr *)&pr->ndpr_prefix;

This comment is incorrect now.

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