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    [from the cache]
©2003-2011 Jasper Spaans