[CRIU] [PATCH v2 1/5] user_ns: Close pid proc in create_user_ns_hierarhy_fn()

[Kirill Tkhai]

On 31.03.2017 21:13, Andrei Vagin wrote:
> On Wed, Mar 29, 2017 at 12:19:55PM +0300, Kirill Tkhai wrote:
>> Child is cloned with CLONE_FILES, so it may rewrite our
>> PROC_SELF file. Close it manually.
>>
>> We can't use call_in_child_process(), because we need
>> additional actions between clone() and wait().
>>
>> Signed-off-by: Kirill Tkhai <ktkhai at virtuozzo.com>
>> ---
>>  criu/namespaces.c |    1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/criu/namespaces.c b/criu/namespaces.c
>> index c1bee49f..3a24099c 100644
>> --- a/criu/namespaces.c
>> +++ b/criu/namespaces.c
>> @@ -2251,6 +2251,7 @@ static int create_user_ns_hierarhy_fn(void *in_arg)
>>  		futex_set_and_wake(p_futex, ret ? NS__ERROR : NS__RESTORED);
>>  	if (arg)
>>  		munmap(arg, sizeof(*arg));
>> +	close_pid_proc();
> 
>                 pid = clone(create_user_ns_hierarhy_fn, stack + 128,
> CLONE_NEWUSER | CLONE_FILES | SIGCHLD, arg);
> 
> This function is called from a process which shares files but doesn't
> share memory. close_pid_proc() closes a file descriptor and changes data
> in memory, but the parent process doesn't see changes in memory and may
> try to use the old file descriptor, which is closed already.
> 
> I want to say that close_pid_proc(), open_proc() can't be used in case,
> because it is unclear how it will affect a parent process.
> 
> https://github.com/xemul/criu/commit/9a38c503

I agree with you, and even started reworked, and after that a better solution
for me seems to be to add CLONE_VM there. We call common primitives
like fdstore_add() and get_self_real_pid() right now, and nobody knows
how they change in the future. If we use CLONE_VM, we'll have generic
solution for any changes, and we won't have problems with this function
anymore. Speaking about memory usage: even when we do not use CLONE_VM, we
allocate pages on COW, so it seems to be almost the same as we allocate
a separate stack. How are you about that?