Module Name: src Committed By: martin Date: Tue Sep 10 16:16:46 UTC 2019
Modified Files: src/sys/kern [netbsd-9]: kern_time.c src/sys/sys [netbsd-9]: timevar.h Log Message: Pull up following revision(s) (requested by maxv in ticket #192): sys/sys/timevar.h: revision 1.39 sys/kern/kern_time.c: revision 1.198 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. To generate a diff of this commit: cvs rdiff -u -r1.197 -r1.197.4.1 src/sys/kern/kern_time.c cvs rdiff -u -r1.38 -r1.38.6.1 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.197.4.1 --- src/sys/kern/kern_time.c:1.197 Sun Mar 10 14:45:53 2019 +++ src/sys/kern/kern_time.c Tue Sep 10 16:16:46 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.197.4.1 2019/09/10 16:16:46 martin 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.197.4.1 2019/09/10 16:16:46 martin 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.38.6.1 --- src/sys/sys/timevar.h:1.38 Thu Apr 19 21:19:07 2018 +++ src/sys/sys/timevar.h Tue Sep 10 16:16:46 2019 @@ -1,4 +1,4 @@ -/* $NetBSD: timevar.h,v 1.38 2018/04/19 21:19:07 christos Exp $ */ +/* $NetBSD: timevar.h,v 1.38.6.1 2019/09/10 16:16:46 martin 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);