[PATCH 0/3] namespaces && fork fixes/cleanups

[PATCH 0/3] namespaces && fork fixes/cleanups

[Oleg Nesterov]

Hello.

The discussion was a bit confusing. I decided to split the changes
to simplify the documentation/review.

Eric, Andy, could you please ack/nack explicitly?

I guess it is too late for 3.11, hopefully Andrew can take these
patches if they pass the review.

OTOH, 1/3 fixes the regression, so perhaps it should cc -stable.

Oleg.

 kernel/fork.c |   22 ++++++++--------------
 1 files changed, 8 insertions(+), 14 deletions(-)

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

[Oleg Nesterov]

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.

Reported-by: Colin Walters <walters@redhat.com>
Signed-off-by: Oleg Nesterov <oleg@redhat.com>
---
 kernel/fork.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/kernel/fork.c b/kernel/fork.c
index e23bb19..86f5376 100644
--- 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.
 	 */
-	if ((clone_flags & (CLONE_VM|CLONE_NEWPID)) &&
+	if ((clone_flags & (CLONE_SIGHAND | CLONE_NEWPID)) &&
 	    (task_active_pid_ns(current) != current->nsproxy->pid_ns))
 		return ERR_PTR(-EINVAL);
 
-- 
1.5.5.1

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

[Andy Lutomirski]

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>

although...

>
> Reported-by: Colin Walters <walters@redhat.com>
> Signed-off-by: Oleg Nesterov <oleg@redhat.com>
> ---
>  kernel/fork.c |    6 +++---
>  1 files changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/kernel/fork.c b/kernel/fork.c
> index e23bb19..86f5376 100644
> --- 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"?

--Andy

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

[Oleg Nesterov]

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.

[PATCH 2/3] pidns: kill the unnecessary CLONE_NEWPID in copy_process()

[Oleg Nesterov]

8382fcac "pidns: Outlaw thread creation after unshare(CLONE_NEWPID)"
nacks CLONE_NEWPID if the forking process unshared pid_ns. This is
correct but unnecessary, copy_pid_ns() does the same check.

Test-case (needs root or CLONE_NEWUSER should be added):

	static int child(void *arg)
	{
		return 0;
	}

	static char stack[16 * 1024];

	int main(void)
	{
		pid_t pid;

		assert(unshare(CLONE_NEWPID) == 0);

		pid = clone(child, stack + sizeof(stack) / 2,
				CLONE_NEWPID | SIGCHLD, NULL);
		assert(pid < 0 && errno == EINVAL);

		return 0;
	}

clone(CLONE_NEWPID) correctly fails with or without this change.

Signed-off-by: Oleg Nesterov <oleg@redhat.com>
---
 kernel/fork.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/kernel/fork.c b/kernel/fork.c
index 86f5376..8d56338 100644
--- a/kernel/fork.c
+++ b/kernel/fork.c
@@ -1176,7 +1176,7 @@ static struct task_struct *copy_process(unsigned long clone_flags,
 	 * If the new process will be in a different pid namespace don't
 	 * allow the creation of threads, or share the signal handlers.
 	 */
-	if ((clone_flags & (CLONE_SIGHAND | CLONE_NEWPID)) &&
+	if ((clone_flags & CLONE_SIGHAND) &&
 	    (task_active_pid_ns(current) != current->nsproxy->pid_ns))
 		return ERR_PTR(-EINVAL);
 
-- 
1.5.5.1

Re: [PATCH 2/3] pidns: kill the unnecessary CLONE_NEWPID in copy_process()

[Andy Lutomirski]

On Thu, Aug 22, 2013 at 10:10 AM, Oleg Nesterov <oleg@redhat.com> wrote:
> 8382fcac "pidns: Outlaw thread creation after unshare(CLONE_NEWPID)"
> nacks CLONE_NEWPID if the forking process unshared pid_ns. This is
> correct but unnecessary, copy_pid_ns() does the same check.
>
> Test-case (needs root or CLONE_NEWUSER should be added):
>
>         static int child(void *arg)
>         {
>                 return 0;
>         }
>
>         static char stack[16 * 1024];
>
>         int main(void)
>         {
>                 pid_t pid;
>
>                 assert(unshare(CLONE_NEWPID) == 0);
>
>                 pid = clone(child, stack + sizeof(stack) / 2,
>                                 CLONE_NEWPID | SIGCHLD, NULL);
>                 assert(pid < 0 && errno == EINVAL);
>
>                 return 0;
>         }
>
> clone(CLONE_NEWPID) correctly fails with or without this change.

Acked-by: Andy Lutomirski <luto@amacapital.net>

>
> Signed-off-by: Oleg Nesterov <oleg@redhat.com>
> ---
>  kernel/fork.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/kernel/fork.c b/kernel/fork.c
> index 86f5376..8d56338 100644
> --- a/kernel/fork.c
> +++ b/kernel/fork.c
> @@ -1176,7 +1176,7 @@ static struct task_struct *copy_process(unsigned long clone_flags,
>          * If the new process will be in a different pid namespace don't
>          * allow the creation of threads, or share the signal handlers.
>          */
> -       if ((clone_flags & (CLONE_SIGHAND | CLONE_NEWPID)) &&
> +       if ((clone_flags & CLONE_SIGHAND) &&
>             (task_active_pid_ns(current) != current->nsproxy->pid_ns))
>                 return ERR_PTR(-EINVAL);
>
> --
> 1.5.5.1
>



-- 
Andy Lutomirski
AMA Capital Management, LLC

[PATCH 3/3] fork: unify and tighten up CLONE_NEWUSER/CLONE_NEWPID checks

[Oleg Nesterov]

do_fork() denies CLONE_THREAD | CLONE_PARENT if NEWUSER | NEWPID.

Then later copy_process() denies CLONE_SIGHAND if the new process
will be in a different pid namespace (task_active_pid_ns() doesn't
match current->nsproxy->pid_ns).

This looks confusing and inconsistent. CLONE_NEWPID is very similar
to the case when ->pid_ns was already unshared, we want the same
restrictions so copy_process() should also nack CLONE_PARENT.

And it would be better to deny CLONE_NEWUSER && CLONE_SIGHAND as
well just for consistency.

Kill the "CLONE_NEWUSER | CLONE_NEWPID" check in do_fork() and
change copy_process() to the same check along with nsproxy->pid_ns
we already have.

Signed-off-by: Oleg Nesterov <oleg@redhat.com>
---
 kernel/fork.c |   22 ++++++++--------------
 1 files changed, 8 insertions(+), 14 deletions(-)

diff --git a/kernel/fork.c b/kernel/fork.c
index 8d56338..fae2ff7 100644
--- a/kernel/fork.c
+++ b/kernel/fork.c
@@ -1173,12 +1173,15 @@ 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, or share the signal handlers.
+	 * If the new process will be in a different pid or user namespace
+	 * don't allow the creation of threads, or share the signal handlers,
+	 * or share the parent.
 	 */
-	if ((clone_flags & CLONE_SIGHAND) &&
-	    (task_active_pid_ns(current) != current->nsproxy->pid_ns))
-		return ERR_PTR(-EINVAL);
+	if (clone_flags & (CLONE_SIGHAND | CLONE_PARENT)) {
+		if ((clone_flags & (CLONE_NEWUSER | CLONE_NEWPID)) ||
+		    (task_active_pid_ns(current) != current->nsproxy->pid_ns))
+			return ERR_PTR(-EINVAL);
+	}
 
 	retval = security_task_create(clone_flags);
 	if (retval)
@@ -1575,15 +1578,6 @@ long do_fork(unsigned long clone_flags,
 	long nr;
 
 	/*
-	 * Do some preliminary argument and permissions checking before we
-	 * actually start allocating stuff
-	 */
-	if (clone_flags & (CLONE_NEWUSER | CLONE_NEWPID)) {
-		if (clone_flags & (CLONE_THREAD|CLONE_PARENT))
-			return -EINVAL;
-	}
-
-	/*
 	 * Determine whether and which event to report to ptracer.  When
 	 * called from kernel_thread or CLONE_UNTRACED is explicitly
 	 * requested, no event is reported; otherwise, report if the event
-- 
1.5.5.1

Re: [PATCH 3/3] fork: unify and tighten up CLONE_NEWUSER/CLONE_NEWPID checks

[Andy Lutomirski]

On Thu, Aug 22, 2013 at 10:10 AM, Oleg Nesterov <oleg@redhat.com> wrote:
> do_fork() denies CLONE_THREAD | CLONE_PARENT if NEWUSER | NEWPID.
>
> Then later copy_process() denies CLONE_SIGHAND if the new process
> will be in a different pid namespace (task_active_pid_ns() doesn't
> match current->nsproxy->pid_ns).
>
> This looks confusing and inconsistent. CLONE_NEWPID is very similar
> to the case when ->pid_ns was already unshared, we want the same
> restrictions so copy_process() should also nack CLONE_PARENT.
>
> And it would be better to deny CLONE_NEWUSER && CLONE_SIGHAND as
> well just for consistency.
>
> Kill the "CLONE_NEWUSER | CLONE_NEWPID" check in do_fork() and
> change copy_process() to the same check along with nsproxy->pid_ns
> we already have.

Did the old code actually prevent clone(CLONE_PARENT | CLONE_NEWPID)?
The new code explicitly does, and that looks like a good thing.

>
> Signed-off-by: Oleg Nesterov <oleg@redhat.com>
> ---
>  kernel/fork.c |   22 ++++++++--------------
>  1 files changed, 8 insertions(+), 14 deletions(-)
>
> diff --git a/kernel/fork.c b/kernel/fork.c
> index 8d56338..fae2ff7 100644
> --- a/kernel/fork.c
> +++ b/kernel/fork.c
> @@ -1173,12 +1173,15 @@ 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, or share the signal handlers.
> +        * If the new process will be in a different pid or user namespace
> +        * don't allow the creation of threads, or share the signal handlers,
> +        * or share the parent.
>          */
> -       if ((clone_flags & CLONE_SIGHAND) &&
> -           (task_active_pid_ns(current) != current->nsproxy->pid_ns))
> -               return ERR_PTR(-EINVAL);
> +       if (clone_flags & (CLONE_SIGHAND | CLONE_PARENT)) {
> +               if ((clone_flags & (CLONE_NEWUSER | CLONE_NEWPID)) ||
> +                   (task_active_pid_ns(current) != current->nsproxy->pid_ns))
> +                       return ERR_PTR(-EINVAL);
> +       }
>
>         retval = security_task_create(clone_flags);
>         if (retval)
> @@ -1575,15 +1578,6 @@ long do_fork(unsigned long clone_flags,
>         long nr;
>
>         /*
> -        * Do some preliminary argument and permissions checking before we
> -        * actually start allocating stuff
> -        */
> -       if (clone_flags & (CLONE_NEWUSER | CLONE_NEWPID)) {
> -               if (clone_flags & (CLONE_THREAD|CLONE_PARENT))
> -                       return -EINVAL;
> -       }
> -
> -       /*
>          * Determine whether and which event to report to ptracer.  When
>          * called from kernel_thread or CLONE_UNTRACED is explicitly
>          * requested, no event is reported; otherwise, report if the event
> --
> 1.5.5.1
>



-- 
Andy Lutomirski
AMA Capital Management, LLC

Re: [PATCH 3/3] fork: unify and tighten up CLONE_NEWUSER/CLONE_NEWPID checks

[Oleg Nesterov]

On 08/22, Andy Lutomirski wrote:
>
> On Thu, Aug 22, 2013 at 10:10 AM, Oleg Nesterov <oleg@redhat.com> wrote:
> > do_fork() denies CLONE_THREAD | CLONE_PARENT if NEWUSER | NEWPID.
> >
> > Then later copy_process() denies CLONE_SIGHAND if the new process
> > will be in a different pid namespace (task_active_pid_ns() doesn't
> > match current->nsproxy->pid_ns).
> >
> > This looks confusing and inconsistent. CLONE_NEWPID is very similar
> > to the case when ->pid_ns was already unshared, we want the same
> > restrictions so copy_process() should also nack CLONE_PARENT.
> >
> > And it would be better to deny CLONE_NEWUSER && CLONE_SIGHAND as
> > well just for consistency.
> >
> > Kill the "CLONE_NEWUSER | CLONE_NEWPID" check in do_fork() and
> > change copy_process() to the same check along with nsproxy->pid_ns
> > we already have.
>
> Did the old code actually prevent clone(CLONE_PARENT | CLONE_NEWPID)?
> The new code explicitly does, and that looks like a good thing.


Yes. Before this patch do_fork() did:

	if (clone_flags & (CLONE_NEWUSER | CLONE_NEWPID)) {
		if (clone_flags & (CLONE_THREAD|CLONE_PARENT))
			return -EINVAL;
	}

however, let me repeat, CLONE_PARENT after unshare(CLONE_NEWPID) was
allowed. With this patch CLONE_PARENT is nacked in both cases.

Oleg.

Re: [PATCH 3/3] fork: unify and tighten up CLONE_NEWUSER/CLONE_NEWPID checks

[Andy Lutomirski]

On Thu, Aug 22, 2013 at 11:15 AM, Oleg Nesterov <oleg@redhat.com> wrote:
> On 08/22, Andy Lutomirski wrote:
>>
>> On Thu, Aug 22, 2013 at 10:10 AM, Oleg Nesterov <oleg@redhat.com> wrote:
>> > do_fork() denies CLONE_THREAD | CLONE_PARENT if NEWUSER | NEWPID.
>> >
>> > Then later copy_process() denies CLONE_SIGHAND if the new process
>> > will be in a different pid namespace (task_active_pid_ns() doesn't
>> > match current->nsproxy->pid_ns).
>> >
>> > This looks confusing and inconsistent. CLONE_NEWPID is very similar
>> > to the case when ->pid_ns was already unshared, we want the same
>> > restrictions so copy_process() should also nack CLONE_PARENT.
>> >
>> > And it would be better to deny CLONE_NEWUSER && CLONE_SIGHAND as
>> > well just for consistency.
>> >
>> > Kill the "CLONE_NEWUSER | CLONE_NEWPID" check in do_fork() and
>> > change copy_process() to the same check along with nsproxy->pid_ns
>> > we already have.
>>
>> Did the old code actually prevent clone(CLONE_PARENT | CLONE_NEWPID)?
>> The new code explicitly does, and that looks like a good thing.
>
>
> Yes. Before this patch do_fork() did:
>
>         if (clone_flags & (CLONE_NEWUSER | CLONE_NEWPID)) {
>                 if (clone_flags & (CLONE_THREAD|CLONE_PARENT))
>                         return -EINVAL;
>         }
>
> however, let me repeat, CLONE_PARENT after unshare(CLONE_NEWPID) was
> allowed. With this patch CLONE_PARENT is nacked in both cases.
>

Is this -stable-worthy?

--Andy


> Oleg.
>



-- 
Andy Lutomirski
AMA Capital Management, LLC

Re: [PATCH 3/3] fork: unify and tighten up CLONE_NEWUSER/CLONE_NEWPID checks

[Oleg Nesterov]

On 08/22, Andy Lutomirski wrote:
>
> On Thu, Aug 22, 2013 at 11:15 AM, Oleg Nesterov <oleg@redhat.com> wrote:
> >
> > Yes. Before this patch do_fork() did:
> >
> >         if (clone_flags & (CLONE_NEWUSER | CLONE_NEWPID)) {
> >                 if (clone_flags & (CLONE_THREAD|CLONE_PARENT))
> >                         return -EINVAL;
> >         }
> >
> > however, let me repeat, CLONE_PARENT after unshare(CLONE_NEWPID) was
> > allowed. With this patch CLONE_PARENT is nacked in both cases.
>
> Is this -stable-worthy?

Honestly, I do not know. I do not want to abuse -stable, and I will
sleep better if this patch won't go into the stable trees ;)

OTOH, I think that at least 1/3 is probably -stable material... Since
I am going to send v2, I would not mind to add stable@vger.kernel.org
if both you and Eric agree.

Oleg.

Re: [PATCH 3/3] fork: unify and tighten up CLONE_NEWUSER/CLONE_NEWPID checks

[Andy Lutomirski]

On Thu, Aug 22, 2013 at 11:32 AM, Oleg Nesterov <oleg@redhat.com> wrote:
> On 08/22, Andy Lutomirski wrote:
>>
>> On Thu, Aug 22, 2013 at 11:15 AM, Oleg Nesterov <oleg@redhat.com> wrote:
>> >
>> > Yes. Before this patch do_fork() did:
>> >
>> >         if (clone_flags & (CLONE_NEWUSER | CLONE_NEWPID)) {
>> >                 if (clone_flags & (CLONE_THREAD|CLONE_PARENT))
>> >                         return -EINVAL;
>> >         }
>> >
>> > however, let me repeat, CLONE_PARENT after unshare(CLONE_NEWPID) was
>> > allowed. With this patch CLONE_PARENT is nacked in both cases.
>>
>> Is this -stable-worthy?
>
> Honestly, I do not know. I do not want to abuse -stable, and I will
> sleep better if this patch won't go into the stable trees ;)
>
> OTOH, I think that at least 1/3 is probably -stable material... Since
> I am going to send v2, I would not mind to add stable@vger.kernel.org
> if both you and Eric agree.

This may allow creation of a process with tgid and pid in different
pid namespaces.  If so, I have no idea what the consequences would be.

>
> Oleg.
>



-- 
Andy Lutomirski
AMA Capital Management, LLC

Re: [PATCH 3/3] fork: unify and tighten up CLONE_NEWUSER/CLONE_NEWPID checks

[Oleg Nesterov]

On 08/22, Andy Lutomirski wrote:
>
> On Thu, Aug 22, 2013 at 11:32 AM, Oleg Nesterov <oleg@redhat.com> wrote:
> > On 08/22, Andy Lutomirski wrote:
> >>
> >> On Thu, Aug 22, 2013 at 11:15 AM, Oleg Nesterov <oleg@redhat.com> wrote:
> >> >
> >> > Yes. Before this patch do_fork() did:
> >> >
> >> >         if (clone_flags & (CLONE_NEWUSER | CLONE_NEWPID)) {
> >> >                 if (clone_flags & (CLONE_THREAD|CLONE_PARENT))
> >> >                         return -EINVAL;
> >> >         }
> >> >
> >> > however, let me repeat, CLONE_PARENT after unshare(CLONE_NEWPID) was
> >> > allowed. With this patch CLONE_PARENT is nacked in both cases.
> >>
> >> Is this -stable-worthy?
> >
> > Honestly, I do not know. I do not want to abuse -stable, and I will
> > sleep better if this patch won't go into the stable trees ;)
> >
> > OTOH, I think that at least 1/3 is probably -stable material... Since
> > I am going to send v2, I would not mind to add stable@vger.kernel.org
> > if both you and Eric agree.
>
> This may allow creation of a process with tgid and pid in different
> pid namespaces.  If so, I have no idea what the consequences would be.

and share the parent with the creator.

Not good. But probably not too bad, one can abuse ->pidns_install()
anyway, create a child in another ns, exit. Like it or not but pid_ns
is "special" and you even sent the patch to reflect this sad^W fact.

Anyway. The main point of this patch is the consistency (plus imho it
cleanups/simplifies the code). Both CLONE_NEWPID and
"task_active_pid_ns() != pid_ns" create a task in another namespace,
we should use the same restrictions.

And you seem to agree with this change, can I take it as your ack ?
I am going to preserve your acks in 1-2 and resend.

Oleg.

Re: [PATCH 3/3] fork: unify and tighten up CLONE_NEWUSER/CLONE_NEWPID checks

[Andy Lutomirski]

On Fri, Aug 23, 2013 at 6:59 AM, Oleg Nesterov <oleg@redhat.com> wrote:
> On 08/22, Andy Lutomirski wrote:
>>
>> On Thu, Aug 22, 2013 at 11:32 AM, Oleg Nesterov <oleg@redhat.com> wrote:
>> > On 08/22, Andy Lutomirski wrote:
>> >>
>> >> On Thu, Aug 22, 2013 at 11:15 AM, Oleg Nesterov <oleg@redhat.com> wrote:
>> >> >
>> >> > Yes. Before this patch do_fork() did:
>> >> >
>> >> >         if (clone_flags & (CLONE_NEWUSER | CLONE_NEWPID)) {
>> >> >                 if (clone_flags & (CLONE_THREAD|CLONE_PARENT))
>> >> >                         return -EINVAL;
>> >> >         }
>> >> >
>> >> > however, let me repeat, CLONE_PARENT after unshare(CLONE_NEWPID) was
>> >> > allowed. With this patch CLONE_PARENT is nacked in both cases.
>> >>
>> >> Is this -stable-worthy?
>> >
>> > Honestly, I do not know. I do not want to abuse -stable, and I will
>> > sleep better if this patch won't go into the stable trees ;)
>> >
>> > OTOH, I think that at least 1/3 is probably -stable material... Since
>> > I am going to send v2, I would not mind to add stable@vger.kernel.org
>> > if both you and Eric agree.
>>
>> This may allow creation of a process with tgid and pid in different
>> pid namespaces.  If so, I have no idea what the consequences would be.
>
> and share the parent with the creator.
>
> Not good. But probably not too bad, one can abuse ->pidns_install()
> anyway, create a child in another ns, exit. Like it or not but pid_ns
> is "special" and you even sent the patch to reflect this sad^W fact.
>
> Anyway. The main point of this patch is the consistency (plus imho it
> cleanups/simplifies the code). Both CLONE_NEWPID and
> "task_active_pid_ns() != pid_ns" create a task in another namespace,
> we should use the same restrictions.
>
> And you seem to agree with this change, can I take it as your ack ?
> I am going to preserve your acks in 1-2 and resend.
>

Yes.  These patches probably want Eric's ack, too, though -- it's his code.

--Andy