On Mon, Jun 24, 2019 at 10:12:11AM -0300, Martin Pieuchot wrote:
> On 24/06/19(Mon) 10:10, Claudio Jeker wrote:
> > rt_ifa_add() and rt_ifs_del() confused rtableid and rtlabelid and so the
> > code does not correcly set RTAX_LABEL. It makes no sense to try to compare
> > rdoamin against if_rtlabelid since those are two different things.
> >
> > This should probably make 'ifconfig em0 rtlabel foo' work the way it is
> > intended. OK?
>
> Was the intend to check if `rdomain' passed as an argument is the one of
> the `ifp'? Should the check be changed to (rdomain == ifp->if_rdomain)?
You are talking about the KASSERT() I guess?
> The only places where this isn't true is MPLS code. David do you
> remember why you added this check?
Indeed for MPLS edge interfaces (mpe(4) and friends) the rdomain of the
interface and of the MPLS stack can differ. So the MPLS labels are
added to a different routing domain than the actual interface rdomain.
For example:
ifconfig mpe10 rdomain 10
ifconfig mpe10 mplslabel 42 tunneldomain 7
The mpe10 device is in rdomain 10 but the MPLS traffic arrives on table 7.
So the rt_ifa_add is called with rdomain 7 even though the device is in
table 10.
It is a bit of an abuse of rt_ifa_add() but it makes the code simpler.
> > Index: net/route.c
> > ===================================================================
> > RCS file: /cvs/src/sys/net/route.c,v
> > retrieving revision 1.386
> > diff -u -p -r1.386 route.c
> > --- net/route.c 21 Jun 2019 17:11:42 -0000 1.386
> > +++ net/route.c 24 Jun 2019 07:57:50 -0000
> > @@ -1094,6 +1094,8 @@ rt_ifa_add(struct ifaddr *ifa, int flags
> > uint8_t prio = ifp->if_priority + RTP_STATIC;
> > int error;
> >
> > + KASSERT(rdomain == rtable_l2(rdomain));
> > +
> > memset(&info, 0, sizeof(info));
> > info.rti_ifa = ifa;
> > info.rti_flags = flags;
> > @@ -1102,12 +1104,7 @@ rt_ifa_add(struct ifaddr *ifa, int flags
> > info.rti_info[RTAX_GATEWAY] = sdltosa(ifp->if_sadl);
> > else
> > info.rti_info[RTAX_GATEWAY] = ifa->ifa_addr;
> > -
> > - KASSERT(rdomain == rtable_l2(rdomain));
> > - if (rdomain == rtable_l2(ifp->if_rtlabelid)) {
> > - info.rti_info[RTAX_LABEL] =
> > - rtlabel_id2sa(ifp->if_rtlabelid, &sa_rl);
> > - }
> > + info.rti_info[RTAX_LABEL] = rtlabel_id2sa(ifp->if_rtlabelid, &sa_rl);
> >
> > #ifdef MPLS
> > if ((flags & RTF_MPLS) == RTF_MPLS)
> > @@ -1151,6 +1148,8 @@ rt_ifa_del(struct ifaddr *ifa, int flags
> > uint8_t prio = ifp->if_priority + RTP_STATIC;
> > int error;
> >
> > + KASSERT(rdomain == rtable_l2(rdomain));
> > +
> > if ((flags & RTF_HOST) == 0 && ifa->ifa_netmask) {
> > m = m_get(M_DONTWAIT, MT_SONAME);
> > if (m == NULL)
> > @@ -1166,11 +1165,7 @@ rt_ifa_del(struct ifaddr *ifa, int flags
> > info.rti_info[RTAX_DST] = dst;
> > if ((flags & RTF_LLINFO) == 0)
> > info.rti_info[RTAX_GATEWAY] = ifa->ifa_addr;
> > -
> > - if (rdomain == rtable_l2(ifp->if_rtlabelid)) {
> > - info.rti_info[RTAX_LABEL] =
> > - rtlabel_id2sa(ifp->if_rtlabelid, &sa_rl);
> > - }
> > + info.rti_info[RTAX_LABEL] = rtlabel_id2sa(ifp->if_rtlabelid, &sa_rl);
> >
> > if ((flags & RTF_HOST) == 0)
> > info.rti_info[RTAX_NETMASK] = ifa->ifa_netmask;
> >
>
--
:wq Claudio
