lkml.org 
[lkml]   [2010]   [Jun]   [22]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: while_each_thread() under rcu_read_lock() is broken?
    On 06/21, Eric W. Biederman wrote:
    >
    > Oleg Nesterov <oleg@redhat.com> writes:
    >
    > >> If
    > >> that's so, then just changing it to avoid the situation seems like it
    > >> would be less invasive overall.
    > >
    > > How? We should change ->group_leader uner write_lock_irq(tasklist),
    > > synchronize_rcu() is not an option. We can't do call_rcu(release_task),
    > > we can't take tasklist for writing in the softirq context. But even
    > > if we could, this can't help in fact or I missed something.
    >
    > We already do: call_rcu(&p->rcu, delayed_put_task_struct); in release_task.
    > We don't call release_task until after we have removed it as leader and
    > dropped the write lock.

    Yes. I meant that while this is needed to ensure that next_thread/etc
    returns the rcu-safe data, this (or more rcu_call's) can help to fix
    while_each_thread.

    I think I was unclear. de_thread() changes ->group_leader, but this does
    not matter at all. I mentioned this only because we discussed the possibility
    to check ->group_leader in while_each_thread.

    What does matter is the single line in __unhash_process()

    list_del_rcu(&p->thread_group);

    this breaks while_each_thread().

    IOW. Why list_for_each_rcu(head) actually works? It works because this
    head itself can't be removed from list.

    while_each_thread(g, t) is almost equal to list_for_each_rcu() and it
    can only work if g can't be removed from list (OK, strictly speaking
    until other sub-threads go away, but this doesn't matter).

    However, g can be removed if a) it is not ->group_leader and exits,
    or b) its subthread execs.

    > At first glance it sounds like the group leader is safe as a stopping
    > point for a rcu while_each_thread, and I expect the fact that
    > de_thread takes everything down to a single thread, could have nice
    > properties here. If pid_alive were only to fail on the group leader
    > when de_thread is called I think we could legitimately say that an event
    > we won't worry about. It is close enough to a new thread being created
    > anyway.

    Not sure I understand exactly... I mean, I don't understand whether
    you agree or not with the fix which adds pid_alive() check into
    next_thread_careful().

    Oleg.



    \
     
     \ /
      Last update: 2010-06-22 16:39    [W:0.022 / U:1.116 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site