public inbox for linux-kernel@vger.kernel.org
 help / color / mirror / Atom feed
From: Christian Brauner <christian.brauner@canonical.com>
To: "Eric W. Biederman" <ebiederm@xmission.com>
Cc: Linus Torvalds <torvalds@linux-foundation.org>,
	Al Viro <viro@zeniv.linux.org.uk>,
	Serge Hallyn <serge@hallyn.com>,
	Stefan Lippers-Hollmann <s.l-h@gmx.de>,
	Christian Brauner <christian.brauner@ubuntu.com>,
	Thorsten Leemhuis <regressions@leemhuis.info>,
	Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH 0/1] devpts: use dynamic_dname() to generate proc name
Date: Fri, 25 Aug 2017 01:37:38 +0200	[thread overview]
Message-ID: <20170824233737.umna3s7vsbuv6oi6@gmail.com> (raw)
In-Reply-To: <877exs7epb.fsf@xmission.com>

On Thu, Aug 24, 2017 at 06:01:36PM -0500, Eric W. Biederman wrote:
> Linus Torvalds <torvalds@linux-foundation.org> writes:
> 
> > On Thu, Aug 24, 2017 at 1:43 PM, Eric W. Biederman
> > <ebiederm@xmission.com> wrote:
> >>
> >> There are just enough weird one off scripts like xen image builder (I
> >> think that was the nasty test case that broke in debian) that I can't
> >> imagine ever being able to responsibly remove the path based lookups in
> >> /dev/ptmx.  I do dream of it sometimes.
> >
> > Not going to happen.
> 
> Which is what I said.
> 
> > The fact is, /dev/ptmx is the simply the standard location.
> > /dev/pts/ptmx simply is *not*.
> 
> The standard is posix_openpt().  That is a syscall on the bsds.
> Opening something called ptmx at this point is a Linuxism.
> 
> There are a lot of programs that are going to be calling posix_openpt()
> simply because /dev/ptmx can not be counted on to exist.
> 
> > So pretty much every single user that ever uses pty's will use
> > /dev/ptmx, it's just how it has always worked.
> >
> > Trying to change it to anything else is just stupid. There's no
> > upside, there is only downsides - mainly the "we'll have to support
> > the standard way anyway, that newfangled way doesn't add anything".
> 
> Except the new fangled way does add quite a bit.  Not everyone who
> mounts devpts has permission to call mknod.  So /dev/ptmx frequently
> winds up either being a bind mount or a symlink to /dev/pts/ptmx in
> containers.

In fact, /dev/ptmx being a symlink or bind-mount is the *standard* in containers
even for non-user namespaced containers or containers that do not retain
CAP_MKNOD.

> 
> It is going to take a long time but device nodes like one of those
> filesystem features thare are very slowly on their way out.

This related to the point above: The fact that we can mount a devpts at its
standard location but are unable to also have/create an additional device node
at the *standard location* is usually quite irritating for people who do not
know about this "legacy" behaviour. But yeah, it's probably going away but
that's going to be a long long time. I agree that userspace is the place to
slowly make the transition though. :)

> 
> > Our "pts" lookup isn't expensive.
> >
> > So quite frankly, we should discourage people from using the
> > non-standard place. It really has no real advantages, and it's simply
> > not worth it.
> 
> The "pts" lookup admitted isn't runtime expensive.  I could propbably
> measure a cost but anyone who is creating ptys fast enough to care
> likely has other issues.
> 
> The "pts" lookup does have some real maintenance costs as it takes
> someone with a pretty deep understanding of things to figure out what is
> going on.  I hope things have finally been abstracted well enough, and
> the code is used heavily enough we don't have to worry about a
> regression there.   I still worry.
> 
> As for non-standard locations.  Anything that isn't /dev/ptmx and
> /dev/pts/NNN simply won't work for anything isn't very specialized.

I was mainly asking about non-standard locations because I experienced weird
behaviour when trying to open("/mnt/<slave-idx", O_RDWR | O_NOCTTY). Mind you I
did all the steps that grantpt() + unlockpt() usually do purely file descriptor
based. But I think this was due to the faulty TIOCGPTPEER implemenation before
which should now be fixed.

> At which point I don't think there is any reason to skip using the ptmx
> node on the devpts filesystem as you have already given up compatibility
> with everything else.
> 
> But I agree it doesn't look worth it to change glibc to deal with an
> alternate location for /dev/ptmx.  I see a huge point in changing glibc
> to use the new TIOCGPTPEER ioctl when available as that is really the
> functionality the glibc internals are after.

