[PATCH 0/4 v5] devpts: handle bind-mounts correctly

[PATCH 0/4 v5] devpts: handle bind-mounts correctly

[Christian Brauner]

Hey everyone,

This is the fith iteration of this patch. Per-patch changes are
summarized in the individual patches:

ChangeLog v4->v5:
* added non-functional patch to document devpts_mntget().
  Reason for putting this in a separate patch is that it allows you,
  Linus and Eric, to simply drop it if judged useless.
* reverse error handling logic to further simplify
* dput() dentry in the non-function patch. This was not really a problem
  since the following patch included a fix for it. But better to get it
  right in all individual patches.
* I did another rewrite of the problem analysis for
  posterity in the patch "Subject: [PATCH 2/3 v3] devpts: resolve devpts
  bind-mounts" and in this cover letter.

ChangeLog v3->v4:
* small logical simplifications
* add test that bind-mounts of /dev/pts/ptmx to locations that do not
  resolve to a valid slave pty path under the originating devpts mount
  fail

ChangeLog v2->v3:
* rewritten commit message to thoroughly analyse the problem for
  posterity in the patch "Subject: [PATCH 2/3 v3] devpts: resolve devpts
  bind-mounts" and in this cover letter.
* extended selftests to test for correct handling of /dev/pts/ptmx
  bind-mounts to /dev/ptmx and non-standard devpts mounts such as
  mount -t devpts devpts /mnt

ChangeLog v1->v2:
* see individual patches
ChangeLog v0->v1:
* see individual patches

Christian Brauner (4):
  devpts: hoist out check for DEVPTS_SUPER_MAGIC
  devpts: resolve devpts bind-mounts
  devpts: comment devpts_mntget()
  selftests: add devpts selftests

 fs/devpts/inode.c                                |  66 +++--
 tools/testing/selftests/Makefile                 |   1 +
 tools/testing/selftests/filesystems/.gitignore   |   1 +
 tools/testing/selftests/filesystems/Makefile     |   2 +-
 tools/testing/selftests/filesystems/devpts_pts.c | 313 +++++++++++++++++++++++
 5 files changed, 363 insertions(+), 20 deletions(-)
 create mode 100644 tools/testing/selftests/filesystems/devpts_pts.c

-- 
2.15.1

[PATCH 1/4 v5] devpts: hoist out check for DEVPTS_SUPER_MAGIC

[Christian Brauner]

Hoist the check whether we have already found a suitable devpts filesystem
out of devpts_ptmx_path() in preparation for the devpts bind-mount
resolution patch. This is a non-functional change.

Signed-off-by: Christian Brauner <christian.brauner-GeWIH/nMZzLQT0dZR+AlfA@public.gmane.org>
---
ChangeLog v4->v5:
* dput() dentry
ChangeLog v3->v4:
* patch unchanged
ChangeLog v2->v3:
* patch unchanged
ChangeLog v1->v2:
* patch added
ChangeLog v0->v1:
* patch not present
---
 fs/devpts/inode.c | 26 +++++++++++++++-----------
 1 file changed, 15 insertions(+), 11 deletions(-)

diff --git a/fs/devpts/inode.c b/fs/devpts/inode.c
index e31d6ed3ec32..71b901936113 100644
--- a/fs/devpts/inode.c
+++ b/fs/devpts/inode.c
@@ -138,10 +138,6 @@ static int devpts_ptmx_path(struct path *path)
 	struct super_block *sb;
 	int err;
 
-	/* Has the devpts filesystem already been found? */
-	if (path->mnt->mnt_sb->s_magic == DEVPTS_SUPER_MAGIC)
-		return 0;
-
 	/* Is a devpts filesystem at "pts" in the same directory? */
 	err = path_pts(path);
 	if (err)
@@ -159,21 +155,25 @@ static int devpts_ptmx_path(struct path *path)
 struct vfsmount *devpts_mntget(struct file *filp, struct pts_fs_info *fsi)
 {
 	struct path path;
-	int err;
+	int err = 0;
 
 	path = filp->f_path;
 	path_get(&path);
 
-	err = devpts_ptmx_path(&path);
+	/* Has the devpts filesystem already been found? */
+	if (path.mnt->mnt_sb->s_magic != DEVPTS_SUPER_MAGIC)
+		err = devpts_ptmx_path(&path);
 	dput(path.dentry);
 	if (err) {
 		mntput(path.mnt);
 		return ERR_PTR(err);
 	}
+
 	if (DEVPTS_SB(path.mnt->mnt_sb) != fsi) {
 		mntput(path.mnt);
 		return ERR_PTR(-ENODEV);
 	}
+
 	return path.mnt;
 }
 
@@ -182,15 +182,19 @@ struct pts_fs_info *devpts_acquire(struct file *filp)
 	struct pts_fs_info *result;
 	struct path path;
 	struct super_block *sb;
-	int err;
 
 	path = filp->f_path;
 	path_get(&path);
 
-	err = devpts_ptmx_path(&path);
-	if (err) {
-		result = ERR_PTR(err);
-		goto out;
+	/* Has the devpts filesystem already been found? */
+	if (path.mnt->mnt_sb->s_magic != DEVPTS_SUPER_MAGIC) {
+		int err;
+
+		err = devpts_ptmx_path(&path);
+		if (err) {
+			result = ERR_PTR(err);
+			goto out;
+		}
 	}
 
 	/*
-- 
2.15.1

Re: [PATCH 1/4 v5] devpts: hoist out check for DEVPTS_SUPER_MAGIC

[Eric W. Biederman]

Christian Brauner <christian.brauner-GeWIH/nMZzLQT0dZR+AlfA@public.gmane.org> writes:

> Hoist the check whether we have already found a suitable devpts filesystem
> out of devpts_ptmx_path() in preparation for the devpts bind-mount
> resolution patch. This is a non-functional change.

Reviewed-by: "Eric W. Biederman" <ebiederm-aS9lmoZGLiVWk0Htik3J/w@public.gmane.org>

>
> Signed-off-by: Christian Brauner <christian.brauner-GeWIH/nMZzLQT0dZR+AlfA@public.gmane.org>
> ---
> ChangeLog v4->v5:
> * dput() dentry
> ChangeLog v3->v4:
> * patch unchanged
> ChangeLog v2->v3:
> * patch unchanged
> ChangeLog v1->v2:
> * patch added
> ChangeLog v0->v1:
> * patch not present
> ---
>  fs/devpts/inode.c | 26 +++++++++++++++-----------
>  1 file changed, 15 insertions(+), 11 deletions(-)
>
> diff --git a/fs/devpts/inode.c b/fs/devpts/inode.c
> index e31d6ed3ec32..71b901936113 100644
> --- a/fs/devpts/inode.c
> +++ b/fs/devpts/inode.c
> @@ -138,10 +138,6 @@ static int devpts_ptmx_path(struct path *path)
>  	struct super_block *sb;
>  	int err;
>  
> -	/* Has the devpts filesystem already been found? */
> -	if (path->mnt->mnt_sb->s_magic == DEVPTS_SUPER_MAGIC)
> -		return 0;
> -
>  	/* Is a devpts filesystem at "pts" in the same directory? */
>  	err = path_pts(path);
>  	if (err)
> @@ -159,21 +155,25 @@ static int devpts_ptmx_path(struct path *path)
>  struct vfsmount *devpts_mntget(struct file *filp, struct pts_fs_info *fsi)
>  {
>  	struct path path;
> -	int err;
> +	int err = 0;
>  
>  	path = filp->f_path;
>  	path_get(&path);
>  
> -	err = devpts_ptmx_path(&path);
> +	/* Has the devpts filesystem already been found? */
> +	if (path.mnt->mnt_sb->s_magic != DEVPTS_SUPER_MAGIC)
> +		err = devpts_ptmx_path(&path);
>  	dput(path.dentry);
>  	if (err) {
>  		mntput(path.mnt);
>  		return ERR_PTR(err);
>  	}
> +
>  	if (DEVPTS_SB(path.mnt->mnt_sb) != fsi) {
>  		mntput(path.mnt);
>  		return ERR_PTR(-ENODEV);
>  	}
> +
>  	return path.mnt;
>  }
>  
> @@ -182,15 +182,19 @@ struct pts_fs_info *devpts_acquire(struct file *filp)
>  	struct pts_fs_info *result;
>  	struct path path;
>  	struct super_block *sb;
> -	int err;
>  
>  	path = filp->f_path;
>  	path_get(&path);
>  
> -	err = devpts_ptmx_path(&path);
> -	if (err) {
> -		result = ERR_PTR(err);
> -		goto out;
> +	/* Has the devpts filesystem already been found? */
> +	if (path.mnt->mnt_sb->s_magic != DEVPTS_SUPER_MAGIC) {
> +		int err;
> +
> +		err = devpts_ptmx_path(&path);
> +		if (err) {
> +			result = ERR_PTR(err);
> +			goto out;
> +		}
>  	}
>  
>  	/*

[PATCH 2/4 v5] devpts: resolve devpts bind-mounts

[Christian Brauner]

Most libcs will still look at /dev/ptmx when opening the master fd of a pty
device. When /dev/ptmx is a bind-mount of /dev/pts/ptmx and the TIOCGPTPEER
ioctl() is used to safely retrieve a file descriptor for the slave side of
the pty based on the master fd, the /proc/self/fd/{0,1,2} symlinks will
point to /. A very simply reproducer for this issue presupposing a libc
that uses TIOCGPTPEER in its openpty() implementation is:

unshare --mount
mount --bind /dev/pts/ptmx /dev/ptmx
chmod 666 /dev/ptmx
script
ls -al /proc/self/fd/0

Having bind-mounts of /dev/pts/ptmx to /dev/ptmx not working correctly is a
regression. In addition, it is also a fairly common scenario in containers
employing user namespaces.

The reason for the current failure is that the kernel tries to verify the
useability of the devpts filesystem without resolving the /dev/ptmx
bind-mount first. This will lead it to detect that the dentry is escaping
its bind-mount. The reason is that while the devpts filesystem mounted at
/dev/pts has the devtmpfs mounted at /dev as its parent mount:

21 -- -- / /dev
-- 21 -- / /dev/pts

devtmpfs and devpts are on different devices

-- -- 0:6  / /dev
-- -- 0:20 / /dev/pts

This has the consequence that the pathname of the parent directory of the
devpts filesystem mount at /dev/pts is /. So if /dev/ptmx is a bind-mount
of /dev/pts/ptmx then the /dev/ptmx bind-mount and the devpts mount at
/dev/pts will end up being located on the same device which is recorded in
the superblock of their vfsmount. This means the parent directory of the
/dev/ptmx bind-mount will be /ptmx:

-- -- ---- /ptmx /dev/ptmx

Without the bind-mount resolution patch the kernel will now perform the
bind-mount escape check directly on /dev/ptmx. The function responsible for
this is devpts_ptmx_path() which calls pts_path() which in turn calls
path_parent_directory(). Based on the above explanation,
path_parent_directory() will yield / as the parent directory for the
/dev/ptmx bind-mount and not the expected /dev. Thus, the kernel detects
that /dev/ptmx is escaping its bind-mount and will set /proc/<pid>/fd/<nr>
to /.

This patch changes the logic to first resolve any bind-mounts. After the
bind-mounts have been resolved (i.e. we have traced it back to the
associated devpts mount) devpts_ptmx_path() can be called. In order to
guarantee correct path generation for the slave file descriptor the kernel
now requires that a pts directory is found in the parent directory of the
ptmx bind-mount. This implies that when doing bind-mounts the ptmx
bind-mount and the devpts mount should have a common parent directory. A
valid example is:

mount -t devpts devpts /dev/pts
mount --bind /dev/pts/ptmx /dev/ptmx

an invalid example is:

mount -t devpts devpts /dev/pts
mount --bind /dev/pts/ptmx /ptmx

This allows us to support:
- calling open on ptmx devices located inside non-standard devpts mounts:
  mount -t devpts devpts /mnt
  master = open("/mnt/ptmx", ...);
  slave = ioctl(master, TIOCGPTPEER, ...);
- calling open on ptmx devices located outside the devpts mount with a
  common ancestor directory:
  mount -t devpts devpts /dev/pts
  mount --bind /dev/pts/ptmx /dev/ptmx
  master = open("/dev/ptmx", ...);
  slave = ioctl(master, TIOCGPTPEER, ...);

while failing on ptmx devices located outside the devpts mount without a
common ancestor directory:
  mount -t devpts devpts /dev/pts
  mount --bind /dev/pts/ptmx /ptmx
  master = open("/ptmx", ...);
  slave = ioctl(master, TIOCGPTPEER, ...);

in which case save path generation cannot be guaranteed.

Signed-off-by: Christian Brauner <christian.brauner-GeWIH/nMZzLQT0dZR+AlfA@public.gmane.org>
Suggested-by: Eric Biederman <ebiederm-aS9lmoZGLiVWk0Htik3J/w@public.gmane.org>
Suggested-by: Linus Torvalds <torvalds-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b@public.gmane.org>
---
ChangeLog v4->v5:
* reverse error handling logic to further simplify (Linus)
ChangeLog v3->v4:
* simplify if condition (Eric)
ChangeLog v2->v3:
* rework logic to account for non-standard devpts mounts such as
  mount -t devpts devpts /mnt (Christian)
ChangeLog v1->v2:
* move removal of if (path->mnt->mnt_sb->s_magic == DEVPTS_SUPER_MAGIC)
  condition to separate patch with non-functional changes (Linus)
ChangeLog v0->v1:
* remove
        /* Has the devpts filesystem already been found? */
        if (path->mnt->mnt_sb->s_magic == DEVPTS_SUPER_MAGIC)
                        return 0
  from devpts_ptmx_path() (Eric)
* check superblock after devpts_ptmx_path() returned (Christian)
---
 fs/devpts/inode.c | 26 ++++++++++++++++----------
 1 file changed, 16 insertions(+), 10 deletions(-)

diff --git a/fs/devpts/inode.c b/fs/devpts/inode.c
index 71b901936113..542364bf923e 100644
--- a/fs/devpts/inode.c
+++ b/fs/devpts/inode.c
@@ -160,21 +160,27 @@ struct vfsmount *devpts_mntget(struct file *filp, struct pts_fs_info *fsi)
 	path = filp->f_path;
 	path_get(&path);
 
-	/* Has the devpts filesystem already been found? */
-	if (path.mnt->mnt_sb->s_magic != DEVPTS_SUPER_MAGIC)
+	/* Walk upward while the start point is a bind mount of
+	 * a single file.
+	 */
+	while (path.mnt->mnt_root == path.dentry)
+		if (follow_up(&path) == 0)
+			break;
+
+	/* devpts_ptmx_path() finds a devpts fs or returns an error. */
+	if ((path.mnt->mnt_sb->s_magic != DEVPTS_SUPER_MAGIC) ||
+	    (DEVPTS_SB(path.mnt->mnt_sb) != fsi))
 		err = devpts_ptmx_path(&path);
 	dput(path.dentry);
-	if (err) {
-		mntput(path.mnt);
-		return ERR_PTR(err);
-	}
+	if (!err) {
+		if (DEVPTS_SB(path.mnt->mnt_sb) == fsi)
+			return path.mnt;
 
-	if (DEVPTS_SB(path.mnt->mnt_sb) != fsi) {
-		mntput(path.mnt);
-		return ERR_PTR(-ENODEV);
+		err = -ENODEV;
 	}
 
-	return path.mnt;
+	mntput(path.mnt);
+	return ERR_PTR(err);
 }
 
 struct pts_fs_info *devpts_acquire(struct file *filp)
-- 
2.15.1

Re: [PATCH 2/4 v5] devpts: resolve devpts bind-mounts

[Eric W. Biederman]

Christian Brauner <christian.brauner-GeWIH/nMZzLQT0dZR+AlfA@public.gmane.org> writes:

Reviewed-by: "Eric W. Biederman" <ebiederm-aS9lmoZGLiVWk0Htik3J/w@public.gmane.org>

It would have been more readable if Linus's suggested cleanup were
in a separate patch.  But unless you need to respin I would not worry
about that.

> Most libcs will still look at /dev/ptmx when opening the master fd of a pty
> device. When /dev/ptmx is a bind-mount of /dev/pts/ptmx and the TIOCGPTPEER
> ioctl() is used to safely retrieve a file descriptor for the slave side of
> the pty based on the master fd, the /proc/self/fd/{0,1,2} symlinks will
> point to /. A very simply reproducer for this issue presupposing a libc
> that uses TIOCGPTPEER in its openpty() implementation is:
>
> unshare --mount
> mount --bind /dev/pts/ptmx /dev/ptmx
> chmod 666 /dev/ptmx
> script
> ls -al /proc/self/fd/0
>
> Having bind-mounts of /dev/pts/ptmx to /dev/ptmx not working correctly is a
> regression. In addition, it is also a fairly common scenario in containers
> employing user namespaces.
>
> The reason for the current failure is that the kernel tries to verify the
> useability of the devpts filesystem without resolving the /dev/ptmx
> bind-mount first. This will lead it to detect that the dentry is escaping
> its bind-mount. The reason is that while the devpts filesystem mounted at
> /dev/pts has the devtmpfs mounted at /dev as its parent mount:
>
> 21 -- -- / /dev
> -- 21 -- / /dev/pts
>
> devtmpfs and devpts are on different devices
>
> -- -- 0:6  / /dev
> -- -- 0:20 / /dev/pts
>
> This has the consequence that the pathname of the parent directory of the
> devpts filesystem mount at /dev/pts is /. So if /dev/ptmx is a bind-mount
> of /dev/pts/ptmx then the /dev/ptmx bind-mount and the devpts mount at
> /dev/pts will end up being located on the same device which is recorded in
> the superblock of their vfsmount. This means the parent directory of the
> /dev/ptmx bind-mount will be /ptmx:
>
> -- -- ---- /ptmx /dev/ptmx
>
> Without the bind-mount resolution patch the kernel will now perform the
> bind-mount escape check directly on /dev/ptmx. The function responsible for
> this is devpts_ptmx_path() which calls pts_path() which in turn calls
> path_parent_directory(). Based on the above explanation,
> path_parent_directory() will yield / as the parent directory for the
> /dev/ptmx bind-mount and not the expected /dev. Thus, the kernel detects
> that /dev/ptmx is escaping its bind-mount and will set /proc/<pid>/fd/<nr>
> to /.
>
> This patch changes the logic to first resolve any bind-mounts. After the
> bind-mounts have been resolved (i.e. we have traced it back to the
> associated devpts mount) devpts_ptmx_path() can be called. In order to
> guarantee correct path generation for the slave file descriptor the kernel
> now requires that a pts directory is found in the parent directory of the
> ptmx bind-mount. This implies that when doing bind-mounts the ptmx
> bind-mount and the devpts mount should have a common parent directory. A
> valid example is:
>
> mount -t devpts devpts /dev/pts
> mount --bind /dev/pts/ptmx /dev/ptmx
>
> an invalid example is:
>
> mount -t devpts devpts /dev/pts
> mount --bind /dev/pts/ptmx /ptmx
>
> This allows us to support:
> - calling open on ptmx devices located inside non-standard devpts mounts:
>   mount -t devpts devpts /mnt
>   master = open("/mnt/ptmx", ...);
>   slave = ioctl(master, TIOCGPTPEER, ...);
> - calling open on ptmx devices located outside the devpts mount with a
>   common ancestor directory:
>   mount -t devpts devpts /dev/pts
>   mount --bind /dev/pts/ptmx /dev/ptmx
>   master = open("/dev/ptmx", ...);
>   slave = ioctl(master, TIOCGPTPEER, ...);
>
> while failing on ptmx devices located outside the devpts mount without a
> common ancestor directory:
>   mount -t devpts devpts /dev/pts
>   mount --bind /dev/pts/ptmx /ptmx
>   master = open("/ptmx", ...);
>   slave = ioctl(master, TIOCGPTPEER, ...);
>
> in which case save path generation cannot be guaranteed.
>
> Signed-off-by: Christian Brauner <christian.brauner-GeWIH/nMZzLQT0dZR+AlfA@public.gmane.org>
> Suggested-by: Eric Biederman <ebiederm-aS9lmoZGLiVWk0Htik3J/w@public.gmane.org>
> Suggested-by: Linus Torvalds <torvalds-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b@public.gmane.org>
> ---
> ChangeLog v4->v5:
> * reverse error handling logic to further simplify (Linus)
> ChangeLog v3->v4:
> * simplify if condition (Eric)
> ChangeLog v2->v3:
> * rework logic to account for non-standard devpts mounts such as
>   mount -t devpts devpts /mnt (Christian)
> ChangeLog v1->v2:
> * move removal of if (path->mnt->mnt_sb->s_magic == DEVPTS_SUPER_MAGIC)
>   condition to separate patch with non-functional changes (Linus)
> ChangeLog v0->v1:
> * remove
>         /* Has the devpts filesystem already been found? */
>         if (path->mnt->mnt_sb->s_magic == DEVPTS_SUPER_MAGIC)
>                         return 0
>   from devpts_ptmx_path() (Eric)
> * check superblock after devpts_ptmx_path() returned (Christian)
> ---
>  fs/devpts/inode.c | 26 ++++++++++++++++----------
>  1 file changed, 16 insertions(+), 10 deletions(-)
>
> diff --git a/fs/devpts/inode.c b/fs/devpts/inode.c
> index 71b901936113..542364bf923e 100644
> --- a/fs/devpts/inode.c
> +++ b/fs/devpts/inode.c
> @@ -160,21 +160,27 @@ struct vfsmount *devpts_mntget(struct file *filp, struct pts_fs_info *fsi)
>  	path = filp->f_path;
>  	path_get(&path);
>  
> -	/* Has the devpts filesystem already been found? */
> -	if (path.mnt->mnt_sb->s_magic != DEVPTS_SUPER_MAGIC)
> +	/* Walk upward while the start point is a bind mount of
> +	 * a single file.
> +	 */
> +	while (path.mnt->mnt_root == path.dentry)
> +		if (follow_up(&path) == 0)
> +			break;
> +
> +	/* devpts_ptmx_path() finds a devpts fs or returns an error. */
> +	if ((path.mnt->mnt_sb->s_magic != DEVPTS_SUPER_MAGIC) ||
> +	    (DEVPTS_SB(path.mnt->mnt_sb) != fsi))
>  		err = devpts_ptmx_path(&path);
>  	dput(path.dentry);
> -	if (err) {
> -		mntput(path.mnt);
> -		return ERR_PTR(err);
> -	}
> +	if (!err) {
> +		if (DEVPTS_SB(path.mnt->mnt_sb) == fsi)
> +			return path.mnt;
>  
> -	if (DEVPTS_SB(path.mnt->mnt_sb) != fsi) {
> -		mntput(path.mnt);
> -		return ERR_PTR(-ENODEV);
> +		err = -ENODEV;
>  	}
>  
> -	return path.mnt;
> +	mntput(path.mnt);
> +	return ERR_PTR(err);
>  }
>  
>  struct pts_fs_info *devpts_acquire(struct file *filp)

[PATCH 3/4 v5] devpts: comment devpts_mntget()

[Christian Brauner]

Signed-off-by: Christian Brauner <christian.brauner-GeWIH/nMZzLQT0dZR+AlfA@public.gmane.org>
---
ChangeLog v4->v5:
* patch added
ChangeLog v3->v4:
* patch not present
ChangeLog v2->v3:
* patch not present
ChangeLog v1->v2:
* patch not present
ChangeLog v0->v1:
* patch not present
---
 fs/devpts/inode.c | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/fs/devpts/inode.c b/fs/devpts/inode.c
index 542364bf923e..e072e955ce33 100644
--- a/fs/devpts/inode.c
+++ b/fs/devpts/inode.c
@@ -152,6 +152,24 @@ static int devpts_ptmx_path(struct path *path)
 	return 0;
 }
 
+/*
+ * Try to find a suitable devpts filesystem. We support the following
+ * scenarios:
+ * - The ptmx device node is located in the same directory as the devpts
+ *   mount where the pts device nodes are located.
+ *   This is e.g. the case when calling open on the /dev/pts/ptmx device
+ *   node when the devpts filesystem is mounted at /dev/pts.
+ * - The ptmx device node is located outside the devpts filesystem mount
+ *   where the pts device nodes are located. For example, the ptmx device
+ *   is a symlink, separate device node, or bind-mount.
+ *   A supported scenario is bind-mounting /dev/pts/ptmx to /dev/ptmx and
+ *   then calling open on /dev/ptmx. In this case a suitable pts
+ *   subdirectory can be found in the common parent directory /dev of the
+ *   devpts mount and the ptmx bind-mount, after resolving the /dev/ptmx
+ *   bind-mount.
+ *   If no suitable pts subdirectory can be found this function will fail.
+ *   This is e.g. the case when bind-mounting /dev/pts/ptmx to /ptmx.
+ */
 struct vfsmount *devpts_mntget(struct file *filp, struct pts_fs_info *fsi)
 {
 	struct path path;
-- 
2.15.1

[PATCH 4/4 v5] selftests: add devpts selftests

[Christian Brauner]

This adds tests to check:
- bind-mounts from /dev/pts/ptmx to /dev/ptmx work
- non-standard mounts of devpts work
- bind-mounts of /dev/pts/ptmx to locations that do not resolve to a valid
  slave pty path under the originating devpts mount fail

Signed-off-by: Christian Brauner <christian.brauner-GeWIH/nMZzLQT0dZR+AlfA@public.gmane.org>
---
ChangeLog v4->v5:
* extend tests to verify failure on ptmx devices located outside the
  devpts mount without a common ancestor directory
ChangeLog v3->v4:
* patch unchanged
ChangeLog v2->v3:
* extend test for non-standard devpts mounts such as
  mount -t devpts e devpts /mnt
ChangeLog v1->v2:
* patch added
ChangeLog v0->v1:
* patch not present
---
 tools/testing/selftests/Makefile                 |   1 +
 tools/testing/selftests/filesystems/.gitignore   |   1 +
 tools/testing/selftests/filesystems/Makefile     |   2 +-
 tools/testing/selftests/filesystems/devpts_pts.c | 313 +++++++++++++++++++++++
 4 files changed, 316 insertions(+), 1 deletion(-)
 create mode 100644 tools/testing/selftests/filesystems/devpts_pts.c

diff --git a/tools/testing/selftests/Makefile b/tools/testing/selftests/Makefile
index 7442dfb73b7f..dbda89c9d9b9 100644
--- a/tools/testing/selftests/Makefile
+++ b/tools/testing/selftests/Makefile
@@ -7,6 +7,7 @@ TARGETS += cpufreq
 TARGETS += cpu-hotplug
 TARGETS += efivarfs
 TARGETS += exec
+TARGETS += filesystems
 TARGETS += firmware
 TARGETS += ftrace
 TARGETS += futex
diff --git a/tools/testing/selftests/filesystems/.gitignore b/tools/testing/selftests/filesystems/.gitignore
index 31d6e426b6d4..8449cf6716ce 100644
--- a/tools/testing/selftests/filesystems/.gitignore
+++ b/tools/testing/selftests/filesystems/.gitignore
@@ -1 +1,2 @@
 dnotify_test
+devpts_pts
diff --git a/tools/testing/selftests/filesystems/Makefile b/tools/testing/selftests/filesystems/Makefile
index 13a73bf725b5..4e6d09fb166f 100644
--- a/tools/testing/selftests/filesystems/Makefile
+++ b/tools/testing/selftests/filesystems/Makefile
@@ -1,5 +1,5 @@
 # SPDX-License-Identifier: GPL-2.0
-TEST_PROGS := dnotify_test
+TEST_PROGS := dnotify_test devpts_pts
 all: $(TEST_PROGS)
 
 include ../lib.mk
diff --git a/tools/testing/selftests/filesystems/devpts_pts.c b/tools/testing/selftests/filesystems/devpts_pts.c
new file mode 100644
index 000000000000..b9055e974289
--- /dev/null
+++ b/tools/testing/selftests/filesystems/devpts_pts.c
@@ -0,0 +1,313 @@
+// SPDX-License-Identifier: GPL-2.0
+#define _GNU_SOURCE
+#include <errno.h>
+#include <fcntl.h>
+#include <sched.h>
+#include <stdbool.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+#include <unistd.h>
+#include <sys/ioctl.h>
+#include <sys/mount.h>
+#include <sys/wait.h>
+
+static bool terminal_dup2(int duplicate, int original)
+{
+	int ret;
+
+	ret = dup2(duplicate, original);
+	if (ret < 0)
+		return false;
+
+	return true;
+}
+
+static int terminal_set_stdfds(int fd)
+{
+	int i;
+
+	if (fd < 0)
+		return 0;
+
+	for (i = 0; i < 3; i++)
+		if (!terminal_dup2(fd, (int[]){STDIN_FILENO, STDOUT_FILENO,
+					       STDERR_FILENO}[i]))
+			return -1;
+
+	return 0;
+}
+
+static int login_pty(int fd)
+{
+	int ret;
+
+	setsid();
+
+	ret = ioctl(fd, TIOCSCTTY, NULL);
+	if (ret < 0)
+		return -1;
+
+	ret = terminal_set_stdfds(fd);
+	if (ret < 0)
+		return -1;
+
+	if (fd > STDERR_FILENO)
+		close(fd);
+
+	return 0;
+}
+
+static int wait_for_pid(pid_t pid)
+{
+	int status, ret;
+
+again:
+	ret = waitpid(pid, &status, 0);
+	if (ret == -1) {
+		if (errno == EINTR)
+			goto again;
+		return -1;
+	}
+	if (ret != pid)
+		goto again;
+
+	if (!WIFEXITED(status) || WEXITSTATUS(status) != 0)
+		return -1;
+
+	return 0;
+}
+
+static int resolve_procfd_symlink(int fd, char *buf, size_t buflen)
+{
+	int ret;
+	char procfd[4096];
+
+	ret = snprintf(procfd, 4096, "/proc/self/fd/%d", fd);
+	if (ret < 0 || ret >= 4096)
+		return -1;
+
+	ret = readlink(procfd, buf, buflen);
+	if (ret < 0 || (size_t)ret >= buflen)
+		return -1;
+
+	buf[ret] = '\0';
+
+	return 0;
+}
+
+static int do_tiocgptpeer(char *ptmx, char *expected_procfd_contents)
+{
+	int ret;
+	int master = -1, slave = -1, fret = -1;
+
+	master = open(ptmx, O_RDWR | O_NOCTTY | O_CLOEXEC);
+	if (master < 0) {
+		fprintf(stderr, "Failed to open \"%s\": %s\n", ptmx,
+			strerror(errno));
+		return -1;
+	}
+
+	/*
+	 * grantpt() makes assumptions about /dev/pts/ so ignore it. It's also
+	 * not really needed.
+	 */
+	ret = unlockpt(master);
+	if (ret < 0) {
+		fprintf(stderr, "Failed to unlock terminal\n");
+		goto do_cleanup;
+	}
+
+#ifdef TIOCGPTPEER
+	slave = ioctl(master, TIOCGPTPEER, O_RDWR | O_NOCTTY | O_CLOEXEC);
+#endif
+	if (slave < 0) {
+		if (errno == EINVAL) {
+			fprintf(stderr, "TIOCGPTPEER is not supported. "
+					"Skipping test.\n");
+			fret = EXIT_SUCCESS;
+		}
+
+		fprintf(stderr, "Failed to perform TIOCGPTPEER ioctl\n");
+		goto do_cleanup;
+	}
+
+	pid_t pid = fork();
+	if (pid < 0)
+		goto do_cleanup;
+
+	if (pid == 0) {
+		char buf[4096];
+
+		ret = login_pty(slave);
+		if (ret < 0) {
+			fprintf(stderr, "Failed to setup terminal\n");
+			_exit(EXIT_FAILURE);
+		}
+
+		ret = resolve_procfd_symlink(STDIN_FILENO, buf, sizeof(buf));
+		if (ret < 0) {
+			fprintf(stderr, "Failed to retrieve pathname of pts "
+					"slave file descriptor\n");
+			_exit(EXIT_FAILURE);
+		}
+
+		if (strncmp(expected_procfd_contents, buf,
+			    strlen(expected_procfd_contents)) != 0) {
+			fprintf(stderr, "Received invalid contents for "
+					"\"/proc/<pid>/fd/%d\" symlink: %s\n",
+					STDIN_FILENO, buf);
+			_exit(-1);
+		}
+
+		fprintf(stderr, "Contents of \"/proc/<pid>/fd/%d\" "
+				"symlink are valid: %s\n", STDIN_FILENO, buf);
+
+		_exit(EXIT_SUCCESS);
+	}
+
+	ret = wait_for_pid(pid);
+	if (ret < 0)
+		goto do_cleanup;
+
+	fret = EXIT_SUCCESS;
+
+do_cleanup:
+	if (master >= 0)
+		close(master);
+	if (slave >= 0)
+		close(slave);
+
+	return fret;
+}
+
+static int verify_non_standard_devpts_mount(void)
+{
+	char *mntpoint;
+	int ret = -1;
+	char devpts[] = P_tmpdir "/devpts_fs_XXXXXX";
+	char ptmx[] = P_tmpdir "/devpts_fs_XXXXXX/ptmx";
+
+	ret = umount("/dev/pts");
+	if (ret < 0) {
+		fprintf(stderr, "Failed to unmount \"/dev/pts\": %s\n",
+				strerror(errno));
+		return -1;
+	}
+
+	(void)umount("/dev/ptmx");
+
+	mntpoint = mkdtemp(devpts);
+	if (!mntpoint) {
+		fprintf(stderr, "Failed to create temporary mountpoint: %s\n",
+				 strerror(errno));
+		return -1;
+	}
+
+	ret = mount("devpts", mntpoint, "devpts", MS_NOSUID | MS_NOEXEC,
+		    "newinstance,ptmxmode=0666,mode=0620,gid=5");
+	if (ret < 0) {
+		fprintf(stderr, "Failed to mount devpts fs to \"%s\" in new "
+				"mount namespace: %s\n", mntpoint,
+				strerror(errno));
+		unlink(mntpoint);
+		return -1;
+	}
+
+	ret = snprintf(ptmx, sizeof(ptmx), "%s/ptmx", devpts);
+	if (ret < 0 || (size_t)ret >= sizeof(ptmx)) {
+		unlink(mntpoint);
+		return -1;
+	}
+
+	ret = do_tiocgptpeer(ptmx, mntpoint);
+	unlink(mntpoint);
+	if (ret < 0)
+		return -1;
+
+	return 0;
+}
+
+static int verify_ptmx_bind_mount(void)
+{
+	int ret;
+
+	ret = mount("/dev/pts/ptmx", "/dev/ptmx", NULL, MS_BIND, NULL);
+	if (ret < 0) {
+		fprintf(stderr, "Failed to bind mount \"/dev/pts/ptmx\" to "
+				"\"/dev/ptmx\" mount namespace\n");
+		return -1;
+	}
+
+	ret = do_tiocgptpeer("/dev/ptmx", "/dev/pts/");
+	if (ret < 0)
+		return -1;
+
+	return 0;
+}
+
+static int verify_invalid_ptmx_bind_mount(void)
+{
+	int ret;
+	char mntpoint_fd;
+	char ptmx[] = P_tmpdir "/devpts_ptmx_XXXXXX";
+
+	mntpoint_fd = mkstemp(ptmx);
+	if (mntpoint_fd < 0) {
+		fprintf(stderr, "Failed to create temporary directory: %s\n",
+				 strerror(errno));
+		return -1;
+	}
+
+	ret = mount("/dev/pts/ptmx", ptmx, NULL, MS_BIND, NULL);
+	close(mntpoint_fd);
+	if (ret < 0) {
+		fprintf(stderr, "Failed to bind mount \"/dev/pts/ptmx\" to "
+				"\"%s\" mount namespace\n", ptmx);
+		return -1;
+	}
+
+	ret = do_tiocgptpeer(ptmx, "/dev/pts/");
+	if (ret == 0)
+		return -1;
+
+	return 0;
+}
+
+int main(int argc, char *argv[])
+{
+	int ret;
+
+	if (!isatty(STDIN_FILENO)) {
+		fprintf(stderr, "Standard input file desciptor is not attached "
+				"to a terminal. Skipping test\n");
+		exit(EXIT_FAILURE);
+	}
+
+	ret = unshare(CLONE_NEWNS);
+	if (ret < 0) {
+		fprintf(stderr, "Failed to unshare mount namespace\n");
+		exit(EXIT_FAILURE);
+	}
+
+	ret = mount("", "/", NULL, MS_PRIVATE | MS_REC, 0);
+	if (ret < 0) {
+		fprintf(stderr, "Failed to make \"/\" MS_PRIVATE in new mount "
+				"namespace\n");
+		exit(EXIT_FAILURE);
+	}
+
+	ret = verify_ptmx_bind_mount();
+	if (ret < 0)
+		exit(EXIT_FAILURE);
+
+	ret = verify_invalid_ptmx_bind_mount();
+	if (ret < 0)
+		exit(EXIT_FAILURE);
+
+	ret = verify_non_standard_devpts_mount();
+	if (ret < 0)
+		exit(EXIT_FAILURE);
+
+	exit(EXIT_SUCCESS);
+}
-- 
2.15.1

Re: [PATCH 0/4 v5] devpts: handle bind-mounts correctly

[Eric W. Biederman]

Christian Brauner <christian.brauner-GeWIH/nMZzLQT0dZR+AlfA@public.gmane.org> writes:

> Hey everyone,
>
> This is the fith iteration of this patch. Per-patch changes are
> summarized in the individual patches:

Acked-by: "Eric W. Biederman" <ebiederm-aS9lmoZGLiVWk0Htik3J/w@public.gmane.org>

I guess we need to answer the question do I pick up this series or does
gregkh.  This series would seem to fit in his tty tree.

You haven't copied gregkh so you may need to resend and do that.

Eric


> ChangeLog v4->v5:
> * added non-functional patch to document devpts_mntget().
>   Reason for putting this in a separate patch is that it allows you,
>   Linus and Eric, to simply drop it if judged useless.
> * reverse error handling logic to further simplify
> * dput() dentry in the non-function patch. This was not really a problem
>   since the following patch included a fix for it. But better to get it
>   right in all individual patches.
> * I did another rewrite of the problem analysis for
>   posterity in the patch "Subject: [PATCH 2/3 v3] devpts: resolve devpts
>   bind-mounts" and in this cover letter.
>
> ChangeLog v3->v4:
> * small logical simplifications
> * add test that bind-mounts of /dev/pts/ptmx to locations that do not
>   resolve to a valid slave pty path under the originating devpts mount
>   fail
>
> ChangeLog v2->v3:
> * rewritten commit message to thoroughly analyse the problem for
>   posterity in the patch "Subject: [PATCH 2/3 v3] devpts: resolve devpts
>   bind-mounts" and in this cover letter.
> * extended selftests to test for correct handling of /dev/pts/ptmx
>   bind-mounts to /dev/ptmx and non-standard devpts mounts such as
>   mount -t devpts devpts /mnt
>
> ChangeLog v1->v2:
> * see individual patches
> ChangeLog v0->v1:
> * see individual patches
>
> Christian Brauner (4):
>   devpts: hoist out check for DEVPTS_SUPER_MAGIC
>   devpts: resolve devpts bind-mounts
>   devpts: comment devpts_mntget()
>   selftests: add devpts selftests
>
>  fs/devpts/inode.c                                |  66 +++--
>  tools/testing/selftests/Makefile                 |   1 +
>  tools/testing/selftests/filesystems/.gitignore   |   1 +
>  tools/testing/selftests/filesystems/Makefile     |   2 +-
>  tools/testing/selftests/filesystems/devpts_pts.c | 313 +++++++++++++++++++++++
>  5 files changed, 363 insertions(+), 20 deletions(-)
>  create mode 100644 tools/testing/selftests/filesystems/devpts_pts.c

Re: [PATCH 0/4 v5] devpts: handle bind-mounts correctly

[Christian Brauner]

On Tue, Mar 13, 2018 at 11:41:00AM -0500, Eric W. Biederman wrote:
> Christian Brauner <christian.brauner-GeWIH/nMZzLQT0dZR+AlfA@public.gmane.org> writes:
> 
> > Hey everyone,
> >
> > This is the fith iteration of this patch. Per-patch changes are
> > summarized in the individual patches:
> 
> Acked-by: "Eric W. Biederman" <ebiederm-aS9lmoZGLiVWk0Htik3J/w@public.gmane.org>
> 
> I guess we need to answer the question do I pick up this series or does
> gregkh.  This series would seem to fit in his tty tree.
> 
> You haven't copied gregkh so you may need to resend and do that.

I'll resend and CC grekh now.

Christian

> 
> Eric
> 
> 
> > ChangeLog v4->v5:
> > * added non-functional patch to document devpts_mntget().
> >   Reason for putting this in a separate patch is that it allows you,
> >   Linus and Eric, to simply drop it if judged useless.
> > * reverse error handling logic to further simplify
> > * dput() dentry in the non-function patch. This was not really a problem
> >   since the following patch included a fix for it. But better to get it
> >   right in all individual patches.
> > * I did another rewrite of the problem analysis for
> >   posterity in the patch "Subject: [PATCH 2/3 v3] devpts: resolve devpts
> >   bind-mounts" and in this cover letter.
> >
> > ChangeLog v3->v4:
> > * small logical simplifications
> > * add test that bind-mounts of /dev/pts/ptmx to locations that do not
> >   resolve to a valid slave pty path under the originating devpts mount
> >   fail
> >
> > ChangeLog v2->v3:
> > * rewritten commit message to thoroughly analyse the problem for
> >   posterity in the patch "Subject: [PATCH 2/3 v3] devpts: resolve devpts
> >   bind-mounts" and in this cover letter.
> > * extended selftests to test for correct handling of /dev/pts/ptmx
> >   bind-mounts to /dev/ptmx and non-standard devpts mounts such as
> >   mount -t devpts devpts /mnt
> >
> > ChangeLog v1->v2:
> > * see individual patches
> > ChangeLog v0->v1:
> > * see individual patches
> >
> > Christian Brauner (4):
> >   devpts: hoist out check for DEVPTS_SUPER_MAGIC
> >   devpts: resolve devpts bind-mounts
> >   devpts: comment devpts_mntget()
> >   selftests: add devpts selftests
> >
> >  fs/devpts/inode.c                                |  66 +++--
> >  tools/testing/selftests/Makefile                 |   1 +
> >  tools/testing/selftests/filesystems/.gitignore   |   1 +
> >  tools/testing/selftests/filesystems/Makefile     |   2 +-
> >  tools/testing/selftests/filesystems/devpts_pts.c | 313 +++++++++++++++++++++++
> >  5 files changed, 363 insertions(+), 20 deletions(-)
> >  create mode 100644 tools/testing/selftests/filesystems/devpts_pts.c
> _______________________________________________
> Containers mailing list
> Containers-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org
> https://lists.linuxfoundation.org/mailman/listinfo/containers