From: mvs <mvs@openbsd.org>
Date: Fri, 26 Mar 2021 22:41:06 +0000 (+0000)
Subject: Push kernel lock within rtable_add(9) and rework it to return 0 in the
X-Git-Url: http://artulab.com/gitweb/?a=commitdiff_plain;h=51772eb6ca0378a11bd1f4965abf57cb7fb874bc;p=openbsd

Push kernel lock within rtable_add(9) and rework it to return 0 in the
case when requested table is already exists.

Except initialization time, route_output() and if_createrdomain() are the
only paths where we call rtable_add(9). We check requested table existence
by rtable_exists(9) and it's not the error condition if the table exists.
Otherwise we are trying to create requested table by rtable_add(9). Those
paths are kernel locked so concurrent thread can't create requested table
just after rtable_exists(9) check. Also rtable_add(9) has internal
rtable_exists(9) check and in this case the table existence assumed as
EEXIST error. This error path is never reached.

We are going to unlock PF_ROUTE sockets. This means route_output() will
not be serialized with if_createrdomain() and concurrent thread could
create requested table. Table existence check and creation should be
serialized and it makes sense to do this within rtable_add(9). This time
kernel lock is used for this so it pushed down to rtable_add(9). The
internal rtable_exists(9) check was modified and table existence is not
error now.

Since the external rtable_exists(9) check is useless it was removed from
if_createrdomain(). It still exists in route_output() path because the
logic is more complicated here.

ok mpi@
---

diff --git a/share/man/man9/rtable_add.9 b/share/man/man9/rtable_add.9
index f82b22a3491..05d91ccb44d 100644
--- a/share/man/man9/rtable_add.9
+++ b/share/man/man9/rtable_add.9
@@ -1,4 +1,4 @@
-.\"     $OpenBSD: rtable_add.9,v 1.8 2016/11/14 10:32:46 mpi Exp $
+.\"     $OpenBSD: rtable_add.9,v 1.9 2021/03/26 22:41:06 mvs Exp $
 .\"
 .\" Copyright (c) 2011 Bret S. Lambert <blambert@openbsd.org>
 .\" All rights reserved.
@@ -15,7 +15,7 @@
 .\" ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF
 .\" OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
 .\"
-.Dd $Mdocdate: November 14 2016 $
+.Dd $Mdocdate: March 26 2021 $
 .Dt RTABLE_ADD 9
 .Os
 .Sh NAME
@@ -83,10 +83,6 @@ can be called during autoconf, from process context, or from interrupt context.
 may fail with:
 .Pp
 .Bl -tag -width Er -compact
-.It Bq Er EEXIST
-A routing table with ID of
-.Fa id
-already exists.
 .It Bq Er ENOMEM
 Memory could not be allocated to extend the list of routing domains.
 .El
diff --git a/sys/net/if.c b/sys/net/if.c
index e5c5e65129d..c11710b75fc 100644
--- a/sys/net/if.c
+++ b/sys/net/if.c
@@ -1,4 +1,4 @@
-/*	$OpenBSD: if.c,v 1.639 2021/03/20 17:08:57 florian Exp $	*/
+/*	$OpenBSD: if.c,v 1.640 2021/03/26 22:41:06 mvs Exp $	*/
 /*	$NetBSD: if.c,v 1.35 1996/05/07 05:26:04 thorpej Exp $	*/
 
 /*
@@ -1780,7 +1780,7 @@ if_createrdomain(int rdomain, struct ifnet *ifp)
 	char loifname[IFNAMSIZ];
 	unsigned int unit = rdomain;
 
-	if (!rtable_exists(rdomain) && (error = rtable_add(rdomain)) != 0)
+	if ((error = rtable_add(rdomain)) != 0)
 		return (error);
 	if (!rtable_empty(rdomain))
 		return (EEXIST);
diff --git a/sys/net/rtable.c b/sys/net/rtable.c
index d88544e2a22..4d00cd101f7 100644
--- a/sys/net/rtable.c
+++ b/sys/net/rtable.c
@@ -1,4 +1,4 @@
-/*	$OpenBSD: rtable.c,v 1.73 2021/03/10 10:21:48 jsg Exp $ */
+/*	$OpenBSD: rtable.c,v 1.74 2021/03/26 22:41:06 mvs Exp $ */
 
 /*
  * Copyright (c) 2014-2016 Martin Pieuchot
@@ -195,15 +195,15 @@ rtable_add(unsigned int id)
 	struct dommp	*dmm;
 	sa_family_t	 af;
 	unsigned int	 off, alen;
-	int		 i;
-
-	KERNEL_ASSERT_LOCKED();
+	int		 i, error = 0;
 
 	if (id > RT_TABLEID_MAX)
 		return (EINVAL);
 
+	KERNEL_LOCK();
+
 	if (rtable_exists(id))
-		return (EEXIST);
+		goto out;
 
 	for (i = 0; (dp = domains[i]) != NULL; i++) {
 		if (dp->dom_rtoffset == 0)
@@ -217,8 +217,10 @@ rtable_add(unsigned int id)
 			rtmap_grow(id + 1, af);
 
 		tbl = rtable_alloc(id, alen, off);
-		if (tbl == NULL)
-			return (ENOMEM);
+		if (tbl == NULL) {
+			error = ENOMEM;
+			goto out;
+		}
 
 		map = srp_get_locked(&afmap[af2idx[af]]);
 		map->tbl[id] = tbl;
@@ -233,8 +235,10 @@ rtable_add(unsigned int id)
 	/* Use main rtable/rdomain by default. */
 	dmm = srp_get_locked(&afmap[0]);
 	dmm->value[id] = 0;
+out:
+	KERNEL_UNLOCK();
 
-	return (0);
+	return (error);
 }
 
 void *