[lkml]   [2005]   [Jun]   [16]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
Messages in this thread
    SubjectRe: [PATCH] Re: [BUG] Race condition with it_real_fn in kernel/itimer.c
    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;


    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

    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to
    More majordomo info at
    Please read the FAQ at

     \ /
      Last update: 2005-06-16 13:38    [W:0.036 / U:1.488 seconds]
    ©2003-2017 Jasper Spaans. hosted at Digital OceanAdvertise on this site