Login | Register For Free | Help
Search for: (Advanced)

Mailing List Archive: Linux: Kernel

[PATCH][V4] Add reboot_pid_ns to handle the reboot syscall

 

 

Linux kernel RSS feed   Index | Next | Previous | View Threaded


daniel.lezcano at free

Dec 11, 2011, 4:17 PM

Post #1 of 5 (53 views)
Permalink
[PATCH][V4] Add reboot_pid_ns to handle the reboot syscall

In the case of a child pid namespace, rebooting the system does not
really makes sense. When the pid namespace is used in conjunction
with the other namespaces in order to create a linux container, the
reboot syscall leads to some problems.

A container can reboot the host. That can be fixed by dropping
the sys_reboot capability but we are unable to correctly poweroff/
halt/reboot a container and the container stays stuck at the shutdown
time with the container's init process waiting indefinitively.

After several attempts, no solution from userspace was found to reliabily
handle the shutdown from a container.

This patch propose to make the init process of the child pid namespace to
exit with a signal status set to : SIGINT if the child pid namespace called
"halt/poweroff" and SIGHUP if the child pid namespace called "reboot".
When the reboot syscall is called and we are not in the initial
pid namespace, we kill the pid namespace for "HALT", "POWEROFF", "RESTART",
and "RESTART2". Otherwise we return EINVAL.

Returning EINVAL is also an easy way to check if this feature is supported
by the kernel when invoking another 'reboot' option like CAD.

By this way the parent process of the child pid namespace knows if
it rebooted or not and can take the right decision.

Signed-off-by: Daniel Lezcano <daniel.lezcano [at] free>
Acked-by: Serge Hallyn <serge.hallyn [at] canonical>
---
include/linux/pid_namespace.h | 8 +++++++-
kernel/pid_namespace.c | 30 ++++++++++++++++++++++++++++++
kernel/sys.c | 3 +++
3 files changed, 40 insertions(+), 1 deletions(-)

diff --git a/include/linux/pid_namespace.h b/include/linux/pid_namespace.h
index e7cf666..3279596 100644
--- a/include/linux/pid_namespace.h
+++ b/include/linux/pid_namespace.h
@@ -32,6 +32,7 @@ struct pid_namespace {
#endif
gid_t pid_gid;
int hide_pid;
+ int reboot;
};

extern struct pid_namespace init_pid_ns;
@@ -47,6 +48,7 @@ static inline struct pid_namespace *get_pid_ns(struct pid_namespace *ns)
extern struct pid_namespace *copy_pid_ns(unsigned long flags, struct pid_namespace *ns);
extern void free_pid_ns(struct kref *kref);
extern void zap_pid_ns_processes(struct pid_namespace *pid_ns);
+extern int reboot_pid_ns(struct pid_namespace *pid_ns, int cmd);

static inline void put_pid_ns(struct pid_namespace *ns)
{
@@ -74,11 +76,15 @@ static inline void put_pid_ns(struct pid_namespace *ns)
{
}

-
static inline void zap_pid_ns_processes(struct pid_namespace *ns)
{
BUG();
}
+
+static inline int reboot_pid_ns(struct pid_namespace *pid_ns, int cmd)
+{
+ BUG();
+}
#endif /* CONFIG_PID_NS */

extern struct pid_namespace *task_active_pid_ns(struct task_struct *tsk);
diff --git a/kernel/pid_namespace.c b/kernel/pid_namespace.c
index a896839..1e93a5a 100644
--- a/kernel/pid_namespace.c
+++ b/kernel/pid_namespace.c
@@ -15,6 +15,7 @@
#include <linux/acct.h>
#include <linux/slab.h>
#include <linux/proc_fs.h>
+#include <linux/reboot.h>

#define BITS_PER_PAGE (PAGE_SIZE*8)

@@ -187,6 +188,9 @@ void zap_pid_ns_processes(struct pid_namespace *pid_ns)
rc = sys_wait4(-1, NULL, __WALL, NULL);
} while (rc != -ECHILD);

