Allow signalling container-init

From: Sukadev Bhattiprolu <sukadev-r/[email protected]>
Subject: [PATCH] Allow signalling container-init

Only the global-init process must be special - any other container-init process must be killable to prevent run-away processes in the system.

kernel/signal.c +0 -0

@@ -1861,11 +1861,9 @@ relock:
 			continue;

Oleg Nesterov [[email protected]] wrote:

| On 08/08, sukadev-r/[email protected] wrote:
| > 
| > From: Sukadev Bhattiprolu <sukadev-r/[email protected]>
| > Subject: [PATCH] Allow signalling container-init
| > 
| > Only the global-init process must be special - any other container-init
| > process must be killable to prevent run-away processes in the system.
| 
| I think you are right, but....
| 
| > --- lx26-23-rc1-mm1.orig/kernel/signal.c	2007-08-07 13:52:12.000000000 -0700
| > +++ lx26-23-rc1-mm1/kernel/signal.c	2007-08-08 15:09:27.000000000 -0700
| > @@ -1861,11 +1861,9 @@ relock:
| >  			continue;
| >  
| >  		/*
| > -		 * Init of a pid space gets no signals it doesn't want from
| > -		 * within that pid space. It can of course get signals from
| > -		 * its parent pid space.
| > +		 * Global init gets no signals it doesn't want.
| >  		 */
| > -		if (current == task_child_reaper(current))
| > +		if (is_global_init(current->group_leader))
| >  			continue;
| 
| ...this breaks exec() from /sbin/init. Note that de_thread() kills other
| sub-threads with SIGKILL. With this patch de_thread() will hang waiting
| for other threads to die.

