Re: [PATCH] proc: readdir race fix

[ebiederm@xmission.com (Eric W. Biederman)]

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