Route timers were not MP safe. Protect the global lists with a

mutex and move the rttimer entries into a temporary list.  Then the
callback and pool put can be called later without holding the mutex.
tested by Hrvoje Popovski; OK claudio@

diff --git a/sys/net/route.c b/sys/net/route.c
index ded733a..3ac9717 100644 (file)
--- a/sys/net/route.c
+++ b/sys/net/route.c
@@ -1,4 +1,4 @@
-/*     $OpenBSD: route.c,v 1.406 2022/04/20 17:58:22 bluhm Exp $       */
+/*     $OpenBSD: route.c,v 1.407 2022/04/28 17:47:41 bluhm Exp $       */
 /*     $NetBSD: route.c,v 1.14 1996/02/13 22:00:46 christos Exp $      */
 
 /*
@@ -1361,7 +1361,8 @@ rt_ifa_purge_walker(struct rtentry *rt, void *vifa, unsigned int rtableid)
  * for multiple queues for efficiency's sake...
  */
 
-LIST_HEAD(, rttimer_queue)     rttimer_queue_head;
+struct mutex                   rttimer_mtx;
+LIST_HEAD(, rttimer_queue)     rttimer_queue_head;     /* [T] */
 
 #define RTTIMER_CALLOUT(r)     {                                       \
        if (r->rtt_func != NULL) {                                      \
@@ -1393,6 +1394,7 @@ rt_timer_init(void)
        pool_init(&rttimer_queue_pool, sizeof(struct rttimer_queue), 0,
            IPL_MPFLOOR, 0, "rttmrq", NULL);
 
+       mtx_init(&rttimer_mtx, IPL_MPFLOOR);
        LIST_INIT(&rttimer_queue_head);
        timeout_set_proc(&rt_timer_timeout, rt_timer_timer, &rt_timer_timeout);
        timeout_add_sec(&rt_timer_timeout, 1);
@@ -1408,7 +1410,10 @@ rt_timer_queue_create(int timeout)
        rtq->rtq_timeout = timeout;
        rtq->rtq_count = 0;
        TAILQ_INIT(&rtq->rtq_head);
+
+       mtx_enter(&rttimer_mtx);
        LIST_INSERT_HEAD(&rttimer_queue_head, rtq, rtq_link);
+       mtx_leave(&rttimer_mtx);
 
        return (rtq);
 }
@@ -1416,28 +1421,36 @@ rt_timer_queue_create(int timeout)
 void
 rt_timer_queue_change(struct rttimer_queue *rtq, int timeout)
 {
+       mtx_enter(&rttimer_mtx);
        rtq->rtq_timeout = timeout;
+       mtx_leave(&rttimer_mtx);
 }
 
 void
 rt_timer_queue_destroy(struct rttimer_queue *rtq)
 {
-       struct rttimer  *r;
+       struct rttimer          *r;
+       TAILQ_HEAD(, rttimer)    rttlist;
 
        NET_ASSERT_LOCKED();
 
+       TAILQ_INIT(&rttlist);
+       mtx_enter(&rttimer_mtx);
        while ((r = TAILQ_FIRST(&rtq->rtq_head)) != NULL) {
                LIST_REMOVE(r, rtt_link);
                TAILQ_REMOVE(&rtq->rtq_head, r, rtt_next);
+               TAILQ_INSERT_TAIL(&rttlist, r, rtt_next);
+               KASSERT(rtq->rtq_count > 0);
+               rtq->rtq_count--;
+       }
+       LIST_REMOVE(rtq, rtq_link);
+       mtx_leave(&rttimer_mtx);
+
+       while ((r = TAILQ_FIRST(&rttlist)) != NULL) {
+               TAILQ_REMOVE(&rttlist, r, rtt_next);
                RTTIMER_CALLOUT(r);
                pool_put(&rttimer_pool, r);
-               if (rtq->rtq_count > 0)
-                       rtq->rtq_count--;
-               else
-                       printf("rt_timer_queue_destroy: rtq_count reached 0\n");
        }
-
-       LIST_REMOVE(rtq, rtq_link);
        pool_put(&rttimer_queue_pool, rtq);
 }
 
