Re: [PATCH] getpt: use /dev/pts/ptmx as default ptmx master

[Christian Brauner <christian dot brauner at canonical dot com>]

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
>