Oleg Nesterov [oleg@tv-sign.ru] wrote:
| On 07/30, sukadev@us.ibm.com wrote:
| >
| > --- lx26-23-rc1-mm1.orig/kernel/exit.c	2007-07-26 20:08:16.000000000 -0700
| > +++ lx26-23-rc1-mm1/kernel/exit.c	2007-07-30 23:10:30.000000000 -0700
| > @@ -915,6 +915,7 @@ fastcall NORET_TYPE void do_exit(long co
| >  {
| >  	struct task_struct *tsk = current;
| >  	int group_dead;
| > +	struct pid_namespace *pid_ns = tsk->nsproxy->pid_ns;
| >  
| >  	profile_task_exit(tsk);
| >  
| > @@ -925,9 +926,10 @@ fastcall NORET_TYPE void do_exit(long co
| >  	if (unlikely(!tsk->pid))
| >  		panic("Attempted to kill the idle task!");
| >  	if (unlikely(tsk == task_child_reaper(tsk))) {
| > -		if (task_active_pid_ns(tsk) != &init_pid_ns)
| > -			task_active_pid_ns(tsk)->child_reaper =
| > -					init_pid_ns.child_reaper;
| > +		if (pid_ns != &init_pid_ns) {
| > +			zap_pid_ns_processes(pid_ns);
| > +			pid_ns->child_reaper = init_pid_ns.child_reaper;
| > +		}
| >  		else
| >  			panic("Attempted to kill init!");
| >  	}
| 
| Just to remind you, this is not right when init is multi-threaded,
| we should do this only when the last thread exits.

Sorry, I needed to clarify somethings about the multi-threaded init. I
got the impresssion that you were sending a patch for the existing bug,
and meant to review/clarify in the context of the patch.

Anyways, re: requirements for multi-threaded init:

	Our current definition of is_container_init() and task_child_reaper()
	refer only to the main-thread of the container-init (since they check
	for pid_t == 1)

	If the main-thread is exiting and is the last thread in the group,
	we want terminate other processes in the pid ns (simple case).

	If the main thread is exiting, but is not the last thread in the
	group, should we let it exit and let the next thread in the group
	the reaper of the pid ns ?
	
	Then we would have the pid ns w/o a container-init (i.e reaper
	does not have a pid_t == 1, but probably does not matter).

	And, when this last thread is exiting, we want to terminate other
	processes in the ns right ?

| 
| > -static long do_wait(pid_t pid, int options, struct siginfo __user *infop,
| > +long do_wait(pid_t pid, int options, struct siginfo __user *infop,
| >  		    int __user *stat_addr, struct rusage __user *ru)
| 
| Small nit, other in-kernel reapers use sys_wait4(), perhaps we can use
| it too, in that case we don't need to export do_wait().

Ok.

| 
| > +void zap_pid_ns_processes(struct pid_namespace *pid_ns)
| > +{
| > +	int nr;
| > +	int rc;
| > +	int options = WEXITED|__WALL;
| > +
| > +	/*
| > +	 * We know pid == 1 is terminating. Find remaining pid_ts
| > +	 * in the namespace, signal them and then wait for them
| > +	 * exit.
| > +	 */
| > +	nr = next_pidmap(pid_ns, 1);
| > +	while (nr > 0) {
| > +		kill_proc_info(SIGKILL, SEND_SIG_PRIV, nr);
| > +		nr = next_pidmap(pid_ns, nr);
| > +	}
| 
| Without tasklist_lock held this is not reliable.

Ok. BTW, find_ge_pid() also walks the pidmap, but does not seem to hold
the tasklist_lock. Is that bc its only used in /proc ?

| 
| Oleg.

Quoting Pavel Emelyanov (xemul@openvz.org):
> [snip]
> 
> >>| Maybe it's worth disabling cross-namespaces ptracing...
> >>
> >>I think so too. Its probably not a serious limitation ?
> >
> >Several people think we will implement 'namespace entering' through a
> >ptrace hack, where maybe the admin ptraces the init in a child pidns,
> 
> Why not implement namespace entering w/o any hacks? :)

I did, as a patch on top of the nsproxy container subsystem.  The
response was that that is a hack, and ptrace is cleaner  :)

So the current options for namespace entering would be:

	* using Cedric's bind_ns() functionality, which assigns an
	  integer global id to a namespace, and allows a process to
	  enter a namespace by that global id
	* using my nsproxy container subsystem patch, which lets
	  a process enter another namespace using
	  	echo pid > /container/some/cont/directory/tasks
	  and eventually might allow construction of custom
	  namespaces, i.e.
	  	mkdir /container/c1/c2
		ln -s /container/c1/c1/network /container/c1/c2/network
		echo $$ > /container/c1/c2/tasks
	* using ptrace to coerce a process in the target namespace
	  into forking and executing the desired program.

> >makes it fork, and makes the child execute what it wants (i.e. ps -ef).
> >
> >You're talking about killing that functionality?
> 
> No. We're talking about disabling the things that are not supposed 
> to work at all.

Uh, well in the abstract that sounds like a sound policy...

-serge

On Tue, 2007-07-31 at 23:16 -0700, sukadev@us.ibm.com wrote:
> Oleg Nesterov [oleg@tv-sign.ru] wrote:
> | On 07/30, sukadev@us.ibm.com wrote:
> | >
> | > --- lx26-23-rc1-mm1.orig/kernel/exit.c	2007-07-26 20:08:16.000000000 -0700
> | > +++ lx26-23-rc1-mm1/kernel/exit.c	2007-07-30 23:10:30.000000000 -0700
> | > @@ -915,6 +915,7 @@ fastcall NORET_TYPE void do_exit(long co
> | >  {
> | >  	struct task_struct *tsk = current;
> | >  	int group_dead;
> | > +	struct pid_namespace *pid_ns = tsk->nsproxy->pid_ns;
> | >  
> | >  	profile_task_exit(tsk);
> | >  
> | > @@ -925,9 +926,10 @@ fastcall NORET_TYPE void do_exit(long co
> | >  	if (unlikely(!tsk->pid))
> | >  		panic("Attempted to kill the idle task!");
> | >  	if (unlikely(tsk == task_child_reaper(tsk))) {
> | > -		if (task_active_pid_ns(tsk) != &init_pid_ns)
> | > -			task_active_pid_ns(tsk)->child_reaper =
> | > -					init_pid_ns.child_reaper;
> | > +		if (pid_ns != &init_pid_ns) {
> | > +			zap_pid_ns_processes(pid_ns);
> | > +			pid_ns->child_reaper = init_pid_ns.child_reaper;
> | > +		}
> | >  		else
> | >  			panic("Attempted to kill init!");
> | >  	}
> | 
> | Just to remind you, this is not right when init is multi-threaded,
> | we should do this only when the last thread exits.
> 
> Sorry, I needed to clarify somethings about the multi-threaded init. I
> got the impresssion that you were sending a patch for the existing bug,
> and meant to review/clarify in the context of the patch.
> 
> Anyways, re: requirements for multi-threaded init:
> 
> 	Our current definition of is_container_init() and task_child_reaper()
> 	refer only to the main-thread of the container-init (since they check
> 	for pid_t == 1)

Remember, the "pid" is actually a tgid:
        
        asmlinkage long sys_getpid(void)
        {
                return current->tgid;
        }

So, there are multiple tasks with a "pid" == 1 with a multithreaded
init. 

> 	If the main-thread is exiting and is the last thread in the group,
> 	we want terminate other processes in the pid ns (simple case).
> 
> 	If the main thread is exiting, but is not the last thread in the
> 	group, should we let it exit and let the next thread in the group
> 	the reaper of the pid ns ?

Well, what happens with a multithreaded init today?
	
-- Dave

On 07/31, sukadev@us.ibm.com wrote:
>
> Oleg Nesterov [oleg@tv-sign.ru] wrote:
> | >  
> | > @@ -925,9 +926,10 @@ fastcall NORET_TYPE void do_exit(long co
> | >  	if (unlikely(!tsk->pid))
> | >  		panic("Attempted to kill the idle task!");
> | >  	if (unlikely(tsk == task_child_reaper(tsk))) {
> | > -		if (task_active_pid_ns(tsk) != &init_pid_ns)
> | > -			task_active_pid_ns(tsk)->child_reaper =
> | > -					init_pid_ns.child_reaper;
> | > +		if (pid_ns != &init_pid_ns) {
> | > +			zap_pid_ns_processes(pid_ns);
> | > +			pid_ns->child_reaper = init_pid_ns.child_reaper;
> | > +		}
> | >  		else
> | >  			panic("Attempted to kill init!");
> | >  	}
> | 
> | Just to remind you, this is not right when init is multi-threaded,
> | we should do this only when the last thread exits.
> 
> Sorry, I needed to clarify somethings about the multi-threaded init. I
> got the impresssion that you were sending a patch for the existing bug,
> and meant to review/clarify in the context of the patch.

Ah, sorry, I forgot to send the patch to fix the bug in mainline.
Will try to do tomorrow, please feel free to do this if you wish.

> 	Our current definition of is_container_init() and task_child_reaper()
> 	refer only to the main-thread of the container-init (since they check
> 	for pid_t == 1)

Yes.

> 	If the main-thread is exiting and is the last thread in the group,
> 	we want terminate other processes in the pid ns (simple case).

Yes.

> 	If the main thread is exiting, but is not the last thread in the
> 	group, should we let it exit and let the next thread in the group
> 	the reaper of the pid ns ?

We can, but why? The main thread's task_struct can't go away until all
sub-threads exit. Its ->nsproxy will be NULL, but this doesn't matter.

> 	Then we would have the pid ns w/o a container-init (i.e reaper
> 	does not have a pid_t == 1, but probably does not matter).
> 
> 	And, when this last thread is exiting, we want to terminate other
> 	processes in the ns right ?

Yes, when this last thread is exiting, the entire process is exiting.

> | > +void zap_pid_ns_processes(struct pid_namespace *pid_ns)
> | > +{
> | > +	int nr;
> | > +	int rc;
> | > +	int options = WEXITED|__WALL;
> | > +
> | > +	/*
> | > +	 * We know pid == 1 is terminating. Find remaining pid_ts
> | > +	 * in the namespace, signal them and then wait for them
> | > +	 * exit.
> | > +	 */
> | > +	nr = next_pidmap(pid_ns, 1);
> | > +	while (nr > 0) {
> | > +		kill_proc_info(SIGKILL, SEND_SIG_PRIV, nr);
> | > +		nr = next_pidmap(pid_ns, nr);
> | > +	}
> | 
> | Without tasklist_lock held this is not reliable.
> 
> Ok. BTW, find_ge_pid() also walks the pidmap, but does not seem to hold
> the tasklist_lock. Is that bc its only used in /proc ?

Yes, but this is something different. With or without tasklist_lock,
find_ge_pid()/next_tgid() is not "reliable" (note that alloc_pid() doesn't
take tasklist), but this doesn't matter for /proc.

We should take tasklist_lock to prevent the new process creation.
We can have the "false positives" (copy_process() in progress, PGID/SID
pids), but this is OK. Note that copy_process() checks signal_pending()
after write_lock_irq(&tasklist_lock), that is why it helps.

Oleg.

On 08/01, Dave Hansen wrote:
>
> > 	If the main thread is exiting, but is not the last thread in the
> > 	group, should we let it exit and let the next thread in the group
> > 	the reaper of the pid ns ?
> 
> Well, what happens with a multithreaded init today?

As it was already discussed, the current code is buggy, and should be
fixed.

Oleg.

Dave Hansen [haveblue@us.ibm.com] wrote:
| On Tue, 2007-07-31 at 23:16 -0700, sukadev@us.ibm.com wrote:
| > Oleg Nesterov [oleg@tv-sign.ru] wrote:
| > | On 07/30, sukadev@us.ibm.com wrote:
| > | >
| > | > --- lx26-23-rc1-mm1.orig/kernel/exit.c	2007-07-26 20:08:16.000000000 -0700
| > | > +++ lx26-23-rc1-mm1/kernel/exit.c	2007-07-30 23:10:30.000000000 -0700
| > | > @@ -915,6 +915,7 @@ fastcall NORET_TYPE void do_exit(long co
| > | >  {
| > | >  	struct task_struct *tsk = current;
| > | >  	int group_dead;
| > | > +	struct pid_namespace *pid_ns = tsk->nsproxy->pid_ns;
| > | >  
| > | >  	profile_task_exit(tsk);
| > | >  
| > | > @@ -925,9 +926,10 @@ fastcall NORET_TYPE void do_exit(long co
| > | >  	if (unlikely(!tsk->pid))
| > | >  		panic("Attempted to kill the idle task!");
| > | >  	if (unlikely(tsk == task_child_reaper(tsk))) {
| > | > -		if (task_active_pid_ns(tsk) != &init_pid_ns)
| > | > -			task_active_pid_ns(tsk)->child_reaper =
| > | > -					init_pid_ns.child_reaper;
| > | > +		if (pid_ns != &init_pid_ns) {
| > | > +			zap_pid_ns_processes(pid_ns);
| > | > +			pid_ns->child_reaper = init_pid_ns.child_reaper;
| > | > +		}
| > | >  		else
| > | >  			panic("Attempted to kill init!");
| > | >  	}
| > | 
| > | Just to remind you, this is not right when init is multi-threaded,
| > | we should do this only when the last thread exits.
| > 
| > Sorry, I needed to clarify somethings about the multi-threaded init. I
| > got the impresssion that you were sending a patch for the existing bug,
| > and meant to review/clarify in the context of the patch.
| > 
| > Anyways, re: requirements for multi-threaded init:
| > 
| > 	Our current definition of is_container_init() and task_child_reaper()
| > 	refer only to the main-thread of the container-init (since they check
| > 	for pid_t == 1)
| 
| Remember, the "pid" is actually a tgid:
|         
|         asmlinkage long sys_getpid(void)
|         {
|                 return current->tgid;
|         }
| 
| So, there are multiple tasks with a "pid" == 1 with a multithreaded
| init. 

Yes, and so am now wondering if is_container_init(), is_global_init()
and the "tsk == task_child_reaper(tsk)" checks be replaced with with
something that covers other threads in the reaper ?

| 
| > 	If the main-thread is exiting and is the last thread in the group,
| > 	we want terminate other processes in the pid ns (simple case).
| > 
| > 	If the main thread is exiting, but is not the last thread in the
| > 	group, should we let it exit and let the next thread in the group
| > 	the reaper of the pid ns ?
| 
| Well, what happens with a multithreaded init today?
| 	
| -- Dave

Oleg Nesterov [oleg@tv-sign.ru] wrote:
| On 07/31, sukadev@us.ibm.com wrote:
| >
| > Oleg Nesterov [oleg@tv-sign.ru] wrote:
| > | >  
| > | > @@ -925,9 +926,10 @@ fastcall NORET_TYPE void do_exit(long co
| > | >  	if (unlikely(!tsk->pid))
| > | >  		panic("Attempted to kill the idle task!");
| > | >  	if (unlikely(tsk == task_child_reaper(tsk))) {
| > | > -		if (task_active_pid_ns(tsk) != &init_pid_ns)
| > | > -			task_active_pid_ns(tsk)->child_reaper =
| > | > -					init_pid_ns.child_reaper;
| > | > +		if (pid_ns != &init_pid_ns) {
| > | > +			zap_pid_ns_processes(pid_ns);
| > | > +			pid_ns->child_reaper = init_pid_ns.child_reaper;
| > | > +		}
| > | >  		else
| > | >  			panic("Attempted to kill init!");
| > | >  	}
| > | 
| > | Just to remind you, this is not right when init is multi-threaded,
| > | we should do this only when the last thread exits.
| > 
| > Sorry, I needed to clarify somethings about the multi-threaded init. I
| > got the impresssion that you were sending a patch for the existing bug,
| > and meant to review/clarify in the context of the patch.
| 
| Ah, sorry, I forgot to send the patch to fix the bug in mainline.
| Will try to do tomorrow, please feel free to do this if you wish.

I can do that, but am still a bit confused about this multi-threaded
init :-)

| 
| > 	Our current definition of is_container_init() and task_child_reaper()
| > 	refer only to the main-thread of the container-init (since they check
| > 	for pid_t == 1)
| 
| Yes.

This means that we cannot have a check like "tsk == task_child_reaper(tsk)"
to properly detect the child reaper process right ? 

Its basically a very dumb question - How do we detect a container_init()
in the multi-threaded case ?  Should we use "task->tgid == 1" ?

IOW to identify if the last thread of a child reaper is exiting, should we
check "task->tgid == 1" and the "group_dead" flag  in do_exit() ?

| 
| > 	If the main-thread is exiting and is the last thread in the group,
| > 	we want terminate other processes in the pid ns (simple case).
| 
| Yes.
| 
| > 	If the main thread is exiting, but is not the last thread in the
| > 	group, should we let it exit and let the next thread in the group
| > 	the reaper of the pid ns ?
| 
| We can, but why? The main thread's task_struct can't go away until all
| sub-threads exit. Its ->nsproxy will be NULL, but this doesn't matter.

After the main thread exits task_child_reaper() would still refer to
the main thread right ? So when one of the other processes in the
namespace calls forget_original_parent(), it would reparent the process
to the main thread - no ? The main thread still has a valid task_struct,
but it has exited and cannot adapt children...

BTW, are there any actual users of multi-threaded init ? Or is this
something that can be considered outside the "core" patchset and
addressed soon, but separately like the signalling-container-init issue ?

| 
| > 	Then we would have the pid ns w/o a container-init (i.e reaper
| > 	does not have a pid_t == 1, but probably does not matter).
| > 
| > 	And, when this last thread is exiting, we want to terminate other
| > 	processes in the ns right ?
| 
| Yes, when this last thread is exiting, the entire process is exiting.
| 
| > | > +void zap_pid_ns_processes(struct pid_namespace *pid_ns)
| > | > +{
| > | > +	int nr;
| > | > +	int rc;
| > | > +	int options = WEXITED|__WALL;
| > | > +
| > | > +	/*
| > | > +	 * We know pid == 1 is terminating. Find remaining pid_ts
| > | > +	 * in the namespace, signal them and then wait for them
| > | > +	 * exit.
| > | > +	 */
| > | > +	nr = next_pidmap(pid_ns, 1);
| > | > +	while (nr > 0) {
| > | > +		kill_proc_info(SIGKILL, SEND_SIG_PRIV, nr);
| > | > +		nr = next_pidmap(pid_ns, nr);
| > | > +	}
| > | 
| > | Without tasklist_lock held this is not reliable.
| > 
| > Ok. BTW, find_ge_pid() also walks the pidmap, but does not seem to hold
| > the tasklist_lock. Is that bc its only used in /proc ?
| 
| Yes, but this is something different. With or without tasklist_lock,
| find_ge_pid()/next_tgid() is not "reliable" (note that alloc_pid() doesn't
| take tasklist), but this doesn't matter for /proc.
| 
| We should take tasklist_lock to prevent the new process creation.
| We can have the "false positives" (copy_process() in progress, PGID/SID
| pids), but this is OK. Note that copy_process() checks signal_pending()
| after write_lock_irq(&tasklist_lock), that is why it helps.

Ok. Thx.

| 
| Oleg.

On 08/02, sukadev@us.ibm.com wrote:
>
> Oleg Nesterov [oleg@tv-sign.ru] wrote:
> | On 07/31, sukadev@us.ibm.com wrote:
> | >
> | > Oleg Nesterov [oleg@tv-sign.ru] wrote:
> | > | >  
> | > | > @@ -925,9 +926,10 @@ fastcall NORET_TYPE void do_exit(long co
> | > | >  	if (unlikely(!tsk->pid))
> | > | >  		panic("Attempted to kill the idle task!");
> | > | >  	if (unlikely(tsk == task_child_reaper(tsk))) {
> | > | > -		if (task_active_pid_ns(tsk) != &init_pid_ns)
> | > | > -			task_active_pid_ns(tsk)->child_reaper =
> | > | > -					init_pid_ns.child_reaper;
> | > | > +		if (pid_ns != &init_pid_ns) {
> | > | > +			zap_pid_ns_processes(pid_ns);
> | > | > +			pid_ns->child_reaper = init_pid_ns.child_reaper;

OOPS. I didn't notice this before, but this is not right too (regardless
of multi-threaded init problems).

We should not "reset" ->child_reaper here, we may have exiting tasks
which will re-parent their ->children to global init.

No, we are still /sbin/init of this namespace even if we are exiting,
->child_reaper should point to us, at least until zap_pid_ns_processes()
completes.

> | > 	Our current definition of is_container_init() and task_child_reaper()
> | > 	refer only to the main-thread of the container-init (since they check
> | > 	for pid_t == 1)
> | 
> | Yes.
> 
> This means that we cannot have a check like "tsk == task_child_reaper(tsk)"
> to properly detect the child reaper process right ? 

Yes, we should use "tsk->group_leader == task_child_reaper(tsk)"

> Its basically a very dumb question - How do we detect a container_init()
> in the multi-threaded case ?

Good point. I think is_container_init(tsk) needs a fix:

	-	pid = task_pid(tsk);
	+	pid = task_pid(tsk->group_leader);

Or, perhaps better, change the callers to use tsk->group_leader.

> Should we use "task->tgid == 1" ?

No, no, "task->tgid == 1" means "global" init.

> IOW to identify if the last thread of a child reaper is exiting, should we
> check "task->tgid == 1" and the "group_dead" flag  in do_exit() ?

See above, but yes, as I said before I think we should do this under
the "if (group_dead)" check below.

> | > 	If the main thread is exiting, but is not the last thread in the
> | > 	group, should we let it exit and let the next thread in the group
> | > 	the reaper of the pid ns ?
> | 
> | We can, but why? The main thread's task_struct can't go away until all
> | sub-threads exit. Its ->nsproxy will be NULL, but this doesn't matter.
> 
> After the main thread exits task_child_reaper() would still refer to
> the main thread right ? So when one of the other processes in the
> namespace calls forget_original_parent(), it would reparent the process
> to the main thread - no ? The main thread still has a valid task_struct,
> but it has exited and cannot adapt children...

Yes it can't, and yes, this is somewhat against the rules.

But, afaics, this should work. Because do_wait() from the alive sub-thread
still can reap the child, note that do_wait() iterates over all sub-threads
->children lists. Please note also that do_notify_parent() uses group
signal, so it will wake up some alive sub-thread.

This is wrong for the "normal" process (because when the last thread exits
main_thread->children is lost), but this seems to be OK for the /sbin/init,
exactly because we are doing zap_pid_ns_processes().

Sukadev, may I ask you to add a fat comment about this in your patch?

> BTW, are there any actual users of multi-threaded init ? Or is this
> something that can be considered outside the "core" patchset and
> addressed soon, but separately like the signalling-container-init issue ?

Well, I don't know. Please also see the reply to Kirill's message...

Oleg.

On 07/26, Pavel Emelyanov wrote:
>
> The reason to release namespaces after reparenting is that when task
> exits it may send a signal to its parent (SIGCHLD), but if the parent
> has already exited its namespaces there will be no way to decide what
> pid to dever to him - parent can be from different namespace.

I almost forgot about this one...

After reading the whole series, I can't understand the above explanation
any longer. The parent can't be from different namespace, either we have
another sub-thread, or we reparent the child to /sbin/init which should
be from the same namespace.

If we are /sbin/init, we kill all tasks from the same namespace before
doing exit_notify().

Could you clarify?

Oleg.

On 08/02, sukadev@us.ibm.com wrote:
>
> Oleg Nesterov [oleg@tv-sign.ru] wrote:
> | > | > | > +		if (pid_ns != &init_pid_ns) {
> | > | > | > +			zap_pid_ns_processes(pid_ns);
> | > | > | > +			pid_ns->child_reaper = init_pid_ns.child_reaper;
> | 
> | OOPS. I didn't notice this before, but this is not right too (regardless
> | of multi-threaded init problems).
> | 
> | We should not "reset" ->child_reaper here, we may have exiting tasks
> | which will re-parent their ->children to global init.
> | 
> | No, we are still /sbin/init of this namespace even if we are exiting,
> | ->child_reaper should point to us, at least until zap_pid_ns_processes()
> | completes.
> 
> Yes, we are resetting the reaper _after_ zap_pid_ns_processes() completes
> right ? (all other processes in the namespace must have exited).

OOPS again :) Can't understand how I managed to misread this code.

This means that we should take care about multi-thread init exit,
otherwise the non-root user can crash the kernel.

>From reply to Kirill's message:

	> Still. A non-root user does clone(CLONE_PIDNS), then clone(CLONE_THREAD),
	> and sys_exit() from the main thread, then proceeds with fork()s. Now this
	> ns has the global init as a child reaper, and admin can't kill entire pid_ns
	> by killing its init. Worse, (see the reply to Sukadev' message), we should
	> not reset pid_ns->child_reaper before zap_pid_ns_processes(). In that case
	> ->child_reaper points to the freed task when the last thread exits, this
	> means the non-root user can crash the kernel.

Oleg.

Oleg Nesterov [oleg@tv-sign.ru] wrote:
| On 08/02, sukadev@us.ibm.com wrote:
| >
| > Oleg Nesterov [oleg@tv-sign.ru] wrote:
| > | On 07/31, sukadev@us.ibm.com wrote:
| > | >
| > | > Oleg Nesterov [oleg@tv-sign.ru] wrote:
| > | > | >  
| > | > | > @@ -925,9 +926,10 @@ fastcall NORET_TYPE void do_exit(long co
| > | > | >  	if (unlikely(!tsk->pid))
| > | > | >  		panic("Attempted to kill the idle task!");
| > | > | >  	if (unlikely(tsk == task_child_reaper(tsk))) {
| > | > | > -		if (task_active_pid_ns(tsk) != &init_pid_ns)
| > | > | > -			task_active_pid_ns(tsk)->child_reaper =
| > | > | > -					init_pid_ns.child_reaper;
| > | > | > +		if (pid_ns != &init_pid_ns) {
| > | > | > +			zap_pid_ns_processes(pid_ns);
| > | > | > +			pid_ns->child_reaper = init_pid_ns.child_reaper;
| 
| OOPS. I didn't notice this before, but this is not right too (regardless
| of multi-threaded init problems).
| 
| We should not "reset" ->child_reaper here, we may have exiting tasks
| which will re-parent their ->children to global init.
| 
| No, we are still /sbin/init of this namespace even if we are exiting,
| ->child_reaper should point to us, at least until zap_pid_ns_processes()
| completes.

Yes, we are resetting the reaper _after_ zap_pid_ns_processes() completes
right ? (all other processes in the namespace must have exited).


| 
| > | > 	Our current definition of is_container_init() and task_child_reaper()
| > | > 	refer only to the main-thread of the container-init (since they check
| > | > 	for pid_t == 1)
| > | 
| > | Yes.
| > 
| > This means that we cannot have a check like "tsk == task_child_reaper(tsk)"
| > to properly detect the child reaper process right ? 
| 
| Yes, we should use "tsk->group_leader == task_child_reaper(tsk)"

Ok.

| 
| > Its basically a very dumb question - How do we detect a container_init()
| > in the multi-threaded case ?
| 
| Good point. I think is_container_init(tsk) needs a fix:
| 
| 	-	pid = task_pid(tsk);
| 	+	pid = task_pid(tsk->group_leader);
| 

Ok.

| Or, perhaps better, change the callers to use tsk->group_leader.

Ok.
| 
| > Should we use "task->tgid == 1" ?
| 
| No, no, "task->tgid == 1" means "global" init.

Grr. I got that mixed up bw my implm and Pavel's :-) task->pid and
task->tgid referred to "active-pid-ns pid" in mine.

| 
| > IOW to identify if the last thread of a child reaper is exiting, should we
| > check "task->tgid == 1" and the "group_dead" flag  in do_exit() ?
| 
| See above, but yes, as I said before I think we should do this under
| the "if (group_dead)" check below.
| 
| > | > 	If the main thread is exiting, but is not the last thread in the
| > | > 	group, should we let it exit and let the next thread in the group
| > | > 	the reaper of the pid ns ?
| > | 
| > | We can, but why? The main thread's task_struct can't go away until all
| > | sub-threads exit. Its ->nsproxy will be NULL, but this doesn't matter.
| > 
| > After the main thread exits task_child_reaper() would still refer to
| > the main thread right ? So when one of the other processes in the
| > namespace calls forget_original_parent(), it would reparent the process
| > to the main thread - no ? The main thread still has a valid task_struct,
| > but it has exited and cannot adapt children...
| 
| Yes it can't, and yes, this is somewhat against the rules.
| 
| But, afaics, this should work. Because do_wait() from the alive sub-thread
| still can reap the child, note that do_wait() iterates over all sub-threads
| ->children lists. Please note also that do_notify_parent() uses group
| signal, so it will wake up some alive sub-thread.
| 
| This is wrong for the "normal" process (because when the last thread exits
| main_thread->children is lost), but this seems to be OK for the /sbin/init,
| exactly because we are doing zap_pid_ns_processes().
| 
| Sukadev, may I ask you to add a fat comment about this in your patch?

Sure.

| 
| > BTW, are there any actual users of multi-threaded init ? Or is this
| > something that can be considered outside the "core" patchset and
| > addressed soon, but separately like the signalling-container-init issue ?
| 
| Well, I don't know. Please also see the reply to Kirill's message...
| 
| Oleg.

Oleg Nesterov [oleg@tv-sign.ru] wrote:
| On 08/02, sukadev@us.ibm.com wrote:
| >
| > Oleg Nesterov [oleg@tv-sign.ru] wrote:
| > | > | > | > +		if (pid_ns != &init_pid_ns) {
| > | > | > | > +			zap_pid_ns_processes(pid_ns);
| > | > | > | > +			pid_ns->child_reaper = init_pid_ns.child_reaper;
| > | 
| > | OOPS. I didn't notice this before, but this is not right too (regardless
| > | of multi-threaded init problems).
| > | 
| > | We should not "reset" ->child_reaper here, we may have exiting tasks
| > | which will re-parent their ->children to global init.
| > | 
| > | No, we are still /sbin/init of this namespace even if we are exiting,
| > | ->child_reaper should point to us, at least until zap_pid_ns_processes()
| > | completes.
| > 
| > Yes, we are resetting the reaper _after_ zap_pid_ns_processes() completes
| > right ? (all other processes in the namespace must have exited).
| 
| OOPS again :) Can't understand how I managed to misread this code.
| 
| This means that we should take care about multi-thread init exit,
| otherwise the non-root user can crash the kernel.
| 
| >From reply to Kirill's message:
| 
| 	> Still. A non-root user does clone(CLONE_PIDNS), then clone(CLONE_THREAD),

Agree we should fix the crash. But we need CAP_SYS_ADMIN to clone
pid or other namespaces - this is enforced in copy_namespaces() and
unshare_nsproxy_namespaces()

Suka

On 08/02, sukadev@us.ibm.com wrote:
>
> Oleg Nesterov [oleg@tv-sign.ru] wrote:
> | 
> | This means that we should take care about multi-thread init exit,
> | otherwise the non-root user can crash the kernel.
> | 
> | >From reply to Kirill's message:
> | 
> | 	> Still. A non-root user does clone(CLONE_PIDNS), then clone(CLONE_THREAD),
> 
> Agree we should fix the crash. But we need CAP_SYS_ADMIN to clone
> pid or other namespaces - this is enforced in copy_namespaces() and
> unshare_nsproxy_namespaces()

Hmm. sys_unshare(CLONE_PIDNS) doesn't (and shouldn't) work anyway, but
I don't see the CAP_SYS_ADMIN check in copy_process()->copy_namespaces()
path.

Perhaps I just missed it (sorry, I already cleared my mbox, so I can't
look at theses patches), but is it a good idea to require CAP_SYS_ADMIN?
I think it would be nice if a normal user can create containers, no?

Oleg.

Quoting Oleg Nesterov (oleg@tv-sign.ru):
> On 08/02, sukadev@us.ibm.com wrote:
> >
> > Oleg Nesterov [oleg@tv-sign.ru] wrote:
> > | 
> > | This means that we should take care about multi-thread init exit,
> > | otherwise the non-root user can crash the kernel.
> > | 
> > | >From reply to Kirill's message:
> > | 
> > | 	> Still. A non-root user does clone(CLONE_PIDNS), then clone(CLONE_THREAD),
> > 
> > Agree we should fix the crash. But we need CAP_SYS_ADMIN to clone
> > pid or other namespaces - this is enforced in copy_namespaces() and
> > unshare_nsproxy_namespaces()
> 
> Hmm. sys_unshare(CLONE_PIDNS) doesn't (and shouldn't) work anyway, but
> I don't see the CAP_SYS_ADMIN check in copy_process()->copy_namespaces()
> path.
> 
> Perhaps I just missed it (sorry, I already cleared my mbox, so I can't
> look at theses patches), but is it a good idea to require CAP_SYS_ADMIN?
> I think it would be nice if a normal user can create containers, no?

For pid namespaces I can't think of any reason why CAP_SYS_ADMIN should
be needed, since you can't hide processes that way.  Same for uts
namespaces.

However for ipc and mount namespaces I'd want to think about it some
more.  Any case I can think of right now where they'd be unsafe, is
unsafe anyway, so maybe it's fine...

-serge

Oleg Nesterov wrote:
> On 07/26, Pavel Emelyanov wrote:
>> The reason to release namespaces after reparenting is that when task
>> exits it may send a signal to its parent (SIGCHLD), but if the parent
>> has already exited its namespaces there will be no way to decide what
>> pid to dever to him - parent can be from different namespace.
> 
> I almost forgot about this one...
> 
> After reading the whole series, I can't understand the above explanation
> any longer. The parent can't be from different namespace, either we have
> another sub-thread, or we reparent the child to /sbin/init which should
> be from the same namespace.

If the child that is a new namespace's init is exiting its parent is from the
different namespace. Moreover, we will probably want to implement "entering"
the pid namespace, so  having tasks with parents from another namespace will
be OK.

> If we are /sbin/init, we kill all tasks from the same namespace before
> doing exit_notify().
> 
> Could you clarify?
> 
> Oleg.
> 
>