Module Name: src
Committed By: riastradh
Date: Tue Aug 6 15:47:55 UTC 2019
Modified Files:
src/sys/kern: kern_time.c
src/sys/sys: timevar.h
Log Message:
Fix race in timer destruction.
Anything we confirmed about the world before callout_halt may cease
to be true afterward, so make sure to start over in that case.
Add some comments explaining what's going on.
Reported-by: [email protected]
To generate a diff of this commit:
cvs rdiff -u -r1.197 -r1.198 src/sys/kern/kern_time.c
cvs rdiff -u -r1.38 -r1.39 src/sys/sys/timevar.h
Please note that diffs are not public domain; they are subject to the
copyright notices on the relevant files.
Modified files:
Index: src/sys/kern/kern_time.c
diff -u src/sys/kern/kern_time.c:1.197 src/sys/kern/kern_time.c:1.198
--- src/sys/kern/kern_time.c:1.197 Sun Mar 10 14:45:53 2019
+++ src/sys/kern/kern_time.c Tue Aug 6 15:47:55 2019
@@ -1,4 +1,4 @@
-/* $NetBSD: kern_time.c,v 1.197 2019/03/10 14:45:53 kre Exp $ */
+/* $NetBSD: kern_time.c,v 1.198 2019/08/06 15:47:55 riastradh Exp $ */
/*-
* Copyright (c) 2000, 2004, 2005, 2007, 2008, 2009 The NetBSD Foundation, Inc.
@@ -61,7 +61,7 @@
*/
#include <sys/cdefs.h>
-__KERNEL_RCSID(0, "$NetBSD: kern_time.c,v 1.197 2019/03/10 14:45:53 kre Exp $");
+__KERNEL_RCSID(0, "$NetBSD: kern_time.c,v 1.198 2019/08/06 15:47:55 riastradh Exp $");
#include <sys/param.h>
#include <sys/resourcevar.h>
@@ -702,6 +702,8 @@ sys_timer_delete(struct lwp *l, const st
pt->pt_active = 0;
}
}
+
+ /* Free the timer and release the lock. */
itimerfree(pts, timerid);
return (0);
@@ -711,8 +713,11 @@ sys_timer_delete(struct lwp *l, const st
* Set up the given timer. The value in pt->pt_time.it_value is taken
* to be an absolute time for CLOCK_REALTIME/CLOCK_MONOTONIC timers and
* a relative time for CLOCK_VIRTUAL/CLOCK_PROF timers.
+ *
+ * If the callout had already fired but not yet run, fails with
+ * ERESTART -- caller must restart from the top to look up a timer.
*/
-void
+int
timer_settime(struct ptimer *pt)
{
struct ptimer *ptn, *pptn;
@@ -721,7 +726,17 @@ timer_settime(struct ptimer *pt)
KASSERT(mutex_owned(&timer_lock));
if (!CLOCK_VIRTUAL_P(pt->pt_type)) {
- callout_halt(&pt->pt_ch, &timer_lock);
+ /*
+ * Try to stop the callout. However, if it had already
+ * fired, we have to drop the lock to wait for it, so
+ * the world may have changed and pt may not be there
+ * any more. In that case, tell the caller to start
+ * over from the top.
+ */
+ if (callout_halt(&pt->pt_ch, &timer_lock))
+ return ERESTART;
+
+ /* Now we can touch pt and start it up again. */
if (timespecisset(&pt->pt_time.it_value)) {
/*
* Don't need to check tshzto() return value, here.
@@ -770,6 +785,9 @@ timer_settime(struct ptimer *pt)
} else
pt->pt_active = 0;
}
+
+ /* Success! */
+ return 0;
}
void
@@ -868,6 +886,7 @@ dotimer_settime(int timerid, struct itim
return error;
mutex_spin_enter(&timer_lock);
+restart:
if ((pt = pts->pts_timers[timerid]) == NULL) {
mutex_spin_exit(&timer_lock);
return EINVAL;
@@ -908,7 +927,12 @@ dotimer_settime(int timerid, struct itim
}
}
- timer_settime(pt);
+ error = timer_settime(pt);
+ if (error == ERESTART) {
+ KASSERT(!CLOCK_VIRTUAL_P(pt->pt_type));
+ goto restart;
+ }
+ KASSERT(error == 0);
mutex_spin_exit(&timer_lock);
if (ovalue)
@@ -1046,12 +1070,17 @@ realtimerexpire(void *arg)
}
/*
+ * Reset the callout, if it's not going away.
+ *
* Don't need to check tshzto() return value, here.
* callout_reset() does it for us.
*/
- callout_reset(&pt->pt_ch, pt->pt_type == CLOCK_MONOTONIC ?
- tshztoup(&pt->pt_time.it_value) : tshzto(&pt->pt_time.it_value),
- realtimerexpire, pt);
+ if (!pt->pt_dying)
+ callout_reset(&pt->pt_ch,
+ (pt->pt_type == CLOCK_MONOTONIC
+ ? tshztoup(&pt->pt_time.it_value)
+ : tshzto(&pt->pt_time.it_value)),
+ realtimerexpire, pt);
mutex_spin_exit(&timer_lock);
}
@@ -1143,6 +1172,7 @@ dosetitimer(struct proc *p, int which, s
struct timespec now;
struct ptimers *pts;
struct ptimer *pt, *spare;
+ int error;
KASSERT((u_int)which <= CLOCK_MONOTONIC);
if (itimerfix(&itvp->it_value) || itimerfix(&itvp->it_interval))
@@ -1161,6 +1191,7 @@ dosetitimer(struct proc *p, int which, s
if (pts == NULL)
pts = timers_alloc(p);
mutex_spin_enter(&timer_lock);
+restart:
pt = pts->pts_timers[which];
if (pt == NULL) {
if (spare == NULL) {
@@ -1218,7 +1249,12 @@ dosetitimer(struct proc *p, int which, s
break;
}
}
- timer_settime(pt);
+ error = timer_settime(pt);
+ if (error == ERESTART) {
+ KASSERT(!CLOCK_VIRTUAL_P(pt->pt_type));
+ goto restart;
+ }
+ KASSERT(error == 0);
mutex_spin_exit(&timer_lock);
if (spare != NULL)
pool_put(&ptimer_pool, spare);
@@ -1305,7 +1341,9 @@ timers_free(struct proc *p, int which)
}
for ( ; i < TIMER_MAX; i++) {
if (pts->pts_timers[i] != NULL) {
+ /* Free the timer and release the lock. */
itimerfree(pts, i);
+ /* Reacquire the lock for the next one. */
mutex_spin_enter(&timer_lock);
}
}
@@ -1326,12 +1364,33 @@ itimerfree(struct ptimers *pts, int inde
KASSERT(mutex_owned(&timer_lock));
pt = pts->pts_timers[index];
+
+ /*
+ * Prevent new references, and notify the callout not to
+ * restart itself.
+ */
pts->pts_timers[index] = NULL;
+ pt->pt_dying = true;
+
+ /*
+ * For non-virtual timers, stop the callout, or wait for it to
+ * run if it has already fired. It cannot restart again after
+ * this point: the callout won't restart itself when dying, no
+ * other users holding the lock can restart it, and any other
+ * users waiting for callout_halt concurrently (timer_settime)
+ * will restart from the top.
+ */
if (!CLOCK_VIRTUAL_P(pt->pt_type))
callout_halt(&pt->pt_ch, &timer_lock);
+
+ /* Remove it from the queue to be signalled. */
if (pt->pt_queued)
TAILQ_REMOVE(&timer_queue, pt, pt_chain);
+
+ /* All done with the global state. */
mutex_spin_exit(&timer_lock);
+
+ /* Destroy the callout, if needed, and free the ptimer. */
if (!CLOCK_VIRTUAL_P(pt->pt_type))
callout_destroy(&pt->pt_ch);
pool_put(&ptimer_pool, pt);
@@ -1351,6 +1410,7 @@ static int
itimerdecr(struct ptimer *pt, int nsec)
{
struct itimerspec *itp;
+ int error;
KASSERT(mutex_owned(&timer_lock));
KASSERT(CLOCK_VIRTUAL_P(pt->pt_type));
@@ -1378,7 +1438,8 @@ expire:
itp->it_value.tv_nsec += 1000000000;
itp->it_value.tv_sec--;
}
- timer_settime(pt);
+ error = timer_settime(pt);
+ KASSERT(error == 0); /* virtual, never fails */
} else
itp->it_value.tv_nsec = 0; /* sec is already 0 */
return (0);
Index: src/sys/sys/timevar.h
diff -u src/sys/sys/timevar.h:1.38 src/sys/sys/timevar.h:1.39
--- src/sys/sys/timevar.h:1.38 Thu Apr 19 21:19:07 2018
+++ src/sys/sys/timevar.h Tue Aug 6 15:47:55 2019
@@ -1,4 +1,4 @@
-/* $NetBSD: timevar.h,v 1.38 2018/04/19 21:19:07 christos Exp $ */
+/* $NetBSD: timevar.h,v 1.39 2019/08/06 15:47:55 riastradh Exp $ */
/*
* Copyright (c) 2005, 2008 The NetBSD Foundation.
@@ -84,6 +84,7 @@ struct ptimer {
int pt_type;
int pt_entry;
int pt_queued;
+ bool pt_dying;
struct proc *pt_proc;
TAILQ_ENTRY(ptimer) pt_chain;
};
@@ -174,7 +175,7 @@ int settimeofday1(const struct timeval *
int timer_create1(timer_t *, clockid_t, struct sigevent *, copyin_t,
struct lwp *);
void timer_gettime(struct ptimer *, struct itimerspec *);
-void timer_settime(struct ptimer *);
+int timer_settime(struct ptimer *);
struct ptimers *timers_alloc(struct proc *);
void timers_free(struct proc *, int);
void timer_tick(struct lwp *, bool);
