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

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

Andrew Morton <akpm@osdl.org> writes:

> Andrew Morton <akpm@osdl.org> wrote:
>>
>> Oleg Nesterov <oleg@tv-sign.ru> wrote:
>> >
>> > -		if (likely(p->tasks.prev != LIST_POISON2))
>> > +		if (likely(p->tasks.prev != LIST_POISON2)) {
>> 
>> argh.
>> 
>> c'mon guys, we can't put a dependency on list_head poisoning into generic
>> code.
>> 
>
> A suitable fix might be to add a new list_del_poison() (or
> list_del_rcu_something()?) and use that everywhere.
>
> But it should use a different poisoning pattern, so we know that the kernel
> will still work correctly when someone removes the list_head debugging.

Agreed.  That is ugly.  I would recommend some new functions
list functions but in thinking about this I believe I see
how we can avoid this case completely.

The first step is to optimize thread_group_leader to be
defined in terms of tasks and not process ids.  Which
is one less pointer dereference.

The second step is to modify de_thread to reduce the
old thread group leader to a thread.  This requires changing the
leaders parents, changing the leaders thread_group leader, unhashing
the leader from the process group and session, and removing
the leader from the task list.

With those two changes exit.c should not need to account for 
the de_thread case.

Oleg please take a hard look and see if you can find anything
that will break with the patch below.

I believe that is all that is needed to cleanly keep do_each_thread
from seeing a single thread multiple times.

Eric

diff --git a/include/linux/sched.h b/include/linux/sched.h
index 541f482..2964a2c 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -1203,7 +1203,7 @@ extern void wait_task_inactive(task_t * 
 #define while_each_thread(g, t) \
 	while ((t = next_thread(t)) != g)
 
-#define thread_group_leader(p)	(p->pid == p->tgid)
+#define thread_group_leader(p)	(p == p->group_leader)
 
 static inline task_t *next_thread(task_t *p)
 {


diff --git a/fs/exec.c b/fs/exec.c
index 0291a68..9b0f9c4 100644
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -721,9 +721,14 @@ static int de_thread(struct task_struct 
 		list_add_tail(&current->tasks, &init_task.tasks);
 
 		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;
-		leader->group_leader = leader;
+		leader->group_leader = current;
+
+		/* Reduce leader to a thread */
+		detach_pid(leader, PIDTYPE_PGID, current->signal->pgrp);
+		detach_pid(leader, PIDTYPE_SID   current->signal->session);
+		list_del_init(&leader->tasks);
 
 		add_parent(current);
 		add_parent(leader);
-
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/