[PATCH tty-next 0/6] Minor devpts cleanups, fixes and enhancments

[PATCH tty-next 0/6] Minor devpts cleanups, fixes and enhancments

[Eric W. Biederman]

Greg please apply the following patches to tty-next.  If tty-next is not
the proper tree please let me know and I will take these patches through
my user namespace tree.

The follow patches perform a few small cleanups and one enhancement to
devpts, with a net decrease in code size.

The big achievement from a code maintenance point of view is being able
to use mount_nodev in devpts_mount.

The only really significant bug fix is handling the very unlikely case
when kzalloc fails in new_pts_fs_info called from devpts_fill_super and
s_fs_info is NULL when devpts_kill_sb is called from deactivate_locked
super.

The final patch is an enhancment to use the fsuid and fsgid of the mounter
of devpts as the owner of /dev/pts/ptmx.  This is a feature that has
been asked for by users of user namespaces a couple of times, so that
they are not required to have uid 0 mapped into a user namespace to use
devpts.

Eric

fs/devpts/inode.c | 71 ++++++++++++++++++++-----------------------------------
 1 file changed, 26 insertions(+), 45 deletions(-)

Eric W. Biederman (6):
      devpts: Move parse_mount_options into fill_super
      devpts: Move the creation of /dev/pts/ptmx into fill_super
      devpts: Simplify devpts_mount by using mount_nodev
      devpts: Make devpts_kill_sb safe if fsi is NULL
      devpts: Remove sync_filesystems
      devpts: Change the owner of /dev/pts/ptmx to the mounter of /dev/pts

[PATCH tty-next 0/6] Minor devpts cleanups, fixes and enhancments

[Eric W. Biederman]

Greg please apply the following patches to tty-next.  If tty-next is not
the proper tree please let me know and I will take these patches through
my user namespace tree.

The follow patches perform a few small cleanups and one enhancement to
devpts, with a net decrease in code size.

The big achievement from a code maintenance point of view is being able
to use mount_nodev in devpts_mount.

The only really significant bug fix is handling the very unlikely case
when kzalloc fails in new_pts_fs_info called from devpts_fill_super and
s_fs_info is NULL when devpts_kill_sb is called from deactivate_locked
super.

The final patch is an enhancment to use the fsuid and fsgid of the mounter
of devpts as the owner of /dev/pts/ptmx.  This is a feature that has
been asked for by users of user namespaces a couple of times, so that
they are not required to have uid 0 mapped into a user namespace to use
devpts.

Eric

fs/devpts/inode.c | 71 ++++++++++++++++++++-----------------------------------
 1 file changed, 26 insertions(+), 45 deletions(-)

Eric W. Biederman (6):
      devpts: Move parse_mount_options into fill_super
      devpts: Move the creation of /dev/pts/ptmx into fill_super
      devpts: Simplify devpts_mount by using mount_nodev
      devpts: Make devpts_kill_sb safe if fsi is NULL
      devpts: Remove sync_filesystems
      devpts: Change the owner of /dev/pts/ptmx to the mounter of /dev/pts

[PATCH tty-next 1/6] devpts: Move parse_mount_options into fill_super

[Eric W. Biederman]

Signed-off-by: "Eric W. Biederman" <ebiederm-aS9lmoZGLiVWk0Htik3J/w@public.gmane.org>
---
 fs/devpts/inode.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/fs/devpts/inode.c b/fs/devpts/inode.c
index 79a5941c2474..c59d39f2d512 100644
--- a/fs/devpts/inode.c
+++ b/fs/devpts/inode.c
@@ -395,6 +395,7 @@ static int
 devpts_fill_super(struct super_block *s, void *data, int silent)
 {
 	struct inode *inode;
+	int error;
 
 	s->s_iflags &= ~SB_I_NODEV;
 	s->s_blocksize = 1024;
@@ -403,10 +404,16 @@ devpts_fill_super(struct super_block *s, void *data, int silent)
 	s->s_op = &devpts_sops;
 	s->s_time_gran = 1;
 
+	error = -ENOMEM;
 	s->s_fs_info = new_pts_fs_info(s);
 	if (!s->s_fs_info)
 		goto fail;
 
+	error = parse_mount_options(data, PARSE_MOUNT, &DEVPTS_SB(s)->mount_opts);
+	if (error)
+		goto fail;
+
+	error = -ENOMEM;
 	inode = new_inode(s);
 	if (!inode)
 		goto fail;
@@ -424,7 +431,7 @@ devpts_fill_super(struct super_block *s, void *data, int silent)
 	pr_err("get root dentry failed\n");
 
 fail:
-	return -ENOMEM;
+	return error;
 }
 
 /*
@@ -437,13 +444,8 @@ static struct dentry *devpts_mount(struct file_system_type *fs_type,
 	int flags, const char *dev_name, void *data)
 {
 	int error;
-	struct pts_mount_opts opts;
 	struct super_block *s;
 
-	error = parse_mount_options(data, PARSE_MOUNT, &opts);
-	if (error)
-		return ERR_PTR(error);
-
 	s = sget(fs_type, NULL, set_anon_super, flags, NULL);
 	if (IS_ERR(s))
 		return ERR_CAST(s);
@@ -455,8 +457,6 @@ static struct dentry *devpts_mount(struct file_system_type *fs_type,
 		s->s_flags |= MS_ACTIVE;
 	}
 
-	memcpy(&(DEVPTS_SB(s))->mount_opts, &opts, sizeof(opts));
-
 	error = mknod_ptmx(s);
 	if (error)
 		goto out_undo_sget;
-- 
2.8.3

[PATCH tty-next 1/6] devpts: Move parse_mount_options into fill_super

[Eric W. Biederman]

Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
---
 fs/devpts/inode.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/fs/devpts/inode.c b/fs/devpts/inode.c
index 79a5941c2474..c59d39f2d512 100644
--- a/fs/devpts/inode.c
+++ b/fs/devpts/inode.c
@@ -395,6 +395,7 @@ static int
 devpts_fill_super(struct super_block *s, void *data, int silent)
 {
 	struct inode *inode;
+	int error;
 
 	s->s_iflags &= ~SB_I_NODEV;
 	s->s_blocksize = 1024;
@@ -403,10 +404,16 @@ devpts_fill_super(struct super_block *s, void *data, int silent)
 	s->s_op = &devpts_sops;
 	s->s_time_gran = 1;
 
+	error = -ENOMEM;
 	s->s_fs_info = new_pts_fs_info(s);
 	if (!s->s_fs_info)
 		goto fail;
 
+	error = parse_mount_options(data, PARSE_MOUNT, &DEVPTS_SB(s)->mount_opts);
+	if (error)
+		goto fail;
+
+	error = -ENOMEM;
 	inode = new_inode(s);
 	if (!inode)
 		goto fail;
@@ -424,7 +431,7 @@ devpts_fill_super(struct super_block *s, void *data, int silent)
 	pr_err("get root dentry failed\n");
 
 fail:
-	return -ENOMEM;
+	return error;
 }
 
 /*
@@ -437,13 +444,8 @@ static struct dentry *devpts_mount(struct file_system_type *fs_type,
 	int flags, const char *dev_name, void *data)
 {
 	int error;
-	struct pts_mount_opts opts;
 	struct super_block *s;
 
-	error = parse_mount_options(data, PARSE_MOUNT, &opts);
-	if (error)
-		return ERR_PTR(error);
-
 	s = sget(fs_type, NULL, set_anon_super, flags, NULL);
 	if (IS_ERR(s))
 		return ERR_CAST(s);
@@ -455,8 +457,6 @@ static struct dentry *devpts_mount(struct file_system_type *fs_type,
 		s->s_flags |= MS_ACTIVE;
 	}
 
-	memcpy(&(DEVPTS_SB(s))->mount_opts, &opts, sizeof(opts));
-
 	error = mknod_ptmx(s);
 	if (error)
 		goto out_undo_sget;
-- 
2.8.3

[PATCH tty-next 2/6] devpts: Move the creation of /dev/pts/ptmx into fill_super

[Eric W. Biederman]

The code makes more sense here and things are just clearer.

Signed-off-by: "Eric W. Biederman" <ebiederm-aS9lmoZGLiVWk0Htik3J/w@public.gmane.org>
---
 fs/devpts/inode.c | 19 +++++++++++--------
 1 file changed, 11 insertions(+), 8 deletions(-)

diff --git a/fs/devpts/inode.c b/fs/devpts/inode.c
index c59d39f2d512..43773543a783 100644
--- a/fs/devpts/inode.c
+++ b/fs/devpts/inode.c
@@ -425,11 +425,19 @@ devpts_fill_super(struct super_block *s, void *data, int silent)
 	set_nlink(inode, 2);
 
 	s->s_root = d_make_root(inode);
-	if (s->s_root)
-		return 0;
+	if (!s->s_root) {
+		pr_err("get root dentry failed\n");
+		goto fail;
+	}
 
-	pr_err("get root dentry failed\n");
+	error = mknod_ptmx(s);
+	if (error)
+		goto fail_dput;
 
+	return 0;
+fail_dput:
+	dput(s->s_root);
+	s->s_root = NULL;
 fail:
 	return error;
 }
@@ -456,11 +464,6 @@ static struct dentry *devpts_mount(struct file_system_type *fs_type,
 			goto out_undo_sget;
 		s->s_flags |= MS_ACTIVE;
 	}
-
-	error = mknod_ptmx(s);
-	if (error)
-		goto out_undo_sget;
-
 	return dget(s->s_root);
 
 out_undo_sget:
-- 
2.8.3

[PATCH tty-next 2/6] devpts: Move the creation of /dev/pts/ptmx into fill_super

[Eric W. Biederman]

The code makes more sense here and things are just clearer.

Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
---
 fs/devpts/inode.c | 19 +++++++++++--------
 1 file changed, 11 insertions(+), 8 deletions(-)

diff --git a/fs/devpts/inode.c b/fs/devpts/inode.c
index c59d39f2d512..43773543a783 100644
--- a/fs/devpts/inode.c
+++ b/fs/devpts/inode.c
@@ -425,11 +425,19 @@ devpts_fill_super(struct super_block *s, void *data, int silent)
 	set_nlink(inode, 2);
 
 	s->s_root = d_make_root(inode);
-	if (s->s_root)
-		return 0;
+	if (!s->s_root) {
+		pr_err("get root dentry failed\n");
+		goto fail;
+	}
 
-	pr_err("get root dentry failed\n");
+	error = mknod_ptmx(s);
+	if (error)
+		goto fail_dput;
 
+	return 0;
+fail_dput:
+	dput(s->s_root);
+	s->s_root = NULL;
 fail:
 	return error;
 }
@@ -456,11 +464,6 @@ static struct dentry *devpts_mount(struct file_system_type *fs_type,
 			goto out_undo_sget;
 		s->s_flags |= MS_ACTIVE;
 	}
-
-	error = mknod_ptmx(s);
-	if (error)
-		goto out_undo_sget;
-
 	return dget(s->s_root);
 
 out_undo_sget:
-- 
2.8.3

[PATCH tty-next 3/6] devpts: Simplify devpts_mount by using mount_nodev

[Eric W. Biederman]

Now that all of the work of setting up a superblock has been moved to
devpts_fill_super simplify devpts_mount by calling mount_nodev instead
of rolling mount_nodev by hand.

Signed-off-by: "Eric W. Biederman" <ebiederm-aS9lmoZGLiVWk0Htik3J/w@public.gmane.org>
---
 fs/devpts/inode.c | 19 +------------------
 1 file changed, 1 insertion(+), 18 deletions(-)

diff --git a/fs/devpts/inode.c b/fs/devpts/inode.c
index 43773543a783..a48b40b0bd1e 100644
--- a/fs/devpts/inode.c
+++ b/fs/devpts/inode.c
@@ -451,24 +451,7 @@ fail:
 static struct dentry *devpts_mount(struct file_system_type *fs_type,
 	int flags, const char *dev_name, void *data)
 {
-	int error;
-	struct super_block *s;
-
-	s = sget(fs_type, NULL, set_anon_super, flags, NULL);
-	if (IS_ERR(s))
-		return ERR_CAST(s);
-
-	if (!s->s_root) {
-		error = devpts_fill_super(s, data, flags & MS_SILENT ? 1 : 0);
-		if (error)
-			goto out_undo_sget;
-		s->s_flags |= MS_ACTIVE;
-	}
-	return dget(s->s_root);
-
-out_undo_sget:
-	deactivate_locked_super(s);
-	return ERR_PTR(error);
+	return mount_nodev(fs_type, flags, data, devpts_fill_super);
 }
 
 static void devpts_kill_sb(struct super_block *sb)
-- 
2.8.3

[PATCH tty-next 3/6] devpts: Simplify devpts_mount by using mount_nodev

[Eric W. Biederman]

Now that all of the work of setting up a superblock has been moved to
devpts_fill_super simplify devpts_mount by calling mount_nodev instead
of rolling mount_nodev by hand.

Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
---
 fs/devpts/inode.c | 19 +------------------
 1 file changed, 1 insertion(+), 18 deletions(-)

diff --git a/fs/devpts/inode.c b/fs/devpts/inode.c
index 43773543a783..a48b40b0bd1e 100644
--- a/fs/devpts/inode.c
+++ b/fs/devpts/inode.c
@@ -451,24 +451,7 @@ fail:
 static struct dentry *devpts_mount(struct file_system_type *fs_type,
 	int flags, const char *dev_name, void *data)
 {
-	int error;
-	struct super_block *s;
-
-	s = sget(fs_type, NULL, set_anon_super, flags, NULL);
-	if (IS_ERR(s))
-		return ERR_CAST(s);
-
-	if (!s->s_root) {
-		error = devpts_fill_super(s, data, flags & MS_SILENT ? 1 : 0);
-		if (error)
-			goto out_undo_sget;
-		s->s_flags |= MS_ACTIVE;
-	}
-	return dget(s->s_root);
-
-out_undo_sget:
-	deactivate_locked_super(s);
-	return ERR_PTR(error);
+	return mount_nodev(fs_type, flags, data, devpts_fill_super);
 }
 
 static void devpts_kill_sb(struct super_block *sb)
-- 
2.8.3

[PATCH tty-next 4/6] devpts: Make devpts_kill_sb safe if fsi is NULL

[Eric W. Biederman]

Signed-off-by: "Eric W. Biederman" <ebiederm-aS9lmoZGLiVWk0Htik3J/w@public.gmane.org>
---
 fs/devpts/inode.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/fs/devpts/inode.c b/fs/devpts/inode.c
index a48b40b0bd1e..e058ec677207 100644
--- a/fs/devpts/inode.c
+++ b/fs/devpts/inode.c
@@ -458,7 +458,8 @@ static void devpts_kill_sb(struct super_block *sb)
 {
 	struct pts_fs_info *fsi = DEVPTS_SB(sb);
 
-	ida_destroy(&fsi->allocated_ptys);
+	if (fsi)
+		ida_destroy(&fsi->allocated_ptys);
 	kfree(fsi);
 	kill_litter_super(sb);
 }
-- 
2.8.3

[PATCH tty-next 4/6] devpts: Make devpts_kill_sb safe if fsi is NULL

[Eric W. Biederman]

Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
---
 fs/devpts/inode.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/fs/devpts/inode.c b/fs/devpts/inode.c
index a48b40b0bd1e..e058ec677207 100644
--- a/fs/devpts/inode.c
+++ b/fs/devpts/inode.c
@@ -458,7 +458,8 @@ static void devpts_kill_sb(struct super_block *sb)
 {
 	struct pts_fs_info *fsi = DEVPTS_SB(sb);
 
-	ida_destroy(&fsi->allocated_ptys);
+	if (fsi)
+		ida_destroy(&fsi->allocated_ptys);
 	kfree(fsi);
 	kill_litter_super(sb);
 }
-- 
2.8.3

[PATCH tty-next 5/6] devpts: Remove sync_filesystems

[Eric W. Biederman]

devpts does not and never will have anything to sync
so don't bother calling sync_filesystems on remount.

Signed-off-by: "Eric W. Biederman" <ebiederm-aS9lmoZGLiVWk0Htik3J/w@public.gmane.org>
---
 fs/devpts/inode.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/fs/devpts/inode.c b/fs/devpts/inode.c
index e058ec677207..17593d3fcdbe 100644
--- a/fs/devpts/inode.c
+++ b/fs/devpts/inode.c
@@ -336,7 +336,6 @@ static int devpts_remount(struct super_block *sb, int *flags, char *data)
 	struct pts_fs_info *fsi = DEVPTS_SB(sb);
 	struct pts_mount_opts *opts = &fsi->mount_opts;
 
-	sync_filesystem(sb);
 	err = parse_mount_options(data, PARSE_REMOUNT, opts);
 
 	/*
-- 
2.8.3

[PATCH tty-next 5/6] devpts: Remove sync_filesystems

[Eric W. Biederman]

devpts does not and never will have anything to sync
so don't bother calling sync_filesystems on remount.

Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
---
 fs/devpts/inode.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/fs/devpts/inode.c b/fs/devpts/inode.c
index e058ec677207..17593d3fcdbe 100644
--- a/fs/devpts/inode.c
+++ b/fs/devpts/inode.c
@@ -336,7 +336,6 @@ static int devpts_remount(struct super_block *sb, int *flags, char *data)
 	struct pts_fs_info *fsi = DEVPTS_SB(sb);
 	struct pts_mount_opts *opts = &fsi->mount_opts;
 
-	sync_filesystem(sb);
 	err = parse_mount_options(data, PARSE_REMOUNT, opts);
 
 	/*
-- 
2.8.3

[PATCH tty-next 6/6] devpts: Change the owner of /dev/pts/ptmx to the mounter of /dev/pts

[Eric W. Biederman]

In 99.99% of the cases only root in a user namespace can mount /dev/pts
and in those cases the owner of /dev/pts/ptmx will remain root.root

In the oddball case where someone else has CAP_SYS_ADMIN this code
modifies the /dev/pts mount code to use current_fsuid and current_fsgid
as the values to use when creating the /dev/ptmx inode.  As is done
when any other file is created.

This is a code simplification, and it allows running without a root
user entirely.

Signed-off-by: "Eric W. Biederman" <ebiederm-aS9lmoZGLiVWk0Htik3J/w@public.gmane.org>
---
 fs/devpts/inode.c | 13 ++++---------
 1 file changed, 4 insertions(+), 9 deletions(-)

diff --git a/fs/devpts/inode.c b/fs/devpts/inode.c
index 17593d3fcdbe..442d1a7e671b 100644
--- a/fs/devpts/inode.c
+++ b/fs/devpts/inode.c
@@ -272,13 +272,8 @@ static int mknod_ptmx(struct super_block *sb)
 	struct dentry *root = sb->s_root;
 	struct pts_fs_info *fsi = DEVPTS_SB(sb);
 	struct pts_mount_opts *opts = &fsi->mount_opts;
-	kuid_t root_uid;
-	kgid_t root_gid;
-
-	root_uid = make_kuid(current_user_ns(), 0);
-	root_gid = make_kgid(current_user_ns(), 0);
-	if (!uid_valid(root_uid) || !gid_valid(root_gid))
-		return -EINVAL;
+	kuid_t ptmx_uid = current_fsuid();
+	kgid_t ptmx_gid = current_fsgid();
 
 	inode_lock(d_inode(root));
 
@@ -309,8 +304,8 @@ static int mknod_ptmx(struct super_block *sb)
 
 	mode = S_IFCHR|opts->ptmxmode;
 	init_special_inode(inode, mode, MKDEV(TTYAUX_MAJOR, 2));
-	inode->i_uid = root_uid;
-	inode->i_gid = root_gid;
+	inode->i_uid = ptmx_uid;
+	inode->i_gid = ptmx_gid;
 
 	d_add(dentry, inode);
 
-- 
2.8.3

[PATCH tty-next 6/6] devpts: Change the owner of /dev/pts/ptmx to the mounter of /dev/pts

[Eric W. Biederman]

In 99.99% of the cases only root in a user namespace can mount /dev/pts
and in those cases the owner of /dev/pts/ptmx will remain root.root

In the oddball case where someone else has CAP_SYS_ADMIN this code
modifies the /dev/pts mount code to use current_fsuid and current_fsgid
as the values to use when creating the /dev/ptmx inode.  As is done
when any other file is created.

This is a code simplification, and it allows running without a root
user entirely.

Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
---
 fs/devpts/inode.c | 13 ++++---------
 1 file changed, 4 insertions(+), 9 deletions(-)

diff --git a/fs/devpts/inode.c b/fs/devpts/inode.c
index 17593d3fcdbe..442d1a7e671b 100644
--- a/fs/devpts/inode.c
+++ b/fs/devpts/inode.c
@@ -272,13 +272,8 @@ static int mknod_ptmx(struct super_block *sb)
 	struct dentry *root = sb->s_root;
 	struct pts_fs_info *fsi = DEVPTS_SB(sb);
 	struct pts_mount_opts *opts = &fsi->mount_opts;
-	kuid_t root_uid;
-	kgid_t root_gid;
-
-	root_uid = make_kuid(current_user_ns(), 0);
-	root_gid = make_kgid(current_user_ns(), 0);
-	if (!uid_valid(root_uid) || !gid_valid(root_gid))
-		return -EINVAL;
+	kuid_t ptmx_uid = current_fsuid();
+	kgid_t ptmx_gid = current_fsgid();
 
 	inode_lock(d_inode(root));
 
@@ -309,8 +304,8 @@ static int mknod_ptmx(struct super_block *sb)
 
 	mode = S_IFCHR|opts->ptmxmode;
 	init_special_inode(inode, mode, MKDEV(TTYAUX_MAJOR, 2));
-	inode->i_uid = root_uid;
-	inode->i_gid = root_gid;
+	inode->i_uid = ptmx_uid;
+	inode->i_gid = ptmx_gid;
 
 	d_add(dentry, inode);
 
-- 
2.8.3

Re: [PATCH tty-next 0/6] Minor devpts cleanups, fixes and enhancments

[Eric W. Biederman]

ebiederm-aS9lmoZGLiVWk0Htik3J/w@public.gmane.org (Eric W. Biederman) writes:

> Greg please apply the following patches to tty-next.  If tty-next is not
> the proper tree please let me know and I will take these patches through
> my user namespace tree.

I have not heard anything so I am taking these patches through my tree.

Eric

Re: [PATCH tty-next 0/6] Minor devpts cleanups, fixes and enhancments

[Eric W. Biederman]

ebiederm@xmission.com (Eric W. Biederman) writes:

> Greg please apply the following patches to tty-next.  If tty-next is not
> the proper tree please let me know and I will take these patches through
> my user namespace tree.

I have not heard anything so I am taking these patches through my tree.

Eric

Re: [PATCH tty-next 0/6] Minor devpts cleanups, fixes and enhancments

[Greg Kroah-Hartman]

On Thu, Sep 22, 2016 at 01:33:59PM -0500, Eric W. Biederman wrote:
> 
> ebiederm@xmission.com (Eric W. Biederman) writes:
> 
> > Greg please apply the following patches to tty-next.  If tty-next is not
> > the proper tree please let me know and I will take these patches through
> > my user namespace tree.
> 
> I have not heard anything so I am taking these patches through my tree.

Ugh, sorry about that, they missed my filters.  I'll be glad to queue
them up today, my fault.

greg k-h

Re: [PATCH tty-next 0/6] Minor devpts cleanups, fixes and enhancments

[Greg Kroah-Hartman]

On Thu, Sep 22, 2016 at 01:33:59PM -0500, Eric W. Biederman wrote:
> 
> ebiederm-aS9lmoZGLiVWk0Htik3J/w@public.gmane.org (Eric W. Biederman) writes:
> 
> > Greg please apply the following patches to tty-next.  If tty-next is not
> > the proper tree please let me know and I will take these patches through
> > my user namespace tree.
> 
> I have not heard anything so I am taking these patches through my tree.

Ugh, sorry about that, they missed my filters.  I'll be glad to queue
them up today, my fault.

greg k-h