Re: PATCH: Race in 2.6.0-test2 timer code

[Andrew Morton <akpm@osdl.org>]

linas@austin.ibm.com wrote:
>
> I have been chasing a pointer corruption bug in the timer code, and
> found the following race in the mod_timer() routine.  I am still 
> testing to see if this fixes my bug ... 

Andrea says that we need to take the per-timer lock in add_timer() and
del_timer(), but I haven't yet got around to working out why.

Does this (untested) patch fix whatever race it is which you are observing?


 25-akpm/kernel/timer.c |   25 ++++++++++++++++---------
 1 files changed, 16 insertions(+), 9 deletions(-)

diff -puN kernel/timer.c~timer-locking-fixes kernel/timer.c
--- 25/kernel/timer.c~timer-locking-fixes	Tue Jul 29 13:52:21 2003
+++ 25-akpm/kernel/timer.c	Tue Jul 29 13:54:55 2003
@@ -167,10 +167,12 @@ void add_timer(struct timer_list *timer)
 
 	check_timer(timer);
 
-	spin_lock_irqsave(&base->lock, flags);
+	spin_lock_irqsave(&timer->lock, flags);
+	spin_lock(&base->lock);
 	internal_add_timer(base, timer);
 	timer->base = base;
-	spin_unlock_irqrestore(&base->lock, flags);
+	spin_unlock(&base->lock);
+	spin_unlock_irqrestore(&timer->lock, flags);
 	put_cpu();
 }
 
@@ -190,10 +192,12 @@ void add_timer_on(struct timer_list *tim
 
 	check_timer(timer);
 
-	spin_lock_irqsave(&base->lock, flags);
+	spin_lock_irqsave(&timer->lock, flags);
+	spin_lock(&base->lock);
 	internal_add_timer(base, timer);
 	timer->base = base;
-	spin_unlock_irqrestore(&base->lock, flags);
+	spin_unlock(&base->lock);
+	spin_unlock_irqrestore(&timer->lock, flags);
 }
 
 /***
@@ -298,19 +302,22 @@ int del_timer(struct timer_list *timer)
 	tvec_base_t *base;
 
 	check_timer(timer);
-
+	spin_lock_irqsave(&timer->lock, flags);
 repeat:
  	base = timer->base;
-	if (!base)
+	if (!base) {
+		spin_unlock_irqrestore(&timer->lock, flags);
 		return 0;
-	spin_lock_irqsave(&base->lock, flags);
+	}
+	spin_lock(&base, flags);
 	if (base != timer->base) {
-		spin_unlock_irqrestore(&base->lock, flags);
+		spin_unlock(&base->lock);
 		goto repeat;
 	}
 	list_del(&timer->entry);
 	timer->base = NULL;
-	spin_unlock_irqrestore(&base->lock, flags);
+	spin_unlock(&base->lock);
+	spin_unlock_irqrestore(&timer->lock, flags);
 
 	return 1;
 }

_