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

Wed Feb 22 05:30:26 UTC 2017

jhujhiti_adjectivism.org added a comment.

  I've updated the patch to address these points aside from the open question about determining neighborship.

  I think the testcase for default router advertisement should look something like this. I'm more familiar with epair interfaces than tap, but I assume the concept is the same.

  First, enable IPv6 forwarding and rfc6204w3. The former is necessary for rtadvd to send RAs and the latter is a workaround to prevent the kernel from disabling default router selection on a router itself.

    # sysctl net.inet6.ip6.rfc6204w3=1
    net.inet6.ip6.rfc6204w3: 0 -> 1
    # sysctl net.inet6.ip6.forwarding=1
    net.inet6.ip6.forwarding: 0 -> 1

  Create epair interfaces and configure "a" as the router and "b" as the client.

    # ifconfig epair0 create
    epair0a
    # ifconfig epair0a fib 1
    # ifconfig epair0a inet6 fd01::1/64 
    # ifconfig epair0a up
    # ifconfig epair0b fib 2
    # ifconfig epair0b inet6 -ifdisabled accept_rtadv
    # ifconfig epair0b up
    # rtadvd epair0a
    # rtsol epair0b

  Sanity check - if epair0b received the RA, it should have an address in fd01::/64.

    # ifconfig epair0b
    epair0b: flags=8843<UP,BROADCAST,RUNNING,SIMPLEX,MULTICAST> metric 0 mtu 1500
    	options=8<VLAN_MTU>
    	ether 02:ff:30:00:05:0b
    	inet6 fe80::ff:30ff:fe00:50b%epair0b prefixlen 64 scopeid 0x5 
    	inet6 fd01::ff:30ff:fe00:50b prefixlen 64 autoconf 
    	nd6 options=23<PERFORMNUD,ACCEPT_RTADV,AUTO_LINKLOCAL>
    	media: Ethernet 10Gbase-T (10Gbase-T <full-duplex>)
    	status: active
    	fib: 2
    	groups: epair 

  Verify that the router was learned and that the default route was installed.

    # ndp -rn
    fe80::ff:e0ff:fe00:40a%epair0b if=epair0b, flags=, pref=medium, expire=28m4s
    # netstat -rnf inet6 -F2
    Routing tables (fib: 2)

    Internet6:
    Destination                       Gateway                       Flags     Netif Expire
    ::/96                             ::1                           UGRS        lo0
    default                           fe80::ff:e0ff:fe00:40a%epair0b UG     epair0b
    ::1                               lo0                           UHS         lo0
    ::ffff:0.0.0.0/96                 ::1                           UGRS        lo0
    fd01::/64                         link#5                        U       epair0b
    fd01::ff:30ff:fe00:50b            link#5                        UHS         lo0
    fe80::/10                         ::1                           UGRS        lo0
    fe80::%epair0b/64                 link#5                        U       epair0b
    fe80::ff:30ff:fe00:50b%epair0b    link#5                        UHS         lo0
    ff02::/16                         ::1                           UGRS        lo0

  The same thing should work simultaneously in FIBs 3 and 4 with epair1a and 1b (and the scope on the default route in FIB 4 should be %epair1b). The MAC address of the epair interfaces can be set to something predictable to make identifying the route easier.

INLINE COMMENTS

> asomers wrote in route.c:2224
> 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.

Ah, of course...

A second look while re-implementing this revealed that rtinit1() actually does the correct thing when passed RT_ALL_FIBS for both additions and deletions, so I've simply reverted my change to line 2224.

> asomers wrote in icmp6.c:2147
> 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.

> At line 2159, do we know the fib of the receiving interface?

It looks like the mbuf passed to icmp6_reflect() will always have the FIB set based on the receiving interface, so we can just use that! Can a more experienced set of eyes confirm?

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

I agree - all callers of in6_selectsrc_socket() unconditionally dereference inp prior to passing it, so this check for NULL is definitely redundant. I've removed it.

> jhujhiti_adjectivism.org wrote in nd6.c:1295
> 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.

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.

> nd6.c:1353
>  	 */
>  	dstaddr = ifa_ifwithdstaddr((const struct sockaddr *)addr, RT_ALL_FIBS);
>  	if (dstaddr != NULL) {

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.

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