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@

3 files changed, 76 insertions, 64 deletions

diff --git a/sys/net/route.c b/sys/net/route.c
index 56d927b9e7e..c9b83e892ba 100644
--- 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 0ad17bf7ec6..5692206e606 100644
--- 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 31fd8f8322b..7b4e544fced 100644
--- 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;