[PATCH RFC] setns: return 0 directly if try to reassociate with current namespace

[PATCH RFC] setns: return 0 directly if try to reassociate with current namespace

[Chen Hanxiao]

We could use setns to join the current ns,
which did a lot of unnecessary work.
This patch will check this senario and
return 0 directly.

Signed-off-by: Chen Hanxiao <chenhanxiao-BthXqXjhjHXQFUHtdCDX3A@public.gmane.org>
---
 kernel/nsproxy.c | 28 ++++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)

diff --git a/kernel/nsproxy.c b/kernel/nsproxy.c
index ef42d0a..66eea63 100644
--- a/kernel/nsproxy.c
+++ b/kernel/nsproxy.c
@@ -236,6 +236,34 @@ SYSCALL_DEFINE2(setns, int, fd, int, nstype)
 	ops = ei->ns_ops;
 	if (nstype && (ops->type != nstype))
 		goto out;
+	switch (ops->type) {
+	case CLONE_NEWIPC:
+		if (ei->ns == tsk->nsproxy->ipc_ns)
+			return 0;
+		break;
+	case CLONE_NEWNET:
+		if (ei->ns == tsk->nsproxy->net_ns)
+			return 0;
+		break;
+	case CLONE_NEWNS:
+		if (ei->ns == tsk->nsproxy->mnt_ns)
+			return 0;
+		break;
+	case CLONE_NEWPID:
+		if (ei->ns == tsk->nsproxy->pid_ns_for_children)
+			return 0;
+		break;
+	case CLONE_NEWUSER:
+		if (ei->ns == current_user_ns())
+			return 0;
+		break;
+	case CLONE_NEWUTS:
+		if (ei->ns == tsk->nsproxy->uts_ns)
+			return 0;
+		break;
+	default:
+		goto out;
+	}
 
 	new_nsproxy = create_new_namespaces(0, tsk, current_user_ns(), tsk->fs);
 	if (IS_ERR(new_nsproxy)) {
-- 
1.9.0

Re: [PATCH RFC] setns: return 0 directly if try to reassociate with current namespace

[Serge Hallyn]

Quoting Chen Hanxiao (chenhanxiao-BthXqXjhjHXQFUHtdCDX3A@public.gmane.org):
> We could use setns to join the current ns,
> which did a lot of unnecessary work.
> This patch will check this senario and
> return 0 directly.
> 
> Signed-off-by: Chen Hanxiao <chenhanxiao-BthXqXjhjHXQFUHtdCDX3A@public.gmane.org>

Plus it's just asking for trouble.

I would ack this, except you need to fclose(file) on the
return paths.  So just set err = 0 and goto out.

> ---
>  kernel/nsproxy.c | 28 ++++++++++++++++++++++++++++
>  1 file changed, 28 insertions(+)
> 
> diff --git a/kernel/nsproxy.c b/kernel/nsproxy.c
> index ef42d0a..66eea63 100644
> --- a/kernel/nsproxy.c
> +++ b/kernel/nsproxy.c
> @@ -236,6 +236,34 @@ SYSCALL_DEFINE2(setns, int, fd, int, nstype)
>  	ops = ei->ns_ops;
>  	if (nstype && (ops->type != nstype))
>  		goto out;
> +	switch (ops->type) {
> +	case CLONE_NEWIPC:
> +		if (ei->ns == tsk->nsproxy->ipc_ns)
> +			return 0;
> +		break;
> +	case CLONE_NEWNET:
> +		if (ei->ns == tsk->nsproxy->net_ns)
> +			return 0;
> +		break;
> +	case CLONE_NEWNS:
> +		if (ei->ns == tsk->nsproxy->mnt_ns)
> +			return 0;
> +		break;
> +	case CLONE_NEWPID:
> +		if (ei->ns == tsk->nsproxy->pid_ns_for_children)
> +			return 0;
> +		break;
> +	case CLONE_NEWUSER:
> +		if (ei->ns == current_user_ns())
> +			return 0;
> +		break;
> +	case CLONE_NEWUTS:
> +		if (ei->ns == tsk->nsproxy->uts_ns)
> +			return 0;
> +		break;
> +	default:
> +		goto out;
> +	}
>  
>  	new_nsproxy = create_new_namespaces(0, tsk, current_user_ns(), tsk->fs);
>  	if (IS_ERR(new_nsproxy)) {
> -- 
> 1.9.0
> 

Re: [PATCH RFC] setns: return 0 directly if try to reassociate with current namespace

[Eric W. Biederman]

Serge Hallyn <serge.hallyn-GeWIH/nMZzLQT0dZR+AlfA@public.gmane.org> writes:

> Quoting Chen Hanxiao (chenhanxiao-BthXqXjhjHXQFUHtdCDX3A@public.gmane.org):
>> We could use setns to join the current ns,
>> which did a lot of unnecessary work.
>> This patch will check this senario and
>> return 0 directly.
>> 
>> Signed-off-by: Chen Hanxiao <chenhanxiao-BthXqXjhjHXQFUHtdCDX3A@public.gmane.org>
>
> Plus it's just asking for trouble.
>
> I would ack this, except you need to fclose(file) on the
> return paths.  So just set err = 0 and goto out.

I completely disagree.

Nacked-by: "Eric W. Biederman" <ebiederm-aS9lmoZGLiVWk0Htik3J/w@public.gmane.org>

This patch adds a new code path to test, and gets that new code path
wrong.  So unless there is a performance advantage for some real world
case I don't see the point.  Is there real software that is rejoining
the a current namespace.

This patch changes the behavior of CLONE_NEWNS (which always does a
chdir and chroot) when you change into the current namespace.

This patch changes the behavior of CLONE_NEWUSER which current errors
out.

This code adds a big switch statement to code that is otherwise table
driven.  With the result that two pieces of code must be looked at
and modified whenever we want to tweak the behavior of setns for a
namespace.

So in general I think this piece of code is a maintenance disaster,
with no apparent redeem virtues.

Eric

Re: [PATCH RFC] setns: return 0 directly if try to reassociate with current namespace

[Serge Hallyn]

Quoting Eric W. Biederman (ebiederm-aS9lmoZGLiVWk0Htik3J/w@public.gmane.org):
> Serge Hallyn <serge.hallyn-GeWIH/nMZzLQT0dZR+AlfA@public.gmane.org> writes:
> 
> > Quoting Chen Hanxiao (chenhanxiao-BthXqXjhjHXQFUHtdCDX3A@public.gmane.org):
> >> We could use setns to join the current ns,
> >> which did a lot of unnecessary work.
> >> This patch will check this senario and
> >> return 0 directly.
> >> 
> >> Signed-off-by: Chen Hanxiao <chenhanxiao-BthXqXjhjHXQFUHtdCDX3A@public.gmane.org>
> >
> > Plus it's just asking for trouble.
> >
> > I would ack this, except you need to fclose(file) on the
> > return paths.  So just set err = 0 and goto out.
> 
> I completely disagree.
> 
> Nacked-by: "Eric W. Biederman" <ebiederm-aS9lmoZGLiVWk0Htik3J/w@public.gmane.org>
> 
> This patch adds a new code path to test, and gets that new code path
> wrong.  So unless there is a performance advantage for some real world
> case I don't see the point.  Is there real software that is rejoining
> the a current namespace.

IMO performance would be a poor reason to do this.  I would feel better
with it because the case of "I've unshared everything, now setns to
my own namespace" seems too easy to get to a point where you
put the last ref to your ns before you get the new ns.  Yes at least
the mntns_install seems to prevent this, and yes it would be a bug,
but I simply consider this good defensive coding.

> This patch changes the behavior of CLONE_NEWNS (which always does a
> chdir and chroot) when you change into the current namespace.
> 
> This patch changes the behavior of CLONE_NEWUSER which current errors
> out.

Yes so currently setns to your own ns behaves differently for different
namespace types.  That also seems like a reason to fix this.

> This code adds a big switch statement to code that is otherwise table
> driven.  With the result that two pieces of code must be looked at
> and modified whenever we want to tweak the behavior of setns for a
> namespace.
> 
> So in general I think this piece of code is a maintenance disaster,
> with no apparent redeem virtues.

I'm not going to push too hard on this, I simply disagree.

-serge

RE: [PATCH RFC] setns: return 0 directly if try to reassociate with current namespace

[Chen, Hanxiao]

> -----Original Message-----
> From: Eric W. Biederman [mailto:ebiederm-aS9lmoZGLiVWk0Htik3J/w@public.gmane.org]
> Sent: Thursday, October 09, 2014 1:56 AM
> 
> Serge Hallyn <serge.hallyn-GeWIH/nMZzLQT0dZR+AlfA@public.gmane.org> writes:
> 
> > Quoting Chen Hanxiao (chenhanxiao-BthXqXjhjHXQFUHtdCDX3A@public.gmane.org):
> >> We could use setns to join the current ns,
> >> which did a lot of unnecessary work.
> >> This patch will check this senario and
> >> return 0 directly.
> >>
> >> Signed-off-by: Chen Hanxiao <chenhanxiao-BthXqXjhjHXQFUHtdCDX3A@public.gmane.org>
> >
> > Plus it's just asking for trouble.
> >
> > I would ack this, except you need to fclose(file) on the
> > return paths.  So just set err = 0 and goto out.
> 
> I completely disagree.
> 
> Nacked-by: "Eric W. Biederman" <ebiederm-aS9lmoZGLiVWk0Htik3J/w@public.gmane.org>
> 
> This patch adds a new code path to test, and gets that new code path
> wrong.  So unless there is a performance advantage for some real world
> case I don't see the point.  Is there real software that is rejoining
> the a current namespace.
> 
> This patch changes the behavior of CLONE_NEWNS (which always does a
> chdir and chroot) when you change into the current namespace.
> 
> This patch changes the behavior of CLONE_NEWUSER which current errors
> out.
> 

As reentering the same namespace looks meaningless,
and handling reentering same ns we behaved differently,
 
How about just *reject* the behaviour of setns to current namespace?

+	switch (ops->type) {
+	case CLONE_NEWIPC:
+		if (ei->ns == tsk->nsproxy->ipc_ns) {
+            err = -EINVAL;
+			goto out;
+        }
...

And things became easy, 6 simply cases could cover the whole scenario
and will not bring troubles to users.

Thanks,
- Chen

> This code adds a big switch statement to code that is otherwise table
> driven.  With the result that two pieces of code must be looked at
> and modified whenever we want to tweak the behavior of setns for a
> namespace.
> 
> So in general I think this piece of code is a maintenance disaster,
> with no apparent redeem virtues.
> 
> Eric

Re: [PATCH RFC] setns: return 0 directly if try to reassociate with current namespace

[Eric W. Biederman]

"Chen, Hanxiao" <chenhanxiao-BthXqXjhjHXQFUHtdCDX3A@public.gmane.org> writes:

>> -----Original Message-----
>> From: Eric W. Biederman [mailto:ebiederm-aS9lmoZGLiVWk0Htik3J/w@public.gmane.org]
>> Sent: Thursday, October 09, 2014 1:56 AM
>> 
>> Serge Hallyn <serge.hallyn-GeWIH/nMZzLQT0dZR+AlfA@public.gmane.org> writes:
>> 
>> > Quoting Chen Hanxiao (chenhanxiao-BthXqXjhjHXQFUHtdCDX3A@public.gmane.org):
>> >> We could use setns to join the current ns,
>> >> which did a lot of unnecessary work.
>> >> This patch will check this senario and
>> >> return 0 directly.
>> >>
>> >> Signed-off-by: Chen Hanxiao <chenhanxiao-BthXqXjhjHXQFUHtdCDX3A@public.gmane.org>
>> >
>> > Plus it's just asking for trouble.
>> >
>> > I would ack this, except you need to fclose(file) on the
>> > return paths.  So just set err = 0 and goto out.
>> 
>> I completely disagree.
>> 
>> Nacked-by: "Eric W. Biederman" <ebiederm-aS9lmoZGLiVWk0Htik3J/w@public.gmane.org>
>> 
>> This patch adds a new code path to test, and gets that new code path
>> wrong.  So unless there is a performance advantage for some real world
>> case I don't see the point.  Is there real software that is rejoining
>> the a current namespace.
>> 
>> This patch changes the behavior of CLONE_NEWNS (which always does a
>> chdir and chroot) when you change into the current namespace.
>> 
>> This patch changes the behavior of CLONE_NEWUSER which current errors
>> out.
>> 
>
> As reentering the same namespace looks meaningless,
> and handling reentering same ns we behaved differently,

It is not meaningless in the case of CLONE_NEWNS.  It is weird but not
meaningless.  Further return -EINVAL won't make the weird semantics go
away it just makes them more expensive to take advantage of.

> How about just *reject* the behaviour of setns to current namespace?

Because we don't break userspace applications without a darn good reason.

> +	switch (ops->type) {
> +	case CLONE_NEWIPC:
> +		if (ei->ns == tsk->nsproxy->ipc_ns) {
> +            err = -EINVAL;
> +			goto out;
> +        }
> ...
>
> And things became easy, 6 simply cases could cover the whole scenario
> and will not bring troubles to users.

Since you are cavalierly suggesting changing the semantics presented to
user space I don't belive the assertion that it will not bring trouble
to users.

Maybe on a day when I am not up to my neck in weird breakage in the
mount namespace.  Because people now care because unprivileged users can
use it we are starting to see bugs as old as 2.4 and they are a pain to
deal with.

Eric