Rework the rttimer code. Instead of a global queue and a global timeout

use a per rttimer struct timeout. On enqueue the struct rttimer belongs
to the timeout, in case the route is removed before the timer fires
cleanup based on the timeout_del() return value. If the timeout currently
running then just clear the rtt_rt pointer and let the timeout handle the
cleanup. This should hopefully fix the icmp_pmtu_timeout crashes reported
by some people.
OK bluhm@

diff --git a/sys/net/route.c b/sys/net/route.c
index 56d927b..c9b83e8 100644 (file)
--- a/sys/net/route.c
+++ b/sys/net/route.c
@@ -1,4 +1,4 @@
-/*     $OpenBSD: route.c,v 1.410 2022/05/05 13:57:40 claudio Exp $     */
+/*     $OpenBSD: route.c,v 1.411 2022/06/27 21:26:46 claudio Exp $     */
 /*     $NetBSD: route.c,v 1.14 1996/02/13 22:00:46 christos Exp $      */
 
 /*
@@ -1361,7 +1361,16 @@ rt_ifa_purge_walker(struct rtentry *rt, void *vifa, unsigned int rtableid)
  */
 
 struct mutex                   rttimer_mtx;
-LIST_HEAD(, rttimer_queue)     rttimer_queue_head;     /* [T] */
+
+struct rttimer {
+       TAILQ_ENTRY(rttimer)    rtt_next;       /* [T] entry on timer queue */
+       LIST_ENTRY(rttimer)     rtt_link;       /* [T] timers per rtentry */
+       struct timeout          rtt_timeout;    /* [I] timeout for this entry */
+       struct rttimer_queue    *rtt_queue;     /* [I] back pointer to queue */
+       struct rtentry          *rtt_rt;        /* [T] back pointer to route */
+       time_t                  rtt_expire;     /* [I] rt expire time */
+       u_int                   rtt_tableid;    /* [I] rtable id of rtt_rt */
+};
 
 #define RTTIMER_CALLOUT(r)     {                                       \
        if (r->rtt_queue->rtq_func != NULL) {                           \
@@ -1388,15 +1397,9 @@ LIST_HEAD(, rttimer_queue)       rttimer_queue_head;     /* [T] */
 void
 rt_timer_init(void)
 {
-       static struct timeout   rt_timer_timeout;
-
        pool_init(&rttimer_pool, sizeof(struct rttimer), 0,
            IPL_MPFLOOR, 0, "rttmr", 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);
 }
 
 void
@@ -1407,10 +1410,6 @@ rt_timer_queue_init(struct rttimer_queue *rtq, int timeout,
        rtq->rtq_count = 0;
        rtq->rtq_func = func;
        TAILQ_INIT(&rtq->rtq_head);
-
-       mtx_enter(&rttimer_mtx);
-       LIST_INSERT_HEAD(&rttimer_queue_head, rtq, rtq_link);
-       mtx_leave(&rttimer_mtx);
 }
 
 void
@@ -1453,6 +1452,25 @@ rt_timer_queue_count(struct rttimer_queue *rtq)
        return (rtq->rtq_count);
 }
 
+static inline struct rttimer *
+rt_timer_unlink(struct rttimer *r)
+{
+       MUTEX_ASSERT_LOCKED(&rttimer_mtx);
+
+       LIST_REMOVE(r, rtt_link);
+       r->rtt_rt = NULL;
+
+       if (timeout_del(&r->rtt_timeout) == 0) {
+               /* timeout fired, so rt_timer_timer will do the cleanup */
+               return NULL;
+       }
+
+       TAILQ_REMOVE(&r->rtt_queue->rtq_head, r, rtt_next);
+       KASSERT(r->rtt_queue->rtq_count > 0);
+       r->rtt_queue->rtq_count--;
+       return r;
+}
+
 void
 rt_timer_remove_all(struct rtentry *rt)
 {
@@ -1462,11 +1480,9 @@ rt_timer_remove_all(struct rtentry *rt)
        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);
