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

[Oleg Nesterov]

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).

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

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.

> 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.

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:

--- MM/fs/exec.c~	2006-04-08 02:19:15.000000000 +0400
+++ MM/fs/exec.c	2006-04-08 18:50:35.000000000 +0400
@@ -704,7 +704,6 @@ static int de_thread(struct task_struct 
 		ptrace_unlink(current);
 		ptrace_unlink(leader);
 		remove_parent(current);
-		remove_parent(leader);
 
 
 		/* Become a process group leader with the old leader's pid.
@@ -718,13 +717,11 @@ static int de_thread(struct task_struct 
 		attach_pid(current, PIDTYPE_SID,  current->signal->session);
 		list_replace_rcu(&leader->tasks, &current->tasks);
 
-		current->parent = current->real_parent = leader->real_parent;
-		leader->parent = leader->real_parent = child_reaper;
+		current->parent = current->real_parent = leader->parent;
 		current->group_leader = current;
 		leader->group_leader = leader;
 
 		add_parent(current);
-		add_parent(leader);
 		if (ptrace) {
 			current->ptrace = ptrace;
 			__ptrace_link(current, parent);

Oleg.

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/