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

1 files changed, 35 insertions, 32 deletions

diff --git a/sys/net/if.c b/sys/net/if.c
index 0a6c4157ff9..f4978c1112d 100644
--- 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 *);
 
@@ -1735,10 +1736,37 @@ if_setlladdr(struct ifnet *ifp, const uint8_t *lladdr)
 }
 
 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;