pidns: reboot_pid_ns: use SEND_SIG_FORCED instead of force_sig()

PATCH In review Oleg Nesterov 3 participants 10 messages Apr 13, 2012 – Apr 18, 2012 Namespaces:

Share & tools

Oleg Nesterov

author

2012-04-13 00:45 UTC #0 lore raw

PATCH 1/1 pidns: reboot_pid_ns: use SEND_SIG_FORCED instead of force_sig()

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 <[email protected]>

kernel/pid_namespace.c +1 -1

index 57bc1fd..ef35e20 100644
@@ -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);

Serge Hallyn

2012-04-13 18:21 UTC #1 lore raw

Quoted 8 lines · Oleg Nesterov

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 <[email protected]>

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.

Quoted 14 lines

kernel/pid_namespace.c +1 -1

index 57bc1fd..ef35e20 100644
@@ -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);

Oleg Nesterov

author

2012-04-13 19:25 UTC #2 lore raw

Hi Serge,

Quoted 9 lines · Serge Hallyn

Quoted 7 lines · Oleg Nesterov
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.

Quoted 7 lines

Quoted 1 line
Signed-off-by Oleg Nesterov <[email protected]>
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.

Serge Hallyn

2012-04-13 19:34 UTC #3 lore raw

Quoted 20 lines · Oleg Nesterov

Hi Serge,
Quoted 9 lines · Serge Hallyn
Quoted 7 lines · Oleg Nesterov
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.
Quoted 7 lines
Quoted 1 line
Signed-off-by Oleg Nesterov <[email protected]>
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.

Quoted 5 lines

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

Serge Hallyn

2012-04-16 13:59 UTC #4 lore raw

In reply to Oleg Nesterov

Quoted 20 lines · Oleg Nesterov

Hi Serge,
Quoted 9 lines · Serge Hallyn
Quoted 7 lines · Oleg Nesterov
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.
Quoted 7 lines
Quoted 1 line
Signed-off-by Oleg Nesterov <[email protected]>
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.

Quoted 5 lines

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

Oleg Nesterov

author

2012-04-16 15:39 UTC #5 lore raw

Quoted 10 lines · Serge Hallyn

Quoted 4 lines · Oleg Nesterov
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.

Serge Hallyn

2012-04-16 16:24 UTC #6 lore raw

Quoted 8 lines · Oleg Nesterov

Quoted 7 lines · Serge Hallyn
Quoted 4 lines · Oleg Nesterov
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 ;)

Quoted 8 lines

Quoted 3 lines
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;
}

Oleg Nesterov

author

2012-04-16 21:26 UTC #7 lore raw

Quoted 17 lines · Serge Hallyn

Quoted 15 lines · Oleg Nesterov
Quoted 10 lines · Serge Hallyn
Quoted 4 lines · Oleg Nesterov
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?

Quoted 6 lines

Quoted 1 line
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.

Oleg Nesterov

author

2012-04-17 18:35 UTC #8 lore raw

Quoted 2 lines · Oleg Nesterov

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.

Serge E. Hallyn

2012-04-18 02:29 UTC #9 lore raw

Quoted 5 lines · Oleg Nesterov

Quoted 2 lines · Oleg Nesterov
I'll investigate and report tomorrow.
Thanks again Serge, everything is clear.

Phew :)

Quoted 24 lines

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.

Quoted 8 lines

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