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

[asomers (Alan Somers)]

asomers requested changes to this revision.
asomers added a subscriber: bz.
asomers added a comment.
This revision now requires changes to proceed.


  In addition to the issues I mentioned inline, could you please also update the review summary to include the full commit message?  Try to mention every scenario where you're intentionally changing behavior.  I'll try to add ATF testcases for all of them.

INLINE COMMENTS

> route.c:2224
>  		/* We do support multiple FIBs. */
> -		fib = RT_ALL_FIBS;
> +		fib = rt_add_addr_allfibs ? RT_ALL_FIBS : ifa->ifa_ifp->if_fib;
>  		break;

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.

> icmp6.c:2147
> +	M_SETFIB(m, fibnum);
> +
>  	if (srcp == NULL) {

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?

> in6.c:180
>  		rt.rt_flags |= RTF_UP;
> -	/* Announce arrival of local address to all FIBs. */
> -	rt_newaddrmsg(cmd, &ia->ia_ifa, 0, &rt);
> +	fibnum = rt_add_addr_allfibs ? RT_ALL_FIBS : ia62ifa(ia)->ifa_ifp->if_fib;
> +	/* Announce arrival of local address to this FIB. */

Should be `V_rt_add_addr_allfibs`

> in6.c:2127
>  	in6_splitscope(&sin6->sin6_addr, &dst, &scopeid);
> -	error = fib6_lookup_nh_basic(RT_DEFAULT_FIB, &dst, scopeid, 0, 0, &nh6);
> +	fibnum = rt_add_addr_allfibs ? RT_DEFAULT_FIB : ifp->if_fib;
> +	error = fib6_lookup_nh_basic(fibnum, &dst, scopeid, 0, 0, &nh6);

Should be V_rt_add_addr_allfibs

> in6_src.c:566
>  
>  	fibnum = (inp != NULL) ? inp->inp_inc.inc_fibnum : RT_DEFAULT_FIB;
>  	retifp = NULL;

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.

> nd6.c:198
>  	rtinfo.rti_addrs = RTA_DST | RTA_GATEWAY;
> +	fibnum = rt_add_addr_allfibs ? RT_ALL_FIBS : ifp->if_fib;
>  	rt_missmsg_fib(type, &rtinfo, RTF_HOST | RTF_LLDATA | (

Should be V_rt_add_addr_allfibs

> nd6.c:1295
>  	info.rti_info[RTAX_DST] = (struct sockaddr *)&rt_key;
> -
> -	/* Always use the default FIB here. XXME - why? */
> -	fibnum = RT_DEFAULT_FIB;
> +	fibnum = ifp->if_fib;
>  

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

> nd6.h:472
>  void defrouter_reset(void);
> -void defrouter_select(void);
> +void defrouter_select(int fibnum);
>  void defrouter_ref(struct nd_defrouter *);

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`.

> nd6_nbr.c:265
>  
> -		/* Always use the default FIB. */
> -		if (rib_lookup_info(RT_DEFAULT_FIB, (struct sockaddr *)&dst6,
> +		if (rib_lookup_info(ifp->if_fib, (struct sockaddr *)&dst6,
>  		    0, 0, &info) == 0) {

As with `nd6_is_new_addr_neighbor`, we should get @bz's review here.

> nd6_rtr.c:735
>  void
> -defrouter_select(void)
> +defrouter_select(int fibnum)
>  {

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.

> nd6_rtr.c:806
>  		IF_AFDATA_RLOCK(installed_dr->ifp);
>  		if ((ln = nd6_lookup(&installed_dr->rtaddr, 0, installed_dr->ifp)) &&
>  		    ND6_IS_LLINFO_PROBREACH(ln) &&

While you're here, you may as well fix the style on this line.  It's > 80 chars.

> nd6_rtr.c:1758
>  
> +	if(rt_add_addr_allfibs) {
> +		fibnum = 0;

Should be V_rt_add_addr_allfibs

> nd6_rtr.c:1854
>  
> +		if (!rt_add_addr_allfibs &&
> +		    opr->ndpr_ifp->if_fib != pr->ndpr_ifp->if_fib)

Should be V_rt_add_addr_allfibs

> nd6_rtr.c:1936
>  
> +	if (rt_add_addr_allfibs) {
> +		fibnum = 0;

Should be V_rt_add_addr_allfibs

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