Re: [PATCH RFC] setns: return 0 directly if try to reassociate with current namespace
"Chen, Hanxiao" writes:
>> -Original Message-
>> From: Eric W. Biederman [mailto:ebied...@xmission.com]
>> Sent: Thursday, October 09, 2014 1:56 AM
>>
>> Serge Hallyn writes:
>>
>> > Quoting Chen Hanxiao (chenhanx...@cn.fujitsu.com):
>> >> 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
>> >
>> > 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"
>>
>> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH RFC] setns: return 0 directly if try to reassociate with current namespace
> -Original Message-
> From: Eric W. Biederman [mailto:ebied...@xmission.com]
> Sent: Thursday, October 09, 2014 1:56 AM
>
> Serge Hallyn writes:
>
> > Quoting Chen Hanxiao (chenhanx...@cn.fujitsu.com):
> >> 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
> >
> > 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"
>
> 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
Quoting Eric W. Biederman (ebied...@xmission.com): > Serge Hallyn writes: > > > Quoting Chen Hanxiao (chenhanx...@cn.fujitsu.com): > >> 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 > > > > 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" > > 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH RFC] setns: return 0 directly if try to reassociate with current namespace
Serge Hallyn writes: > Quoting Chen Hanxiao (chenhanx...@cn.fujitsu.com): >> 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 > > 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" 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH RFC] setns: return 0 directly if try to reassociate with current namespace
Quoting Chen Hanxiao (chenhanx...@cn.fujitsu.com):
> 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
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
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH RFC] setns: return 0 directly if try to reassociate with current namespace
Quoting Chen Hanxiao (chenhanx...@cn.fujitsu.com):
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 chenhanx...@cn.fujitsu.com
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
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH RFC] setns: return 0 directly if try to reassociate with current namespace
Serge Hallyn serge.hal...@ubuntu.com writes: Quoting Chen Hanxiao (chenhanx...@cn.fujitsu.com): 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 chenhanx...@cn.fujitsu.com 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 ebied...@xmission.com 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 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH RFC] setns: return 0 directly if try to reassociate with current namespace
Quoting Eric W. Biederman (ebied...@xmission.com): Serge Hallyn serge.hal...@ubuntu.com writes: Quoting Chen Hanxiao (chenhanx...@cn.fujitsu.com): 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 chenhanx...@cn.fujitsu.com 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 ebied...@xmission.com 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 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH RFC] setns: return 0 directly if try to reassociate with current namespace
-Original Message-
From: Eric W. Biederman [mailto:ebied...@xmission.com]
Sent: Thursday, October 09, 2014 1:56 AM
Serge Hallyn serge.hal...@ubuntu.com writes:
Quoting Chen Hanxiao (chenhanx...@cn.fujitsu.com):
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 chenhanx...@cn.fujitsu.com
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 ebied...@xmission.com
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
Chen, Hanxiao chenhanx...@cn.fujitsu.com writes:
-Original Message-
From: Eric W. Biederman [mailto:ebied...@xmission.com]
Sent: Thursday, October 09, 2014 1:56 AM
Serge Hallyn serge.hal...@ubuntu.com writes:
Quoting Chen Hanxiao (chenhanx...@cn.fujitsu.com):
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 chenhanx...@cn.fujitsu.com
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 ebied...@xmission.com
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
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
