* [PATCH] pidns: reboot_pid_ns: use SEND_SIG_FORCED instead of force_sig()
@ 2012-04-13 0:44 Oleg Nesterov
2012-04-13 18:20 ` Serge Hallyn
0 siblings, 1 reply; 10+ messages in thread
From: Oleg Nesterov @ 2012-04-13 0:44 UTC (permalink / raw)
To: Andrew Morton; +Cc: Daniel Lezcano, Serge Hallyn, linux-kernel
cf3f8921 "pidns: add reboot_pid_ns() to handle the reboot syscall"
uses force_sig() to kill the SIGNAL_UNKILLABLE cinit, this was the
only choice.
After 629d362b we can use SEND_SIG_FORCED instead, force_sig() has
the special semantics and should be avoided if possible.
Signed-off-by: Oleg Nesterov <oleg@redhat.com>
diff --git a/kernel/pid_namespace.c b/kernel/pid_namespace.c
index 57bc1fd..ef35e20 100644
--- a/kernel/pid_namespace.c
+++ b/kernel/pid_namespace.c
@@ -241,7 +241,7 @@ int reboot_pid_ns(struct pid_namespace *pid_ns, int cmd)
}
read_lock(&tasklist_lock);
- force_sig(SIGKILL, pid_ns->child_reaper);
+ do_send_sig_info(SIGKILL, SEND_SIG_FORCED, pid_ns->child_reaper, true);
read_unlock(&tasklist_lock);
do_exit(0);
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH] pidns: reboot_pid_ns: use SEND_SIG_FORCED instead of force_sig()
2012-04-13 0:44 [PATCH] pidns: reboot_pid_ns: use SEND_SIG_FORCED instead of force_sig() Oleg Nesterov
@ 2012-04-13 18:20 ` Serge Hallyn
2012-04-13 19:24 ` Oleg Nesterov
0 siblings, 1 reply; 10+ messages in thread
From: Serge Hallyn @ 2012-04-13 18:20 UTC (permalink / raw)
To: Oleg Nesterov; +Cc: Andrew Morton, Daniel Lezcano, linux-kernel
Quoting Oleg Nesterov (oleg@redhat.com):
> cf3f8921 "pidns: add reboot_pid_ns() to handle the reboot syscall"
> uses force_sig() to kill the SIGNAL_UNKILLABLE cinit, this was the
> only choice.
>
> After 629d362b we can use SEND_SIG_FORCED instead, force_sig() has
> the special semantics and should be avoided if possible.
>
> Signed-off-by: Oleg Nesterov <oleg@redhat.com>
Hi Oleg,
I like the patch in theory, however after applying it containers are
no longer able to shutdown/reboot. I can externally kill them with
lxc-stop, but internal shutdown fails.
> diff --git a/kernel/pid_namespace.c b/kernel/pid_namespace.c
> index 57bc1fd..ef35e20 100644
> --- a/kernel/pid_namespace.c
> +++ b/kernel/pid_namespace.c
> @@ -241,7 +241,7 @@ int reboot_pid_ns(struct pid_namespace *pid_ns, int cmd)
> }
>
> read_lock(&tasklist_lock);
> - force_sig(SIGKILL, pid_ns->child_reaper);
> + do_send_sig_info(SIGKILL, SEND_SIG_FORCED, pid_ns->child_reaper, true);
> read_unlock(&tasklist_lock);
>
> do_exit(0);
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] pidns: reboot_pid_ns: use SEND_SIG_FORCED instead of force_sig()
2012-04-13 18:20 ` Serge Hallyn
@ 2012-04-13 19:24 ` Oleg Nesterov
2012-04-13 19:34 ` Serge Hallyn
2012-04-16 13:59 ` Serge Hallyn
0 siblings, 2 replies; 10+ messages in thread
From: Oleg Nesterov @ 2012-04-13 19:24 UTC (permalink / raw)
To: Serge Hallyn; +Cc: Andrew Morton, Daniel Lezcano, linux-kernel
Hi Serge,
On 04/13, Serge Hallyn wrote:
>
> Quoting Oleg Nesterov (oleg@redhat.com):
> > cf3f8921 "pidns: add reboot_pid_ns() to handle the reboot syscall"
> > uses force_sig() to kill the SIGNAL_UNKILLABLE cinit, this was the
> > only choice.
> >
> > After 629d362b we can use SEND_SIG_FORCED instead, force_sig() has
> > the special semantics and should be avoided if possible.
> >
> > Signed-off-by: Oleg Nesterov <oleg@redhat.com>
>
> Hi Oleg,
>
> I like the patch in theory, however after applying it containers are
> no longer able to shutdown/reboot. I can externally kill them with
> lxc-stop, but internal shutdown fails.
Oh, thanks a lot for testing.
Strange... I even tested this change, but only in the root namespace.
OK, thanks, I'll investigate. I hope you verified your kernel has
629d362b commit ;)
Oleg.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] pidns: reboot_pid_ns: use SEND_SIG_FORCED instead of force_sig()
2012-04-13 19:24 ` Oleg Nesterov
@ 2012-04-13 19:34 ` Serge Hallyn
2012-04-16 13:59 ` Serge Hallyn
1 sibling, 0 replies; 10+ messages in thread
From: Serge Hallyn @ 2012-04-13 19:34 UTC (permalink / raw)
To: Oleg Nesterov; +Cc: Andrew Morton, Daniel Lezcano, linux-kernel
Quoting Oleg Nesterov (oleg@redhat.com):
> Hi Serge,
>
> On 04/13, Serge Hallyn wrote:
> >
> > Quoting Oleg Nesterov (oleg@redhat.com):
> > > cf3f8921 "pidns: add reboot_pid_ns() to handle the reboot syscall"
> > > uses force_sig() to kill the SIGNAL_UNKILLABLE cinit, this was the
> > > only choice.
> > >
> > > After 629d362b we can use SEND_SIG_FORCED instead, force_sig() has
> > > the special semantics and should be avoided if possible.
> > >
> > > Signed-off-by: Oleg Nesterov <oleg@redhat.com>
> >
> > Hi Oleg,
> >
> > I like the patch in theory, however after applying it containers are
> > no longer able to shutdown/reboot. I can externally kill them with
> > lxc-stop, but internal shutdown fails.
>
> Oh, thanks a lot for testing.
>
> Strange... I even tested this change, but only in the root namespace.
> OK, thanks, I'll investigate. I hope you verified your kernel has
> 629d362b commit ;)
You know, I did. But in the wrong tree!
Sorry. Will add that patch, re-build and re-test.
-serge
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] pidns: reboot_pid_ns: use SEND_SIG_FORCED instead of force_sig()
2012-04-13 19:24 ` Oleg Nesterov
2012-04-13 19:34 ` Serge Hallyn
@ 2012-04-16 13:59 ` Serge Hallyn
2012-04-16 15:38 ` Oleg Nesterov
1 sibling, 1 reply; 10+ messages in thread
From: Serge Hallyn @ 2012-04-16 13:59 UTC (permalink / raw)
To: Oleg Nesterov; +Cc: Andrew Morton, Daniel Lezcano, linux-kernel
Quoting Oleg Nesterov (oleg@redhat.com):
> Hi Serge,
>
> On 04/13, Serge Hallyn wrote:
> >
> > Quoting Oleg Nesterov (oleg@redhat.com):
> > > cf3f8921 "pidns: add reboot_pid_ns() to handle the reboot syscall"
> > > uses force_sig() to kill the SIGNAL_UNKILLABLE cinit, this was the
> > > only choice.
> > >
> > > After 629d362b we can use SEND_SIG_FORCED instead, force_sig() has
> > > the special semantics and should be avoided if possible.
> > >
> > > Signed-off-by: Oleg Nesterov <oleg@redhat.com>
> >
> > Hi Oleg,
> >
> > I like the patch in theory, however after applying it containers are
> > no longer able to shutdown/reboot. I can externally kill them with
> > lxc-stop, but internal shutdown fails.
>
> Oh, thanks a lot for testing.
>
> Strange... I even tested this change, but only in the root namespace.
> OK, thanks, I'll investigate. I hope you verified your kernel has
> 629d362b commit ;)
I took friday's HEAD of Linus tree. All went fine. Added this patch.
The container can shutdown, but reboot causes shutdown. Don't undersrtand
why.
-serge
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] pidns: reboot_pid_ns: use SEND_SIG_FORCED instead of force_sig()
2012-04-16 13:59 ` Serge Hallyn
@ 2012-04-16 15:38 ` Oleg Nesterov
2012-04-16 16:24 ` Serge Hallyn
0 siblings, 1 reply; 10+ messages in thread
From: Oleg Nesterov @ 2012-04-16 15:38 UTC (permalink / raw)
To: Serge Hallyn; +Cc: Andrew Morton, Daniel Lezcano, linux-kernel
On 04/16, Serge Hallyn wrote:
>
> Quoting Oleg Nesterov (oleg@redhat.com):
> >
> > Strange... I even tested this change, but only in the root namespace.
> > OK, thanks, I'll investigate. I hope you verified your kernel has
> > 629d362b commit ;)
>
> I took friday's HEAD of Linus tree. All went fine. Added this patch.
> The container can shutdown, but reboot causes shutdown. Don't undersrtand
> why.
Heh. This looks "impossible". I'd like to understand the reason.
So, LINUX_REBOOT_CMD_RESTART results in WIFSIGNALED() == SIGINT?
OK, I'll try to test this patch in the non-root namespace.
Oleg.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] pidns: reboot_pid_ns: use SEND_SIG_FORCED instead of force_sig()
2012-04-16 15:38 ` Oleg Nesterov
@ 2012-04-16 16:24 ` Serge Hallyn
2012-04-16 21:26 ` Oleg Nesterov
0 siblings, 1 reply; 10+ messages in thread
From: Serge Hallyn @ 2012-04-16 16:24 UTC (permalink / raw)
To: Oleg Nesterov; +Cc: Andrew Morton, Daniel Lezcano, linux-kernel
Quoting Oleg Nesterov (oleg@redhat.com):
> On 04/16, Serge Hallyn wrote:
> >
> > Quoting Oleg Nesterov (oleg@redhat.com):
> > >
> > > Strange... I even tested this change, but only in the root namespace.
> > > OK, thanks, I'll investigate. I hope you verified your kernel has
> > > 629d362b commit ;)
> >
> > I took friday's HEAD of Linus tree. All went fine. Added this patch.
> > The container can shutdown, but reboot causes shutdown. Don't undersrtand
> > why.
>
> Heh. This looks "impossible". I'd like to understand the reason.
> So, LINUX_REBOOT_CMD_RESTART results in WIFSIGNALED() == SIGINT?
>
> OK, I'll try to test this patch in the non-root namespace.
Playing with the test case below (based on the one Daniel submitted
alongside his container reboot patches), it looks like
WIFSIGNALED(status) and WTERMSIG(status) are 0 with this kernel, not
1 and sig.
-serge
#include <alloca.h>
#include <stdio.h>
#include <sched.h>
#include <unistd.h>
#include <signal.h>
#include <sys/reboot.h>
#include <sys/types.h>
#include <sys/wait.h>
#include <linux/sched.h>
#include <linux/reboot.h>
static int do_reboot(void *arg)
{
int *cmd = arg;
if (reboot(*cmd))
printf("failed to reboot(%d): %m\n", *cmd);
}
int test_reboot(int cmd, int sig)
{
long stack_size = 4096;
void *stack = alloca(stack_size) + stack_size;
int status;
pid_t ret;
ret = clone(do_reboot, stack, CLONE_NEWPID | SIGCHLD, &cmd);
if (ret < 0) {
printf("failed to clone: %m\n");
return -1;
}
if (wait(&status) < 0) {
printf("unexpected wait error: %m\n");
return -1;
}
printf("WIFSIGNALED is %d\n", WIFSIGNALED(status));
printf("signal termination is %d, expected %d)\n", WTERMSIG(status), sig);
if (!WIFSIGNALED(status)) {
if (sig != -1)
printf("child process exited but was not signaled\n");
return -1;
}
if (WTERMSIG(status) != sig) {
printf("signal termination is not the one expected\n");
return -1;
}
return 0;
}
static int have_reboot_patch(void)
{
FILE *f = fopen("/proc/sys/kernel/ctrl-alt-del", "r");
int ret;
int v;
if (!f)
return 0;
ret = fscanf(f, "%d", &v);
fclose(f);
if (ret != 1)
return 0;
ret = reboot(v ? LINUX_REBOOT_CMD_CAD_ON : LINUX_REBOOT_CMD_CAD_OFF);
if (ret != -1)
return 0;
return 1;
}
int main(int argc, char *argv[])
{
int status;
if (getuid() != 0) {
printf("Must run as root.\n");
return 1;
}
status = have_reboot_patch();
if (status != 0) {
printf("Your kernel does not have the container reboot patch\n");
return 1;
}
#if 0
status = test_reboot(LINUX_REBOOT_CMD_CAD_ON, -1);
if (status >= 0) {
printf("reboot(LINUX_REBOOT_CMD_CAD_ON) should have failed\n");
return 1;
}
printf("reboot(LINUX_REBOOT_CMD_CAD_ON) has failed as expected\n");
#endif
status = test_reboot(LINUX_REBOOT_CMD_RESTART, SIGHUP);
if (status < 0)
return 1;
printf("reboot(LINUX_REBOOT_CMD_RESTART) succeed\n");
status = test_reboot(LINUX_REBOOT_CMD_RESTART2, SIGHUP);
if (status < 0)
return 1;
printf("reboot(LINUX_REBOOT_CMD_RESTART2) succeed\n");
status = test_reboot(LINUX_REBOOT_CMD_HALT, SIGINT);
if (status < 0)
return 1;
printf("reboot(LINUX_REBOOT_CMD_HALT) succeed\n");
status = test_reboot(LINUX_REBOOT_CMD_POWER_OFF, SIGINT);
if (status < 0)
return 1;
printf("reboot(LINUX_REBOOT_CMD_POWERR_OFF) succeed\n");
printf("All tests passed\n");
return 0;
}
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] pidns: reboot_pid_ns: use SEND_SIG_FORCED instead of force_sig()
2012-04-16 16:24 ` Serge Hallyn
@ 2012-04-16 21:26 ` Oleg Nesterov
2012-04-17 18:35 ` Oleg Nesterov
0 siblings, 1 reply; 10+ messages in thread
From: Oleg Nesterov @ 2012-04-16 21:26 UTC (permalink / raw)
To: Serge Hallyn; +Cc: Andrew Morton, Daniel Lezcano, linux-kernel
On 04/16, Serge Hallyn wrote:
>
> Quoting Oleg Nesterov (oleg@redhat.com):
> > On 04/16, Serge Hallyn wrote:
> > >
> > > Quoting Oleg Nesterov (oleg@redhat.com):
> > > >
> > > > Strange... I even tested this change, but only in the root namespace.
> > > > OK, thanks, I'll investigate. I hope you verified your kernel has
> > > > 629d362b commit ;)
> > >
> > > I took friday's HEAD of Linus tree. All went fine. Added this patch.
> > > The container can shutdown, but reboot causes shutdown. Don't undersrtand
> > > why.
> >
> > Heh. This looks "impossible". I'd like to understand the reason.
> > So, LINUX_REBOOT_CMD_RESTART results in WIFSIGNALED() == SIGINT?
> >
> > OK, I'll try to test this patch in the non-root namespace.
>
> Playing with the test case below (based on the one Daniel submitted
> alongside his container reboot patches), it looks like
> WIFSIGNALED(status) and WTERMSIG(status) are 0 with this kernel, not
> 1 and sig.
Great, thanks Serge. I'll investigate and report tomorrow.
Oleg.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] pidns: reboot_pid_ns: use SEND_SIG_FORCED instead of force_sig()
2012-04-16 21:26 ` Oleg Nesterov
@ 2012-04-17 18:35 ` Oleg Nesterov
2012-04-18 2:29 ` Serge E. Hallyn
0 siblings, 1 reply; 10+ messages in thread
From: Oleg Nesterov @ 2012-04-17 18:35 UTC (permalink / raw)
To: Serge Hallyn; +Cc: Andrew Morton, Daniel Lezcano, linux-kernel
On 04/16, Oleg Nesterov wrote:
>
> I'll investigate and report tomorrow.
Thanks again Serge, everything is clear. Afaics this needs another
(almost off-topic) patch.
Please forget about sys_reboot() for the moment. There is a minor
and known problem with SIGKILL && SIGNAL_UNKILLABLE. If you send
SIGKILL from the parent namespace to cinit, this doesn't make it
SIGNAL_GROUP_EXIT immediately. Only after cinit actually dequeues
SIGKILL it does do_group_exit() and set this flag. This should be
fixed anyway, but not now.
And this explain the problem. Note that your test-case does
sys_reboot() from the init process, and reboot_pid_ns() does
kill(pid_ns->child_reaper, SIGKILL);
do_exit(0);
so it sends SIGKILL to itself and exit. This means that cinit
(current) obviously can't dequeue SIGKILL, and without
SIGNAL_GROUP_EXIT wait_task_zombie() ignores
signal->group_exit_code.
That is why the patch makes a difference.
But why reboot_pid_ns() does do_exit? Yes, it was copied from
sys_reboot() and I didn't notice. It should use do_group_exit(),
and sys_reboot() too. This is the patch I am going to send, but
I'll try to think a bit more.
Thanks!
Oleg.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] pidns: reboot_pid_ns: use SEND_SIG_FORCED instead of force_sig()
2012-04-17 18:35 ` Oleg Nesterov
@ 2012-04-18 2:29 ` Serge E. Hallyn
0 siblings, 0 replies; 10+ messages in thread
From: Serge E. Hallyn @ 2012-04-18 2:29 UTC (permalink / raw)
To: Oleg Nesterov; +Cc: Serge Hallyn, Andrew Morton, Daniel Lezcano, linux-kernel
Quoting Oleg Nesterov (oleg@redhat.com):
> On 04/16, Oleg Nesterov wrote:
> >
> > I'll investigate and report tomorrow.
>
> Thanks again Serge, everything is clear.
Phew :)
> Afaics this needs another
> (almost off-topic) patch.
>
> Please forget about sys_reboot() for the moment. There is a minor
> and known problem with SIGKILL && SIGNAL_UNKILLABLE. If you send
> SIGKILL from the parent namespace to cinit, this doesn't make it
> SIGNAL_GROUP_EXIT immediately. Only after cinit actually dequeues
> SIGKILL it does do_group_exit() and set this flag. This should be
> fixed anyway, but not now.
>
> And this explain the problem. Note that your test-case does
> sys_reboot() from the init process, and reboot_pid_ns() does
>
> kill(pid_ns->child_reaper, SIGKILL);
>
> do_exit(0);
>
> so it sends SIGKILL to itself and exit. This means that cinit
> (current) obviously can't dequeue SIGKILL, and without
> SIGNAL_GROUP_EXIT wait_task_zombie() ignores
> signal->group_exit_code.
>
> That is why the patch makes a difference.
>
> But why reboot_pid_ns() does do_exit? Yes, it was copied from
> sys_reboot() and I didn't notice. It should use do_group_exit(),
> and sys_reboot() too. This is the patch I am going to send, but
> I'll try to think a bit more.
>
> Thanks!
>
> Oleg.
Great, glad it makes sense! :)
I'll test when you have a patch ready. Thanks, Oleg.
-serge
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2012-04-18 2:29 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2012-04-13 0:44 [PATCH] pidns: reboot_pid_ns: use SEND_SIG_FORCED instead of force_sig() Oleg Nesterov
2012-04-13 18:20 ` Serge Hallyn
2012-04-13 19:24 ` Oleg Nesterov
2012-04-13 19:34 ` Serge Hallyn
2012-04-16 13:59 ` Serge Hallyn
2012-04-16 15:38 ` Oleg Nesterov
2012-04-16 16:24 ` Serge Hallyn
2012-04-16 21:26 ` Oleg Nesterov
2012-04-17 18:35 ` Oleg Nesterov
2012-04-18 2:29 ` Serge E. Hallyn
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.