rework art_walk so it will behave in an mpsafe world.

art_walk now explicitly takes the same lock used to serialise change
made via rtable_insert and _delete, so it can safely adjust the
refcnts on tables while it recurses into them. they need to still
exist when returning out of the recursion.

it uses srps to access nodes and drops the lock before calling the
callback function. this is because some callbacks sleep (eg, copyout
in the sysctl code that dumps an rtable to userland), which you
shouldnt hold a lock accross. other callbacks attempt to modify
the rtable (eg, marking routes as down when then interface theyre
on goes down), which tries to take the lock again, which probably
wont work in the future.

ok jmatthew@ mpi@

2 files changed, 76 insertions, 50 deletions

diff --git a/sys/net/art.c b/sys/net/art.c
index 8fec52a0a4c..e809c395c01 100644
--- a/sys/net/art.c
+++ b/sys/net/art.c
@@ -1,4 +1,4 @@
-/*	$OpenBSD: art.c,v 1.19 2016/06/14 04:42:02 jmatthew Exp $ */
+/*	$OpenBSD: art.c,v 1.20 2016/06/22 06:32:32 dlg Exp $ */
 
 /*
  * Copyright (c) 2015 Martin Pieuchot
@@ -78,10 +78,13 @@ struct art_node		*art_table_insert(struct art_root *, struct art_table *,
 			     int, struct art_node *);
 struct art_node		*art_table_delete(struct art_root *, struct art_table *,
 			     int, struct art_node *);
-void			 art_table_ref(struct art_root *, struct art_table *);
+struct art_table	*art_table_ref(struct art_root *, struct art_table *);
 int			 art_table_free(struct art_root *, struct art_table *);
 int			 art_table_walk(struct art_root *, struct art_table *,
 			     int (*f)(struct art_node *, void *), void *);
+int			 art_walk_apply(struct art_root *,
+			     struct art_node *, struct art_node *,
+			     int (*f)(struct art_node *, void *), void *);
 void			 art_table_gc(void *);
 void			 art_gc(void *);
 
@@ -565,10 +568,11 @@ art_table_delete(struct art_root *ar, struct art_table *at, int i,
 	return (an);
 }
 
-void
+struct art_table *
 art_table_ref(struct art_root *ar, struct art_table *at)
 {
 	at->at_refcnt++;
+	return (at);
 }
 
 static inline int
@@ -604,42 +608,44 @@ art_table_free(struct art_root *ar, struct art_table *at)
 int
 art_walk(struct art_root *ar, int (*f)(struct art_node *, void *), void *arg)
 {
+	struct srp_ref		 sr;
 	struct art_table	*at;
 	struct art_node		*node;
-	int			 error;
-
-	KERNEL_ASSERT_LOCKED();
+	int			 error = 0;
 
+	KERNEL_LOCK();
 	at = srp_get_locked(&ar->ar_root);
-	if (at == NULL)
-		return (0);
+	if (at != NULL) {
+		art_table_ref(ar, at);
 
-	/*
-	 * The default route should be processed here because the root
-	 * table does not have a parent.
-	 */
-	node = srp_get_locked(&at->at_default);
-	if (node != NULL) {
-		error = (*f)(node, arg);
-		if (error)
-			return (error);
+		/*
+		 * The default route should be processed here because the root
+		 * table does not have a parent.
+		 */
+		node = srp_enter(&sr, &at->at_default);
+		error = art_walk_apply(ar, node, NULL, f, arg);
+		srp_leave(&sr);
+
+		if (error == 0)
+			error = art_table_walk(ar, at, f, arg);
+
+		art_table_free(ar, at);
 	}
+	KERNEL_UNLOCK();
 