Again for threaded-init I guess :-(

Well, we discussed last week about allowing non-root users to clone their pid namespace. The user can then create a container-init and this process would become immune to signal even by a root user ?

| 
| I think it is better to not change the current behaviour which is not
| perfect (buggy), until we actually protect /sbin/init from unwanted
| signals.

Can we preserve the existing behavior by checking only the main thread of global init (i.e pass in 'current' rather than 'current->group_leader' to is_global_init()) ?

Quoted 24 lines · On 08/09, sukadev-r/[email protected] wrote:

Oleg Nesterov [[email protected]] wrote:

| On 08/08, sukadev-r/[email protected] wrote:
| > | > From: Sukadev Bhattiprolu <sukadev-r/[email protected]>
| > Subject: [PATCH] Allow signalling container-init
| > | > Only the global-init process must be special - any other container-init
| > process must be killable to prevent run-away processes in the system.
| | I think you are right, but....
| | > --- lx26-23-rc1-mm1.orig/kernel/signal.c	2007-08-07 13:52:12.000000000 -0700
| > +++ lx26-23-rc1-mm1/kernel/signal.c	2007-08-08 15:09:27.000000000 -0700
| > @@ -1861,11 +1861,9 @@ relock:
| >  			continue;
| >  | >  		/*
| > -		 * Init of a pid space gets no signals it doesn't want from
| > -		 * within that pid space. It can of course get signals from
| > -		 * its parent pid space.
| > +		 * Global init gets no signals it doesn't want.
| >  		 */
| > -		if (current == task_child_reaper(current))
| > +		if (is_global_init(current->group_leader))
| >  			continue;
| | ...this breaks exec() from /sbin/init. Note that de_thread() kills other
| sub-threads with SIGKILL. With this patch de_thread() will hang waiting
| for other threads to die.

Quoted 5 lines

Again for threaded-init I guess :-(

Well, we discussed last week about allowing non-root users to clone their pid namespace. The user can then create a container-init and this process would become immune to signal even by a root user ?

please see below,

Quoted 4 lines

| | I think it is better to not change the current behaviour which is not
| perfect (buggy), until we actually protect /sbin/init from unwanted
| signals.

Quoted 3 lines

Can we preserve the existing behavior by checking only the main thread of global init (i.e pass in 'current' rather than 'current->group_leader' to is_global_init()) ?

Yes, this is what I meant, this is what we have in Linus's tree. This way a container-init could be killed. This all is not correct, but we shouldn't replace one bug with another.

Oleg.

Pavel Emelianov [[email protected]] wrote:

| Oleg Nesterov wrote:
| >>| 
| >>| I think it is better to not change the current behaviour which is not
| >>| perfect (buggy), until we actually protect /sbin/init from unwanted
| >>| signals.
| >>
| >>Can we preserve the existing behavior by checking only the main thread
| >>of global init (i.e pass in 'current' rather than 'current->group_leader'
| >>to is_global_init()) ?
| >
| >Yes, this is what I meant, this is what we have in Linus's tree.
| >This way a container-init could be killed. This all is not correct,
| >but we shouldn't replace one bug with another.
| 
| Well, I agree with Oleg.  I think that we should keep the patches 
| without the signal handling until this set is in (at least) -mm. 
| init pid namespace will work without it as used to do, and we'll 
| develop a better signal handling and fix existing BUGs.
| 
| I know that this creates a hole for creating unkillable process,
| but since this is for root user only (CAP_SYS_ADMIN) this is OK.

But I think there is a difference bw what you are saying and what Oleg is saying.

Only the global-init process must be special - any other container-init process must be killable to prevent run-away processes in the system.

TODO: Ideally we should allow killing the container-init only from parent
container and prevent it being killed from within the container. But that is a more complex change and will be addressed by a follow-on patch. For now allow the container-init to be terminated by any process with sufficient privileges.

Signed-off-by Sukadev Bhattiprolu <sukadev-r/[email protected]>

---
 kernel/signal.c |    6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

Index: lx26-23-rc1-mm1/kernel/signal.c
===================================================================

kernel/signal.c +0 -0

@@ -1861,11 +1861,9 @@ relock:
 			continue;

Should we include this in the patchset ?

Only the global-init process must be special - any other container-init process must be killable to prevent run-away processes in the system.

TODO: Ideally we should allow killing the container-init only from
ancestor containers and prevent it being killed from that or descendant containers. But that is a more complex change and will be addressed by a follow-on patch. For now allow the container-init to be terminated by any process with sufficient privileges.

Quoted 1 line · sukadev-r/[email protected] writes:

Should we include this in the patchset ?

[...]

Quoted 3 lines

Only the global-init process must be special - any other container-init process must be killable to prevent run-away processes in the system.

One problem I hit while using OpenVZ is that some init processes -- notable upstart used by Ubuntu -- depend on the special signal processing extended to init by the kernel.

The problem here was that a signal the kernel would otherwise have blocked was sent to upstart, the default handler was invoked and init was terminated.

Quoted 6 lines

TODO: Ideally we should allow killing the container-init only from
ancestor containers and prevent it being killed from that or descendant containers. But that is a more complex change and will be addressed by a follow-on patch. For now allow the container-init to be terminated by any process with sufficient privileges.

This will break, as far as I can see, by allowing the container root to send signals to init that it doesn't expect.

Quoted 1 line · Daniel Pittman

sukadev-r/[email protected] writes:

Quoted 9 lines

Quoted 6 lines

TODO: Ideally we should allow killing the container-init only from
ancestor containers and prevent it being killed from that or descendant containers. But that is a more complex change and will be addressed by a follow-on patch. For now allow the container-init to be terminated by any process with sufficient privileges.

This will break, as far as I can see, by allowing the container root to send signals to init that it doesn't expect.

Yes, in the end what we want is for a container init to receive

1. all signals from a (authorized) process in a parent pid namespace.
2. for signals sent from inside it's pid namespace, only exactly those signals for which it has installed a custom signal handler, no others.

In other words to a process in an ancestor pid namespace, the init of a container is like any other process. To a process inside the namespace for which it is init, it is as /sbin/init is to the system now.

Actually achieving that without affecting performance for all signalers is nontrivial. The current patchset is complex enough that I'd like to see us settle on non-optimal semantics for now, and once these patches have settled implement the ideal signaling.

Quoted 2 lines · Serge E. Hallyn

Quoted 1 line · Daniel Pittman

sukadev-r/[email protected] writes:

[...]

Quoted 18 lines

Quoted 9 lines

Quoted 6 lines

TODO: Ideally we should allow killing the container-init only from
ancestor containers and prevent it being killed from that or descendant containers. But that is a more complex change and will be addressed by a follow-on patch. For now allow the container-init to be terminated by any process with sufficient privileges.

This will break, as far as I can see, by allowing the container root to send signals to init that it doesn't expect.

Yes, in the end what we want is for a container init to receive

1. all signals from a (authorized) process in a parent pid namespace.
2. for signals sent from inside it's pid namespace, only exactly those signals for which it has installed a custom signal handler, no others.

Quoted 3 lines

In other words to a process in an ancestor pid namespace, the init of a container is like any other process. To a process inside the namespace for which it is init, it is as /sbin/init is to the system now.

That makes sense.

Quoted 4 lines

Actually achieving that without affecting performance for all signalers is nontrivial. The current patchset is complex enough that I'd like to see us settle on non-optimal semantics for now, and once these patches have settled implement the ideal signaling.

I appreciate that. I figured to make you aware that this will make it impossible to run upstart and, probably, other versions of init in your container as expected.

Since this was a somewhat subtle bug to track down it is, I think, work documenting so that people trying to use this code are aware of the limitation.

Quoted 1 line · sukadev-r/[email protected] writes:

Should we include this in the patchset ?

[...]

Quoted 3 lines

Only the global-init process must be special - any other container-init process must be killable to prevent run-away processes in the system.

One problem I hit while using OpenVZ is that some init processes -- notable upstart used by Ubuntu -- depend on the special signal processing extended to init by the kernel.

The problem here was that a signal the kernel would otherwise have blocked was sent to upstart, the default handler was invoked and init was terminated.

Quoted 6 lines

TODO: Ideally we should allow killing the container-init only from
ancestor containers and prevent it being killed from that or descendant containers. But that is a more complex change and will be addressed by a follow-on patch. For now allow the container-init to be terminated by any process with sufficient privileges.

This will break, as far as I can see, by allowing the container root to send signals to init that it doesn't expect.