-               TAILQ_INSERT_TAIL(&rttlist, r, rtt_next);
-               KASSERT(r->rtt_queue->rtq_count > 0);
-               r->rtt_queue->rtq_count--;
+               r = rt_timer_unlink(r);
+               if (r != NULL)
+                       TAILQ_INSERT_TAIL(&rttlist, r, rtt_next);
        }
        mtx_leave(&rttimer_mtx);
 
@@ -1476,41 +1492,52 @@ rt_timer_remove_all(struct rtentry *rt)
        }
 }
 
+time_t
+rt_timer_get_expire(const struct rtentry *rt)
+{
+       const struct rttimer    *r;
+       time_t                   expire = 0;
+       
+       mtx_enter(&rttimer_mtx);
+       LIST_FOREACH(r, &rt->rt_timer, rtt_link) {
+               if (expire == 0 || expire > r->rtt_expire)
+                       expire = r->rtt_expire;
+       }
+       mtx_leave(&rttimer_mtx);
+
+       return expire;
+}
+
 int
 rt_timer_add(struct rtentry *rt, struct rttimer_queue *queue, u_int rtableid)
 {
        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();
-
        rnew->rtt_rt = rt;
-       rnew->rtt_time = current_time;
        rnew->rtt_queue = queue;
        rnew->rtt_tableid = rtableid;
+       rnew->rtt_expire = getuptime() + queue->rtq_timeout;
+       timeout_set_proc(&rnew->rtt_timeout, rt_timer_timer, rnew);
 
        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.
         */
        LIST_FOREACH(r, &rt->rt_timer, rtt_link) {
                if (r->rtt_queue == queue) {
-                       LIST_REMOVE(r, rtt_link);
-                       TAILQ_REMOVE(&r->rtt_queue->rtq_head, r, rtt_next);
-                       KASSERT(r->rtt_queue->rtq_count > 0);
-                       r->rtt_queue->rtq_count--;
+                       r = rt_timer_unlink(r);
                        break;  /* only one per list, so we can quit... */
                }
        }
 
        LIST_INSERT_HEAD(&rt->rt_timer, rnew, rtt_link);
        TAILQ_INSERT_TAIL(&queue->rtq_head, rnew, rtt_next);
+       timeout_add_sec(&rnew->rtt_timeout, queue->rtq_timeout);
        rnew->rtt_queue->rtq_count++;
        mtx_leave(&rttimer_mtx);
 
@@ -1523,37 +1550,25 @@ rt_timer_add(struct rtentry *rt, struct rttimer_queue *queue, u_int rtableid)
 void
 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);
+       struct rttimer          *r = arg;
+       struct rttimer_queue    *rtq = r->rtt_queue;
 
        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);
-                       TAILQ_INSERT_TAIL(&rttlist, r, rtt_next);
-                       KASSERT(rtq->rtq_count > 0);
-                       rtq->rtq_count--;
-               }
-       }
+
+       if (r->rtt_rt != NULL)
+               LIST_REMOVE(r, rtt_link);
+       TAILQ_REMOVE(&rtq->rtq_head, 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);
+       if (r->rtt_rt != NULL)
                RTTIMER_CALLOUT(r);
-               pool_put(&rttimer_pool, r);
-       }
        NET_UNLOCK();
 
-       timeout_add_sec(to, 1);
+       pool_put(&rttimer_pool, r);
 }
 
 #ifdef MPLS

diff --git a/sys/net/route.h b/sys/net/route.h
index 0ad17bf..5692206 100644 (file)
--- a/sys/net/route.h
+++ b/sys/net/route.h
@@ -1,4 +1,4 @@
-/*     $OpenBSD: route.h,v 1.194 2022/05/05 13:57:40 claudio Exp $     */
+/*     $OpenBSD: route.h,v 1.195 2022/06/27 21:26:46 claudio Exp $     */
 /*     $NetBSD: route.h,v 1.9 1996/02/13 22:00:49 christos Exp $       */
 
 /*
@@ -92,6 +92,8 @@ struct rt_metrics {
 #include <sys/queue.h>
 #include <net/rtable.h>
 
+struct rttimer;
+
 /*
  * We distinguish between routes to hosts and routes to networks,
  * preferring the former if available.  For each route we infer
@@ -405,15 +407,6 @@ rtstat_inc(enum rtstat_counters c)
  * add,timer} functions all used with the kind permission of BSDI.
  * These allow functions to be called for routes at specific times.
  */
-struct rttimer {
-       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 */
-       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;       /* [T] */
        LIST_ENTRY(rttimer_queue)       rtq_link;       /* [T] */
@@ -461,6 +454,7 @@ void                rt_timer_init(void);
 int            rt_timer_add(struct rtentry *,
                    struct rttimer_queue *, u_int);
 void           rt_timer_remove_all(struct rtentry *);
+time_t         rt_timer_get_expire(const struct rtentry *);
 void           rt_timer_queue_init(struct rttimer_queue *, int,
                    void(*)(struct rtentry *, u_int));
 void           rt_timer_queue_change(struct rttimer_queue *, int);

diff --git a/sys/net/rtsock.c b/sys/net/rtsock.c
index 31fd8f8..7b4e544 100644 (file)
--- a/sys/net/rtsock.c
+++ b/sys/net/rtsock.c
@@ -1,4 +1,4 @@
-/*     $OpenBSD: rtsock.c,v 1.332 2022/06/27 17:15:35 bluhm Exp $      */
+/*     $OpenBSD: rtsock.c,v 1.333 2022/06/27 21:26:46 claudio Exp $    */
 /*     $NetBSD: rtsock.c,v 1.18 1996/03/29 00:32:10 cgd Exp $  */
 
 /*
@@ -132,7 +132,7 @@ int          rtm_xaddrs(caddr_t, caddr_t, struct rt_addrinfo *);
 int             rtm_validate_proposal(struct rt_addrinfo *);
 void            rtm_setmetrics(u_long, const struct rt_metrics *,
                     struct rt_kmetrics *);
-void            rtm_getmetrics(const struct rt_kmetrics *,
+void            rtm_getmetrics(const struct rtentry *,
                     struct rt_metrics *);
 
 int             sysctl_iflist(int, struct walkarg *);
@@ -674,7 +674,7 @@ rtm_report(struct rtentry *rt, u_char type, int seq, int tableid)
        rtm->rtm_flags = rt->rt_flags;
        rtm->rtm_pid = curproc->p_p->ps_pid;
        rtm->rtm_seq = seq;
-       rtm_getmetrics(&rt->rt_rmx, &rtm->rtm_rmx);
+       rtm_getmetrics(rt, &rtm->rtm_rmx);
        rtm->rtm_addrs = info.rti_addrs;
 #ifdef MPLS
        rtm->rtm_mpls = info.rti_mpls;
@@ -1391,11 +1391,14 @@ rtm_setmetrics(u_long which, const struct rt_metrics *in,
 }
 
 void
-rtm_getmetrics(const struct rt_kmetrics *in, struct rt_metrics *out)
+rtm_getmetrics(const struct rtentry *rt, struct rt_metrics *out)
 {
+       const struct rt_kmetrics *in = &rt->rt_rmx;
        int64_t expire;
 
        expire = in->rmx_expire;
+       if (expire == 0)
+               expire = rt_timer_get_expire(rt);
        if (expire != 0) {
                expire -= getuptime();
                expire += gettime();
@@ -1998,7 +2001,7 @@ sysctl_dumpentry(struct rtentry *rt, void *v, unsigned int id)
                rtm->rtm_pid = curproc->p_p->ps_pid;
                rtm->rtm_flags = RTF_DONE | rt->rt_flags;
                rtm->rtm_priority = rt->rt_priority & RTP_MASK;
-               rtm_getmetrics(&rt->rt_rmx, &rtm->rtm_rmx);
+               rtm_getmetrics(rt, &rtm->rtm_rmx);
                /* Do not account the routing table's reference. */
                rtm->rtm_rmx.rmx_refcnt = rt->rt_refcnt - 1;
                rtm->rtm_index = rt->rt_ifidx;