Re: [PATCH rc1-mm] de_thread: fix deadlockable process addition

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

Oleg Nesterov <oleg@tv-sign.ru> writes:

> On 04/08, Eric W. Biederman wrote:
>>
>> Agreed.  That is ugly.
>
> Yes, I agree also.
>
>>
>> -#define thread_group_leader(p)	(p->pid == p->tgid)
>> +#define thread_group_leader(p)	(p == p->group_leader)
>>
>> ...
>>
>> -		leader->group_leader = leader;
>> +		leader->group_leader = current;
>
> I thought about similar change too, but I am unsure about
> release_task(old_leader)->proc_flush_task() path (because
> I don't understand this code).

proc_flush_task() is purely an optimization to kill the
proc dcache entries when a process exits.  So we don't
plug up the dcache with old proc entries.

All it looks at currently and pid an tgid.

So proc_flush_task() should not be a non-issue.

proc_pid_unhash() in -linus is a little more
serious but it only unhashes things.

> This change can confuse next_tid(), but this is minor.
> I don't see other problems.

next_tid?

> However, I think we can do something better instead of
> attach_pid(current)/detach_pid(leader):
>
> 	void exec_pid(task_t *old, task_t * new, enum pid_type type)
> 	{
> 		new->pids[type].pid = old->pids[type].pid;
> 		hlist_replace_rcu(&old->pids[type].node, &new->pids[type].node);
> 		old->pids[type].pid = NULL;
> 	}
>
> So de_thread() can do
>
> 	exec_pid(leader, current, PIDTYPE_PGID);
> 	exec_pid(leader, current, PIDTYPE_SID);
>
> This allows us to iterate over pgrp/session lockless without
> seeing the same task twice, btw. But may be it is just unneeded
> complication.

I think it may be worthwhile.  Currently we can't do any process
group or session traversal lockless so it isn't worth looking
at until we get the bug fix handled.

Ultimately I think I would like PGID and SID to live in ->signal
in which case we would not need to touch them at all.

>> This requires changing the leaders parents
>>
>>  		current->parent = current->real_parent = leader->real_parent;
>> -		leader->parent = leader->real_parent = child_reaper;
>> +		leader->parent = leader->real_parent = current;
>>  		current->group_leader = current;
>
> I don't understand why do we need this change.

I was just trying to come as close as I could to normal
thread semantics.  The fewer special cases in de_thread,
the fewer problems it is.

> Actually, I think leader doesn't need reparenting at all.
> ptrace_unlink(leader) already restored leader->parent = ->real_parent
> and ->sibling. So I think we can do (for review only, should go in a
> separate patch) this:

Duh.  All processes in a thread group share the same real_parent.
It looks like the only practical thing we accomplish
with remove_parent/add_parent is to change our place on
our parents list of children.

Since ptrace_link and ptrace_unlink already does this we don't have a
guaranteed order on that list, so skipping the order change
should definitely be safe.

This means your patch doesn't go far enough.  We should be
able to kill all of the parent list manipulation in
de_thread.   Doing reduces the places that assign
real_parent to just fork and exit.

Eric