+ if (pid_ns->reboot)
+ current->signal->group_exit_code = pid_ns->reboot;
+
acct_exit_ns(pid_ns);
return;
}
@@ -221,6 +225,32 @@ static struct ctl_table pid_ns_ctl_table[] = {

static struct ctl_path kern_path[] = { { .procname = "kernel", }, { } };

+int reboot_pid_ns(struct pid_namespace *pid_ns, int cmd)
+{
+ switch(cmd) {
+ case LINUX_REBOOT_CMD_RESTART2:
+ case LINUX_REBOOT_CMD_RESTART:
+ pid_ns->reboot = SIGHUP;
+ break;
+
+ case LINUX_REBOOT_CMD_POWER_OFF:
+ case LINUX_REBOOT_CMD_HALT:
+ pid_ns->reboot = SIGINT;
+ break;
+ default:
+ return -EINVAL;
+ }
+
+ read_lock(&tasklist_lock);
+ force_sig(SIGKILL, pid_ns->child_reaper);
+ read_unlock(&tasklist_lock);
+
+ do_exit(0);
+
+ /* Not reached */
+ return 0;
+}
+
static __init int pid_namespaces_init(void)
{
pid_ns_cachep = KMEM_CACHE(pid_namespace, SLAB_PANIC);
diff --git a/kernel/sys.c b/kernel/sys.c
index ddf8155..31acf63 100644
--- a/kernel/sys.c
+++ b/kernel/sys.c
@@ -444,6 +444,9 @@ SYSCALL_DEFINE4(reboot, int, magic1, int, magic2, unsigned int, cmd,
magic2 != LINUX_REBOOT_MAGIC2C))
return -EINVAL;

+ if (task_active_pid_ns(current) != &init_pid_ns)
+ return reboot_pid_ns(task_active_pid_ns(current), cmd);
+
/* Instead of trying to make the power_off code look like
* halt when pm_power_off is not set do it the easy way.
*/
--
1.7.5.4

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


serge at hallyn

Dec 12, 2011, 3:14 PM

Post #2 of 5 (44 views)
Permalink
Re: [PATCH][V4] Add reboot_pid_ns to handle the reboot syscall [In reply to]

Quoting Daniel Lezcano (daniel.lezcano [at] free):
> In the case of a child pid namespace, rebooting the system does not
> really makes sense. When the pid namespace is used in conjunction
> with the other namespaces in order to create a linux container, the
> reboot syscall leads to some problems.
>
> A container can reboot the host. That can be fixed by dropping
> the sys_reboot capability but we are unable to correctly poweroff/
> halt/reboot a container and the container stays stuck at the shutdown
> time with the container's init process waiting indefinitively.
>
> After several attempts, no solution from userspace was found to reliabily
> handle the shutdown from a container.
>
> This patch propose to make the init process of the child pid namespace to
> exit with a signal status set to : SIGINT if the child pid namespace called
> "halt/poweroff" and SIGHUP if the child pid namespace called "reboot".
> When the reboot syscall is called and we are not in the initial
> pid namespace, we kill the pid namespace for "HALT", "POWEROFF", "RESTART",
> and "RESTART2". Otherwise we return EINVAL.
>
> Returning EINVAL is also an easy way to check if this feature is supported
> by the kernel when invoking another 'reboot' option like CAD.
>
> By this way the parent process of the child pid namespace knows if
> it rebooted or not and can take the right decision.
>
> Signed-off-by: Daniel Lezcano <daniel.lezcano [at] free>
> Acked-by: Serge Hallyn <serge.hallyn [at] canonical>

Thanks for pushing on this, Daniel. It'll be really nice if this can get
in soon so we can get rid of the utmp watching hack, which at this point is
the last blocker to being able to use the same disk image for container and
bare metal installs.

> ---
> include/linux/pid_namespace.h | 8 +++++++-
> kernel/pid_namespace.c | 30 ++++++++++++++++++++++++++++++
> kernel/sys.c | 3 +++
> 3 files changed, 40 insertions(+), 1 deletions(-)
>
> diff --git a/include/linux/pid_namespace.h b/include/linux/pid_namespace.h
> index e7cf666..3279596 100644
> --- a/include/linux/pid_namespace.h
> +++ b/include/linux/pid_namespace.h
> @@ -32,6 +32,7 @@ struct pid_namespace {
> #endif
> gid_t pid_gid;
> int hide_pid;
> + int reboot;
> };
>
> extern struct pid_namespace init_pid_ns;
> @@ -47,6 +48,7 @@ static inline struct pid_namespace *get_pid_ns(struct pid_namespace *ns)
> extern struct pid_namespace *copy_pid_ns(unsigned long flags, struct pid_namespace *ns);
> extern void free_pid_ns(struct kref *kref);
> extern void zap_pid_ns_processes(struct pid_namespace *pid_ns);
> +extern int reboot_pid_ns(struct pid_namespace *pid_ns, int cmd);
>
> static inline void put_pid_ns(struct pid_namespace *ns)
> {
> @@ -74,11 +76,15 @@ static inline void put_pid_ns(struct pid_namespace *ns)
> {
> }
>
> -
> static inline void zap_pid_ns_processes(struct pid_namespace *ns)
> {
> BUG();
> }
> +
> +static inline int reboot_pid_ns(struct pid_namespace *pid_ns, int cmd)
> +{
> + BUG();
> +}
> #endif /* CONFIG_PID_NS */
>
> extern struct pid_namespace *task_active_pid_ns(struct task_struct *tsk);
> diff --git a/kernel/pid_namespace.c b/kernel/pid_namespace.c
> index a896839..1e93a5a 100644
> --- a/kernel/pid_namespace.c
> +++ b/kernel/pid_namespace.c
> @@ -15,6 +15,7 @@
> #include <linux/acct.h>
> #include <linux/slab.h>
> #include <linux/proc_fs.h>
> +#include <linux/reboot.h>
>
> #define BITS_PER_PAGE (PAGE_SIZE*8)
>
> @@ -187,6 +188,9 @@ void zap_pid_ns_processes(struct pid_namespace *pid_ns)
> rc = sys_wait4(-1, NULL, __WALL, NULL);
> } while (rc != -ECHILD);
>
> + if (pid_ns->reboot)
> + current->signal->group_exit_code = pid_ns->reboot;
> +
> acct_exit_ns(pid_ns);
> return;
> }
> @@ -221,6 +225,32 @@ static struct ctl_table pid_ns_ctl_table[] = {
>
> static struct ctl_path kern_path[] = { { .procname = "kernel", }, { } };
>
> +int reboot_pid_ns(struct pid_namespace *pid_ns, int cmd)
> +{
> + switch(cmd) {
> + case LINUX_REBOOT_CMD_RESTART2:
> + case LINUX_REBOOT_CMD_RESTART:
> + pid_ns->reboot = SIGHUP;
> + break;
> +
> + case LINUX_REBOOT_CMD_POWER_OFF:
> + case LINUX_REBOOT_CMD_HALT:
> + pid_ns->reboot = SIGINT;
> + break;
> + default:
> + return -EINVAL;
> + }
> +
> + read_lock(&tasklist_lock);
> + force_sig(SIGKILL, pid_ns->child_reaper);
> + read_unlock(&tasklist_lock);
> +
> + do_exit(0);
> +
> + /* Not reached */
> + return 0;
> +}
> +
> static __init int pid_namespaces_init(void)
> {
> pid_ns_cachep = KMEM_CACHE(pid_namespace, SLAB_PANIC);
> diff --git a/kernel/sys.c b/kernel/sys.c
> index ddf8155..31acf63 100644
> --- a/kernel/sys.c
> +++ b/kernel/sys.c
> @@ -444,6 +444,9 @@ SYSCALL_DEFINE4(reboot, int, magic1, int, magic2, unsigned int, cmd,
> magic2 != LINUX_REBOOT_MAGIC2C))
> return -EINVAL;
>
> + if (task_active_pid_ns(current) != &init_pid_ns)
> + return reboot_pid_ns(task_active_pid_ns(current), cmd);
> +
> /* Instead of trying to make the power_off code look like
> * halt when pm_power_off is not set do it the easy way.
> */
> --
> 1.7.5.4
>
> _______________________________________________
> Containers mailing list
> Containers [at] lists
> https://lists.linuxfoundation.org/mailman/listinfo/containers
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/


akpm at linux-foundation

Dec 13, 2011, 4:22 PM

Post #3 of 5 (45 views)
Permalink
Re: [PATCH][V4] Add reboot_pid_ns to handle the reboot syscall [In reply to]

On Mon, 12 Dec 2011 01:17:44 +0100
Daniel Lezcano <daniel.lezcano [at] free> wrote:

> In the case of a child pid namespace, rebooting the system does not
> really makes sense. When the pid namespace is used in conjunction
> with the other namespaces in order to create a linux container, the
> reboot syscall leads to some problems.
>
> A container can reboot the host. That can be fixed by dropping
> the sys_reboot capability but we are unable to correctly poweroff/
> halt/reboot a container and the container stays stuck at the shutdown
> time with the container's init process waiting indefinitively.
>
> After several attempts, no solution from userspace was found to reliabily
> handle the shutdown from a container.
>
> This patch propose to make the init process of the child pid namespace to
> exit with a signal status set to : SIGINT if the child pid namespace called
> "halt/poweroff" and SIGHUP if the child pid namespace called "reboot".
> When the reboot syscall is called and we are not in the initial
> pid namespace, we kill the pid namespace for "HALT", "POWEROFF", "RESTART",
> and "RESTART2". Otherwise we return EINVAL.
>
> Returning EINVAL is also an easy way to check if this feature is supported
> by the kernel when invoking another 'reboot' option like CAD.
>
> By this way the parent process of the child pid namespace knows if
> it rebooted or not and can take the right decision.
>
> ...
>
> +static inline int reboot_pid_ns(struct pid_namespace *pid_ns, int cmd)
> +{
> + BUG();
> +}
> #endif /* CONFIG_PID_NS */

I'd recommend compile-testing this...

> --- a/kernel/sys.c
> +++ b/kernel/sys.c
> @@ -444,6 +444,9 @@ SYSCALL_DEFINE4(reboot, int, magic1, int, magic2, unsigned int, cmd,
> magic2 != LINUX_REBOOT_MAGIC2C))
> return -EINVAL;
>
> + if (task_active_pid_ns(current) != &init_pid_ns)
> + return reboot_pid_ns(task_active_pid_ns(current), cmd);
> +
> /* Instead of trying to make the power_off code look like
> * halt when pm_power_off is not set do it the easy way.
> */

