Re: [path][rfc] add PR_DETACH prctl command

[Oleg Nesterov <oleg@redhat.com>]

Hi Stas,

On 04/04, Stas Sergeev wrote:
>
> Here's the patch that addresses your concerns
> about the late deleting from list.
> Also, the patch is shrunk twice.
> I think it is about to be trivial this time.

But it is very wrong at first glance...

> I still haven't solved the problems with checking
> parent and checking ptrace, so ignore them for
> now (or give me the hints:)

Not sure I understand your question...

But, once again. You should not use ptrace_reparented(). Reparanted or
not, the ptraced thread must not change its ->parent.

Sorry Stas, I have no time to read the patch, just one thing...

> +			me->detach_code = arg2 << 8;
> ...
> +			if (notif != DEATH_REAP)
> +				me->detaching = 1;
> +			else
> +				list_move_tail(&me->sibling,
> +					&me->real_parent->children);

This is simply wrong unless the caller is the group_leader. Probably
there was some confusion, I thought we already discussed this. But this
is minor...

> +			while_each_thread(me, p) {
> +				if (!ptrace_reparented(p))
> +					p->parent = pid_ns->child_reaper;
> +				p->real_parent = pid_ns->child_reaper;

Eek. Even ignoring ptrace, this is weird. We change parent/real_parent,
but we do not do list_move_tail(sibling) until wait_task_detached() !
No, I think we should not do this even if this was correct. I'll try
to nack this in any case, even if there were no immediate problems ;)
IMHO, this is insane.

But this is wrong. Well. Suppose that the caller of PR_DETACH exits
before the old parent does do_wait(). What /sbin/init (who is the new
parent) can do after it gets SIGCHLD? If can't see this zombie. Nor
the old parent can release this task due to ->detaching. Eventually
/sbin/init can reap it if it does, say, waitpid(-1), but still this
is wrong.

Or. Suppose that the old parent exits after its child does PR_DETACH.
You changed forget_original_parent() but this is not enough, note that
find_new_reaper() can pick the live sub-thread. In this case the child
will be moved to init's ->children list, and after that we are changing
->real_parent back.

Oleg.