Message ID | 20180315120651.14107-1-christian.brauner@ubuntu.com |
---|---|
State | New, archived |
Headers |
Received: (qmail 21572 invoked by alias); 15 Mar 2018 12:07:05 -0000 Mailing-List: contact libc-alpha-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: <libc-alpha.sourceware.org> List-Unsubscribe: <mailto:libc-alpha-unsubscribe-##L=##H@sourceware.org> List-Subscribe: <mailto:libc-alpha-subscribe@sourceware.org> List-Archive: <http://sourceware.org/ml/libc-alpha/> List-Post: <mailto:libc-alpha@sourceware.org> List-Help: <mailto:libc-alpha-help@sourceware.org>, <http://sourceware.org/ml/#faqs> Sender: libc-alpha-owner@sourceware.org Delivered-To: mailing list libc-alpha@sourceware.org Received: (qmail 21560 invoked by uid 89); 15 Mar 2018 12:07:05 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-26.9 required=5.0 tests=BAYES_00, FREEMAIL_FROM, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, RCVD_IN_DNSWL_NONE, SPF_PASS autolearn=ham version=3.3.2 spammy=H*MI:ubuntu, H*m:ubuntu, terminals, resides X-HELO: mail-wr0-f193.google.com X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:from:to:cc:subject:date:message-id; bh=yrNsacWMM12sXDJRel7BHP8iyYctJRIpaQWqjECX3u8=; b=CyCQfbI0JK7OQvMADrSNyD5w62ANsqSU7m29Txp7hOdBM4746agcU93rEAcJ6OjWj2 VqP7DHUOHmKiF1bgI4Y+cjlVvBJRwp92ni14ovgn/qBzTZ1B/SpTiHM3pxCpG+BGBTEY ffoB+YnfrxMIclc7hpyB66VUwdS7nbyu7+AsO6dXLfd4bNRyaHTMTh5tcP2F40ICqw04 WObElVYa8KmwWaGDeWUJZalZWwB0J5cpOBX2Ye1N0Ge1NRcG9P4/sPfsWdeJ/eD4P0Nz r6FyeRN/98y2fO5kxAQFUlDTiL3DZsmGoSXA7cA3WrTtV7FEIaP+C0DljXwaOzQtfSTD J/Mg== X-Gm-Message-State: AElRT7FJOpbsk+rtkOS+uYbC5IDlDIudjrH/9RrOxWSV4lIuY5/jcJPo hAWuJojP7qOwK5nw+PeFsu8= X-Google-Smtp-Source: AG47ELulVyOgJ0OXYmsbrtN7swfI0k9C2cn3klNTYtfv2NHgbWYqE5Kv2haH3+j2TvEOvlhLRpfnCQ== X-Received: by 10.223.156.10 with SMTP id f10mr7395392wrc.174.1521115620825; Thu, 15 Mar 2018 05:07:00 -0700 (PDT) From: Christian Brauner <christian.brauner@ubuntu.com> To: ebiederm@xmission.com, libc-alpha@sourceware.org Cc: Christian Brauner <christian.brauner@ubuntu.com> Subject: [PATCH] getpt: use /dev/pts/ptmx as default ptmx master Date: Thu, 15 Mar 2018 13:06:51 +0100 Message-Id: <20180315120651.14107-1-christian.brauner@ubuntu.com> |
Commit Message
Christian Brauner
March 15, 2018, 12:06 p.m. UTC
For a long time now Linux has placed the ptmx character device directly
under the devpts mount at /dev/pts/ptmx. The /dev/ptmx path today is
usually either a symlink, an additional character device or a bind-mount.
The idea has always been to slowly fade-out /dev/ptmx and switch to using
/dev/pts/ptmx exclusively. The kernel currently maintains code to retain
backwards compatibility for anyone going through /dev/ptmx.
Specifically, if the ptmx device is opened through /dev/ptmx the kernel
will look for a "pts" directory in the same directory where the /dev/ptmx
device node resides. This implies that the devpts mount at /dev/pts and the
/dev/ptmx mount need to have a common ancestor directory. This assumption
is usually fulfilled when a symlink or separate device node is used.
However, this assumption will be broken when /dev/ptmx is a bind-mount of
/dev/pts/ptmx because they are located on different devices. For a detailed
analysis of this problem please refer to my upstream patch [1].
It is time to start switching to using /dev/pts/ptmx and use /dev/ptmx as a
fallback only. As far as I can tell, we have three cases to reason about:
1. /dev/ptmx is a symlink to /dev/pts/ptmx
In this case devpts must have either been mounted with ptmxmode=0666 or
chmod 0666 /dev/pts/ptmx must have been called.
So any open() on /dev/pts/ptmx will succeed.
2. /dev/ptmx is a bind-mount of /dev/pts/ptmx
Analogous to 1. devpts must have either been mounted with ptmxmode=0666
or chmod 0666 /dev/pts/ptmx must have been called.
So any open() on /dev/pts/ptmx will succeed.
3. /dev/ptmx is a separate ptmx device node
In this case devpts can either be mounted with ptmxmode=0666 or
ptmxmode=0000. In the latter case privileged opens of /dev/pts/ptmx will
succeed while unprivileged opens will fail. The unprivileged failure
case will be unproblematic since we always fallback to opening /dev/ptmx
which should have permission 0666. If it doesn't then we would fail the
exact same way we always did.
[1]: https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/tty.git/commit/?h=tty-next&id=a319b01d9095da6f6c54bd20c1f1300762506255
Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com>
---
ChangeLog | 6 ++++++
sysdeps/unix/sysv/linux/getpt.c | 18 +++++++++++++++---
2 files changed, 21 insertions(+), 3 deletions(-)
Comments
On Thu, Mar 15, 2018 at 01:06:51PM +0100, Christian Brauner wrote: > For a long time now Linux has placed the ptmx character device directly > under the devpts mount at /dev/pts/ptmx. The /dev/ptmx path today is > usually either a symlink, an additional character device or a bind-mount. > > The idea has always been to slowly fade-out /dev/ptmx and switch to using > /dev/pts/ptmx exclusively. The kernel currently maintains code to retain > backwards compatibility for anyone going through /dev/ptmx. > > Specifically, if the ptmx device is opened through /dev/ptmx the kernel > will look for a "pts" directory in the same directory where the /dev/ptmx > device node resides. This implies that the devpts mount at /dev/pts and the > /dev/ptmx mount need to have a common ancestor directory. This assumption > is usually fulfilled when a symlink or separate device node is used. > However, this assumption will be broken when /dev/ptmx is a bind-mount of > /dev/pts/ptmx because they are located on different devices. For a detailed > analysis of this problem please refer to my upstream patch [1]. > > It is time to start switching to using /dev/pts/ptmx and use /dev/ptmx as a > fallback only. As far as I can tell, we have three cases to reason about: > > 1. /dev/ptmx is a symlink to /dev/pts/ptmx > In this case devpts must have either been mounted with ptmxmode=0666 or > chmod 0666 /dev/pts/ptmx must have been called. > So any open() on /dev/pts/ptmx will succeed. > 2. /dev/ptmx is a bind-mount of /dev/pts/ptmx > Analogous to 1. devpts must have either been mounted with ptmxmode=0666 > or chmod 0666 /dev/pts/ptmx must have been called. > So any open() on /dev/pts/ptmx will succeed. > 3. /dev/ptmx is a separate ptmx device node > In this case devpts can either be mounted with ptmxmode=0666 or > ptmxmode=0000. In the latter case privileged opens of /dev/pts/ptmx will > succeed while unprivileged opens will fail. The unprivileged failure > case will be unproblematic since we always fallback to opening /dev/ptmx > which should have permission 0666. If it doesn't then we would fail the > exact same way we always did. > > [1]: https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/tty.git/commit/?h=tty-next&id=a319b01d9095da6f6c54bd20c1f1300762506255 > > Signed-off-by: Christian Brauner <christian.brauner@ubuntu.com> In addition to the usual consensus I'd like Eric to comment on this as well. Thanks! Christian > --- > ChangeLog | 6 ++++++ > sysdeps/unix/sysv/linux/getpt.c | 18 +++++++++++++++--- > 2 files changed, 21 insertions(+), 3 deletions(-) > > diff --git a/ChangeLog b/ChangeLog > index 38154c20ab..01926472cc 100644 > --- a/ChangeLog > +++ b/ChangeLog > @@ -1,3 +1,9 @@ > +2018-03-15 Christian Brauner <christian.brauner@ubuntu.com> > + > + * sysdeps/unix/sysv/linux/getpt.c (__posix_openpt): Try to open > + ptmx device node through /dev/pts/ptmx and use /dev/ptmx as a > + fallback. > + > 2018-03-15 Siddhesh Poyarekar <siddhesh@sourceware.org> > > * sysdeps/aarch64/strncmp.S (strncmp): Use lsr instead of > diff --git a/sysdeps/unix/sysv/linux/getpt.c b/sysdeps/unix/sysv/linux/getpt.c > index 77aa468d83..c12a984a36 100644 > --- a/sysdeps/unix/sysv/linux/getpt.c > +++ b/sysdeps/unix/sysv/linux/getpt.c > @@ -25,11 +25,15 @@ > > #include "linux_fsinfo.h" > > -/* Path to the master pseudo terminal cloning device. */ > -#define _PATH_DEVPTMX _PATH_DEV "ptmx" > /* Directory containing the UNIX98 pseudo terminals. */ > #define _PATH_DEVPTS _PATH_DEV "pts" > > +/* Path to the master pseudo terminal cloning device. */ > +#define _PATH_DEVPTMX _PATH_DEV "ptmx" > + > +/* Path to the master pseudo terminal cloning device under devpts mount. */ > +#define _PATH_DEVPTS_PTMX _PATH_DEVPTS "/ptmx" > + > /* Prototype for function that opens BSD-style master pseudo-terminals. */ > extern int __bsd_getpt (void) attribute_hidden; > > @@ -42,7 +46,15 @@ __posix_openpt (int oflag) > > if (!have_no_dev_ptmx) > { > - fd = __open (_PATH_DEVPTMX, oflag); > + /* Try to open ptmx master pseudo terminal cloning device under the > + * devpts mount. > + */ > + fd = __open (_PATH_DEVPTS_PTMX, oflag); > + if (fd == -1) > + /* Fallback to opening the legacy ptmx master pseudo terminal > + * cloning device. > + */ > + fd = __open (_PATH_DEVPTMX, oflag); > if (fd != -1) > { > struct statfs fsbuf; > -- > 2.15.1 >
On Thu, Mar 15, 2018 at 8:06 AM, Christian Brauner <christian.brauner@ubuntu.com> wrote: > For a long time now Linux has placed the ptmx character device directly > under the devpts mount at /dev/pts/ptmx. Exactly which kernel version started doing this? > It is time to start switching to using /dev/pts/ptmx and use /dev/ptmx as a > fallback only. Application code is entitled to do open ('/dev/ptmx", O_RDWR) itself rather than calling posix_openpt. It is not OK to break those applications. That was the recommended practice prior to the introduction of posix_openpt, and I am suspicious of posix_openpt not existing on still-reasonable portability targets. Since /dev/ptmx must stick around for the sake of those applications, I am inclined to say that libc's posix_openpt should continue using /dev/ptmx as well, in order to ensure that that configuration continues to be tested. I am also inclined to say that, on new kernels where the devpts filesystem provides the ptmx node, using a bind-mount rather than a symlink for /dev/ptmx is a misconfiguration (and on older kernels, obviously it needs to be an actual device node). zw
On Thu, Mar 15, 2018 at 10:02:56AM -0400, Zack Weinberg wrote: > On Thu, Mar 15, 2018 at 8:06 AM, Christian Brauner > <christian.brauner@ubuntu.com> wrote: > > For a long time now Linux has placed the ptmx character device directly > > under the devpts mount at /dev/pts/ptmx. > > Exactly which kernel version started doing this? The article about the patch is here. https://lwn.net/Articles/689539/ > > > It is time to start switching to using /dev/pts/ptmx and use /dev/ptmx as a > > fallback only. > > Application code is entitled to do open ('/dev/ptmx", O_RDWR) itself > rather than calling posix_openpt. It is not OK to break those > applications. That was the recommended practice prior to the > introduction of posix_openpt, and I am suspicious of posix_openpt not > existing on still-reasonable portability targets. > > Since /dev/ptmx must stick around for the sake of those applications, > I am inclined to say that libc's posix_openpt should continue using > /dev/ptmx as well, in order to ensure that that configuration > continues to be tested. I am also inclined to say that, on new > kernels where the devpts filesystem provides the ptmx node, using a > bind-mount rather than a symlink for /dev/ptmx is a misconfiguration > (and on older kernels, obviously it needs to be an actual device > node). Neither the kernel nor this patch breaks userspace applications. Maybe I'm being dense but what argument supports this assumption? This patch extends __posix_openpt() to try and open(/dev/pts/ptmx) first and if it fails for any reason retry with open(/dev/ptmx). Christian
On Thu, Mar 15, 2018 at 10:10 AM, Christian Brauner <christian.brauner@canonical.com> wrote: > On Thu, Mar 15, 2018 at 10:02:56AM -0400, Zack Weinberg wrote: >> On Thu, Mar 15, 2018 at 8:06 AM, Christian Brauner >> <christian.brauner@ubuntu.com> wrote: >> > For a long time now Linux has placed the ptmx character device directly >> > under the devpts mount at /dev/pts/ptmx. >> >> Exactly which kernel version started doing this? > > The article about the patch is here. > https://lwn.net/Articles/689539/ I'm afraid I don't know how to work out from that article which _release version_ of the kernel first provided ptmx inside devpts. However, that's dated 2016 and the oldest kernel that glibc still supports for runtime use is 3.2, which came out in 2012, so I conclude we do need the fallback code, at least. >> > It is time to start switching to using /dev/pts/ptmx and use /dev/ptmx as a >> > fallback only. >> >> Application code is entitled to do open ('/dev/ptmx", O_RDWR) itself >> rather than calling posix_openpt. It is not OK to break those >> applications. That was the recommended practice prior to the >> introduction of posix_openpt, and I am suspicious of posix_openpt not >> existing on still-reasonable portability targets. >> >> Since /dev/ptmx must stick around for the sake of those applications, >> I am inclined to say that libc's posix_openpt should continue using >> /dev/ptmx as well, in order to ensure that that configuration >> continues to be tested. I am also inclined to say that, on new >> kernels where the devpts filesystem provides the ptmx node, using a >> bind-mount rather than a symlink for /dev/ptmx is a misconfiguration >> (and on older kernels, obviously it needs to be an actual device >> node). > > Neither the kernel nor this patch breaks userspace applications. Maybe > I'm being dense but what argument supports this assumption? > This patch extends __posix_openpt() to try and open(/dev/pts/ptmx) first > and if it fails for any reason retry with open(/dev/ptmx). Sorry, I was unclear. The scenario I want to avoid is, five to ten years in the future, someone - perhaps a sloppy container constructor - thinks that /dev/ptmx can be dropped. This is not OK, even that far ahead, because it might break applications. In order to prevent that from happening, I think glibc should continue to use only /dev/ptmx, so that the breakage will be immediate and obviously the fault of the sloppy container constructor. I have no objection to your _kernel_ patch, I just think it's silly to use a bind-mount for /dev/ptmx when a symlink will work just as well (in fact, better). zw
On Thu, Mar 15, 2018 at 10:38:07AM -0400, Zack Weinberg wrote: > On Thu, Mar 15, 2018 at 10:10 AM, Christian Brauner > <christian.brauner@canonical.com> wrote: > > On Thu, Mar 15, 2018 at 10:02:56AM -0400, Zack Weinberg wrote: > >> On Thu, Mar 15, 2018 at 8:06 AM, Christian Brauner > >> <christian.brauner@ubuntu.com> wrote: > >> > For a long time now Linux has placed the ptmx character device directly > >> > under the devpts mount at /dev/pts/ptmx. > >> > >> Exactly which kernel version started doing this? > > > > The article about the patch is here. > > https://lwn.net/Articles/689539/ > > I'm afraid I don't know how to work out from that article which > _release version_ of the kernel first provided ptmx inside devpts. > > However, that's dated 2016 and the oldest kernel that glibc still > supports for runtime use is 3.2, which came out in 2012, so I conclude > we do need the fallback code, at least. > > >> > It is time to start switching to using /dev/pts/ptmx and use /dev/ptmx as a > >> > fallback only. > >> > >> Application code is entitled to do open ('/dev/ptmx", O_RDWR) itself > >> rather than calling posix_openpt. It is not OK to break those > >> applications. That was the recommended practice prior to the > >> introduction of posix_openpt, and I am suspicious of posix_openpt not > >> existing on still-reasonable portability targets. > >> > >> Since /dev/ptmx must stick around for the sake of those applications, > >> I am inclined to say that libc's posix_openpt should continue using > >> /dev/ptmx as well, in order to ensure that that configuration > >> continues to be tested. I am also inclined to say that, on new > >> kernels where the devpts filesystem provides the ptmx node, using a > >> bind-mount rather than a symlink for /dev/ptmx is a misconfiguration > >> (and on older kernels, obviously it needs to be an actual device > >> node). > > > > Neither the kernel nor this patch breaks userspace applications. Maybe > > I'm being dense but what argument supports this assumption? > > This patch extends __posix_openpt() to try and open(/dev/pts/ptmx) first > > and if it fails for any reason retry with open(/dev/ptmx). > > Sorry, I was unclear. > > The scenario I want to avoid is, five to ten years in the future, > someone - perhaps a sloppy container constructor - thinks that > /dev/ptmx can be dropped. This is not OK, even that far ahead, > because it might break applications. In order to prevent that from > happening, I think glibc should continue to use only /dev/ptmx, so > that the breakage will be immediate and obviously the fault of the > sloppy container constructor. By that logic adding superseeding features should never be supported because of the possibility that someone might *accidently* drop support for the legacy version. That doesn't make sense. Dropping /dev/ptmx will not happen and is not intended. If that's the core of the concern we can put a comment in there statig explictly "Never ever remove the fallback as we support old kernels that do not have /dev/pts/ptmx." Furthermore, such fallbacks exist in multiple locations in glibc already. One recent example is that glibc already supports the TIOCGPTPEER ioctl() call to allocate the slave side file descriptor of a pty solely on the master fd and uses unsafe path-based retrieval only as a fallback option. Nothing in that patch implies that path-based retrieveal will ever be removed. Neither does anything in this patch imply that /dev/ptmx will. Christian > > I have no objection to your _kernel_ patch, I just think it's silly to > use a bind-mount for /dev/ptmx when a symlink will work just as well > (in fact, better). > > zw
On Thu, Mar 15, 2018 at 02:41:53PM +0000, Phil Blundell wrote: > On Thu, 2018-03-15 at 13:06 +0100, Christian Brauner wrote: > > The idea has always been to slowly fade-out /dev/ptmx and switch to using > > /dev/pts/ptmx exclusively. > > What's the reason for doing that? We've had /dev/ptmx for 20 years and > it is widely documented as the interface for opening ptys (even if it > was never formally standardised as such). Clearly it needs to stay > around for compatibility purposes for the foreseeable future and it's > not really obvious to me what advantage would be gained by phasing it > out even over the longer term. Can you elaborate? > > Thanks Sure, it is **not** intended to be faded out and it never will. It will be the fallback and it will stay there just as we have fallbacks in other places. Sorry, if this gives the impression that something will be faded out. This is **not** the case or intention. Opening the ptmx device node through /dev/pts/ptmx let's the kernel easily verify that the devpts filesystem is available especially in the face of bind-mounts where it has to resolve the bind-mounts. And like it or not bind-mounts of /dev/ptmx are supported and not a bug. This has worked forever. Christian
Christian Brauner <christian.brauner@ubuntu.com> writes: > For a long time now Linux has placed the ptmx character device directly > under the devpts mount at /dev/pts/ptmx. The /dev/ptmx path today is > usually either a symlink, an additional character device or a bind-mount. > > The idea has always been to slowly fade-out /dev/ptmx and switch to using > /dev/pts/ptmx exclusively. The kernel currently maintains code to retain > backwards compatibility for anyone going through /dev/ptmx. It depends on who you talk to Linus is against the slow fade-out. > Specifically, if the ptmx device is opened through /dev/ptmx the kernel > will look for a "pts" directory in the same directory where the /dev/ptmx > device node resides. This implies that the devpts mount at /dev/pts and the > /dev/ptmx mount need to have a common ancestor directory. This assumption > is usually fulfilled when a symlink or separate device node is used. > However, this assumption will be broken when /dev/ptmx is a bind-mount of > /dev/pts/ptmx because they are located on different devices. For a detailed > analysis of this problem please refer to my upstream patch [1]. We just finished merging the patches that causes this to be a problem for TIOCGTPEER. > It is time to start switching to using /dev/pts/ptmx and use /dev/ptmx as a > fallback only. As far as I can tell, we have three cases to reason > about: If we had always had /dev/pts/ptmx there definitely would have been advantages. What advantages does it have now to switch to opening /dev/pts/ptmx? Especially given that the default permissions are 0000, so opening /dev/pts/ptmx will fail on most ordinary configurations today. Only in containers is /dev/pts/ptmx the prefered device to open. There is a strong argument for using TIOCGTPEER (this fixes possible races). There is a strong argument for having each mount of devpts be a distinct filesystem (this fixes mount options from a chroot b0rking the main system). I don't see a similarly strong argument for asking glibc to always open /dev/pts/ptmx if available. It would simplify things, but we have already dealt with the issues. So I don't see any real world issues that it fixes. Eric
On Thu, Mar 15, 2018 at 03:02:14PM -0500, Eric W. Biederman wrote: > Christian Brauner <christian.brauner@ubuntu.com> writes: > > > For a long time now Linux has placed the ptmx character device directly > > under the devpts mount at /dev/pts/ptmx. The /dev/ptmx path today is > > usually either a symlink, an additional character device or a bind-mount. > > > > The idea has always been to slowly fade-out /dev/ptmx and switch to using > > /dev/pts/ptmx exclusively. The kernel currently maintains code to retain > > backwards compatibility for anyone going through /dev/ptmx. > > It depends on who you talk to Linus is against the slow fade-out. > > > Specifically, if the ptmx device is opened through /dev/ptmx the kernel > > will look for a "pts" directory in the same directory where the /dev/ptmx > > device node resides. This implies that the devpts mount at /dev/pts and the > > /dev/ptmx mount need to have a common ancestor directory. This assumption > > is usually fulfilled when a symlink or separate device node is used. > > However, this assumption will be broken when /dev/ptmx is a bind-mount of > > /dev/pts/ptmx because they are located on different devices. For a detailed > > analysis of this problem please refer to my upstream patch [1]. > > We just finished merging the patches that causes this to be a problem > for TIOCGTPEER. > > > It is time to start switching to using /dev/pts/ptmx and use /dev/ptmx as a > > fallback only. As far as I can tell, we have three cases to reason > > about: > > If we had always had /dev/pts/ptmx there definitely would have been > advantages. What advantages does it have now to switch to opening > /dev/pts/ptmx? Especially given that the default permissions are 0000, > so opening /dev/pts/ptmx will fail on most ordinary configurations > today. Only in containers is /dev/pts/ptmx the prefered device to open. > > There is a strong argument for using TIOCGTPEER (this fixes possible > races). There is a strong argument for having each mount of devpts > be a distinct filesystem (this fixes mount options from a chroot b0rking > the main system). > > I don't see a similarly strong argument for asking glibc to always open > /dev/pts/ptmx if available. It would simplify things, but we have > already dealt with the issues. So I don't see any real world issues > that it fixes. Thanks for stopping by Eric, if you think it's not worth it I'm not going to pursue this further. My main reason for pushing the /dev/pts/ptmx first, /dev/ptmx second was that all bugs we ever had that caused /proc/<pid>/fd/<nr> symlinks to be messed were caused by going through /dev/ptmx causing the kernel to go through "needless" lookup logic in case the devpts filesystem couldn't immediately be found where the ptmx device was. Christian
Christian Brauner <christian.brauner@canonical.com> writes: > On Thu, Mar 15, 2018 at 03:02:14PM -0500, Eric W. Biederman wrote: >> Christian Brauner <christian.brauner@ubuntu.com> writes: >> >> > For a long time now Linux has placed the ptmx character device directly >> > under the devpts mount at /dev/pts/ptmx. The /dev/ptmx path today is >> > usually either a symlink, an additional character device or a bind-mount. >> > >> > The idea has always been to slowly fade-out /dev/ptmx and switch to using >> > /dev/pts/ptmx exclusively. The kernel currently maintains code to retain >> > backwards compatibility for anyone going through /dev/ptmx. >> >> It depends on who you talk to Linus is against the slow fade-out. >> >> > Specifically, if the ptmx device is opened through /dev/ptmx the kernel >> > will look for a "pts" directory in the same directory where the /dev/ptmx >> > device node resides. This implies that the devpts mount at /dev/pts and the >> > /dev/ptmx mount need to have a common ancestor directory. This assumption >> > is usually fulfilled when a symlink or separate device node is used. >> > However, this assumption will be broken when /dev/ptmx is a bind-mount of >> > /dev/pts/ptmx because they are located on different devices. For a detailed >> > analysis of this problem please refer to my upstream patch [1]. >> >> We just finished merging the patches that causes this to be a problem >> for TIOCGTPEER. >> >> > It is time to start switching to using /dev/pts/ptmx and use /dev/ptmx as a >> > fallback only. As far as I can tell, we have three cases to reason >> > about: >> >> If we had always had /dev/pts/ptmx there definitely would have been >> advantages. What advantages does it have now to switch to opening >> /dev/pts/ptmx? Especially given that the default permissions are 0000, >> so opening /dev/pts/ptmx will fail on most ordinary configurations >> today. Only in containers is /dev/pts/ptmx the prefered device to open. >> >> There is a strong argument for using TIOCGTPEER (this fixes possible >> races). There is a strong argument for having each mount of devpts >> be a distinct filesystem (this fixes mount options from a chroot b0rking >> the main system). >> >> I don't see a similarly strong argument for asking glibc to always open >> /dev/pts/ptmx if available. It would simplify things, but we have >> already dealt with the issues. So I don't see any real world issues >> that it fixes. > > Thanks for stopping by Eric, if you think it's not worth it I'm not > going to pursue this further. > > My main reason for pushing the /dev/pts/ptmx first, /dev/ptmx second was > that all bugs we ever had that caused /proc/<pid>/fd/<nr> symlinks to be > messed were caused by going through /dev/ptmx causing the kernel to go > through "needless" lookup logic in case the devpts filesystem couldn't > immediately be found where the ptmx device was. If we sell this on we are much less likely to encounter kernel bugs, because the code paths are fundamentally simpler. Then I will agree. But let's keep it clear that is why this patch is being prompted. The counter case is that by glibc and everything else always using /dev/ptmx we get a large test base for that code path. It would make my life easier if everyone always used /dev/pts/ptmx or posix_granpt. Because in truth what it took to fix /dev/ptmx and TIOCGPTPERR in the kernel is not something many people the expertise to double check. Using /dev/pts/ptmx when it has the proper permissions is probably even a slightly faster. But I don't think it is anyone's bottleneck performance wise. Pro: Fewer bugs. Cons: Slightly less backwards compatible. Personally *Shrug*. If the fewer bugs and less chance of a security issue with the interface because of that holds sway. Please merge this. Otherwise don't worry about it. Eric
diff --git a/ChangeLog b/ChangeLog index 38154c20ab..01926472cc 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,3 +1,9 @@ +2018-03-15 Christian Brauner <christian.brauner@ubuntu.com> + + * sysdeps/unix/sysv/linux/getpt.c (__posix_openpt): Try to open + ptmx device node through /dev/pts/ptmx and use /dev/ptmx as a + fallback. + 2018-03-15 Siddhesh Poyarekar <siddhesh@sourceware.org> * sysdeps/aarch64/strncmp.S (strncmp): Use lsr instead of diff --git a/sysdeps/unix/sysv/linux/getpt.c b/sysdeps/unix/sysv/linux/getpt.c index 77aa468d83..c12a984a36 100644 --- a/sysdeps/unix/sysv/linux/getpt.c +++ b/sysdeps/unix/sysv/linux/getpt.c @@ -25,11 +25,15 @@ #include "linux_fsinfo.h" -/* Path to the master pseudo terminal cloning device. */ -#define _PATH_DEVPTMX _PATH_DEV "ptmx" /* Directory containing the UNIX98 pseudo terminals. */ #define _PATH_DEVPTS _PATH_DEV "pts" +/* Path to the master pseudo terminal cloning device. */ +#define _PATH_DEVPTMX _PATH_DEV "ptmx" + +/* Path to the master pseudo terminal cloning device under devpts mount. */ +#define _PATH_DEVPTS_PTMX _PATH_DEVPTS "/ptmx" + /* Prototype for function that opens BSD-style master pseudo-terminals. */ extern int __bsd_getpt (void) attribute_hidden; @@ -42,7 +46,15 @@ __posix_openpt (int oflag) if (!have_no_dev_ptmx) { - fd = __open (_PATH_DEVPTMX, oflag); + /* Try to open ptmx master pseudo terminal cloning device under the + * devpts mount. + */ + fd = __open (_PATH_DEVPTS_PTMX, oflag); + if (fd == -1) + /* Fallback to opening the legacy ptmx master pseudo terminal + * cloning device. + */ + fd = __open (_PATH_DEVPTMX, oflag); if (fd != -1) { struct statfs fsbuf;