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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <5871495633F38949900D2BF2DC04883E5E0FAC@G08CNEXMBPEKD02.g08.fujitsu.local>
Date:	Thu, 9 Oct 2014 04:14:23 +0000
From:	"Chen, Hanxiao" <chenhanxiao@...fujitsu.com>
To:	"Eric W. Biederman" <ebiederm@...ssion.com>,
	Serge Hallyn <serge.hallyn@...ntu.com>
CC:	"containers@...ts.linux-foundation.org" 
	<containers@...ts.linux-foundation.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: RE: [PATCH RFC] setns: return 0 directly if try to reassociate with
 current namespace



> -----Original Message-----
> From: Eric W. Biederman [mailto:ebiederm@...ssion.com]
> Sent: Thursday, October 09, 2014 1:56 AM
> 
> Serge Hallyn <serge.hallyn@...ntu.com> writes:
> 
> > Quoting Chen Hanxiao (chenhanxiao@...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@...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@...ssion.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