Messages in this thread Patch in this message | | | Date | Sat, 8 Apr 2006 21:27:45 +0400 | From | Oleg Nesterov <> | Subject | Re: [PATCH rc1-mm] de_thread: fix deadlockable process addition |
| |
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, ¤t->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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |