Last change reworking art_walk() introduced a regression leading to a
of-by-one in rtable_walk().  You can trigger it by running the route(8)
regress test 5:

# cd /usr/src/regress/sbin/route && make rttest 5
# route -T5 -n show
Routing tables

Internet:
Destination        Gateway            Flags   Refs      Use   Mtu  Prio Iface
10.8.1/24          192.0.2.2          UGS        0        0 32768    18 (null)
10.8.1/24          192.0.2.2          GS         0        1 32768    16 (null)

See the two dangling pointers?

Problem is that rtable_walk_helper() have to keep a reference to the next
entry in the multipath chain otherwise we might forget it when purging all
the items.  Here it should be to use the _LOCKED() variant since the helper
will be called inside art_walk() which will grab the mutex.

Diff below also correct the KASSERT() when deleting an entry.  The refcount
*must* be at least 2 at this point: 1 for being in the table and 1 for the
caller.  This should be committed separately.

Comments, ok?

Index: net/art.c
===================================================================
RCS file: /cvs/src/sys/net/art.c,v
retrieving revision 1.20
diff -u -p -r1.20 art.c
--- net/art.c   22 Jun 2016 06:32:32 -0000      1.20
+++ net/art.c   1 Jul 2016 10:06:06 -0000
@@ -706,11 +706,11 @@ art_walk_apply(struct art_root *ar,
 {
        int error = 0;
 
+       KERNEL_ASSERT_LOCKED();
+
        if ((an != NULL) && (an != next)) {
                /* this assumes an->an_dst is not used by f */
-               KERNEL_UNLOCK();
                error = (*f)(an, arg);
-               KERNEL_LOCK();
        }
 
        return (error);
Index: net/rtable.c
===================================================================
RCS file: /cvs/src/sys/net/rtable.c,v
retrieving revision 1.48
diff -u -p -r1.48 rtable.c
--- net/rtable.c        22 Jun 2016 06:32:32 -0000      1.48
+++ net/rtable.c        1 Jul 2016 10:03:52 -0000
@@ -819,7 +819,7 @@ rtable_delete(unsigned int rtableid, str
                npaths++;
 
        if (npaths > 1) {
-               KASSERT(rt->rt_refcnt >= 1);
+               KASSERT(rt->rt_refcnt > 1);
                SRPL_REMOVE_LOCKED(&rt_rc, &an->an_rtlist, rt, rtentry,
                    rt_next);
 
@@ -834,7 +834,7 @@ rtable_delete(unsigned int rtableid, str
        if (art_delete(ar, an, addr, plen) == NULL)
                return (ESRCH);
 
-       KASSERT(rt->rt_refcnt >= 1);
+       KASSERT(rt->rt_refcnt > 1);
        SRPL_REMOVE_LOCKED(&rt_rc, &an->an_rtlist, rt, rtentry, rt_next);
 
        art_put(an);
@@ -853,16 +853,19 @@ 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;
+       struct rtentry                  *rt, *nrt;
        int                              error = 0;
 
-       SRPL_FOREACH(rt, &sr, &an->an_rtlist, rt_next) {
-               if ((error = (*rwc->rwc_func)(rt, rwc->rwc_arg, rwc->rwc_rid)))
+       KERNEL_ASSERT_LOCKED();
+
+       SRPL_FOREACH_SAFE_LOCKED(rt, &an->an_rtlist, rt_next, nrt) {
+               KERNEL_UNLOCK();
+               error = (*rwc->rwc_func)(rt, rwc->rwc_arg, rwc->rwc_rid);
+               KERNEL_LOCK();
+               if (error)
                        break;
        }
-       SRPL_LEAVE(&sr);
 
        return (error);
 }

Reply via email to