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) {
