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

[Kirill Tkhai]

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