[PATCH] fix hrtimer_enqueue_reprogram race

* [PATCH] fix hrtimer_enqueue_reprogram race
@ 2013-02-04 12:33 Izik Eidus
  2013-02-05 10:44 ` Thomas Gleixner
  2013-02-05 11:02 ` [tip:timers/core] hrtimer: Prevent " tip-bot for Leonid Shatz
  0 siblings, 2 replies; 6+ messages in thread
From: Izik Eidus @ 2013-02-04 12:33 UTC (permalink / raw)
  To: Thomas Gleixner; +Cc: Andrea Arcangeli, linux-kernel, leonid Shatz, Izik Eidus

From: leonid Shatz <leonid.shatz@ravellosystems.com>

it seems like hrtimer_enqueue_reprogram contain a race which could result in
timer.base switch during unlock/lock sequence.

See the code at __hrtimer_start_range_ns where it calls
hrtimer_enqueue_reprogram. The later is releasing lock protecting the timer
base for a short time and timer base switch can occur from a different CPU
thread. Later when __hrtimer_start_range_ns calls unlock_hrtimer_base, a base
switch could have happened and this causes the bug

Try to start the same hrtimer from two different threads in kernel running
each one on a different CPU. Eventually one of the calls will cause timer base
switch while another thread is not expecting it.

This can happen in virtualized environment where one thread can be delayed by
lower hypervisor, and due to time delay a different CPU is taking care of
missed timer start and runs the timer start logic on its own.

Signed-off-by: Leonid Shatz <leonid.shatz@ravellosystems.com>
Signed-off-by: Izik Eidus <izik.eidus@ravellosystems.com>
---
 kernel/hrtimer.c |   32 ++++++++++++++------------------
 1 file changed, 14 insertions(+), 18 deletions(-)

diff --git a/kernel/hrtimer.c b/kernel/hrtimer.c
index 6db7a5e..0c8c6cd 100644
--- a/kernel/hrtimer.c
+++ b/kernel/hrtimer.c
@@ -640,21 +640,9 @@ static inline void hrtimer_init_hres(struct hrtimer_cpu_base *base)
  * and expiry check is done in the hrtimer_interrupt or in the softirq.
  */
 static inline int hrtimer_enqueue_reprogram(struct hrtimer *timer,
-					    struct hrtimer_clock_base *base,
-					    int wakeup)
+					    struct hrtimer_clock_base *base)
 {
-	if (base->cpu_base->hres_active && hrtimer_reprogram(timer, base)) {
-		if (wakeup) {
-			raw_spin_unlock(&base->cpu_base->lock);
-			raise_softirq_irqoff(HRTIMER_SOFTIRQ);
-			raw_spin_lock(&base->cpu_base->lock);
-		} else
-			__raise_softirq_irqoff(HRTIMER_SOFTIRQ);
-
-		return 1;
-	}
-
-	return 0;
+	return base->cpu_base->hres_active && hrtimer_reprogram(timer, base);
 }
 
 static inline ktime_t hrtimer_update_base(struct hrtimer_cpu_base *base)
@@ -735,8 +723,7 @@ static inline int hrtimer_switch_to_hres(void) { return 0; }
 static inline void
 hrtimer_force_reprogram(struct hrtimer_cpu_base *base, int skip_equal) { }
 static inline int hrtimer_enqueue_reprogram(struct hrtimer *timer,
-					    struct hrtimer_clock_base *base,
-					    int wakeup)
+					    struct hrtimer_clock_base *base)
 {
 	return 0;
 }
@@ -995,8 +982,17 @@ int __hrtimer_start_range_ns(struct hrtimer *timer, ktime_t tim,
 	 *
 	 * XXX send_remote_softirq() ?
 	 */
-	if (leftmost && new_base->cpu_base == &__get_cpu_var(hrtimer_bases))
-		hrtimer_enqueue_reprogram(timer, new_base, wakeup);
+	if (leftmost && new_base->cpu_base == &__get_cpu_var(hrtimer_bases)
+		&& hrtimer_enqueue_reprogram(timer, new_base)) {
+		if (wakeup) {
+			raw_spin_unlock(&new_base->cpu_base->lock);
+			raise_softirq_irqoff(HRTIMER_SOFTIRQ);
+			local_irq_restore(flags);
+			return ret;
+		} else {
+			__raise_softirq_irqoff(HRTIMER_SOFTIRQ);
+		}
+	}
 
 	unlock_hrtimer_base(timer, &flags);
 
-- 
1.7.10.4


^ permalink raw reply related	[flat|nested] 6+ messages in thread