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
_______________________________________________
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"