I'll repeat my cruelly-ignored review comment for v3:

This adds a bunch of useless code if CONFIG_PID_NS=n. It would be
better to do

#ifdef CONFIG_PID_NS
extern void pidns_handle_reboot(int cmd);
#else
static inline void pidns_handle_reboot(int cmd)
{
}
#endif

(And thereby move the additional code into pid_namespace.c)
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/


oleg at redhat

Dec 14, 2011, 11:17 AM

Post #4 of 5 (41 views)
Permalink
Re: [PATCH][V4] Add reboot_pid_ns to handle the reboot syscall [In reply to]

On 12/13, Andrew Morton wrote:
>
> > --- a/kernel/sys.c
> > +++ b/kernel/sys.c
> > @@ -444,6 +444,9 @@ SYSCALL_DEFINE4(reboot, int, magic1, int, magic2, unsigned int, cmd,
> > magic2 != LINUX_REBOOT_MAGIC2C))
> > return -EINVAL;
> >
> > + if (task_active_pid_ns(current) != &init_pid_ns)
> > + return reboot_pid_ns(task_active_pid_ns(current), cmd);
> > +
> > /* Instead of trying to make the power_off code look like
> > * halt when pm_power_off is not set do it the easy way.
> > */
>
> I'll repeat my cruelly-ignored review comment for v3:
>
> This adds a bunch of useless code if CONFIG_PID_NS=n.

