Re: [PATCH] Add reboot_pid_ns to handle the reboot syscall

[Oleg Nesterov <oleg@redhat.com>]

On 12/03, Daniel Lezcano wrote:
>
> This patch propose to store the reboot value in the 16 upper bits of the
> exit code from the processes belonging to a pid namespace which has
> rebooted. When the reboot syscall is called and we are not in the initial
> pid namespace, we kill the pid namespace.

OK, this is close to what we discussed before.

But why does this patch uglify wait_task_zombie() ?

> @@ -1192,6 +1192,7 @@ static int wait_task_zombie(struct wait_opts *wo, struct task_struct *p)
>  	pid_t pid = task_pid_vnr(p);
>  	uid_t uid = __task_cred(p)->uid;
>  	struct siginfo __user *infop;
> +	struct pid_namespace *pid_ns = task_active_pid_ns(p);
>
>  	if (!likely(wo->wo_flags & WEXITED))
>  		return 0;
> @@ -1291,8 +1292,10 @@ static int wait_task_zombie(struct wait_opts *wo, struct task_struct *p)
>  		? getrusage(p, RUSAGE_BOTH, wo->wo_rusage) : 0;
>  	status = (p->signal->flags & SIGNAL_GROUP_EXIT)
>  		? p->signal->group_exit_code : p->exit_code;
> -	if (!retval && wo->wo_stat)
> +	if (!retval && wo->wo_stat) {
> +		status |= (pid_ns->reboot & ~0xffff);
>  		retval = put_user(status, wo->wo_stat);
> +	}

This doesn't cover WNOWAIT.

But I think this change is not needed at all. Instead, can't you
add something like

	if (pid_ns->reboot)
		current->signal->group_exit_code = pid_ns->reboot;

into zap_pid_ns_processes() ? IIRC this was discussed too, I do
not understand why do you think we should hack do_wait()...


> +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 = SYSTEM_RESTART << 16;
> +		break;
> +
> +	case LINUX_REBOOT_CMD_HALT:
> +		pid_ns->reboot = SYSTEM_HALT << 16;
> +		break;
> +
> +	case LINUX_REBOOT_CMD_POWER_OFF:
> +		pid_ns->reboot = SYSTEM_POWER_OFF << 16;
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	force_sig(SIGKILL, pid_ns->child_reaper);

In theory this is racy. Nothing protects ->child_reaper if it is
multi-threaded. read_lock(tasklist) should help.

> +
> +	return 0;
> +}

I am not sure "return 0" is really correct. Perhaps HALT/POWER_OFF
should do do_exit() like the the "normal" sys_reboot() does ?

>  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..02d9645 100644
> --- a/kernel/sys.c
> +++ b/kernel/sys.c
> @@ -429,6 +429,7 @@ static DEFINE_MUTEX(reboot_mutex);
>  SYSCALL_DEFINE4(reboot, int, magic1, int, magic2, unsigned int, cmd,
>  		void __user *, arg)
>  {
> +	struct pid_namespace *pid_ns = current->nsproxy->pid_ns;
>  	char buffer[256];
>  	int ret = 0;
>
> @@ -450,6 +451,9 @@ SYSCALL_DEFINE4(reboot, int, magic1, int, magic2, unsigned int, cmd,
>  	if ((cmd == LINUX_REBOOT_CMD_POWER_OFF) && !pm_power_off)
>  		cmd = LINUX_REBOOT_CMD_HALT;
>
> +	if (pid_ns != &init_pid_ns)
> +		return reboot_pid_ns(pid_ns, cmd);

Cosmetic nit,

	if (task_active_pid_ns(current) != &init_pid_ns)
		return reboot_pid_ns(cmd);

this way we do not need the new variable.

Also. I do not know if this is important, but perhaps it makes
sense to move this code up, before the !pm_power_off check which
can transform POWER_OFF into HALT?

Oleg.