Re: [PATCH 1/3] pidns: fix vfork() after unshare(CLONE_NEWPID)

[Oleg Nesterov <oleg@redhat.com>]

On 08/22, Andy Lutomirski wrote:
>
> On Thu, Aug 22, 2013 at 10:09 AM, Oleg Nesterov <oleg@redhat.com> wrote:
> > 8382fcac "pidns: Outlaw thread creation after unshare(CLONE_NEWPID)"
> > nacks CLONE_VM if the forking process unshared pid_ns, this obviously
> > breaks vfork:
> >
> >         int main(void)
> >         {
> >                 assert(unshare(CLONE_NEWUSER | CLONE_NEWPID) == 0);
> >                 assert(vfork() >= 0);
> >                 _exit(0);
> >                 return 0;
> >         }
> >
> > fails without this patch.
> >
> > Change this check to use CLONE_SIGHAND instead. This also forbids
> > CLONE_THREAD automatically, and this is what the comment implies.
> >
> > We could probably even drop CLONE_SIGHAND and use CLONE_THREAD,
> > but it would be safer to not do this. The current check denies
> > CLONE_SIGHAND implicitely and there is no reason to change this.
>
> Acked-by: Andy Lutomirski <luto@amacapital.net>

Thanks Andy for taking a look.

> > --- a/kernel/fork.c
> > +++ b/kernel/fork.c
> > @@ -1173,10 +1173,10 @@ static struct task_struct *copy_process(unsigned long clone_flags,
> >                 return ERR_PTR(-EINVAL);
> >
> >         /*
> > -        * If the new process will be in a different pid namespace
> > -        * don't allow the creation of threads.
> > +        * If the new process will be in a different pid namespace don't
> > +        * allow the creation of threads, or share the signal handlers.
>
> ...how about "If the new process will be in a different pid namespace,
> don't allow it to share a thread group or signal handlers with the
> parent"?

Agreed... But in this case I do not how 3/3 should explain CLONE_PARENT,
it adds "or share the parent" into this comment.

OK, I'll wait for other comments, then try to update this comment somehow
and send v2.

Perhaps "... with the forking task" would be fine?

Thanks.

Oleg.