-	return (art_table_walk(ar, at, f, arg));
+	return (error);
 }
 
 int
 art_table_walk(struct art_root *ar, struct art_table *at,
     int (*f)(struct art_node *, void *), void *arg)
 {
-	struct art_node		*next, *an = NULL;
-	struct art_node		*node;
+	struct srp_ref		 sr;
+	struct art_node		*node, *next;
+	struct art_table	*nat;
 	int			 i, j, error = 0;
 	uint32_t		 maxfringe = (at->at_minfringe << 1);
 
-	/* Prevent this table to be freed while we're manipulating it. */
-	art_table_ref(ar, at);
-
 	/*
 	 * Iterate non-fringe nodes in ``natural'' order.
 	 */
@@ -651,12 +657,13 @@ art_table_walk(struct art_root *ar, struct art_table *at,
 		 */
 		for (i = max(j, 2); i < at->at_minfringe; i <<= 1) {
 			next = srp_get_locked(&at->at_heap[i >> 1].node);
-			an = srp_get_locked(&at->at_heap[i].node);
-			if ((an != NULL) && (an != next)) {
-				error = (*f)(an, arg);
-				if (error)
-					goto out;
-			}
+
+			node = srp_enter(&sr, &at->at_heap[i].node);
+			error = art_walk_apply(ar, node, next, f, arg);
+			srp_leave(&sr);
+
+			if (error != 0)
+				return (error);
 		}
 	}
 
@@ -665,28 +672,47 @@ art_table_walk(struct art_root *ar, struct art_table *at,
 	 */
 	for (i = at->at_minfringe; i < maxfringe; i++) {
 		next = srp_get_locked(&at->at_heap[i >> 1].node);
-		node = srp_get_locked(&at->at_heap[i].node);
-		if (!ISLEAF(node))
-			an = srp_get_locked(&SUBTABLE(node)->at_default);
-		else
-			an = node;
 
-		if ((an != NULL) && (an != next)) {
-			error = (*f)(an, arg);
-			if (error)
-				goto out;
+		node = srp_enter(&sr, &at->at_heap[i].node);
+		if (!ISLEAF(node)) {
+			nat = art_table_ref(ar, SUBTABLE(node));
+			node = srp_follow(&sr, &nat->at_default);
+		} else
+			nat = NULL;
+
+		error = art_walk_apply(ar, node, next, f, arg);
+		srp_leave(&sr);
+
+		if (error != 0) {
+			art_table_free(ar, nat);
+			return (error);
 		}
 
-		if (ISLEAF(node))
-			continue;
+		if (nat != NULL) {
+			error = art_table_walk(ar, nat, f, arg);
+			art_table_free(ar, nat);
+			if (error != 0)
+				return (error);
+		}
+	}
 
-		error = art_table_walk(ar, SUBTABLE(node), f, arg);
-		if (error)
-			break;
+	return (0);
+}
+
+int
+art_walk_apply(struct art_root *ar,
+    struct art_node *an, struct art_node *next,
+    int (*f)(struct art_node *, void *), void *arg)
+{
+	int error = 0;
+
+	if ((an != NULL) && (an != next)) {
+		/* this assumes an->an_dst is not used by f */
+		KERNEL_UNLOCK();
+		error = (*f)(an, arg);
+		KERNEL_LOCK();
 	}
 
-out:
-	art_table_free(ar, at);
 	return (error);
 }
 
diff --git a/sys/net/rtable.c b/sys/net/rtable.c
index 24fce7d9ea4..77bd9517012 100644
--- a/sys/net/rtable.c
+++ b/sys/net/rtable.c
@@ -1,4 +1,4 @@
-/*	$OpenBSD: rtable.c,v 1.47 2016/06/14 04:42:02 jmatthew Exp $ */
+/*	$OpenBSD: rtable.c,v 1.48 2016/06/22 06:32:32 dlg Exp $ */
 
 /*
  * Copyright (c) 2014-2015 Martin Pieuchot
@@ -853,16 +853,16 @@ struct rtable_walk_cookie {
 int
 rtable_walk_helper(struct art_node *an, void *xrwc)
 {
+	struct srp_ref			 sr;
 	struct rtable_walk_cookie	*rwc = xrwc;
-	struct rtentry			*rt, *nrt;
+	struct rtentry			*rt;
 	int				 error = 0;
 
-	KERNEL_ASSERT_LOCKED();
-
-	SRPL_FOREACH_SAFE_LOCKED(rt, &an->an_rtlist, rt_next, nrt) {
+	SRPL_FOREACH(rt, &sr, &an->an_rtlist, rt_next) {
 		if ((error = (*rwc->rwc_func)(rt, rwc->rwc_arg, rwc->rwc_rid)))
 			break;
 	}
+	SRPL_LEAVE(&sr);
 
 	return (error);
 }