From 4cc6c382050db818c73b4b4df1930d38556647ff Mon Sep 17 00:00:00 2001
From: henning <henning@openbsd.org>
Date: Mon, 10 Sep 2018 16:07:20 +0000
Subject: [PATCH] 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

---
 sys/net/if.c | 67 +++++++++++++++++++++++++++-------------------------
 1 file 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 *);
 
@@ -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;
 
-- 
2.20.1