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

[Andrei Vagin]

On Sat, Apr 01, 2017 at 02:29:32PM +0300, Kirill Tkhai wrote:
> 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?

I don't have any objections about this.