[PATCH] del_timer() vs. mod_timer() SMP race

[PATCH] del_timer() vs. mod_timer() SMP race

[Benjamin Herrenschmidt]

Hi !

We just spent some days fighting a rare race in one of the distro's who backported
some of timer.c from 2.6 to 2.4 (though they missed a bit).

The actual race we found didn't happen in 2.6 _but_ code inspection showed that a
similar race is still present in 2.6, explanation below:

Code removing a timer from a list (run_timers or del_timer) takes that CPU list
lock, does list_del, then timer->base = NULL.

It is mandatory that this timer->base = NULL is visible to other CPUs only after
the list_del() is complete. If not, then mod timer could see it NULL, thus take it's
own CPU list lock and not the one for the CPU the timer was beeing removed from the
list, and thus the list_add in mod_timer() could race with the list_del() from
run_timers() or del_timer().

Our race happened with run_timers(), which _DOES_ contain a proper smp_wmb() in the
right spot in 2.6, but didn't in the "backport" we were fighting with.

However, del_timer() doesn't have such a barrier, and thus is subject to this race in
2.6 as well. This patch fixes it.

Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>

Index: linux-work/kernel/timer.c
===================================================================
--- linux-work.orig/kernel/timer.c	2004-11-22 11:50:59.000000000 +1100
+++ linux-work/kernel/timer.c	2004-11-22 13:35:38.928448032 +1100
@@ -308,6 +308,7 @@
 		goto repeat;
 	}
 	list_del(&timer->entry);
+	smp_wmb();
 	timer->base = NULL;
 	spin_unlock_irqrestore(&base->lock, flags);
 

Re: [PATCH] del_timer() vs. mod_timer() SMP race

[Andrew Morton]

Benjamin Herrenschmidt <benh@kernel.crashing.org> wrote:
>
> --- linux-work.orig/kernel/timer.c	2004-11-22 11:50:59.000000000 +1100
>  +++ linux-work/kernel/timer.c	2004-11-22 13:35:38.928448032 +1100
>  @@ -308,6 +308,7 @@
>   		goto repeat;
>   	}
>   	list_del(&timer->entry);
>  +	smp_wmb();

Pretty please, always add a comment when putting an open-coded barrier into
the kernel.  Otherwise people cannot tell why it is there.

Re: [PATCH] del_timer() vs. mod_timer() SMP race

[Benjamin Herrenschmidt]

On Sun, 2004-11-21 at 18:54 -0800, Andrew Morton wrote:
> Benjamin Herrenschmidt <benh@kernel.crashing.org> wrote:
> >
> > --- linux-work.orig/kernel/timer.c	2004-11-22 11:50:59.000000000 +1100
> >  +++ linux-work/kernel/timer.c	2004-11-22 13:35:38.928448032 +1100
> >  @@ -308,6 +308,7 @@
> >   		goto repeat;
> >   	}
> >   	list_del(&timer->entry);
> >  +	smp_wmb();
> 
> Pretty please, always add a comment when putting an open-coded barrier into
> the kernel.  Otherwise people cannot tell why it is there.

Ok, I also added the comment to the "other" smp_wmb() which was already there just in case...

It is mandatory that this timer->base = NULL is visible to other CPUs only after
the list_del() is complete. If not, then mod timer could see it NULL, thus take it's
own CPU list lock and not the one for the CPU the timer was beeing removed from the
list, and thus the list_add in mod_timer() could race with the list_del() from
run_timers() or del_timer().

Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>

Index: linux-work/kernel/timer.c
===================================================================
--- linux-work.orig/kernel/timer.c	2004-11-22 11:50:59.000000000 +1100
+++ linux-work/kernel/timer.c	2004-11-22 14:01:05.537368200 +1100
@@ -308,6 +308,10 @@
 		goto repeat;
 	}
 	list_del(&timer->entry);
+	smp_wmb(); /* the list del must have taken effect before timer->base
+		    * change is visible to other CPUs, or a concurrent mod_timer
+		    * would cause a race with list_add
+		    */
 	timer->base = NULL;
 	spin_unlock_irqrestore(&base->lock, flags);
 
@@ -460,7 +464,10 @@
 
 			list_del(&timer->entry);
 			set_running_timer(base, timer);
-			smp_wmb();
+			smp_wmb(); /* the list del must have taken effect before timer->base
+				    * change is visible to other CPUs, or a concurrent mod_timer
+				    * would cause a race with list_add
+				    */
 			timer->base = NULL;
 			spin_unlock_irq(&base->lock);
 			fn(data);