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@

diff --git a/sys/net/art.c b/sys/net/art.c
index 8fec52a..e809c39 100644 (file)
--- 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 24fce7d..77bd951 100644 (file)
--- 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);
 }