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

[Daniel Lezcano <daniel.lezcano@free.fr>]

On 12/03/2011 05:49 PM, Oleg Nesterov wrote:
> 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() ?

Right, see below :)

>> @@ -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()...

Gah ! Right ! I puzzled myself, I will change that.

>> +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.

Ok.

>> +
>> +	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 ?

Yes.

>>  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?

Yes.

Thanks Oleg for your comments and your help. I will send a new patch.

  -- Daniel