lkml.org 
[lkml]   [2006]   [Sep]   [5]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    SubjectRe: [PATCH] proc: readdir race fix
    Date
    Oleg Nesterov <oleg@tv-sign.ru> writes:

    > On 09/04, Eric W. Biederman wrote:
    >>
    >> -static struct task_struct *next_tgid(struct task_struct *start)
    >> -{
    >> - struct task_struct *pos;
    >> + task = NULL;
    >> rcu_read_lock();
    >> - pos = start;
    >> - if (pid_alive(start))
    >> - pos = next_task(start);
    >> - if (pid_alive(pos) && (pos != &init_task)) {
    >> - get_task_struct(pos);
    >> - goto done;
    >> +retry:
    >> + pid = find_next_pid(tgid);
    >> + if (pid) {
    >> + tgid = pid->nr + 1;
    >> + task = pid_task(pid, PIDTYPE_PID);
    >> + if (!task || !thread_group_leader(task))
    > ^^^^^^^^^^^^^^^^^^^
    > There is a window while de_thread() switches leadership, so next_tgid()
    > may skip a task doing exec. What do you think about
    >
    > // needs a comment
    > if (!task || task->pid != task->tgid)
    > goto retry;
    >
    > instead? Currently first_tgid() has the same (very minor) problem.

    I see the problem, and your test will certainly alleviate the symptom.
    You are making the test has this process ever been a thread group leader.

    I guess alleviating the symptom is all that is necessary there.

    Grumble. I hate that entire pid transfer case, too bad glibc cares.

    If I could in the fix for this I would like to add a clean concept
    that we are testing for wrapped in a helper function. Otherwise
    even with a big fat comment this will be easy to break next time
    someone refactors the code.


    >> + goto retry;
    >> + get_task_struct(task);
    >> }
    >> - pos = NULL;
    >> -done:
    >> rcu_read_unlock();
    >> - put_task_struct(start);
    >> - return pos;
    >> + return task;
    >> +
    >> }
    >
    > Emply line before '}'
    >
    >> +struct pid *find_next_pid(int nr)
    >> +{
    >> + struct pid *next;
    >> +
    >> + next = find_pid(nr);
    >> + while (!next) {
    >> + nr = next_pidmap(nr);
    >> + if (nr <= 0)
    >> + break;
    >> + next = find_pid(nr);
    >> + }
    >> + return next;
    >> +}
    >
    > This is strange that we are doing find_pid() before and at the end of loop,
    > I'd suggest this code:
    >
    > struct pid *find_next_pid(int nr)
    > {
    > struct pid *pid;
    >
    > do {
    > pid = find_pid(nr);
    > if (pid != NULL)
    > break;
    > nr = next_pidmap(nr);
    > } while (nr > 0);
    >
    > return pid;
    > }
    >
    > Imho, a bit easier to read.
    It is at least not worse, so it is probably worth doing.

    Eric

    -
    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: 2006-09-05 13:41    [W:0.025 / U:0.868 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site