Merge branch 'timers-urgent-for-linus' of git://git.kernel.org/pub/scm/linux/kernel...

Pull timer fixes from Ingo Molnar:
 "Fix four timer locking races: two were noticed by Linus while
  reviewing the code while chasing for a corruption bug, and two
  from fixing spurious USB timeouts"

* 'timers-urgent-for-linus' of git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip:
  timers: Prevent base clock corruption when forwarding
  timers: Prevent base clock rewind when forwarding clock
  timers: Lock base for same bucket optimization
  timers: Plug locking race vs. timer migration

diff --git a/kernel/time/timer.c b/kernel/time/timer.c
index 2d47980..c611c47 100644 (file)
--- a/kernel/time/timer.c
+++ b/kernel/time/timer.c
@@ -878,7 +878,7 @@ static inline struct timer_base *get_timer_base(u32 tflags)
 
 #ifdef CONFIG_NO_HZ_COMMON
 static inline struct timer_base *
-__get_target_base(struct timer_base *base, unsigned tflags)
+get_target_base(struct timer_base *base, unsigned tflags)
 {
 #ifdef CONFIG_SMP
        if ((tflags & TIMER_PINNED) || !base->migration_enabled)
@@ -891,25 +891,27 @@ __get_target_base(struct timer_base *base, unsigned tflags)
 
 static inline void forward_timer_base(struct timer_base *base)
 {
+       unsigned long jnow = READ_ONCE(jiffies);
+
        /*
         * We only forward the base when it's idle and we have a delta between
         * base clock and jiffies.
         */
-       if (!base->is_idle || (long) (jiffies - base->clk) < 2)
+       if (!base->is_idle || (long) (jnow - base->clk) < 2)
                return;
 
        /*
         * If the next expiry value is > jiffies, then we fast forward to
         * jiffies otherwise we forward to the next expiry value.
         */
-       if (time_after(base->next_expiry, jiffies))
-               base->clk = jiffies;
+       if (time_after(base->next_expiry, jnow))
+               base->clk = jnow;
        else
                base->clk = base->next_expiry;
 }
 #else
 static inline struct timer_base *
-__get_target_base(struct timer_base *base, unsigned tflags)
+get_target_base(struct timer_base *base, unsigned tflags)
 {
        return get_timer_this_cpu_base(tflags);
 }
@@ -917,14 +919,6 @@ __get_target_base(struct timer_base *base, unsigned tflags)
 static inline void forward_timer_base(struct timer_base *base) { }
 #endif
 
-static inline struct timer_base *
-get_target_base(struct timer_base *base, unsigned tflags)
-{
-       struct timer_base *target = __get_target_base(base, tflags);
-
-       forward_timer_base(target);
-       return target;
-}
 
 /*
  * We are using hashed locking: Holding per_cpu(timer_bases[x]).lock means
@@ -943,7 +937,14 @@ static struct timer_base *lock_timer_base(struct timer_list *timer,
 {
        for (;;) {
                struct timer_base *base;
-               u32 tf = timer->flags;
+               u32 tf;
+
+               /*
+                * We need to use READ_ONCE() here, otherwise the compiler
+                * might re-read @tf between the check for TIMER_MIGRATING
+                * and spin_lock().
+                */
+               tf = READ_ONCE(timer->flags);
 
                if (!(tf & TIMER_MIGRATING)) {
                        base = get_timer_base(tf);
@@ -964,6 +965,8 @@ __mod_timer(struct timer_list *timer, unsigned long expires, bool pending_only)
        unsigned long clk = 0, flags;
        int ret = 0;
 
+       BUG_ON(!timer->function);
+
        /*
         * This is a common optimization triggered by the networking code - if
         * the timer is re-modified to have the same timeout or ends up in the
@@ -972,13 +975,16 @@ __mod_timer(struct timer_list *timer, unsigned long expires, bool pending_only)
        if (timer_pending(timer)) {
                if (timer->expires == expires)
                        return 1;
+
                /*
-                * Take the current timer_jiffies of base, but without holding
-                * the lock!
+                * We lock timer base and calculate the bucket index right
+                * here. If the timer ends up in the same bucket, then we
+                * just update the expiry time and avoid the whole
+                * dequeue/enqueue dance.
                 */
-               base = get_timer_base(timer->flags);
-               clk = base->clk;
+               base = lock_timer_base(timer, &flags);
 
+               clk = base->clk;
                idx = calc_wheel_index(expires, clk);
 
                /*
@@ -988,14 +994,14 @@ __mod_timer(struct timer_list *timer, unsigned long expires, bool pending_only)
                 */
                if (idx == timer_get_idx(timer)) {
                        timer->expires = expires;
-                       return 1;
+                       ret = 1;
+                       goto out_unlock;
                }
+       } else {
+               base = lock_timer_base(timer, &flags);
        }
 
        timer_stats_timer_set_start_info(timer);
-       BUG_ON(!timer->function);
-
-       base = lock_timer_base(timer, &flags);
 
        ret = detach_if_pending(timer, base, false);
        if (!ret && pending_only)
@@ -1025,12 +1031,16 @@ __mod_timer(struct timer_list *timer, unsigned long expires, bool pending_only)
                }
        }
 
+       /* Try to forward a stale timer base clock */
+       forward_timer_base(base);
+
        timer->expires = expires;
        /*
         * If 'idx' was calculated above and the base time did not advance
-        * between calculating 'idx' and taking the lock, only enqueue_timer()
-        * and trigger_dyntick_cpu() is required. Otherwise we need to
-        * (re)calculate the wheel index via internal_add_timer().
+        * between calculating 'idx' and possibly switching the base, only
+        * enqueue_timer() and trigger_dyntick_cpu() is required. Otherwise
+        * we need to (re)calculate the wheel index via
+        * internal_add_timer().
         */
        if (idx != UINT_MAX && clk == base->clk) {
                enqueue_timer(base, timer, idx);
@@ -1510,12 +1520,16 @@ u64 get_next_timer_interrupt(unsigned long basej, u64 basem)
        is_max_delta = (nextevt == base->clk + NEXT_TIMER_MAX_DELTA);
        base->next_expiry = nextevt;
        /*
-        * We have a fresh next event. Check whether we can forward the base:
+        * We have a fresh next event. Check whether we can forward the
+        * base. We can only do that when @basej is past base->clk
+        * otherwise we might rewind base->clk.
         */
-       if (time_after(nextevt, jiffies))
-               base->clk = jiffies;
-       else if (time_after(nextevt, base->clk))
-               base->clk = nextevt;
+       if (time_after(basej, base->clk)) {
+               if (time_after(nextevt, basej))
+                       base->clk = basej;
+               else if (time_after(nextevt, base->clk))
+                       base->clk = nextevt;
+       }
 
        if (time_before_eq(nextevt, basej)) {
                expires = basem;