lkml.org 
[lkml]   [2007]   [Apr]   [5]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH] Fix race between attach_task and cpuset_exit
    On Wed, Apr 04, 2007 at 10:55:01PM -0700, Paul Menage wrote:
    > >@@ -1257,8 +1260,8 @@ static int attach_task(struct cpuset *cs
    > >
    > > put_task_struct(tsk);
    > > synchronize_rcu();
    > >- if (atomic_dec_and_test(&oldcs->count))
    > >- check_for_release(oldcs, ppathbuf);
    > >+ if (oldcs_to_be_released)
    > >+ check_for_release(oldcs_to_be_released, ppathbuf);
    > > return 0;
    > > }
    >
    > Is this part of the patch necessary? If we're adding a task_lock() in
    > cpuset_exit(), then the problem that Vatsa described (both
    > cpuset_attach_task() and cpuset_exit() decrementing the same cpuset
    > count, and cpuset_attach_task() incrementing the count on a cpuset
    > that the task doesn't eventually end up in) go away, since only one
    > thread will retrieve the old value of the task's cpuset in order to
    > decrement its count.

    You *have* to drop/inc the refcount inside the task_lock, otherwise it is
    racy.

    task_lock(T1);
    old_cs = T1->cputset (C1)
    atomic_inc(&C2->count);
    T1->cputset = C2;
    task_unlock();

    ...

    synchronize_rcu();

    if (atomic_dec_and_test(&C1->count))
    check_for_release(..)

    is incorrect. For ex: T1's refcount on C1 may have already been dropped
    by now in cpuset_exit() and dropping the refcount again can lead to
    negative refcounts.
    .
    > > void cpuset_exit(struct task_struct *tsk)
    > > {
    > > struct cpuset *cs;
    > >+ struct cpuset *oldcs_to_be_released = NULL;
    > >
    > >+ task_lock(tsk);
    > > cs = tsk->cpuset;
    > > tsk->cpuset = &top_cpuset; /* the_top_cpuset_hack - see above
    > > */
    > >+ if (atomic_dec_and_test(&cs->count))
    > >+ oldcs_to_be_released = cs;
    > >+ task_unlock(tsk);
    > >
    >
    > I think this is still racy - at this point we're holding a reference
    > on a cpuset that could have a zero count,

    How's that possible? That you have a zero-refcount cpuset with non empty
    tasks in it?

    > and we don't hold
    > manage_mutex or callback_mutex. So a concurrent rmdir could zap the
    > directory and free the cpuset.

    I don't think that is possible. Can you explain?

    > Shouldn't we just put a task_lock()/task_unlock() around these lines
    > and leave everything else as-is?
    >
    > task_lock(tsk);
    > cs = tsk->cpuset;
    > tsk->cpuset = &top_cpuset; /* the_top_cpuset_hack - see above */
    > task_unlock(tsk)

    If we don't drop refcount inside task_lock() it makes it racy with
    attach_task(). 'cs' derived above may not be the right cpuset to drop
    refcount on later in cpuset_exit.

    --
    Regards,
    vatsa
    -
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

    \
     
     \ /
      Last update: 2007-04-05 08:55    [W:0.027 / U:0.812 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site