[CRIU] [PATCH RFC] namespaces: use CLONE_VFORK with CLONE_VM when it is possible

[Andrei Vagin]

On Mon, Mar 20, 2017 at 03:38:19PM +0300, Kirill Tkhai wrote:
> On 20.03.2017 14:48, Kirill Tkhai wrote:
> > Looks good
> > 
> > On 18.03.2017 01:29, Andrei Vagin wrote:
> >> From: Andrei Vagin <avagin at virtuozzo.com>
> >>
> >> We use CLONE_VM to dump and restore namespaces. Currently we allocate
> >> a new stack, but CLONE_VFORK allows to work on the current stack, because
> >> a parent process is frozen while a child process is running.
> >>
> >> Signed-off-by: Andrei Vagin <avagin at virtuozzo.com>
> > 
> > Reviewed-byL Kirill Tkhai <ktkhai at virtuozzo.com>
> 
> Rethinked once again, and now this solution does not seem so good for me.
> clone() is glibc wrapper, and we can't do any assumption about its stack
> usage. Even if 256 bytes looks enough now, it may be enough only on our
> (your or my) machines and configurations.

If 256 will be not enough, we will need to think how to work without
glibc;). We can replace 256 on 1024. Anyway it is better than allocating
a new 2Mb vma

>  
> >> ---
> >>  criu/clone-noasan.c |  8 ++++----
> >>  criu/namespaces.c   | 18 +++++-------------
> >>  2 files changed, 9 insertions(+), 17 deletions(-)
> >>
> >> diff --git a/criu/clone-noasan.c b/criu/clone-noasan.c
> >> index c5171b1..1b19f3e 100644
> >> --- a/criu/clone-noasan.c
> >> +++ b/criu/clone-noasan.c
> >> @@ -19,13 +19,13 @@
> >>   */
> >>  int clone_noasan(int (*fn)(void *), int flags, void *arg)
> >>  {
> >> +	int ret;
> >>  	/*
> >>  	 * Reserve some space for clone() to locate arguments
> >> -	 * and retcode in this place
> >> +	 * and retcode in this place. stack_ptr is set out of the current used
> >> +	 * stack to be able to use CLONE_VFORK | CLONE_VM.
> >>  	 */
> >> -	char stack[128] __stack_aligned__;
> >> -	char *stack_ptr = &stack[sizeof(stack)];
> >> -	int ret;
> >> +	void *stack_ptr = (void *) round_down(((unsigned long) &ret) - 256, 16);
> >>  
> >>  	ret = clone(fn, stack_ptr, flags, arg);
> >>  	return ret;
> >> diff --git a/criu/namespaces.c b/criu/namespaces.c
> >> index a4c3063..f3bcbbd 100644
> >> --- a/criu/namespaces.c
> >> +++ b/criu/namespaces.c
> >> @@ -33,6 +33,7 @@
> >>  #include "common/scm.h"
> >>  #include "fdstore.h"
> >>  #include "proc_parse.h"
> >> +#include "clone-noasan.h"
> >>  
> >>  static struct ns_desc *ns_desc_array[] = {
> >>  	&net_ns_desc,
> >> @@ -985,19 +986,17 @@ static int dump_user_ns(void *arg)
> >>  
> >>  	if (switch_ns(ns->parent->ns_pid, &user_ns_desc, NULL) < 0) {
> >>  		pr_err("Can't enter user namespace\n");
> >> -		return -1;
> >> +		_exit(1);
> >>  	}
> >> -
> >> -	return __dump_user_ns(ns);
> >> +	_exit(__dump_user_ns(ns) == 0 ? 0 : 1);
> >>  }
> >>  
> >>  int collect_user_ns(struct ns_id *ns, void *oarg)
> >>  {
> >> -	int status, stack_size;
> >> +	int status;
> >>  	struct ns_id *p_ns;
> >>  	pid_t pid = -1;
> >>  	UsernsEntry *e;
> >> -	char *stack;
> >>  
> >>  	p_ns = ns->parent;
> >>  
> >> @@ -1026,13 +1025,7 @@ int collect_user_ns(struct ns_id *ns, void *oarg)
> >>  		 * may do changes about CRIU's internal files states in memory,
> >>  		 * so pass CLONE_FILES to reflect that.
> >>  		 */
> >> -		stack_size = 2 * 1024 * 1024;
> >> -		stack = mmap(NULL, stack_size, PROT_WRITE | PROT_READ, MAP_ANONYMOUS | MAP_PRIVATE, -1, 0);
> >> -		if (stack == MAP_FAILED) {
> >> -			pr_perror("Can't allocate stack");
> >> -			return -1;
> >> -		}
> >> -		pid = clone(dump_user_ns, stack + stack_size, CLONE_VM | CLONE_FILES | SIGCHLD, ns);
> >> +		pid = clone_noasan(dump_user_ns, CLONE_VM | CLONE_FILES | SIGCHLD | CLONE_VFORK, ns);
> >>  		if (pid == -1) {
> >>  			pr_perror("Can't clone");
> >>  			return -1;
> >> @@ -1045,7 +1038,6 @@ int collect_user_ns(struct ns_id *ns, void *oarg)
> >>  			pr_err("Can't dump nested user_ns: %x\n", status);
> >>  			return -1;
> >>  		}
> >> -		munmap(stack, stack_size);
> >>  		return 0;
> >>  	} else {
> >>  		if (__dump_user_ns(ns))
> >>