if_setrdomain could potentially call if_clone_create recursively in the create

rdomain case leading to locking issues and lots of headscratching. turns out
the only case where if_setrdomain could actually create an rdomain and thus
end up with that pattern is the ioctl path.
make if_setrdomain never create an rdomain, return error if it doesn't exist
already, introduce if_createrdomain, and adjust the ioctl path to use it.
ok sashan bluhm claudio

diff --git a/sys/net/if.c b/sys/net/if.c
index 0a6c415..f4978c1 100644 (file)
--- a/sys/net/if.c
+++ b/sys/net/if.c
@@ -1,4 +1,4 @@
-/*     $OpenBSD: if.c,v 1.561 2018/09/09 10:09:06 henning Exp $        */
+/*     $OpenBSD: if.c,v 1.562 2018/09/10 16:07:20 henning Exp $        */
 /*     $NetBSD: if.c,v 1.35 1996/05/07 05:26:04 thorpej Exp $  */
 
 /*
@@ -132,6 +132,7 @@
 void   if_attachsetup(struct ifnet *);
 void   if_attachdomain(struct ifnet *);
 void   if_attach_common(struct ifnet *);
+int    if_createrdomain(int, struct ifnet *);
 int    if_setrdomain(struct ifnet *, int);
 void   if_slowtimo(void *);
 
@@ -1734,11 +1735,38 @@ if_setlladdr(struct ifnet *ifp, const uint8_t *lladdr)
        return (0);
 }
 
+int
+if_createrdomain(int rdomain, struct ifnet *ifp)
+{
+       int error;
+       struct ifnet *loifp;
+       char loifname[IFNAMSIZ];
+       unsigned int unit = rdomain;
+
+       if (!rtable_exists(rdomain) && (error = rtable_add(rdomain)) != 0)
+               return (error);
+       if (!rtable_empty(rdomain))
+               return (EEXIST);
+
+       /* Create rdomain including its loopback if with unit == rdomain */
+       snprintf(loifname, sizeof(loifname), "lo%u", unit);
+       error = if_clone_create(loifname, 0);
+       if ((loifp = ifunit(loifname)) == NULL)
+               return (ENXIO);
+       if (error && (ifp != loifp || error != EEXIST))
+               return (error);
+
+       rtable_l2set(rdomain, rdomain, loifp->if_index);
+       loifp->if_rdomain = rdomain;
+
+       return (0);
+}
+
 int
 if_setrdomain(struct ifnet *ifp, int rdomain)
 {
        struct ifreq ifr;
-       int error, createerr, up = 0, s;
+       int error, up = 0, s;
 
        if (rdomain < 0 || rdomain > RT_TABLEID_MAX)
                return (EINVAL);
@@ -1748,35 +1776,8 @@ if_setrdomain(struct ifnet *ifp, int rdomain)
            (ifp->if_index == rtable_loindex(ifp->if_rdomain)))
                return (EPERM);
 
-       /*
-        * Create the routing table if it does not exist, including its
-        * loopback interface with unit == rdomain.
-        */
-       if (!rtable_exists(rdomain) || rtable_empty(rdomain)) {
-               struct ifnet *loifp;
-               char loifname[IFNAMSIZ];
-               unsigned int unit = rdomain;
-
-               snprintf(loifname, sizeof(loifname), "lo%u", unit);
-               createerr = if_clone_create(loifname, 0);
-               if (createerr && createerr != EEXIST)
-                       return (createerr);
-
-               if ((loifp = ifunit(loifname)) == NULL)
-                       return (ENXIO);
-
-               if (!rtable_exists(rdomain)) {
-                       error = rtable_add(rdomain);
-                       if (error) {
-                               if (createerr != EEXIST)
-                                       if_clone_destroy(loifname);
-                               return (error);
-                       }
-               }
-
-               rtable_l2set(rdomain, rdomain, loifp->if_index);
-               loifp->if_rdomain = rdomain;
-       }
+       if (!rtable_exists(rdomain))
+               return (ESRCH);
 
        /* make sure that the routing table is a real rdomain */
        if (rdomain != rtable_l2(rdomain))
@@ -2066,7 +2067,9 @@ ifioctl(struct socket *so, u_long cmd, caddr_t data, struct proc *p)
                if ((error = suser(p)) != 0)
                        break;
                NET_LOCK();
-               error = if_setrdomain(ifp, ifr->ifr_rdomainid);
+               error = if_createrdomain(ifr->ifr_rdomainid, ifp);
+               if (!error || error == EEXIST)
+                       error = if_setrdomain(ifp, ifr->ifr_rdomainid);
                NET_UNLOCK();
                break;