From: ebiederm@xmission.com (Eric W. Biederman)
To: "Chen\, Hanxiao" <chenhanxiao@cn.fujitsu.com>
Cc: Serge Hallyn <serge.hallyn@ubuntu.com>,
        "containers\@lists.linux-foundation.org" 
	<containers@lists.linux-foundation.org>,
        "linux-kernel\@vger.kernel.org" <linux-kernel@vger.kernel.org>
References: <1412759587-21320-1-git-send-email-chenhanxiao@cn.fujitsu.com>
	<20141008145740.GD30569@ubuntumail>
	<87r3yih2e2.fsf@x220.int.ebiederm.org>
	<5871495633F38949900D2BF2DC04883E5E0FAC@G08CNEXMBPEKD02.g08.fujitsu.local>
Date: Wed, 08 Oct 2014 22:52:26 -0700
In-Reply-To: <5871495633F38949900D2BF2DC04883E5E0FAC@G08CNEXMBPEKD02.g08.fujitsu.local>
	(Hanxiao Chen's message of "Thu, 9 Oct 2014 04:14:23 +0000")
Message-ID: <871tqheqn9.fsf@x220.int.ebiederm.org>
User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/24.3 (gnu/linux)
MIME-Version: 1.0
Content-Type: text/plain
Subject: Re: [PATCH RFC] setns: return 0 directly if try to reassociate with current namespace
Sender: linux-kernel-owner@vger.kernel.org

"Chen, Hanxiao" <chenhanxiao@cn.fujitsu.com> writes:

>> -----Original Message-----
>> From: Eric W. Biederman [mailto:ebiederm@xmission.com]
>> Sent: Thursday, October 09, 2014 1:56 AM
>> 
>> Serge Hallyn <serge.hallyn@ubuntu.com> writes:
>> 
>> > Quoting Chen Hanxiao (chenhanxiao@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 <chenhanxiao@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" <ebiederm@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 majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/