@@ -1450,15 +1463,22 @@ rt_timer_queue_count(struct rttimer_queue *rtq)
 void
 rt_timer_remove_all(struct rtentry *rt)
 {
-       struct rttimer  *r;
+       struct rttimer          *r;
+       TAILQ_HEAD(, rttimer)    rttlist;
 
+       TAILQ_INIT(&rttlist);
+       mtx_enter(&rttimer_mtx);
        while ((r = LIST_FIRST(&rt->rt_timer)) != NULL) {
                LIST_REMOVE(r, rtt_link);
                TAILQ_REMOVE(&r->rtt_queue->rtq_head, r, rtt_next);
-               if (r->rtt_queue->rtq_count > 0)
-                       r->rtt_queue->rtq_count--;
-               else
-                       printf("rt_timer_remove_all: rtq_count reached 0\n");
+               TAILQ_INSERT_TAIL(&rttlist, r, rtt_next);
+               KASSERT(r->rtt_queue->rtq_count > 0);
+               r->rtt_queue->rtq_count--;
+       }
+       mtx_leave(&rttimer_mtx);
+
+       while ((r = TAILQ_FIRST(&rttlist)) != NULL) {
+               TAILQ_REMOVE(&rttlist, r, rtt_next);
                pool_put(&rttimer_pool, r);
        }
 }
@@ -1467,12 +1487,23 @@ int
 rt_timer_add(struct rtentry *rt, void (*func)(struct rtentry *,
     struct rttimer *), struct rttimer_queue *queue, u_int rtableid)
 {
-       struct rttimer  *r;
+       struct rttimer  *r, *rnew;
        time_t           current_time;
 
+       rnew = pool_get(&rttimer_pool, PR_NOWAIT | PR_ZERO);
+       if (rnew == NULL)
+               return (ENOBUFS);
+
        current_time = getuptime();
-       rt->rt_expire = current_time + queue->rtq_timeout;
 
+       rnew->rtt_rt = rt;
+       rnew->rtt_time = current_time;
+       rnew->rtt_func = func;
+       rnew->rtt_queue = queue;
+       rnew->rtt_tableid = rtableid;
+
+       mtx_enter(&rttimer_mtx);
+       rt->rt_expire = current_time + queue->rtq_timeout;
        /*
         * If there's already a timer with this action, destroy it before
         * we add a new one.
@@ -1481,27 +1512,19 @@ rt_timer_add(struct rtentry *rt, void (*func)(struct rtentry *,
                if (r->rtt_func == func) {
                        LIST_REMOVE(r, rtt_link);
                        TAILQ_REMOVE(&r->rtt_queue->rtq_head, r, rtt_next);
-                       if (r->rtt_queue->rtq_count > 0)
-                               r->rtt_queue->rtq_count--;
-                       else
-                               printf("rt_timer_add: rtq_count reached 0\n");
-                       pool_put(&rttimer_pool, r);
+                       KASSERT(r->rtt_queue->rtq_count > 0);
+                       r->rtt_queue->rtq_count--;
                        break;  /* only one per list, so we can quit... */
                }
        }
 
-       r = pool_get(&rttimer_pool, PR_NOWAIT | PR_ZERO);
-       if (r == NULL)
-               return (ENOBUFS);
+       LIST_INSERT_HEAD(&rt->rt_timer, rnew, rtt_link);
+       TAILQ_INSERT_TAIL(&queue->rtq_head, rnew, rtt_next);
+       rnew->rtt_queue->rtq_count++;
+       mtx_leave(&rttimer_mtx);
 
-       r->rtt_rt = rt;
-       r->rtt_time = current_time;
-       r->rtt_func = func;
-       r->rtt_queue = queue;
-       r->rtt_tableid = rtableid;
-       LIST_INSERT_HEAD(&rt->rt_timer, r, rtt_link);
-       TAILQ_INSERT_TAIL(&queue->rtq_head, r, rtt_next);
-       r->rtt_queue->rtq_count++;
+       if (r != NULL)
+               pool_put(&rttimer_pool, r);
 
        return (0);
 }
@@ -1512,24 +1535,31 @@ rt_timer_timer(void *arg)
        struct timeout          *to = (struct timeout *)arg;
        struct rttimer_queue    *rtq;
        struct rttimer          *r;
+       TAILQ_HEAD(, rttimer)    rttlist;
        time_t                   current_time;
 
        current_time = getuptime();
+       TAILQ_INIT(&rttlist);
 
        NET_LOCK();
+       mtx_enter(&rttimer_mtx);
        LIST_FOREACH(rtq, &rttimer_queue_head, rtq_link) {
                while ((r = TAILQ_FIRST(&rtq->rtq_head)) != NULL &&
                    (r->rtt_time + rtq->rtq_timeout) < current_time) {
                        LIST_REMOVE(r, rtt_link);
                        TAILQ_REMOVE(&rtq->rtq_head, r, rtt_next);
-                       RTTIMER_CALLOUT(r);
-                       pool_put(&rttimer_pool, r);
-                       if (rtq->rtq_count > 0)
-                               rtq->rtq_count--;
-                       else
-                               printf("rt_timer_timer: rtq_count reached 0\n");
+                       TAILQ_INSERT_TAIL(&rttlist, r, rtt_next);
+                       KASSERT(rtq->rtq_count > 0);
+                       rtq->rtq_count--;
                }
        }
+       mtx_leave(&rttimer_mtx);
+
+       while ((r = TAILQ_FIRST(&rttlist)) != NULL) {
+               TAILQ_REMOVE(&rttlist, r, rtt_next);
+               RTTIMER_CALLOUT(r);
+               pool_put(&rttimer_pool, r);
+       }
        NET_UNLOCK();
 
        timeout_add_sec(to, 1);

diff --git a/sys/net/route.h b/sys/net/route.h
index 096308e..23d46bb 100644 (file)
--- a/sys/net/route.h
+++ b/sys/net/route.h
@@ -1,4 +1,4 @@
-/*     $OpenBSD: route.h,v 1.190 2022/04/20 17:58:22 bluhm Exp $       */
+/*     $OpenBSD: route.h,v 1.191 2022/04/28 17:47:41 bluhm Exp $       */
 /*     $NetBSD: route.h,v 1.9 1996/02/13 22:00:49 christos Exp $       */
 
 /*
@@ -35,6 +35,12 @@
 #ifndef _NET_ROUTE_H_
 #define _NET_ROUTE_H_
 
+/*
+ * Locks used to protect struct members in this file:
+ *     I       immutable after creation
+ *     T       rttimer_mtx             route timer lists
+ */
+
 /*
  * Kernel resident routing tables.
  *
@@ -400,21 +406,21 @@ rtstat_inc(enum rtstat_counters c)
  * These allow functions to be called for routes at specific times.
  */
 struct rttimer {
-       TAILQ_ENTRY(rttimer)    rtt_next;  /* entry on timer queue */
-       LIST_ENTRY(rttimer)     rtt_link;  /* multiple timers per rtentry */
-       struct rttimer_queue    *rtt_queue;/* back pointer to queue */
-       struct rtentry          *rtt_rt;   /* Back pointer to the route */
-       void                    (*rtt_func)(struct rtentry *,
-                                                struct rttimer *);
-       time_t                  rtt_time; /* When this timer was registered */
-       u_int                   rtt_tableid;    /* routing table id of rtt_rt */
+       TAILQ_ENTRY(rttimer)    rtt_next;       /* [T] entry on timer queue */
+       LIST_ENTRY(rttimer)     rtt_link;       /* [T] timers per rtentry */
+       struct rttimer_queue    *rtt_queue;     /* [T] back pointer to queue */
+       struct rtentry          *rtt_rt;        /* [I] back pointer to route */
+       void                    (*rtt_func)     /* [I] callback */
+                                   (struct rtentry *, struct rttimer *);
+       time_t                  rtt_time;       /* [I] when timer registered */
+       u_int                   rtt_tableid;    /* [I] rtable id of rtt_rt */
 };
 
 struct rttimer_queue {
-       TAILQ_HEAD(, rttimer)           rtq_head;
-       LIST_ENTRY(rttimer_queue)       rtq_link;
-       unsigned long                   rtq_count;
-       int                             rtq_timeout;
+       TAILQ_HEAD(, rttimer)           rtq_head;       /* [T] */
+       LIST_ENTRY(rttimer_queue)       rtq_link;       /* [T] */
+       unsigned long                   rtq_count;      /* [T] */
+       int                             rtq_timeout;    /* [T] */
 };
 
 const char     *rtlabel_id2name(u_int16_t);