[CRIU] [RFC] compel: kill self-unmap in parasite

[Andrei Vagin]

On Fri, Nov 25, 2016 at 03:51:40PM +0300, Dmitry Safonov wrote:
> Why should we have self-unmapping code in parasite?
> It looks like, we can drop this code using simple sys_unmap()
> injection (like that I did for `criu exec` action and for cases where we
> failed to insert parasite by some reason, but still need to unmap remotes).
> 
> It's an RFC, so just a suggestion - maybe I miss something you have in
> mind - please, describe that/those things.
> 
> My motivation is:
> - less code, defined commands for PIE, one BUG() less, one jump to PIE less
> - I'm making one 64-bit parasite on x86 instead of two 32 and 64 bit.
>   It works (branch 32-one-parasite) with long-jump in the beginning to
>   64-bit code from 32-bit task.
>   On parasite curing it sig-returns from 64-bit parasite to 32-bit task,
>   this point we're trapping in CRIU. After that we command parasite to
>   unmap itself, so it long-jumps again to parasite 64-bit code, unmaps,
>   we caught task after sys_unmap and the task is with 64-bit CS.
>   We can't set 32-bit registers after this - kernel checks that
>   registers set is the same on PTRACE_SETREGSET:
> > static int ptrace_regset(struct task_struct *task, int req, unsigned int type,
> >                        struct iovec *kiov)
> ...
> >       if (!regset || (kiov->iov_len % regset->size) != 0)
> >               return -EINVAL;
>   So, to return again to 32-bit task I need sigreturn() again or add
>   long-jump with 32-bit CS.
>   I've disable that for 32-bit testing with (in compel_cure_remote):
> -       if (ctl->addr_cmd) {
> +       if (ctl->addr_cmd && user_regs_native(&ctl->orig.regs)) {
>   And it works. It also works for native tasks, so why should we keep it?
> 
> Cc: Cyrill Gorcunov <gorcunov at openvz.org>
> Cc: Pavel Emelyanov <xemul at virtuozzo.com>
> Cc: Andrei Vagin <avagin at virtuozzo.com>

Acked-by: Andrei Vagin <avagin at virtuozzo.com>

> Signed-off-by: Dmitry Safonov <dsafonov at virtuozzo.com>
> ---
>  compel/include/rpc-pie-priv.h |  1 -
>  compel/plugins/std/infect.c   | 20 +-------------------
>  compel/src/lib/infect.c       | 31 +++++++++----------------------
>  3 files changed, 10 insertions(+), 42 deletions(-)
> 
> diff --git a/compel/include/rpc-pie-priv.h b/compel/include/rpc-pie-priv.h
> index 3d9091159737..f25ca89eb344 100644
> --- a/compel/include/rpc-pie-priv.h
> +++ b/compel/include/rpc-pie-priv.h
> @@ -22,7 +22,6 @@ enum {
>  	PARASITE_CMD_ACK,
>  
>  	PARASITE_CMD_INIT_DAEMON,
> -	PARASITE_CMD_UNMAP,
>  
>  	/*
>  	 * This must be greater than INITs.
> diff --git a/compel/plugins/std/infect.c b/compel/plugins/std/infect.c
> index 4d06814516c4..e0cdb644a27f 100644
> --- a/compel/plugins/std/infect.c
> +++ b/compel/plugins/std/infect.c
> @@ -141,20 +141,6 @@ out:
>  	return 0;
>  }
>  
> -static noinline int unmap_itself(void *data)
> -{
> -	struct parasite_unmap_args *args = data;
> -
> -	sys_munmap((void*)(uintptr_t)args->parasite_start, args->parasite_len);
> -	/*
> -	 * This call to sys_munmap must never return. Instead, the controlling
> -	 * process must trap us on the exit from munmap.
> -	 */
> -
> -	BUG();
> -	return -1;
> -}
> -
>  static noinline __used int parasite_init_daemon(void *data)
>  {
>  	struct parasite_init_args *args = data;
> @@ -203,12 +189,8 @@ int __used __parasite_entry parasite_service(unsigned int cmd, void *args)
>  {
>  	pr_info("Parasite cmd %d/%x process\n", cmd, cmd);
>  
> -	switch (cmd) {
> -	case PARASITE_CMD_INIT_DAEMON:
> +	if (cmd == PARASITE_CMD_INIT_DAEMON)
>  		return parasite_init_daemon(args);
> -	case PARASITE_CMD_UNMAP:
> -		return unmap_itself(args);
> -	}
>  
>  	return parasite_trap_cmd(cmd, args);
>  }
> diff --git a/compel/src/lib/infect.c b/compel/src/lib/infect.c
> index b85f36ec6ecb..df4a3201e2af 100644
> --- a/compel/src/lib/infect.c
> +++ b/compel/src/lib/infect.c
> @@ -1286,34 +1286,21 @@ int compel_stop_daemon(struct parasite_ctl *ctl)
>  
>  int compel_cure_remote(struct parasite_ctl *ctl)
>  {
> +	unsigned long ret;
> +
>  	if (compel_stop_daemon(ctl))
>  		return -1;
>  
>  	if (!ctl->remote_map)
>  		return 0;
>  
> -	/* Unseizing task with parasite -- it does it himself */
> -	if (ctl->addr_cmd) {
> -		struct parasite_unmap_args *args;
> -
> -		*ctl->addr_cmd = PARASITE_CMD_UNMAP;
> -
> -		args = compel_parasite_args(ctl, struct parasite_unmap_args);
> -		args->parasite_start = (uintptr_t)ctl->remote_map;
> -		args->parasite_len = ctl->map_length;
> -		if (compel_unmap(ctl, ctl->parasite_ip))
> -			return -1;
> -	} else {
> -		unsigned long ret;
> -
> -		compel_syscall(ctl, __NR(munmap, !compel_mode_native(ctl)), &ret,
> -				(unsigned long)ctl->remote_map, ctl->map_length,
> -				0, 0, 0, 0);
> -		if (ret) {
> -			pr_err("munmap for remote map %p, %lu returned %lu\n",
> -					ctl->remote_map, ctl->map_length, ret);
> -			return -1;
> -		}
> +	compel_syscall(ctl, __NR(munmap, !compel_mode_native(ctl)), &ret,
> +			(unsigned long)ctl->remote_map, ctl->map_length,
> +			0, 0, 0, 0);
> +	if (ret) {
> +		pr_err("munmap for remote map %p, %lu returned %lu\n",
> +				ctl->remote_map, ctl->map_length, ret);
> +		return -1;
>  	}
>  
>  	return 0;
> -- 
> 2.10.2
>