Re: [REVIEW][PATCH 2/3] fork: Allow CLONE_PARENT after setns(CLONE_NEWPID)

["Serge E. Hallyn" <serge-A9i7LUbDfNHQT0dZR+AlfA@public.gmane.org>]

Quoting Eric W. Biederman (ebiederm-aS9lmoZGLiVWk0Htik3J/w@public.gmane.org):
> 
> Serge Hallyn <serge.hallyn-GeWIH/nMZzLQT0dZR+AlfA@public.gmane.org> writes:
> > Hi Oleg,
> >
> > commit 40a0d32d1eaffe6aac7324ca92604b6b3977eb0e :
> > "fork: unify and tighten up CLONE_NEWUSER/CLONE_NEWPID checks"
> > breaks lxc-attach in 3.12.  That code forks a child which does
> > setns() and then does a clone(CLONE_PARENT).  That way the
> > grandchild can be in the right namespaces (which the child was
> > not) and be a child of the original task, which is the monitor.
> >
> > lxc-attach in 3.11 was working fine with no side effects that I
> > could see.  Is there a real danger in allowing CLONE_PARENT
> > when current->nsproxy->pidns_for_children is not our pidns,
> > or was this done out of an "over-abundance of caution"?  Can we
> > safely revert that new extra check?
> 
> The two fundamental things I know we can not allow are:
> - A shared signal queue aka CLONE_THREAD.  Because we compute the pid
>   and uid of the signal when we place it in the queue.
> 
> - Changing the pid and by extention pid_namespace of an existing
>   process.
> 
> >From a parents perspective there is nothing special about the pid
> namespace, to deny CLONE_PARENT, because the parent simply won't know or
> care.
> 
> >From the childs perspective all that is special really are shared signal
> queues.
> 
> User mode threading with CLONE_PARENT|CLONE_VM|CLONE_SIGHAND and tasks
> in different pid namespaces is almost certainly going to break because
> it is complicated.  But shared signal handlers can look at per thread
> information to know which pid namespace a process is in, so I don't know
> of any reason not to support CLONE_PARENT|CLONE_VM|CLONE_SIGHAND threads
> at the kernel level.  It would be absolutely stupid to implement but
> that is a different thing.
> 
> So hmm.
> 
> Because it can do no harm, and because it is a regression let's remove
> the CLONE_PARENT check and send it stable.
> Cc: stable-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> Acked-by: Oleg Nesterov <oleg-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> Acked-by: Andy Lutomirski <luto-kltTT9wpgjJwATOyAt5JVQ@public.gmane.org>
> Acked-by: Serge E. Hallyn <serge.hallyn-GeWIH/nMZzLQT0dZR+AlfA@public.gmane.org>

Thanks, Eric.

> Signed-off-by: "Eric W. Biederman" <ebiederm-aS9lmoZGLiVWk0Htik3J/w@public.gmane.org>
> ---
>  kernel/fork.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/kernel/fork.c b/kernel/fork.c
> index 728d5be9548c..f82fa2ee7581 100644
> --- a/kernel/fork.c
> +++ b/kernel/fork.c
> @@ -1171,7 +1171,7 @@ static struct task_struct *copy_process(unsigned long clone_flags,
>  	 * do not allow it to share a thread group or signal handlers or
>  	 * parent with the forking task.
>  	 */
> -	if (clone_flags & (CLONE_SIGHAND | CLONE_PARENT)) {
> +	if (clone_flags & CLONE_SIGHAND) {
>  		if ((clone_flags & (CLONE_NEWUSER | CLONE_NEWPID)) ||
>  		    (task_active_pid_ns(current) !=
>  				current->nsproxy->pid_ns_for_children))
> -- 
> 1.7.5.4