Source-Changes-HG archive

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index][Old Index]

[src/trunk]: src/sys Fix race in timer destruction.



details:   https://anonhg.NetBSD.org/src/rev/d67446e7109c
branches:  trunk
changeset: 1002564:d67446e7109c
user:      riastradh <riastradh%NetBSD.org@localhost>
date:      Tue Aug 06 15:47:55 2019 +0000

description:
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: syzbot+d58da99969f58c1a024a%syzkaller.appspotmail.com@localhost

diffstat:

 sys/kern/kern_time.c |  81 +++++++++++++++++++++++++++++++++++++++++++++------
 sys/sys/timevar.h    |   5 +-
 2 files changed, 74 insertions(+), 12 deletions(-)

diffs (230 lines):

diff -r 2b211148a64c -r d67446e7109c sys/kern/kern_time.c
--- a/sys/kern/kern_time.c      Tue Aug 06 11:40:15 2019 +0000
+++ b/sys/kern/kern_time.c      Tue Aug 06 15:47:55 2019 +0000
@@ -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 @@
                        pt->pt_active = 0;
                }
        }
+
+       /* Free the timer and release the lock.  */
        itimerfree(pts, timerid);
 
        return (0);
@@ -711,8 +713,11 @@
  * 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 @@
        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 @@
                } else
                        pt->pt_active = 0;
        }
+
+       /* Success!  */
+       return 0;
 }
 
 void
@@ -868,6 +886,7 @@
                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 @@
                }
        }
 
-       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 @@
        }
 
        /*
+        * 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 @@
        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 @@
        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 @@
                        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 @@
        }
        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 @@
        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 @@
 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 @@
                        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);
diff -r 2b211148a64c -r d67446e7109c sys/sys/timevar.h
--- a/sys/sys/timevar.h Tue Aug 06 11:40:15 2019 +0000
+++ b/sys/sys/timevar.h Tue Aug 06 15:47:55 2019 +0000
@@ -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 @@
        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    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);



Home | Main Index | Thread Index | Old Index