Re: [PATCH] Re: [BUG] Race condition with it_real_fn in kernel/itimer.c

[Steven Rostedt <rostedt@goodmis.org>]

On Thu, 2005-06-16 at 11:44 +0400, Oleg Nesterov wrote:
> Steven Rostedt wrote:
> >
> > So, timer_pending tests if timer->base is NULL, but here we see that
> > timer->base IS NULL before the function is called, and as I have said
> > earlier, the it_real_arm can be called on two CPUS simultaneously. So
> > here's another patch that should fix this race condition too.
> >
> > [...]
> >
> > +		/*
> > +		 * Call del_timer_sync unconditionally, since we don't
> > +		 * know if it is running or not. We also need to unlock
> > +		 * the siglock so that the it_real_fn called by ksoftirqd
> > +		 * doesn't wait for us.
> > +		 */
> > +		spin_unlock(&tsk->sighand->siglock);
> > +		del_timer_sync(&tsk->signal->real_timer);
> > +		spin_lock(&tsk->sighand->siglock);
> 
> I don't think this is 100% correct. After del_timer_sync() returns another
> thread can come and call do_setitimer() and re-arm the timer (because with
> your patch we are dropping tsk->sighand->siglock here). So this patch does
> not garantees that the timer is not queued/running after del_timer_sync(),
> and the it_real_arm can be called on two CPUS simultaneously again.
> 

I first thought that too, but then looking at the code I noticed:

int do_setitimer(int which, struct itimerval *value, struct itimerval *ovalue)
{
        struct task_struct *tsk = current;

Where tsk is current.  So the only ones that can change the
tsk->signal->real_timer seems to be the task itself and ksoftirqd. So
between del_timer_sync (which handles the ksoftirqd part) and the
spin_lock, there's no one else that can modify tsk->signal->real_timer.
So I don't believe that there is a race condition here.

[thinks about this a little]

Oh wait, is ->signal shared among threads?  Damn, I think so! So you are
right, another _thread_ can come and change this. I forgot about threads
(they're evil! ;-).

> There is a try_to_del_timer_sync() in the -mm tree which is suitable here:
> 
> 	again:
> 		spin_lock_irq(&tsk->sighand->siglock);
> 		if (try_to_del_timer_sync(&tsk->signal->real_timer) < 0) {
> 			spin_unlock_irq(&tsk->sighand->siglock);
> 			goto again;
> 		}

OK, for the -mm branch this may work. But for the current tree, we may
need to do something else.  Like this ugly patch. But it should work!


int do_setitimer(int which, struct itimerval *value, struct itimerval *ovalue)
{
        struct task_struct *tsk = current;
	static spinlock_t lock = SPIN_LOCK_UNLOCKED;

[...]
		spin_lock(&lock);
		spin_unlock(&tsk->sighand->siglock);
		del_timer_sync(&tsk->signal->real_timer);
		spin_lock(&tsk->sighand->siglock);
		spin_unlock(&lock);

This would handle the case for threads in the main line kernel, but it
looks (to me) pretty ugly, but should work. I also don't like this
because it is shared among all tasks!

Andrew, (or Roland since I see Andrew added you to the list)
  
 What do you think? Should try_to_del_timer_sync be brought over to the
mainline, or have the above ugly code added?

-- Steve