Agreed.

> It would be
> better to do
>
> #ifdef CONFIG_PID_NS
> extern void pidns_handle_reboot(int cmd);
> #else
> static inline void pidns_handle_reboot(int cmd)
> {
> }
> #endif

Can't resist.

Why the kernel always prefers to do it this way, adding the ugly
do-nothing inlines?

Isn't it better to simply call pidns_handle_reboot(cmd) under
CONFIG_PID_NS in sys_reboot() ?

#ifdef CONFIG_PID_NS
if (task_active_pid_ns(current) != &init_pid_ns)
return reboot_pid_ns(cmd);
#endif

This way, if you look at sys_reboot() you can immediately see what
happens, no need to inspect the !CONFIG_PID_NS definition. Plus this
doesn't add the "unnecesary" entry into tag file.

OK, </flame> ;)

Otherwise,

Reviewed-by: Oleg Nesterov <oleg [at] redhat>

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


akpm at linux-foundation

Dec 15, 2011, 2:00 PM

Post #5 of 5 (38 views)
Permalink
Re: [PATCH][V4] Add reboot_pid_ns to handle the reboot syscall [In reply to]

On Wed, 14 Dec 2011 20:17:39 +0100
Oleg Nesterov <oleg [at] redhat> wrote:

> > It would be
> > better to do
> >
> > #ifdef CONFIG_PID_NS
> > extern void pidns_handle_reboot(int cmd);
> > #else
> > static inline void pidns_handle_reboot(int cmd)
> > {
> > }
> > #endif
>
> Can't resist.
>
> Why the kernel always prefers to do it this way, adding the ugly
> do-nothing inlines?
>
> Isn't it better to simply call pidns_handle_reboot(cmd) under
> CONFIG_PID_NS in sys_reboot() ?
>
> #ifdef CONFIG_PID_NS
> if (task_active_pid_ns(current) != &init_pid_ns)
> return reboot_pid_ns(cmd);
> #endif

Imagine what the code would look like if we took all the existing empty
inline stubs and replaced them with #if/#else/#endif.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo [at] vger
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Linux kernel RSS feed   Index | Next | Previous | View Threaded
 
 


Interested in having your list archived? Contact Gossamer Threads
 
  Web Applications & Managed Hosting Powered by Gossamer Threads Inc.