That's a patch I've been looking into. But TIOCGPTPEER alone won't be enough. A
couple of other function such as grantpt() need to switch from path-based
operation to file descriptor based operations too (Something I tried to point
out in one of my previous mails.). The whole user-space api could do - imho -
with a redo. The kernel is doing the right thing and exposing the right bits
mostly; TIOCGPTPEER being a good step. But user-space wise it's actually a
little security nightmare as soon as namespaces and - sorry for the buzzword -
*containers* come into play. @Eric, are you going to be at Plumbers again this
year? That's maybe a good chance to discuss some of this if there's still
interest.

Christian

> 
> Eric

  parent reply	other threads:[~2017-08-24 23:37 UTC|newest]

Thread overview: 55+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-08-16 17:12 [PATCH 0/1] devpts: use dynamic_dname() to generate proc name Christian Brauner
2017-08-16 17:12 ` [PATCH 1/1] " Christian Brauner
2017-08-16 18:26 ` [PATCH 0/1] " Linus Torvalds
2017-08-16 18:48   ` Linus Torvalds
2017-08-16 19:48     ` Christian Brauner
2017-08-16 19:56       ` Linus Torvalds
2017-08-16 20:19         ` Linus Torvalds
2017-08-16 20:30           ` Linus Torvalds
2017-08-16 21:03             ` Linus Torvalds
2017-08-16 21:37               ` Christian Brauner
2017-08-16 21:45                 ` Linus Torvalds
2017-08-16 21:55                   ` Linus Torvalds
2017-08-16 22:05                     ` Christian Brauner
2017-08-16 22:28                   ` Christian Brauner
2017-08-23 15:31                     ` Eric W. Biederman
2017-08-23 21:15                       ` Christian Brauner
2017-08-16 22:46               ` Eric W. Biederman
2017-08-16 22:58                 ` Linus Torvalds
2017-08-16 23:51                   ` Eric W. Biederman
2017-08-17  0:08                     ` Linus Torvalds
2017-08-17  1:24                       ` Eric W. Biederman
2017-08-24  0:24                       ` Stefan Lippers-Hollmann
2017-08-24  0:42                         ` Linus Torvalds
2017-08-24  1:16                           ` Linus Torvalds
2017-08-24  1:25                           ` Eric W. Biederman
2017-08-24  1:32                             ` Linus Torvalds
2017-08-24  1:49                               ` Linus Torvalds
2017-08-24  2:01                                 ` Linus Torvalds
2017-08-24  3:11                                   ` Eric W. Biederman
2017-08-24  3:24                                     ` Linus Torvalds
2017-08-24 15:51                                       ` Eric W. Biederman
2017-08-24  4:24                                     ` Stefan Lippers-Hollmann
2017-08-24 15:54                                       ` Eric W. Biederman
2017-08-24 17:52                                         ` Linus Torvalds
2017-08-24 18:06                                           ` Linus Torvalds
2017-08-24 18:13                                             ` Linus Torvalds
2017-08-24 18:31                                               ` Linus Torvalds
2017-08-24 18:36                                                 ` Linus Torvalds
2017-08-24 20:24                                                   ` Stefan Lippers-Hollmann
2017-08-24 20:27                                                     ` Linus Torvalds
2017-08-24 18:40                                               ` Eric W. Biederman
2017-08-24 18:51                                                 ` Linus Torvalds
2017-08-24 19:23                                                   ` Eric W. Biederman
2017-08-24 20:13                                                   ` [PATCH v3] pty: Repair TIOCGPTPEER Eric W. Biederman
2017-08-24 21:01                                                     ` Stefan Lippers-Hollmann
     [not found]                                                 ` <CAPP7u0WHqDfxTW6hmc=DsmHuoALZcrWdU-Odu=FfoTX26SGHQg@mail.gmail.com>
2017-08-24 19:22                                                   ` [PATCH 0/1] devpts: use dynamic_dname() to generate proc name Linus Torvalds
2017-08-24 19:25                                                     ` Linus Torvalds
2017-08-24 20:43                                                     ` Eric W. Biederman
2017-08-24 21:07                                                       ` Linus Torvalds
2017-08-24 23:01                                                         ` Eric W. Biederman
2017-08-24 23:27                                                           ` Linus Torvalds
2017-08-24 23:37                                                           ` Christian Brauner [this message]
2017-08-26  1:00                                                             ` Linus Torvalds
2017-08-24 19:48                                                   ` Eric W. Biederman
2017-08-17  1:37           ` Eric W. Biederman

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20170824233737.umna3s7vsbuv6oi6@gmail.com \
    --to=christian.brauner@canonical.com \
    --cc=christian.brauner@ubuntu.com \
    --cc=ebiederm@xmission.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=regressions@leemhuis.info \
    --cc=s.l-h@gmx.de \
    --cc=serge@hallyn.com \
    --cc=torvalds@linux-foundation.org \
    --cc=viro@zeniv.linux.org.uk \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox