Re: PATCH: Race in 2.6.0-test2 timer code

[Andrea Arcangeli <andrea@suse.de>]

On Wed, Jul 30, 2003 at 12:31:23PM +0200, Ingo Molnar wrote:
> 
> On Wed, 30 Jul 2003, Andrea Arcangeli wrote:
> 
> > 
> > 	cpu0			cpu1
> > 	------------		--------------------
> > 
> > 	do_setitimer
> > 				it_real_fn
> > 	del_timer_sync		add_timer	-> crash
> 
> would you mind to elaborate the precise race? I cannot see how the above
> sequence could crash on current 2.6:
> 
> del_timer_sync() takes the base pointer in timer->base and locks it, then
> checks whether the timer has this base or not and repeats the sequence if
> not. add_timer() locks the current CPU's base, then installs the timer and
> sets timer->base. Where's the race?

not sure why you mention timer->base, you know the base lock means
nothing if it's in two different cpus (if it meant anything, it would
mean we'd still suffer the cacheline bouncing with timer ops running in
different cpus). To make it more clear I edit timer.c deleting all
spin_lock(&base->lock) so the base->lock won't make it look like it can
serialize anything. I delete the local_irqsave too since it doesn't
matter too for this example (also assume premption turned off so I don't
need to add the preempt disable in place of the local irq save).

so for this example we have this:

int del_timer(struct timer_list *timer)
{
	tvec_base_t *base;

repeat:
 	base = timer->base;
	if (!base)
		return 0;
	if (base != timer->base) {
		goto repeat;
	}
	list_del(&timer->entry);
	timer->base = NULL;

	return 1;
}

against this:

void add_timer(struct timer_list *timer)
{
	tvec_base_t *base = &get_cpu_var(tvec_bases);
  
  	BUG_ON(timer_pending(timer) || !timer->function);

	internal_add_timer(base, timer);
	timer->base = base;
	put_cpu_var(tvec_bases);
}


in del_timer, list_del can be reordered after the timer->base = NULL,
the C compiler can do that. so list_del will run at the same time of
internal_add_timer(base, timer) that does the list_add_tail.

list_del and list_add_tail running at the same time will crash.

Andrea