From: ebiederm@xmission.com (Eric W. Biederman)
To: Linus Torvalds <torvalds@linux-foundation.org>
Cc: "H. Peter Anvin" <hpa@zytor.com>,
        One Thousand Gnomes <gnomes@lxorguk.ukuu.org.uk>,
        Peter Hurley <peter@hurleysoftware.com>, Greg KH <greg@kroah.com>,
        Jiri Slaby <jslaby@suse.com>, Aurelien Jarno <aurelien@aurel32.net>,
        Andy Lutomirski <luto@amacapital.net>,
        Florian Weimer <fw@deneb.enyo.de>, Al Viro <viro@zeniv.linux.org.uk>,
        Serge Hallyn <serge.hallyn@ubuntu.com>, Jann Horn <jann@thejh.net>,
        "security\@kernel.org" <security@kernel.org>,
        "security\@ubuntu.com \>\> security" <security@ubuntu.com>,
        security@debian.org, Willy Tarreau <w@1wt.eu>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
References: <878u0s3orx.fsf_-_@x220.int.ebiederm.org>
	<1459819769-30387-1-git-send-email-ebiederm@xmission.com>
	<CA+55aFwv7d8XfeN8KvYaVcSs7V9XoHtS-1cqtkG4gF4yJwB5nA@mail.gmail.com>
	<87twjcorwg.fsf@x220.int.ebiederm.org>
	<20160409140909.42315e6d@lxorguk.ukuu.org.uk>
	<83FE8CD2-C0A2-4ADB-AEBD-8DD89AD4F88A@zytor.com>
	<87bn5ij0x1.fsf@x220.int.ebiederm.org>
	<78205895-E11D-417F-91DC-4BCA0B61A122@zytor.com>
	<CA+55aFyB7y8TfQ=_EvMAFONS6oSPs=uY88f+S6n_QWh4MkKTHg@mail.gmail.com>
Date: Mon, 11 Apr 2016 15:05:21 -0500
In-Reply-To: <CA+55aFyB7y8TfQ=_EvMAFONS6oSPs=uY88f+S6n_QWh4MkKTHg@mail.gmail.com>
	(Linus Torvalds's message of "Sat, 9 Apr 2016 17:01:27 -0700")
Message-ID: <87bn5fgbcu.fsf@x220.int.ebiederm.org>
User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/24.5 (gnu/linux)
MIME-Version: 1.0
Content-Type: text/plain
Subject: Re: [PATCH 01/13] devpts: Teach /dev/ptmx to find the associated devpts via path lookup
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 2573
Lines: 59

Linus Torvalds <torvalds@linux-foundation.org> writes:

> On Sat, Apr 9, 2016 at 3:37 PM, H. Peter Anvin <hpa@zytor.com> wrote:
>>
>> On the flipside, if we were to allow ourselves to break userspace, at this point I would suggest making /dev/pts/ptmx have a different device number and make the legacy /dev/ptmx print a warning message, after which it can at least eventually be deleted.
>
> You don't need a different device number.
>
> The /dev/pts/ptmx file may look like it's the same node as /dev/ptmx,
> but it is trivial to recognize as the pts one:
>
>      if (dentry->d_sb->s_magic == DEVPTS_SUPER_MAGIC)
>
> and you're done.

We can actually do better and set f_ops in devpts and bypass the whole
lookup by device number.  In my pile of cleanups that are waiting for
the mess to resolve I actually have a patch that does that for the slave
ttys.

>
> But nobody actually uses /dev/pts/ptmx, because it has never had sane
> permissions.

Now that is an interesting misconception to see.  There is actually a
lot more software that uses /dev/pts/ptmx (with a symlink from /dev/ptmx
or a bind mount to /dev/ptmx) than there is that actually needs the new
compatibility behavior.

Carefully written and maintained container software like lxc and docker
do use "-o newinstance".  Fixing the permissions and redirecting the
/dev/ptmx path to /dev/pts/ptmx are not a problem when you know you are
setting up a special environment.

It is the one off sloppily created automation scripts like
xen-create-image that gets this wrong.  I say sloppily created as in
practice every mount of devpts needs "-o gid=5,mode=620".  If
xen-create-image and related software had gotting those mount options
correct pt_chown could have been done away with, with no one noticing a
long time ago.

Those sloppy pieces of code are probably the things we want to
break least as they work after their fasion and whoever wrote them
is likely long gone.  So I would be surprised if there was anyone to
pick up the pieces if we break them.

> But when we fix bad semantics (and always just looking up the initial
> pts mount really is crazy semantics) that doesn't mean that we have to
> bend over backwards to not make the changed semantics visible. We
> don't _break_ user space, but we also don't care about some random
> test-program that checks for particular semantics.

No.  Bending over backwards as you call it just makes the software test
matrix smaller.

That part is now settled in my book and those extra permission checks
will not be in the next version of this patchset.

Eric