Hi,
mvs@ reminded me of a crash I have seen in December. Route timers
are not MP safe, but I think this can be fixed with a mutex. The
idea is to protect the global lists with a mutex and move the rttimer
into a temporary list. Then the callback and pool put can be called
later without mutex.
It survived a full regress with witness.
Hrvoje: Can you put this on your test machine together with parallel
IP forwarding?
ok?
bluhm
Index: net/route.c
===================================================================
RCS file: /cvs/src/sys/net/route.c,v
retrieving revision 1.406
diff -u -p -r1.406 route.c
--- net/route.c 20 Apr 2022 17:58:22 -0000 1.406
+++ net/route.c 20 Apr 2022 18:00:39 -0000
@@ -1361,7 +1361,8 @@ rt_ifa_purge_walker(struct rtentry *rt,
* 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_queu
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);
}
}
@@ -1471,8 +1491,9 @@ rt_timer_add(struct rtentry *rt, void (*
time_t current_time;
current_time = getuptime();
- rt->rt_expire = current_time + queue->rtq_timeout;
+ 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 +1502,31 @@ rt_timer_add(struct rtentry *rt, void (*
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... */
}
}
+ mtx_leave(&rttimer_mtx);
- r = pool_get(&rttimer_pool, PR_NOWAIT | PR_ZERO);
- if (r == NULL)
- return (ENOBUFS);
+ if (r == NULL) {
+ r = pool_get(&rttimer_pool, PR_NOWAIT | PR_ZERO);
+ if (r == NULL)
+ return (ENOBUFS);
+ } else
+ memset(r, 0, sizeof(*r));
r->rtt_rt = rt;
r->rtt_time = current_time;
r->rtt_func = func;
r->rtt_queue = queue;
r->rtt_tableid = rtableid;
+
+ mtx_enter(&rttimer_mtx);
LIST_INSERT_HEAD(&rt->rt_timer, r, rtt_link);
TAILQ_INSERT_TAIL(&queue->rtq_head, r, rtt_next);
r->rtt_queue->rtq_count++;
+ mtx_leave(&rttimer_mtx);
return (0);
}
@@ -1512,23 +1537,30 @@ 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();
Index: net/route.h
===================================================================
RCS file: /cvs/src/sys/net/route.h,v
retrieving revision 1.190
diff -u -p -r1.190 route.h
--- net/route.h 20 Apr 2022 17:58:22 -0000 1.190
+++ net/route.h 20 Apr 2022 18:00:39 -0000
@@ -36,6 +36,12 @@
#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.
*
* The routing tables are initialized when interface addresses
@@ -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);
