On Fri, Mar 26, 2021 at 01:16:32PM +0100, Klemens Nanni wrote:
> On Wed, Mar 17, 2021 at 04:47:36PM +0100, Klemens Nanni wrote:
> > `ifconfig mp* mplslabel N' validates the label both in ifconfig and each
> > driver's ioctl handler, but there is one case where all drivers install
> > a route without looking at the label at all.
> >
> > SIOCSLIFPHYRTABLE in all three drivers just validates the rdomain and
> > sets the label to itself (0 after clone) such that the route is
> > (re)installed accordingly.
> >
> > The problem is that respective routines, e.g. mpe_set_label(), do not
> > validate the label themselves but instead expect the callee to do so;
> > all of them do so except the SIOCSLIFPHYRTABLE ioctl handler case:
> >
> > case SIOCSLIFPHYRTABLE:
> > if (ifr->ifr_rdomainid < 0 ||
> > ...) {
> > error = EINVAL;
> > break;
> > }
> >
> > if (sc->sc_rdomain != ifr->ifr_rdomainid) {
> > error = mpe_set_label(sc, sc->sc_smpls.smpls_label,
> > ifr->ifr_rdomainid);
> > }
> > break;
> >
> > Given a pristine setup:
> >
> > # ifconfig mpw
> > mpw: no such interface
> > # route -n show -mpls
> > Routing tables
> > # route -nT2 show -mpls
> > Routing tables
> >
> > that means we can install routes for the explicit NULL label in
> > non-default routing tables:
> >
> > # ifconfig mpw0 tunneldomain 2
> > # ifconfig mpw0 | grep label
> > mpls: label (unset) pwe3 remote label (unset) nocw nofat
> > # route -nT2 show -mpls
> > Routing tables
> >
> > MPLS:
> > In label Out label Op Gateway Flags Refs Use
> > Mtu Prio Interface
> > 0 - POP fe:e1:ba:db:eb:89 UTl 0 0
> > - 1 mpw0
> >
> > and can never clean them up without reboot:
> >
> > # ifconfig mpw0 -tunneldomain
> > # route -nT2 show -mpls
> > Routing tables
> >
> > MPLS:
> > In label Out label Op Gateway Flags Refs Use
> > Mtu Prio Interface
> > 0 - POP fe:e1:ba:db:eb:89 UTl 0 0
> > - 1 (null)
> > # ifconfig mpw0 destroy
> > # route -nT2 show -mpls
> > Routing tables
> >
> > MPLS:
> > In label Out label Op Gateway Flags Refs Use
> > Mtu Prio Interface
> > 0 - POP fe:e1:ba:db:eb:89 UTl 0 0
> > - 1 (null)
>
> After that you can create new interfaces but won't be able to use them
> with that rdomain since the stale route blocks creating the new one in
> that rtable:
>
> # ifconfig mpw0 create
> # ifconfig mpw0 tunneldomain 2
> ifconfig: SIOCSLIFPHYRTABLE: File exists
>
> > My first approach was removing the label check from mp*_clone_destroy()
> > to unconditionally delete the respective route, but my understanding is
> > that we must not install such bogus routes to begin with.
> >
> > So fix this by adding the inverted check to the mp*_set_label() routines
> > after updating the rdomain and before route reinstallation to skip those
> > nonexistent ones, i.e. to avoid installing bogus ones.
> >
> > I'd argue this is better factored out into some label validating helper
> > function that can then be used from multiple callees, but I don't want
> > to touch so much code without a proper setup at hand -- plus, the diff
> > is much easier to understand and review this way.
> >
> >
> > Note that I've only tested this with ifconfig(8) and route(8), I have
> > no ldpd(8) or proper MPLS link to test with.
> >
> > Feedback? Objections? OK?
>
> Ping.
>
>
> Index: if_mpw.c
> ===================================================================
> RCS file: /cvs/src/sys/net/if_mpw.c,v
> retrieving revision 1.61
> diff -u -p -r1.61 if_mpw.c
> --- if_mpw.c 17 Mar 2021 18:53:25 -0000 1.61
> +++ if_mpw.c 18 Mar 2021 16:43:10 -0000
> @@ -172,6 +172,10 @@ mpw_set_route(struct mpw_softc *sc, uint
> sc->sc_smpls.smpls_label = label;
> sc->sc_rdomain = rdomain;
>
> + /* only install with a label or mpw_clone_destroy() will ignore it */
> + if (sc->sc_smpls.smpls_label == MPLS_LABEL2SHIM(0))
> + return 0;
> +
> error = rt_ifa_add(&sc->sc_ifa, RTF_MPLS|RTF_LOCAL,
> smplstosa(&sc->sc_smpls), sc->sc_rdomain);
> if (error != 0)
> Index: if_mpe.c
> ===================================================================
> RCS file: /cvs/src/sys/net/if_mpe.c,v
> retrieving revision 1.99
> diff -u -p -r1.99 if_mpe.c
> --- if_mpe.c 18 Mar 2021 14:47:17 -0000 1.99
> +++ if_mpe.c 18 Mar 2021 16:43:11 -0000
> @@ -339,6 +339,10 @@ mpe_set_label(struct mpe_softc *sc, uint
> sc->sc_smpls.smpls_label = label;
> sc->sc_rdomain = rdomain;
>
> + /* only install with a label or mpe_clone_destroy() will ignore it */
> + if (sc->sc_smpls.smpls_label == MPLS_LABEL2SHIM(0))
> + return 0;
> +
> error = rt_ifa_add(&sc->sc_ifa, RTF_MPLS|RTF_LOCAL,
> smplstosa(&sc->sc_smpls), sc->sc_rdomain);
> if (error)
> Index: if_mpip.c
> ===================================================================
> RCS file: /cvs/src/sys/net/if_mpip.c,v
> retrieving revision 1.14
> diff -u -p -r1.14 if_mpip.c
> --- if_mpip.c 17 Mar 2021 14:30:09 -0000 1.14
> +++ if_mpip.c 18 Mar 2021 16:43:11 -0000
> @@ -170,6 +170,10 @@ mpip_set_route(struct mpip_softc *sc, ui
> sc->sc_smpls.smpls_label = shim;
> sc->sc_rdomain = rdomain;
>
> + /* only install with a label or mpip_clone_destroy() will ignore it */
> + if (sc->sc_smpls.smpls_label == MPLS_LABEL2SHIM(0))
> + return 0;
> +
> error = rt_ifa_add(&sc->sc_ifa, RTF_MPLS | RTF_LOCAL,
> smplstosa(&sc->sc_smpls), sc->sc_rdomain);
> if (error) {
>
OK claudio
--
:wq Claudio
