[PATCH 0/5][v2] Simplify devpts code

[PATCH 0/5][v2] Simplify devpts code

[Sukadev Bhattiprolu]

This patchset tries to address Christoph Hellwig's review comments on the
support for multiple-instances in devpts.  http://lkml.org/lkml/2009/1/3/84
It breaks up the patch into smaller pieces for (hopefully)  easier review.

IIUC, the key observation was that most of do_remount_sb() (MS_RDONLY flag
and shrink_dcache() does not apply to devpts and only the parsing of
options does. Moving the parsing code into devpts enables us to parse
the options just once and vastly simplifies the code.

This patchset does not change any functionality/behavior. But it does
depend on following two related patches.

	http://lkml.org/lkml/2009/1/29/10
	http://lkml.org/lkml/2009/1/29/11

Patches in this set:

	[PATCH 1/5] Unroll essentials of do_remount_sb() into devpts
	[PATCH 2/5] Parse mount options just once and copy them to super block
	[PATCH 3/5] Move common mknod_ptmx() calls into caller
	[PATCH 4/5] Remove get_init_pts_sb()
	[PATCH 5/5] Merge code for single and multiple-instance mounts

Changelog[v3]:
	- (Serge Hallyn): Remove unnecessary printks

Changelog[v2]:
	- (Christoph Hellwig) Patch 2/5 remove '//check' comment. Initialized
	  object using memset().
	- (Christoph Hellwig) - Patch 5/5; Removed the do_pts_mount() function
	   and moved that code into devpts_get_sb()

TODO: (Independent patch set)

	- Merge CONFIG_DEVPTS_MULTIPLE_INSTANCES token with other container-
	  related tokens ? 

Re: [PATCH 0/5][v2] Simplify devpts code

[Sukadev Bhattiprolu]

From: Sukadev Bhattiprolu <sukadev@linux.vnet.ibm.com>
Subject: [v3][PATCH 1/5] Unroll essentials of do_remount_sb() into devpts

On remount, devpts fs only needs to parse the mount options. Users cannot
directly create/dirty files in /dev/pts so the MS_RDONLY flag and
shrinking the dcache does not really apply to devpts.

So effectively on remount, devpts only parses the mount options and updates
these options in its super block. As such, we could replace do_remount_sb()
call with a direct parse_mount_options().

Doing so enables subsequent patches to avoid parsing the mount options twice
and simplify the code.

Signed-off-by: Sukadev Bhattiprolu <sukadev@linux.vnet.ibm.com>
Acked-by: Serge Hallyn <serue@us.ibm.com>
---
 fs/devpts/inode.c |    7 ++++++-
 1 files changed, 6 insertions(+), 1 deletions(-)

diff --git a/fs/devpts/inode.c b/fs/devpts/inode.c
index ad186b4..de15e73 100644
--- a/fs/devpts/inode.c
+++ b/fs/devpts/inode.c
@@ -442,6 +442,8 @@ static int get_init_pts_sb(struct file_system_type *fs_type, int flags,
 		void *data, struct vfsmount *mnt)
 {
 	struct super_block *s;
+	struct pts_mount_opts *opts;
+	struct pts_fs_info *fsi;
 	int error;
 
 	s = sget(fs_type, compare_init_pts_sb, set_anon_super, NULL);
@@ -458,7 +460,10 @@ static int get_init_pts_sb(struct file_system_type *fs_type, int flags,
 		}
 		s->s_flags |= MS_ACTIVE;
 	}
-	do_remount_sb(s, flags, data, 0);
+	fsi = DEVPTS_SB(s);
+	opts = &fsi->mount_opts;
+	parse_mount_options(data, PARSE_REMOUNT, opts);
+
 	simple_set_mnt(mnt, s);
 	return 0;
 }
-- 
1.5.2.5

Re: [PATCH 0/5][v2] Simplify devpts code

[Sukadev Bhattiprolu]

Pls ignore this mail. I have resent patch 1/5 with correct subject line.



Sukadev Bhattiprolu [sukadev@linux.vnet.ibm.com] wrote:
| From: Sukadev Bhattiprolu <sukadev@linux.vnet.ibm.com>
| Subject: [v3][PATCH 1/5] Unroll essentials of do_remount_sb() into devpts
| 
| On remount, devpts fs only needs to parse the mount options. Users cannot
| directly create/dirty files in /dev/pts so the MS_RDONLY flag and
| shrinking the dcache does not really apply to devpts.
| 
| So effectively on remount, devpts only parses the mount options and updates
| these options in its super block. As such, we could replace do_remount_sb()
| call with a direct parse_mount_options().
| 
| Doing so enables subsequent patches to avoid parsing the mount options twice
| and simplify the code.
| 
| Signed-off-by: Sukadev Bhattiprolu <sukadev@linux.vnet.ibm.com>
| Acked-by: Serge Hallyn <serue@us.ibm.com>
| ---
|  fs/devpts/inode.c |    7 ++++++-
|  1 files changed, 6 insertions(+), 1 deletions(-)
| 
| diff --git a/fs/devpts/inode.c b/fs/devpts/inode.c
| index ad186b4..de15e73 100644
| --- a/fs/devpts/inode.c
| +++ b/fs/devpts/inode.c
| @@ -442,6 +442,8 @@ static int get_init_pts_sb(struct file_system_type *fs_type, int flags,
|  		void *data, struct vfsmount *mnt)
|  {
|  	struct super_block *s;
| +	struct pts_mount_opts *opts;
| +	struct pts_fs_info *fsi;
|  	int error;
| 
|  	s = sget(fs_type, compare_init_pts_sb, set_anon_super, NULL);
| @@ -458,7 +460,10 @@ static int get_init_pts_sb(struct file_system_type *fs_type, int flags,
|  		}
|  		s->s_flags |= MS_ACTIVE;
|  	}
| -	do_remount_sb(s, flags, data, 0);
| +	fsi = DEVPTS_SB(s);
| +	opts = &fsi->mount_opts;
| +	parse_mount_options(data, PARSE_REMOUNT, opts);
| +
|  	simple_set_mnt(mnt, s);
|  	return 0;
|  }
| -- 
| 1.5.2.5

[v3][PATCH 2/5] Parse mount options just once and copy them to super block

[Sukadev Bhattiprolu]

From: Sukadev Bhattiprolu <sukadev@linux.vnet.ibm.com>
Subject: [v3][PATCH 2/5] Parse mount options just once and copy them to super block

Since all the mount option parsing is done in devpts, we could do it
just once and pass it around in devpts functions and eventually store
it in the super block.

Signed-off-by: Sukadev Bhattiprolu <sukadev@linux.vnet.ibm.com>
---
 fs/devpts/inode.c |  100 ++++++++++++-----------------------------------------
 1 files changed, 22 insertions(+), 78 deletions(-)

diff --git a/fs/devpts/inode.c b/fs/devpts/inode.c
index de15e73..9b24de8 100644
--- a/fs/devpts/inode.c
+++ b/fs/devpts/inode.c
@@ -325,49 +325,14 @@ static int compare_init_pts_sb(struct super_block *s, void *p)
 }
 
 /*
- * Safely parse the mount options in @data and update @opts.
- *
- * devpts ends up parsing options two times during mount, due to the
- * two modes of operation it supports. The first parse occurs in
- * devpts_get_sb() when determining the mode (single-instance or
- * multi-instance mode). The second parse happens in devpts_remount()
- * or new_pts_mount() depending on the mode.
- *
- * Parsing of options modifies the @data making subsequent parsing
- * incorrect. So make a local copy of @data and parse it.
- *
- * Return: 0 On success, -errno on error
- */
-static int safe_parse_mount_options(void *data, struct pts_mount_opts *opts)
-{
-	int rc;
-	void *datacp;
-
-	if (!data)
-		return 0;
-
-	/* Use kstrdup() ?  */
-	datacp = kmalloc(PAGE_SIZE, GFP_KERNEL);
-	if (!datacp)
-		return -ENOMEM;
-
-	memcpy(datacp, data, PAGE_SIZE);
-	rc = parse_mount_options((char *)datacp, PARSE_MOUNT, opts);
-	kfree(datacp);
-
-	return rc;
-}
-
-/*
  * Mount a new (private) instance of devpts.  PTYs created in this
  * instance are independent of the PTYs in other devpts instances.
  */
 static int new_pts_mount(struct file_system_type *fs_type, int flags,
-		void *data, struct vfsmount *mnt)
+		void *data, struct pts_mount_opts *opts, struct vfsmount *mnt)
 {
 	int err;
 	struct pts_fs_info *fsi;
-	struct pts_mount_opts *opts;
 
 	printk(KERN_NOTICE "devpts: newinstance mount\n");
 
@@ -376,11 +341,7 @@ static int new_pts_mount(struct file_system_type *fs_type, int flags,
 		return err;
 
 	fsi = DEVPTS_SB(mnt->mnt_sb);
-	opts = &fsi->mount_opts;
-
-	err = parse_mount_options(data, PARSE_MOUNT, opts);
-	if (err)
-		goto fail;
+	memcpy(&fsi->mount_opts, opts, sizeof(opts));
 
 	err = mknod_ptmx(mnt->mnt_sb);
 	if (err)
@@ -396,28 +357,6 @@ fail:
 }
 
 /*
- * Check if 'newinstance' mount option was specified in @data.
- *
- * Return: -errno  	on error (eg: invalid mount options specified)
- * 	 : 1 		if 'newinstance' mount option was specified
- * 	 : 0 		if 'newinstance' mount option was NOT specified
- */
-static int is_new_instance_mount(void *data)
-{
-	int rc;
-	struct pts_mount_opts opts;
-
-	if (!data)
-		return 0;
-
-	rc = safe_parse_mount_options(data, &opts);
-	if (!rc)
-		rc = opts.newinstance;
-
-	return rc;
-}
-
-/*
  * get_init_pts_sb()
  *
  *     This interface is needed to support multiple namespace semantics in
@@ -439,10 +378,9 @@ static int is_new_instance_mount(void *data)
  *     presence of the private namespace (i.e 'newinstance') super-blocks.
  */
 static int get_init_pts_sb(struct file_system_type *fs_type, int flags,
-		void *data, struct vfsmount *mnt)
+		void *data, struct pts_mount_opts *opts, struct vfsmount *mnt)
 {
 	struct super_block *s;
-	struct pts_mount_opts *opts;
 	struct pts_fs_info *fsi;
 	int error;
 
@@ -460,11 +398,12 @@ static int get_init_pts_sb(struct file_system_type *fs_type, int flags,
 		}
 		s->s_flags |= MS_ACTIVE;
 	}
-	fsi = DEVPTS_SB(s);
-	opts = &fsi->mount_opts;
-	parse_mount_options(data, PARSE_REMOUNT, opts);
 
 	simple_set_mnt(mnt, s);
+
+	fsi = DEVPTS_SB(mnt->mnt_sb);
+	memcpy(&fsi->mount_opts, opts, sizeof(opts));
+
 	return 0;
 }
 
@@ -474,11 +413,11 @@ static int get_init_pts_sb(struct file_system_type *fs_type, int flags,
  * kernel still allows multiple-instances.
  */
 static int init_pts_mount(struct file_system_type *fs_type, int flags,
-		void *data, struct vfsmount *mnt)
+		void *data, struct pts_mount_opts *opts, struct vfsmount *mnt)
 {
 	int err;
 
-	err = get_init_pts_sb(fs_type, flags, data, mnt);
+	err = get_init_pts_sb(fs_type, flags, data, opts, mnt);
 	if (err)
 		return err;
 
@@ -495,17 +434,22 @@ static int init_pts_mount(struct file_system_type *fs_type, int flags,
 static int devpts_get_sb(struct file_system_type *fs_type,
 	int flags, const char *dev_name, void *data, struct vfsmount *mnt)
 {
-	int new;
-
-	new = is_new_instance_mount(data);
-	if (new < 0)
-		return new;
+	int error;
+	struct pts_mount_opts opts;
 
-	if (new)
-		return new_pts_mount(fs_type, flags, data, mnt);
+	memset(&opts, 0, sizeof(opts)); 
+	if (data) {
+		error = parse_mount_options(data, PARSE_MOUNT, &opts);
+		if (error)
+			return error;
+	}
 
-	return init_pts_mount(fs_type, flags, data, mnt);
+	if (opts.newinstance)
+		return new_pts_mount(fs_type, flags, data, &opts, mnt);
+	else
+		return init_pts_mount(fs_type, flags, data, &opts, mnt);
 }
+
 #else
 /*
  * This supports only the legacy single-instance semantics (no
-- 
1.5.2.5

[v3][PATCH 3/5] Move common mknod_ptmx() calls into caller

[Sukadev Bhattiprolu]

From: Sukadev Bhattiprolu <sukadev@linux.vnet.ibm.com>
Subject: [v3][PATCH 3/5] Move common mknod_ptmx() calls into caller

We create 'ptmx' node in both single-instance and multiple-instance
mounts. So devpts_get_sb() can call mknod_ptmx() once rather than
have both modes calling mknod_ptmx() separately.

Signed-off-by: Sukadev Bhattiprolu <sukadev@linux.vnet.ibm.com>
Acked-by: Serge Hallyn <serue@us.ibm.com>
---
 fs/devpts/inode.c |   36 +++++++++++++++++-------------------
 1 files changed, 17 insertions(+), 19 deletions(-)

diff --git a/fs/devpts/inode.c b/fs/devpts/inode.c
index 9b24de8..f7e887c 100644
--- a/fs/devpts/inode.c
+++ b/fs/devpts/inode.c
@@ -343,17 +343,7 @@ static int new_pts_mount(struct file_system_type *fs_type, int flags,
 	fsi = DEVPTS_SB(mnt->mnt_sb);
 	memcpy(&fsi->mount_opts, opts, sizeof(opts));
 
-	err = mknod_ptmx(mnt->mnt_sb);
-	if (err)
-		goto fail;
-
 	return 0;
-
-fail:
-	dput(mnt->mnt_sb->s_root);
-	up_write(&mnt->mnt_sb->s_umount);
-	deactivate_super(mnt->mnt_sb);
-	return err;
 }
 
 /*
@@ -421,13 +411,6 @@ static int init_pts_mount(struct file_system_type *fs_type, int flags,
 	if (err)
 		return err;
 
-	err = mknod_ptmx(mnt->mnt_sb);
-	if (err) {
-		dput(mnt->mnt_sb->s_root);
-		up_write(&mnt->mnt_sb->s_umount);
-		deactivate_super(mnt->mnt_sb);
-	}
-
 	return err;
 }
 
@@ -445,9 +428,24 @@ static int devpts_get_sb(struct file_system_type *fs_type,
 	}
 
 	if (opts.newinstance)
-		return new_pts_mount(fs_type, flags, data, &opts, mnt);
+		error = new_pts_mount(fs_type, flags, data, &opts, mnt);
 	else
-		return init_pts_mount(fs_type, flags, data, &opts, mnt);
+		error = init_pts_mount(fs_type, flags, data, &opts, mnt);
+
+	if (error)
+		return error;
+
+	error = mknod_ptmx(mnt->mnt_sb);
+	if (error)
+		goto out_dput;
+
+	return 0;
+
+out_dput:
+	dput(mnt->mnt_sb->s_root);
+	up_write(&mnt->mnt_sb->s_umount);
+	deactivate_super(mnt->mnt_sb);
+	return error;
 }
 
 #else
-- 
1.5.2.5

[v3][PATCH 4/5] Remove get_init_pts_sb()

[Sukadev Bhattiprolu]

From: Sukadev Bhattiprolu <sukadev@linux.vnet.ibm.com>
Subject: [v3][PATCH 4/5] Remove get_init_pts_sb()

With mknod_ptmx() moved to devpts_get_sb(), init_pts_mount() becomes
a wrapper around get_init_pts_sb(). Remove get_init_pts_sb() and
fold code into init_pts_mount().

Signed-off-by: Sukadev Bhattiprolu <sukadev@linux.vnet.ibm.com>
Acked-by: Serge Hallyn <serue@us.ibm.com>
---
 fs/devpts/inode.c |   25 ++++++-------------------
 1 files changed, 6 insertions(+), 19 deletions(-)

diff --git a/fs/devpts/inode.c b/fs/devpts/inode.c
index f7e887c..c821c9a 100644
--- a/fs/devpts/inode.c
+++ b/fs/devpts/inode.c
@@ -347,7 +347,11 @@ static int new_pts_mount(struct file_system_type *fs_type, int flags,
 }
 
 /*
- * get_init_pts_sb()
+ * init_pts_mount()
+ *
+ *     Mount or remount the initial kernel mount of devpts. This type of
+ *     mount maintains the legacy, single-instance semantics, while the
+ *     kernel still allows multiple-instances.
  *
  *     This interface is needed to support multiple namespace semantics in
  *     devpts while preserving backward compatibility of the current 'single-
@@ -367,7 +371,7 @@ static int new_pts_mount(struct file_system_type *fs_type, int flags,
  *     consistently selects the 'single-namespace' superblock even in the
  *     presence of the private namespace (i.e 'newinstance') super-blocks.
  */
-static int get_init_pts_sb(struct file_system_type *fs_type, int flags,
+static int init_pts_mount(struct file_system_type *fs_type, int flags,
 		void *data, struct pts_mount_opts *opts, struct vfsmount *mnt)
 {
 	struct super_block *s;
@@ -397,23 +401,6 @@ static int get_init_pts_sb(struct file_system_type *fs_type, int flags,
 	return 0;
 }
 
-/*
- * Mount or remount the initial kernel mount of devpts. This type of
- * mount maintains the legacy, single-instance semantics, while the
- * kernel still allows multiple-instances.
- */
-static int init_pts_mount(struct file_system_type *fs_type, int flags,
-		void *data, struct pts_mount_opts *opts, struct vfsmount *mnt)
-{
-	int err;
-
-	err = get_init_pts_sb(fs_type, flags, data, opts, mnt);
-	if (err)
-		return err;
-
-	return err;
-}
-
 static int devpts_get_sb(struct file_system_type *fs_type,
 	int flags, const char *dev_name, void *data, struct vfsmount *mnt)
 {
-- 
1.5.2.5

[v3][PATCH 5/5] Merge code for single and multiple-instance mounts

[Sukadev Bhattiprolu]

From: Sukadev Bhattiprolu <sukadev@linux.vnet.ibm.com>
Subject: [v3][PATCH 5/5] Merge code for single and multiple-instance mounts

new_pts_mount() (including the get_sb_nodev()), shares a lot of code
with init_pts_mount(). The only difference between them is the 'test-super'
function passed into sget().

Move all common code into devpts_get_sb() and remove the new_pts_mount() and
init_pts_mount() functions,

Changelog[v3]:
	[Serge Hallyn]: Remove unnecessary printk()s
Changelog[v2]:
	(Christoph Hellwig): Merge code in 'do_pts_mount()' into devpts_get_sb()

Signed-off-by: Sukadev Bhattiprolu <sukadev@linux.vnet.ibm.com>
Acked-by: Serge Hallyn <serue@us.ibm.com>
Tested-by: Serge Hallyn <serue@us.ibm.com>
---
 fs/devpts/inode.c |  115 ++++++++++++++++++----------------------------------
 1 files changed, 40 insertions(+), 75 deletions(-)

diff --git a/fs/devpts/inode.c b/fs/devpts/inode.c
index c821c9a..c25fb34 100644
--- a/fs/devpts/inode.c
+++ b/fs/devpts/inode.c
@@ -325,87 +325,38 @@ static int compare_init_pts_sb(struct super_block *s, void *p)
 }
 
 /*
- * Mount a new (private) instance of devpts.  PTYs created in this
- * instance are independent of the PTYs in other devpts instances.
- */
-static int new_pts_mount(struct file_system_type *fs_type, int flags,
-		void *data, struct pts_mount_opts *opts, struct vfsmount *mnt)
-{
-	int err;
-	struct pts_fs_info *fsi;
-
-	printk(KERN_NOTICE "devpts: newinstance mount\n");
-
-	err = get_sb_nodev(fs_type, flags, data, devpts_fill_super, mnt);
-	if (err)
-		return err;
-
-	fsi = DEVPTS_SB(mnt->mnt_sb);
-	memcpy(&fsi->mount_opts, opts, sizeof(opts));
-
-	return 0;
-}
-
-/*
- * init_pts_mount()
+ * devpts_get_sb()
+ *
+ *     If the '-o newinstance' mount option was specified, mount a new
+ *     (private) instance of devpts.  PTYs created in this instance are
+ *     independent of the PTYs in other devpts instances.
  *
- *     Mount or remount the initial kernel mount of devpts. This type of
- *     mount maintains the legacy, single-instance semantics, while the
- *     kernel still allows multiple-instances.
+ *     If the '-o newinstance' option was not specified, mount/remount the
+ *     initial kernel mount of devpts.  This type of mount gives the
+ *     legacy, single-instance semantics.
  *
- *     This interface is needed to support multiple namespace semantics in
- *     devpts while preserving backward compatibility of the current 'single-
- *     namespace' semantics. i.e all mounts of devpts without the 'newinstance'
- *     mount option should bind to the initial kernel mount, like
- *     get_sb_single().
+ *     The 'newinstance' option is needed to support multiple namespace
+ *     semantics in devpts while preserving backward compatibility of the
+ *     current 'single-namespace' semantics. i.e all mounts of devpts
+ *     without the 'newinstance' mount option should bind to the initial
+ *     kernel mount, like get_sb_single().
  *
- *     Mounts with 'newinstance' option create a new private namespace.
+ *     Mounts with 'newinstance' option create a new, private namespace.
  *
- *     But for single-mount semantics, devpts cannot use get_sb_single(),
+ *     NOTE:
+ *
+ *     For single-mount semantics, devpts cannot use get_sb_single(),
  *     because get_sb_single()/sget() find and use the super-block from
  *     the most recent mount of devpts. But that recent mount may be a
  *     'newinstance' mount and get_sb_single() would pick the newinstance
  *     super-block instead of the initial super-block.
- *
- *     This interface is identical to get_sb_single() except that it
- *     consistently selects the 'single-namespace' superblock even in the
- *     presence of the private namespace (i.e 'newinstance') super-blocks.
  */
-static int init_pts_mount(struct file_system_type *fs_type, int flags,
-		void *data, struct pts_mount_opts *opts, struct vfsmount *mnt)
-{
-	struct super_block *s;
-	struct pts_fs_info *fsi;
-	int error;
-
-	s = sget(fs_type, compare_init_pts_sb, set_anon_super, NULL);
-	if (IS_ERR(s))
-		return PTR_ERR(s);
-
-	if (!s->s_root) {
-		s->s_flags = flags;
-		error = devpts_fill_super(s, data, flags & MS_SILENT ? 1 : 0);
-		if (error) {
-			up_write(&s->s_umount);
-			deactivate_super(s);
-			return error;
-		}
-		s->s_flags |= MS_ACTIVE;
-	}
-
-	simple_set_mnt(mnt, s);
-
-	fsi = DEVPTS_SB(mnt->mnt_sb);
-	memcpy(&fsi->mount_opts, opts, sizeof(opts));
-
-	return 0;
-}
-
 static int devpts_get_sb(struct file_system_type *fs_type,
 	int flags, const char *dev_name, void *data, struct vfsmount *mnt)
 {
 	int error;
 	struct pts_mount_opts opts;
+	struct super_block *s;
 
 	memset(&opts, 0, sizeof(opts)); 
 	if (data) {
@@ -415,23 +366,37 @@ static int devpts_get_sb(struct file_system_type *fs_type,
 	}
 
 	if (opts.newinstance)
-		error = new_pts_mount(fs_type, flags, data, &opts, mnt);
+		s = sget(fs_type, NULL, set_anon_super, NULL);
 	else
-		error = init_pts_mount(fs_type, flags, data, &opts, mnt);
+		s = sget(fs_type, compare_init_pts_sb, set_anon_super, NULL);
 
-	if (error)
-		return error;
+	if (IS_ERR(s))
+		return PTR_ERR(s);
 
-	error = mknod_ptmx(mnt->mnt_sb);
+	if (!s->s_root) {
+		s->s_flags = flags;
+		error = devpts_fill_super(s, data, flags & MS_SILENT ? 1 : 0);
+		if (error)
+			goto out_undo_sget;
+		s->s_flags |= MS_ACTIVE;
+	}
+
+	simple_set_mnt(mnt, s);
+
+	memcpy(&(DEVPTS_SB(s))->mount_opts, &opts, sizeof(opts));
+
+	error = mknod_ptmx(s);
 	if (error)
 		goto out_dput;
 
 	return 0;
 
 out_dput:
-	dput(mnt->mnt_sb->s_root);
-	up_write(&mnt->mnt_sb->s_umount);
-	deactivate_super(mnt->mnt_sb);
+	dput(s->s_root);
+
+out_undo_sget:
+	up_write(&s->s_umount);
+	deactivate_super(s);
 	return error;
 }
 
-- 
1.5.2.5

Re: [v3][PATCH 5/5] Merge code for single and multiple-instance mounts

[Eric Paris]

On Sat, Mar 7, 2009 at 2:12 PM, Sukadev Bhattiprolu
<sukadev@linux.vnet.ibm.com> wrote:
>
> From: Sukadev Bhattiprolu <sukadev@linux.vnet.ibm.com>
> Subject: [v3][PATCH 5/5] Merge code for single and multiple-instance mounts

I just tried to load the linux-next kernel on F11 and ran into a
problem.  X started, I could log in, I could start programs like
firefox and evolution, but not gnome-terminal.  It would just flash
and disappear.  Running xterm resulted in a window, that I could type
in, but it wasn't a shell.  It didn't do anything.

I switched to vt2 set the display to my X session and tried to run
xterm.  It said something about a permission being denied, so I
decided to strace it.  I saw EACCESS returning from calls dealing with
/dev/pts/0.  This lead me to git bisect start fs/devpts from the
latest in linux-next as bad and 2.6.29 as good.  Couple interations
later and I find that this commit (1bd7903560f1f7) breaks
gnome-terminal xterm!

So I have no idea why and haven't tried to figure it out (I'll try to
poke it more tonight), but this is the commit that git bisect blames!


> new_pts_mount() (including the get_sb_nodev()), shares a lot of code
> with init_pts_mount(). The only difference between them is the 'test-super'
> function passed into sget().
>
> Move all common code into devpts_get_sb() and remove the new_pts_mount() and
> init_pts_mount() functions,
>
> Changelog[v3]:
>        [Serge Hallyn]: Remove unnecessary printk()s
> Changelog[v2]:
>        (Christoph Hellwig): Merge code in 'do_pts_mount()' into devpts_get_sb()
>
> Signed-off-by: Sukadev Bhattiprolu <sukadev@linux.vnet.ibm.com>
> Acked-by: Serge Hallyn <serue@us.ibm.com>
> Tested-by: Serge Hallyn <serue@us.ibm.com>
> ---
>  fs/devpts/inode.c |  115 ++++++++++++++++++----------------------------------
>  1 files changed, 40 insertions(+), 75 deletions(-)
>
> diff --git a/fs/devpts/inode.c b/fs/devpts/inode.c
> index c821c9a..c25fb34 100644
> --- a/fs/devpts/inode.c
> +++ b/fs/devpts/inode.c
> @@ -325,87 +325,38 @@ static int compare_init_pts_sb(struct super_block *s, void *p)
>  }
>
>  /*
> - * Mount a new (private) instance of devpts.  PTYs created in this
> - * instance are independent of the PTYs in other devpts instances.
> - */
> -static int new_pts_mount(struct file_system_type *fs_type, int flags,
> -               void *data, struct pts_mount_opts *opts, struct vfsmount *mnt)
> -{
> -       int err;
> -       struct pts_fs_info *fsi;
> -
> -       printk(KERN_NOTICE "devpts: newinstance mount\n");
> -
> -       err = get_sb_nodev(fs_type, flags, data, devpts_fill_super, mnt);
> -       if (err)
> -               return err;
> -
> -       fsi = DEVPTS_SB(mnt->mnt_sb);
> -       memcpy(&fsi->mount_opts, opts, sizeof(opts));
> -
> -       return 0;
> -}
> -
> -/*
> - * init_pts_mount()
> + * devpts_get_sb()
> + *
> + *     If the '-o newinstance' mount option was specified, mount a new
> + *     (private) instance of devpts.  PTYs created in this instance are
> + *     independent of the PTYs in other devpts instances.
>  *
> - *     Mount or remount the initial kernel mount of devpts. This type of
> - *     mount maintains the legacy, single-instance semantics, while the
> - *     kernel still allows multiple-instances.
> + *     If the '-o newinstance' option was not specified, mount/remount the
> + *     initial kernel mount of devpts.  This type of mount gives the
> + *     legacy, single-instance semantics.
>  *
> - *     This interface is needed to support multiple namespace semantics in
> - *     devpts while preserving backward compatibility of the current 'single-
> - *     namespace' semantics. i.e all mounts of devpts without the 'newinstance'
> - *     mount option should bind to the initial kernel mount, like
> - *     get_sb_single().
> + *     The 'newinstance' option is needed to support multiple namespace
> + *     semantics in devpts while preserving backward compatibility of the
> + *     current 'single-namespace' semantics. i.e all mounts of devpts
> + *     without the 'newinstance' mount option should bind to the initial
> + *     kernel mount, like get_sb_single().
>  *
> - *     Mounts with 'newinstance' option create a new private namespace.
> + *     Mounts with 'newinstance' option create a new, private namespace.
>  *
> - *     But for single-mount semantics, devpts cannot use get_sb_single(),
> + *     NOTE:
> + *
> + *     For single-mount semantics, devpts cannot use get_sb_single(),
>  *     because get_sb_single()/sget() find and use the super-block from
>  *     the most recent mount of devpts. But that recent mount may be a
>  *     'newinstance' mount and get_sb_single() would pick the newinstance
>  *     super-block instead of the initial super-block.
> - *
> - *     This interface is identical to get_sb_single() except that it
> - *     consistently selects the 'single-namespace' superblock even in the
> - *     presence of the private namespace (i.e 'newinstance') super-blocks.
>  */
> -static int init_pts_mount(struct file_system_type *fs_type, int flags,
> -               void *data, struct pts_mount_opts *opts, struct vfsmount *mnt)
> -{
> -       struct super_block *s;
> -       struct pts_fs_info *fsi;
> -       int error;
> -
> -       s = sget(fs_type, compare_init_pts_sb, set_anon_super, NULL);
> -       if (IS_ERR(s))
> -               return PTR_ERR(s);
> -
> -       if (!s->s_root) {
> -               s->s_flags = flags;
> -               error = devpts_fill_super(s, data, flags & MS_SILENT ? 1 : 0);
> -               if (error) {
> -                       up_write(&s->s_umount);
> -                       deactivate_super(s);
> -                       return error;
> -               }
> -               s->s_flags |= MS_ACTIVE;
> -       }
> -
> -       simple_set_mnt(mnt, s);
> -
> -       fsi = DEVPTS_SB(mnt->mnt_sb);
> -       memcpy(&fsi->mount_opts, opts, sizeof(opts));
> -
> -       return 0;
> -}
> -
>  static int devpts_get_sb(struct file_system_type *fs_type,
>        int flags, const char *dev_name, void *data, struct vfsmount *mnt)
>  {
>        int error;
>        struct pts_mount_opts opts;
> +       struct super_block *s;
>
>        memset(&opts, 0, sizeof(opts));
>        if (data) {
> @@ -415,23 +366,37 @@ static int devpts_get_sb(struct file_system_type *fs_type,
>        }
>
>        if (opts.newinstance)
> -               error = new_pts_mount(fs_type, flags, data, &opts, mnt);
> +               s = sget(fs_type, NULL, set_anon_super, NULL);
>        else
> -               error = init_pts_mount(fs_type, flags, data, &opts, mnt);
> +               s = sget(fs_type, compare_init_pts_sb, set_anon_super, NULL);
>
> -       if (error)
> -               return error;
> +       if (IS_ERR(s))
> +               return PTR_ERR(s);
>
> -       error = mknod_ptmx(mnt->mnt_sb);
> +       if (!s->s_root) {
> +               s->s_flags = flags;
> +               error = devpts_fill_super(s, data, flags & MS_SILENT ? 1 : 0);
> +               if (error)
> +                       goto out_undo_sget;
> +               s->s_flags |= MS_ACTIVE;
> +       }
> +
> +       simple_set_mnt(mnt, s);
> +
> +       memcpy(&(DEVPTS_SB(s))->mount_opts, &opts, sizeof(opts));
> +
> +       error = mknod_ptmx(s);
>        if (error)
>                goto out_dput;
>
>        return 0;
>
>  out_dput:
> -       dput(mnt->mnt_sb->s_root);
> -       up_write(&mnt->mnt_sb->s_umount);
> -       deactivate_super(mnt->mnt_sb);
> +       dput(s->s_root);
> +
> +out_undo_sget:
> +       up_write(&s->s_umount);
> +       deactivate_super(s);
>        return error;
>  }
>
> --
> 1.5.2.5
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
>

Re: [v3][PATCH 5/5] Merge code for single and multiple-instance mounts

[Sukadev Bhattiprolu]

Eric Paris [eparis@parisplace.org] wrote:
| On Sat, Mar 7, 2009 at 2:12 PM, Sukadev Bhattiprolu
| <sukadev@linux.vnet.ibm.com> wrote:
| >
| > From: Sukadev Bhattiprolu <sukadev@linux.vnet.ibm.com>
| > Subject: [v3][PATCH 5/5] Merge code for single and multiple-instance mounts
| 
| I just tried to load the linux-next kernel on F11 and ran into a
| problem.  X started, I could log in, I could start programs like
| firefox and evolution, but not gnome-terminal.  It would just flash
| and disappear.  Running xterm resulted in a window, that I could type
| in, but it wasn't a shell.  It didn't do anything.
| 
| I switched to vt2 set the display to my X session and tried to run
| xterm.  It said something about a permission being denied, so I
| decided to strace it.  I saw EACCESS returning from calls dealing with
| /dev/pts/0.  This lead me to git bisect start fs/devpts from the
| latest in linux-next as bad and 2.6.29 as good.  Couple interations
| later and I find that this commit (1bd7903560f1f7) breaks
| gnome-terminal xterm!

Interesting :-) -EACCESS makes me suspect that maybe 'mode' 'uid' or 'gid'
mount options are wrong. Of course we would still need to understand if/
why this patch changes the settings.

Can you paste the output of following commands: (both in success and failure
cases).

	$ grep devpts /proc/mounts

	$ ls -al /dev/pts

	$ stat /dev/ptmx

Also, is CONFIG_DEVPTS_MULTIPLE_INSTANCES set in your .config ?

With this patch, does gnome-terminal run when logged in as root ?

Thanks,

Sukadev

Re: [v3][PATCH 5/5] Merge code for single and multiple-instance mounts

[Eric Paris]

On Thu, 2009-05-07 at 14:24 -0700, Sukadev Bhattiprolu wrote: 
> Eric Paris [eparis@parisplace.org] wrote:
> | On Sat, Mar 7, 2009 at 2:12 PM, Sukadev Bhattiprolu
> | <sukadev@linux.vnet.ibm.com> wrote:
> | >
> | > From: Sukadev Bhattiprolu <sukadev@linux.vnet.ibm.com>
> | > Subject: [v3][PATCH 5/5] Merge code for single and multiple-instance mounts
> | 
> | I just tried to load the linux-next kernel on F11 and ran into a
> | problem.  X started, I could log in, I could start programs like
> | firefox and evolution, but not gnome-terminal.  It would just flash
> | and disappear.  Running xterm resulted in a window, that I could type
> | in, but it wasn't a shell.  It didn't do anything.
> | 
> | I switched to vt2 set the display to my X session and tried to run
> | xterm.  It said something about a permission being denied, so I
> | decided to strace it.  I saw EACCESS returning from calls dealing with
> | /dev/pts/0.  This lead me to git bisect start fs/devpts from the
> | latest in linux-next as bad and 2.6.29 as good.  Couple interations
> | later and I find that this commit (1bd7903560f1f7) breaks
> | gnome-terminal xterm!
> 
> Interesting :-) -EACCESS makes me suspect that maybe 'mode' 'uid' or 'gid'
> mount options are wrong. Of course we would still need to understand if/
> why this patch changes the settings.
> 
> Can you paste the output of following commands: (both in success and failure
> cases).
> 
> 	$ grep devpts /proc/mounts

Success: devpts /dev/pts devpts rw,relatime,mode=600,ptmxmode=000 0 0
Failure: devpts /dev/pts devpts rw,seclabel,relatime,mode=000,ptmxmode=000 0 0

> 	$ ls -al /dev/pts
Success:
[root@dhcp231-142 ~]# ls -al /dev/pts
total 0
drwxr-xr-x.  2 root  root       0 2009-05-07 16:04 .
drwxr-xr-x. 20 root  root    5300 2009-05-07 18:13 ..
crw--w----.  1 paris paris 136, 0 2009-05-07 18:17 0
crw--w----.  1 paris paris 136, 1 2009-05-07 16:07 1
crw--w----.  1 paris paris 136, 2 2009-05-07 18:13 2
crw--w----.  1 paris paris 136, 3 2009-05-07 18:17 3
c---------.  1 root  root    5, 2 2009-05-07 16:04 ptmx 

Failure:
[root@dhcp231-142 ~]# ls -al /dev/pts
total 0
drwxr-xr-x.  2 root  root       0 2009-05-07 18:22 .
drwxr-xr-x. 20 root  root    5460 2009-05-07 18:28 ..
c---------.  1 root  root    5, 2 2009-05-07 18:22 ptmx

> 	$ stat /dev/ptmx


Failure:
  File: `/dev/ptmx'
  Size: 0               Blocks: 0          IO Block: 4096   character special file
Device: eh/14d  Inode: 704         Links: 1     Device type: 5,2
Access: (0666/crw-rw-rw-)  Uid: (    0/    root)   Gid: (    5/     tty)
Access: 2009-05-07 18:23:20.695621624 -0400
Modify: 2009-05-07 18:22:25.460004885 -0400
Change: 2009-05-07 18:22:31.911129737 -0400

> 
> Also, is CONFIG_DEVPTS_MULTIPLE_INSTANCES set in your .config ?

CONFIG_DEVPTS_MULTIPLE_INSTANCES=y

> With this patch, does gnome-terminal run when logged in as root ?

gnome-terminal did not, but xterm did appear in my window as root.  I
assumed it was just some gnomism to not let me launch a root
gnome-terminal inside the user owned X session.


I unmounted /dev/pts and remounted it with mode=600 and it works now!

So something about this patch caused it to change from being mounted 600
to 000.....

Re: [v3][PATCH 5/5] Merge code for single and multiple-instancemounts

[Sukadev Bhattiprolu]

Eric Paris [eparis@redhat.com] wrote:
| On Thu, 2009-05-07 at 14:24 -0700, Sukadev Bhattiprolu wrote: 
| > Eric Paris [eparis@parisplace.org] wrote:
| > | On Sat, Mar 7, 2009 at 2:12 PM, Sukadev Bhattiprolu
| > | <sukadev@linux.vnet.ibm.com> wrote:
| > | >
| > | > From: Sukadev Bhattiprolu <sukadev@linux.vnet.ibm.com>
| > | > Subject: [v3][PATCH 5/5] Merge code for single and multiple-instance mounts
| > | 
| > | I just tried to load the linux-next kernel on F11 and ran into a
| > | problem.  X started, I could log in, I could start programs like
| > | firefox and evolution, but not gnome-terminal.  It would just flash
| > | and disappear.  Running xterm resulted in a window, that I could type
| > | in, but it wasn't a shell.  It didn't do anything.
| > | 
| > | I switched to vt2 set the display to my X session and tried to run
| > | xterm.  It said something about a permission being denied, so I
| > | decided to strace it.  I saw EACCESS returning from calls dealing with
| > | /dev/pts/0.  This lead me to git bisect start fs/devpts from the
| > | latest in linux-next as bad and 2.6.29 as good.  Couple interations
| > | later and I find that this commit (1bd7903560f1f7) breaks
| > | gnome-terminal xterm!
| > 
| > Interesting :-) -EACCESS makes me suspect that maybe 'mode' 'uid' or 'gid'
| > mount options are wrong. Of course we would still need to understand if/
| > why this patch changes the settings.
| > 
| > Can you paste the output of following commands: (both in success and failure
| > cases).
| > 
| > 	$ grep devpts /proc/mounts
| 
| Success: devpts /dev/pts devpts rw,relatime,mode=600,ptmxmode=000 0 0
| Failure: devpts /dev/pts devpts rw,seclabel,relatime,mode=000,ptmxmode=000 0 0
| 
| > 	$ ls -al /dev/pts
| Success:
| [root@dhcp231-142 ~]# ls -al /dev/pts
| total 0
| drwxr-xr-x.  2 root  root       0 2009-05-07 16:04 .
| drwxr-xr-x. 20 root  root    5300 2009-05-07 18:13 ..
| crw--w----.  1 paris paris 136, 0 2009-05-07 18:17 0
| crw--w----.  1 paris paris 136, 1 2009-05-07 16:07 1
| crw--w----.  1 paris paris 136, 2 2009-05-07 18:13 2
| crw--w----.  1 paris paris 136, 3 2009-05-07 18:17 3
| c---------.  1 root  root    5, 2 2009-05-07 16:04 ptmx 
| 
| Failure:
| [root@dhcp231-142 ~]# ls -al /dev/pts
| total 0
| drwxr-xr-x.  2 root  root       0 2009-05-07 18:22 .
| drwxr-xr-x. 20 root  root    5460 2009-05-07 18:28 ..
| c---------.  1 root  root    5, 2 2009-05-07 18:22 ptmx
| 
| > 	$ stat /dev/ptmx
| 
| 
| Failure:
|   File: `/dev/ptmx'
|   Size: 0               Blocks: 0          IO Block: 4096   character special file
| Device: eh/14d  Inode: 704         Links: 1     Device type: 5,2
| Access: (0666/crw-rw-rw-)  Uid: (    0/    root)   Gid: (    5/     tty)
| Access: 2009-05-07 18:23:20.695621624 -0400
| Modify: 2009-05-07 18:22:25.460004885 -0400
| Change: 2009-05-07 18:22:31.911129737 -0400
| 
| > 
| > Also, is CONFIG_DEVPTS_MULTIPLE_INSTANCES set in your .config ?
| 
| CONFIG_DEVPTS_MULTIPLE_INSTANCES=y
| 
| > With this patch, does gnome-terminal run when logged in as root ?
| 
| gnome-terminal did not, but xterm did appear in my window as root.  I
| assumed it was just some gnomism to not let me launch a root
| gnome-terminal inside the user owned X session.
| 
| 
| I unmounted /dev/pts and remounted it with mode=600 and it works now!
| 
| So something about this patch caused it to change from being mounted 600
| to 000.....

Must be the

	memset(&opts, 0, sizeof(opts));

in devpts_get_sb() (first statement). It should probably set the options to
default values for cases where 'data' is NULL.

For now, can you try replacing the memset with:

        opts->mode    = DEVPTS_DEFAULT_MODE;
        opts->ptmxmode = DEVPTS_DEFAULT_PTMX_MODE;

Sukadev

Re: [v3][PATCH 5/5] Merge code for single andmultiple-instancemounts

[Sukadev Bhattiprolu]

| Must be the
| 
| 	memset(&opts, 0, sizeof(opts));
| 
| in devpts_get_sb() (first statement). It should probably set the options to
| default values for cases where 'data' is NULL.
| 
| For now, can you try replacing the memset with:

Er. I meant add following lines after the memset().

|         opts->mode    = DEVPTS_DEFAULT_MODE;
|         opts->ptmxmode = DEVPTS_DEFAULT_PTMX_MODE;
| 
| Sukadev

Re: [v3][PATCH 5/5] Merge code for single andmultiple-instancemounts

[Peter Staubach]

Sukadev Bhattiprolu wrote:
> | Must be the
> | 
> | 	memset(&opts, 0, sizeof(opts));
> | 
> | in devpts_get_sb() (first statement). It should probably set the options to
> | default values for cases where 'data' is NULL.
> | 
> | For now, can you try replacing the memset with:
>
> Er. I meant add following lines after the memset().
>
> |         opts->mode    = DEVPTS_DEFAULT_MODE;
> |         opts->ptmxmode = DEVPTS_DEFAULT_PTMX_MODE;
> | 
> | Sukadev

With the obvious syntactic changes to account for opts being a
struct as opposed to being a pointer to a struct, these two lines
do seem to make things better.

       ps

Re: [v3][PATCH 5/5] Merge code for single andmultiple-instancemounts

[Marc Dionne]

On 05/07/2009 07:21 PM, Sukadev Bhattiprolu wrote:
> Er. I meant add following lines after the memset().
>
> |         opts->mode    = DEVPTS_DEFAULT_MODE;
> |         opts->ptmxmode = DEVPTS_DEFAULT_PTMX_MODE;

Adding those two lines (with . instead of ->) does fix the issue for me, 
thanks.

| > Of course we would still need to understand if/
| > why this patch changes the settings.

That particular patch changed things because the original memcpy in 
new_pts_mount() did this:

memcpy(&fsi->mount_opts, opts, sizeof(opts));

where opts was a pointer, not a structure.  So only the first few bytes 
of the blank opts actually got copied over.  The patch moved this memcpy 
to devpts_get_sb() and in the process fixed the sizeof error.

Marc

Re: [v3][PATCH 5/5] Merge code for single andmultiple-instancemounts

[Sukadev Bhattiprolu]

Marc Dionne [marc.c.dionne@gmail.com] wrote:
> On 05/07/2009 07:21 PM, Sukadev Bhattiprolu wrote:
>> Er. I meant add following lines after the memset().
>>
>> |         opts->mode    = DEVPTS_DEFAULT_MODE;
>> |         opts->ptmxmode = DEVPTS_DEFAULT_PTMX_MODE;
>
> Adding those two lines (with . instead of ->) does fix the issue for me, 
> thanks.
>
> | > Of course we would still need to understand if/
> | > why this patch changes the settings.
>
> That particular patch changed things because the original memcpy in 
> new_pts_mount() did this:
>
> memcpy(&fsi->mount_opts, opts, sizeof(opts));
>
> where opts was a pointer, not a structure.  So only the first few bytes of 
> the blank opts actually got copied over.  The patch moved this memcpy to 
> devpts_get_sb() and in the process fixed the sizeof error.

Here is a cleaner fix - When user space (/etc/rc.sysinit on RHEL5) mounts
devpts, this problem might be masked since the mount system call could pass
a non-NULL 'data' parmeter to devpts_get_sb().

I tested this patch by directly calling the system call mount() with a NULL
data parameter. If you/Eric/Peter can confirm that this works for you, I will
send this patch to Andrew.

Thanks,
---
>From 2f7746f4df78ff57125c4714f0cd64e739ccf804 Mon Sep 17 00:00:00 2001
From: Sukadev Bhattiprolu <sukadev@linux.vnet.ibm.com>
Date: Mon, 11 May 2009 13:11:11 -0700
Subject: [PATCH] devpts: Correctly set default options

devpts_get_sb() calls memset(0) to clear mount options and calls
parse_mount_options() if user specified any mount options. The memset(0) is
bogus since the 'mode' and 'ptmxmode' options are non-zero by default.
parse_mount_options() restores options to default anyway and can properly deal
with NULL mount options.

So in devpts_get_sb() remove memset(0) and call parse_mount_options() even for
NULL mount options.

Bug reported by Eric Paris: http://lkml.org/lkml/2009/5/7/448.

Signed-off-by: Sukadev Bhattiprolu (sukadev@us.ibm.com)
---
 fs/devpts/inode.c |   18 ++++++++++++------
 1 files changed, 12 insertions(+), 6 deletions(-)

diff --git a/fs/devpts/inode.c b/fs/devpts/inode.c
index 63a4a59..b7a954e 100644
--- a/fs/devpts/inode.c
+++ b/fs/devpts/inode.c
@@ -90,6 +90,15 @@ static inline struct super_block *pts_sb_from_inode(struct inode *inode)
 #define PARSE_MOUNT	0
 #define PARSE_REMOUNT	1
 
+/*
+ * parse_mount_options():
+ * 	Set @opts to mount options specified in @data. If an option is not
+ * 	specified in @data, set it to its default value. The exception is
+ * 	'newinstance' option which can only be set/cleared on a mount (i.e.
+ * 	cannot be changed during remount).
+ *
+ * Note: @data may be NULL (in which case all options are set to default).
+ */
 static int parse_mount_options(char *data, int op, struct pts_mount_opts *opts)
 {
 	char *p;
@@ -355,12 +364,9 @@ static int devpts_get_sb(struct file_system_type *fs_type,
 	struct pts_mount_opts opts;
 	struct super_block *s;
 
-	memset(&opts, 0, sizeof(opts));
-	if (data) {
-		error = parse_mount_options(data, PARSE_MOUNT, &opts);
-		if (error)
-			return error;
-	}
+	error = parse_mount_options(data, PARSE_MOUNT, &opts);
+	if (error)
+		return error;
 
 	if (opts.newinstance)
 		s = sget(fs_type, NULL, set_anon_super, NULL);

Re: [v3][PATCH 5/5] Merge code for single andmultiple-instancemounts

[H. Peter Anvin]

Sukadev Bhattiprolu wrote:
> 
> I tested this patch by directly calling the system call mount() with a NULL
> data parameter. If you/Eric/Peter can confirm that this works for you, I will
> send this patch to Andrew.
> 

Makes a lot more sense to me.  In particular, NULL should be handled the 
same as "".

	-hpa

Re: [v3][PATCH 5/5] Merge code for singleandmultiple-instancemounts

[Serge E. Hallyn]

Quoting Sukadev Bhattiprolu (sukadev@linux.vnet.ibm.com):
...

> >From 2f7746f4df78ff57125c4714f0cd64e739ccf804 Mon Sep 17 00:00:00 2001
> From: Sukadev Bhattiprolu <sukadev@linux.vnet.ibm.com>
> Date: Mon, 11 May 2009 13:11:11 -0700
> Subject: [PATCH] devpts: Correctly set default options
> 
> devpts_get_sb() calls memset(0) to clear mount options and calls
> parse_mount_options() if user specified any mount options. The memset(0) is
> bogus since the 'mode' and 'ptmxmode' options are non-zero by default.
> parse_mount_options() restores options to default anyway and can properly deal
> with NULL mount options.
> 
> So in devpts_get_sb() remove memset(0) and call parse_mount_options() even for
> NULL mount options.
> 
> Bug reported by Eric Paris: http://lkml.org/lkml/2009/5/7/448.

Would you almost say... Reported-by: ?  :)
> 
> Signed-off-by: Sukadev Bhattiprolu (sukadev@us.ibm.com)

Looks good.

Acked-by: Serge Hallyn <serue@us.ibm.com>

> ---
>  fs/devpts/inode.c |   18 ++++++++++++------
>  1 files changed, 12 insertions(+), 6 deletions(-)
> 
> diff --git a/fs/devpts/inode.c b/fs/devpts/inode.c
> index 63a4a59..b7a954e 100644
> --- a/fs/devpts/inode.c
> +++ b/fs/devpts/inode.c
> @@ -90,6 +90,15 @@ static inline struct super_block *pts_sb_from_inode(struct inode *inode)
>  #define PARSE_MOUNT	0
>  #define PARSE_REMOUNT	1
> 
> +/*
> + * parse_mount_options():
> + * 	Set @opts to mount options specified in @data. If an option is not
> + * 	specified in @data, set it to its default value. The exception is
> + * 	'newinstance' option which can only be set/cleared on a mount (i.e.
> + * 	cannot be changed during remount).
> + *
> + * Note: @data may be NULL (in which case all options are set to default).
> + */
>  static int parse_mount_options(char *data, int op, struct pts_mount_opts *opts)
>  {
>  	char *p;
> @@ -355,12 +364,9 @@ static int devpts_get_sb(struct file_system_type *fs_type,
>  	struct pts_mount_opts opts;
>  	struct super_block *s;
> 
> -	memset(&opts, 0, sizeof(opts));
> -	if (data) {
> -		error = parse_mount_options(data, PARSE_MOUNT, &opts);
> -		if (error)
> -			return error;
> -	}
> +	error = parse_mount_options(data, PARSE_MOUNT, &opts);
> +	if (error)
> +		return error;
> 
>  	if (opts.newinstance)
>  		s = sget(fs_type, NULL, set_anon_super, NULL);

Re: [v3][PATCH 5/5] Merge code for singleandmultiple-instancemounts

[H. Peter Anvin]

Serge E. Hallyn wrote:
> Quoting Sukadev Bhattiprolu (sukadev@linux.vnet.ibm.com):
> ...
> 
>> >From 2f7746f4df78ff57125c4714f0cd64e739ccf804 Mon Sep 17 00:00:00 2001
>> From: Sukadev Bhattiprolu <sukadev@linux.vnet.ibm.com>
>> Date: Mon, 11 May 2009 13:11:11 -0700
>> Subject: [PATCH] devpts: Correctly set default options
>>
>> devpts_get_sb() calls memset(0) to clear mount options and calls
>> parse_mount_options() if user specified any mount options. The memset(0) is
>> bogus since the 'mode' and 'ptmxmode' options are non-zero by default.
>> parse_mount_options() restores options to default anyway and can properly deal
>> with NULL mount options.
>>
>> So in devpts_get_sb() remove memset(0) and call parse_mount_options() even for
>> NULL mount options.
>>
>> Bug reported by Eric Paris: http://lkml.org/lkml/2009/5/7/448.
> 
> Would you almost say... Reported-by: ?  :)
>> Signed-off-by: Sukadev Bhattiprolu (sukadev@us.ibm.com)
> 
> Looks good.
> 
> Acked-by: Serge Hallyn <serue@us.ibm.com>
> 

Reviewed-by: H. Peter Anvin <hpa@zytor.com>

	-hpa

Re: [v3][PATCH 5/5] Merge code for single andmultiple-instancemounts

[Andrew Morton]

On Mon, 11 May 2009 15:15:02 -0700
Sukadev Bhattiprolu <sukadev@linux.vnet.ibm.com> wrote:

> Marc Dionne [marc.c.dionne@gmail.com] wrote:
> > On 05/07/2009 07:21 PM, Sukadev Bhattiprolu wrote:
> >> Er. I meant add following lines after the memset().
> >>
> >> |         opts->mode    = DEVPTS_DEFAULT_MODE;
> >> |         opts->ptmxmode = DEVPTS_DEFAULT_PTMX_MODE;
> >
> > Adding those two lines (with . instead of ->) does fix the issue for me, 
> > thanks.
> >
> > | > Of course we would still need to understand if/
> > | > why this patch changes the settings.
> >
> > That particular patch changed things because the original memcpy in 
> > new_pts_mount() did this:
> >
> > memcpy(&fsi->mount_opts, opts, sizeof(opts));
> >
> > where opts was a pointer, not a structure.  So only the first few bytes of 
> > the blank opts actually got copied over.  The patch moved this memcpy to 
> > devpts_get_sb() and in the process fixed the sizeof error.
> 
> Here is a cleaner fix - When user space (/etc/rc.sysinit on RHEL5) mounts
> devpts, this problem might be masked since the mount system call could pass
> a non-NULL 'data' parmeter to devpts_get_sb().
> 
> I tested this patch by directly calling the system call mount() with a NULL
> data parameter. If you/Eric/Peter can confirm that this works for you, I will
> send this patch to Andrew.

You already sent it ;)

> >From 2f7746f4df78ff57125c4714f0cd64e739ccf804 Mon Sep 17 00:00:00 2001
> From: Sukadev Bhattiprolu <sukadev@linux.vnet.ibm.com>
> Date: Mon, 11 May 2009 13:11:11 -0700
> Subject: [PATCH] devpts: Correctly set default options
> 
> devpts_get_sb() calls memset(0) to clear mount options and calls
> parse_mount_options() if user specified any mount options. The memset(0) is
> bogus since the 'mode' and 'ptmxmode' options are non-zero by default.
> parse_mount_options() restores options to default anyway and can properly deal
> with NULL mount options.
> 
> So in devpts_get_sb() remove memset(0) and call parse_mount_options() even for
> NULL mount options.
> 
> Bug reported by Eric Paris: http://lkml.org/lkml/2009/5/7/448.

This patch fixes a post-2.6.29 regression, yes?

> Signed-off-by: Sukadev Bhattiprolu (sukadev@us.ibm.com)

Please use angled brackets around the email address.  Some lame
person's scripts probably require it.  <whistles innocently>

Under the he-merged-it-last rule, this patch should formally go via
Al's VFS tree.  But I would happily accept a stfu-and-merge it?

Re: [v3][PATCH 5/5] Merge code for single andmultiple-instancemounts

[Eric Paris]

On Mon, 2009-05-11 at 15:40 -0700, Andrew Morton wrote:
> On Mon, 11 May 2009 15:15:02 -0700
> Sukadev Bhattiprolu <sukadev@linux.vnet.ibm.com> wrote:
> 
> > Marc Dionne [marc.c.dionne@gmail.com] wrote:
> > > On 05/07/2009 07:21 PM, Sukadev Bhattiprolu wrote:
> > >> Er. I meant add following lines after the memset().
> > >>
> > >> |         opts->mode    = DEVPTS_DEFAULT_MODE;
> > >> |         opts->ptmxmode = DEVPTS_DEFAULT_PTMX_MODE;
> > >
> > > Adding those two lines (with . instead of ->) does fix the issue for me, 
> > > thanks.
> > >
> > > | > Of course we would still need to understand if/
> > > | > why this patch changes the settings.
> > >
> > > That particular patch changed things because the original memcpy in 
> > > new_pts_mount() did this:
> > >
> > > memcpy(&fsi->mount_opts, opts, sizeof(opts));
> > >
> > > where opts was a pointer, not a structure.  So only the first few bytes of 
> > > the blank opts actually got copied over.  The patch moved this memcpy to 
> > > devpts_get_sb() and in the process fixed the sizeof error.
> > 
> > Here is a cleaner fix - When user space (/etc/rc.sysinit on RHEL5) mounts
> > devpts, this problem might be masked since the mount system call could pass
> > a non-NULL 'data' parmeter to devpts_get_sb().
> > 
> > I tested this patch by directly calling the system call mount() with a NULL
> > data parameter. If you/Eric/Peter can confirm that this works for you, I will
> > send this patch to Andrew.
> 
> You already sent it ;)
> 
> > >From 2f7746f4df78ff57125c4714f0cd64e739ccf804 Mon Sep 17 00:00:00 2001
> > From: Sukadev Bhattiprolu <sukadev@linux.vnet.ibm.com>
> > Date: Mon, 11 May 2009 13:11:11 -0700
> > Subject: [PATCH] devpts: Correctly set default options
> > 
> > devpts_get_sb() calls memset(0) to clear mount options and calls
> > parse_mount_options() if user specified any mount options. The memset(0) is
> > bogus since the 'mode' and 'ptmxmode' options are non-zero by default.
> > parse_mount_options() restores options to default anyway and can properly deal
> > with NULL mount options.
> > 
> > So in devpts_get_sb() remove memset(0) and call parse_mount_options() even for
> > NULL mount options.
> > 
> > Bug reported by Eric Paris: http://lkml.org/lkml/2009/5/7/448.
> 
> This patch fixes a post-2.6.29 regression, yes?

I believe so, yes.

> > Signed-off-by: Sukadev Bhattiprolu (sukadev@us.ibm.com)
> 
> Please use angled brackets around the email address.  Some lame
> person's scripts probably require it.  <whistles innocently>
> 
> Under the he-merged-it-last rule, this patch should formally go via
> Al's VFS tree.  But I would happily accept a stfu-and-merge it?

You have another tested, reviewed, and ack'd on this patch from me.  I
personally would say send it away!

-Eric

Re: [v3][PATCH 5/5] Merge code for single andmultiple-instancemounts

[Marc Dionne]

On Mon, May 11, 2009 at 6:15 PM, Sukadev Bhattiprolu
<sukadev@linux.vnet.ibm.com> wrote:
> Here is a cleaner fix - When user space (/etc/rc.sysinit on RHEL5) mounts
> devpts, this problem might be masked since the mount system call could pass
> a non-NULL 'data' parmeter to devpts_get_sb().
>
> I tested this patch by directly calling the system call mount() with a NULL
> data parameter. If you/Eric/Peter can confirm that this works for you, I will
> send this patch to Andrew.
>
> Thanks,

Works fine here, thanks.

Marc

[v3][PATCH 2/5] Parse mount options just once and copy them to super block

[Sukadev Bhattiprolu]

From: Sukadev Bhattiprolu <sukadev@linux.vnet.ibm.com>
Subject: [v3][PATCH 2/5] Parse mount options just once and copy them to super block

Since all the mount option parsing is done in devpts, we could do it
just once and pass it around in devpts functions and eventually store
it in the super block.

Signed-off-by: Sukadev Bhattiprolu <sukadev@linux.vnet.ibm.com>
---
 fs/devpts/inode.c |  100 ++++++++++++-----------------------------------------
 1 files changed, 22 insertions(+), 78 deletions(-)

diff --git a/fs/devpts/inode.c b/fs/devpts/inode.c
index de15e73..9b24de8 100644
--- a/fs/devpts/inode.c
+++ b/fs/devpts/inode.c
@@ -325,49 +325,14 @@ static int compare_init_pts_sb(struct super_block *s, void *p)
 }
 
 /*
- * Safely parse the mount options in @data and update @opts.
- *
- * devpts ends up parsing options two times during mount, due to the
- * two modes of operation it supports. The first parse occurs in
- * devpts_get_sb() when determining the mode (single-instance or
- * multi-instance mode). The second parse happens in devpts_remount()
- * or new_pts_mount() depending on the mode.
- *
- * Parsing of options modifies the @data making subsequent parsing
- * incorrect. So make a local copy of @data and parse it.
- *
- * Return: 0 On success, -errno on error
- */
-static int safe_parse_mount_options(void *data, struct pts_mount_opts *opts)
-{
-	int rc;
-	void *datacp;
-
-	if (!data)
-		return 0;
-
-	/* Use kstrdup() ?  */
-	datacp = kmalloc(PAGE_SIZE, GFP_KERNEL);
-	if (!datacp)
-		return -ENOMEM;
-
-	memcpy(datacp, data, PAGE_SIZE);
-	rc = parse_mount_options((char *)datacp, PARSE_MOUNT, opts);
-	kfree(datacp);
-
-	return rc;
-}
-
-/*
  * Mount a new (private) instance of devpts.  PTYs created in this
  * instance are independent of the PTYs in other devpts instances.
  */
 static int new_pts_mount(struct file_system_type *fs_type, int flags,
-		void *data, struct vfsmount *mnt)
+		void *data, struct pts_mount_opts *opts, struct vfsmount *mnt)
 {
 	int err;
 	struct pts_fs_info *fsi;
-	struct pts_mount_opts *opts;
 
 	printk(KERN_NOTICE "devpts: newinstance mount\n");
 
@@ -376,11 +341,7 @@ static int new_pts_mount(struct file_system_type *fs_type, int flags,
 		return err;
 
 	fsi = DEVPTS_SB(mnt->mnt_sb);
-	opts = &fsi->mount_opts;
-
-	err = parse_mount_options(data, PARSE_MOUNT, opts);
-	if (err)
-		goto fail;
+	memcpy(&fsi->mount_opts, opts, sizeof(opts));
 
 	err = mknod_ptmx(mnt->mnt_sb);
 	if (err)
@@ -396,28 +357,6 @@ fail:
 }
 
 /*
- * Check if 'newinstance' mount option was specified in @data.
- *
- * Return: -errno  	on error (eg: invalid mount options specified)
- * 	 : 1 		if 'newinstance' mount option was specified
- * 	 : 0 		if 'newinstance' mount option was NOT specified
- */
-static int is_new_instance_mount(void *data)
-{
-	int rc;
-	struct pts_mount_opts opts;
-
-	if (!data)
-		return 0;
-
-	rc = safe_parse_mount_options(data, &opts);
-	if (!rc)
-		rc = opts.newinstance;
-
-	return rc;
-}
-
-/*
  * get_init_pts_sb()
  *
  *     This interface is needed to support multiple namespace semantics in
@@ -439,10 +378,9 @@ static int is_new_instance_mount(void *data)
  *     presence of the private namespace (i.e 'newinstance') super-blocks.
  */
 static int get_init_pts_sb(struct file_system_type *fs_type, int flags,
-		void *data, struct vfsmount *mnt)
+		void *data, struct pts_mount_opts *opts, struct vfsmount *mnt)
 {
 	struct super_block *s;
-	struct pts_mount_opts *opts;
 	struct pts_fs_info *fsi;
 	int error;
 
@@ -460,11 +398,12 @@ static int get_init_pts_sb(struct file_system_type *fs_type, int flags,
 		}
 		s->s_flags |= MS_ACTIVE;
 	}
-	fsi = DEVPTS_SB(s);
-	opts = &fsi->mount_opts;
-	parse_mount_options(data, PARSE_REMOUNT, opts);
 
 	simple_set_mnt(mnt, s);
+
+	fsi = DEVPTS_SB(mnt->mnt_sb);
+	memcpy(&fsi->mount_opts, opts, sizeof(opts));
+
 	return 0;
 }
 
@@ -474,11 +413,11 @@ static int get_init_pts_sb(struct file_system_type *fs_type, int flags,
  * kernel still allows multiple-instances.
  */
 static int init_pts_mount(struct file_system_type *fs_type, int flags,
-		void *data, struct vfsmount *mnt)
+		void *data, struct pts_mount_opts *opts, struct vfsmount *mnt)
 {
 	int err;
 
-	err = get_init_pts_sb(fs_type, flags, data, mnt);
+	err = get_init_pts_sb(fs_type, flags, data, opts, mnt);
 	if (err)
 		return err;
 
@@ -495,17 +434,22 @@ static int init_pts_mount(struct file_system_type *fs_type, int flags,
 static int devpts_get_sb(struct file_system_type *fs_type,
 	int flags, const char *dev_name, void *data, struct vfsmount *mnt)
 {
-	int new;
-
-	new = is_new_instance_mount(data);
-	if (new < 0)
-		return new;
+	int error;
+	struct pts_mount_opts opts;
 
-	if (new)
-		return new_pts_mount(fs_type, flags, data, mnt);
+	memset(&opts, 0, sizeof(opts)); 
+	if (data) {
+		error = parse_mount_options(data, PARSE_MOUNT, &opts);
+		if (error)
+			return error;
+	}
 
-	return init_pts_mount(fs_type, flags, data, mnt);
+	if (opts.newinstance)
+		return new_pts_mount(fs_type, flags, data, &opts, mnt);
+	else
+		return init_pts_mount(fs_type, flags, data, &opts, mnt);
 }
+
 #else
 /*
  * This supports only the legacy single-instance semantics (no
-- 
1.5.2.5

[v3][PATCH 1/5] Unroll essentials of do_remount_sb() into devpts

[Sukadev Bhattiprolu]

From: Sukadev Bhattiprolu <sukadev@linux.vnet.ibm.com>
Subject: [v3][PATCH 1/5] Unroll essentials of do_remount_sb() into devpts

On remount, devpts fs only needs to parse the mount options. Users cannot
directly create/dirty files in /dev/pts so the MS_RDONLY flag and
shrinking the dcache does not really apply to devpts.

So effectively on remount, devpts only parses the mount options and updates
these options in its super block. As such, we could replace do_remount_sb()
call with a direct parse_mount_options().

Doing so enables subsequent patches to avoid parsing the mount options twice
and simplify the code.

Signed-off-by: Sukadev Bhattiprolu <sukadev@linux.vnet.ibm.com>
Acked-by: Serge Hallyn <serue@us.ibm.com>
---
 fs/devpts/inode.c |    7 ++++++-
 1 files changed, 6 insertions(+), 1 deletions(-)

diff --git a/fs/devpts/inode.c b/fs/devpts/inode.c
index ad186b4..de15e73 100644
--- a/fs/devpts/inode.c
+++ b/fs/devpts/inode.c
@@ -442,6 +442,8 @@ static int get_init_pts_sb(struct file_system_type *fs_type, int flags,
 		void *data, struct vfsmount *mnt)
 {
 	struct super_block *s;
+	struct pts_mount_opts *opts;
+	struct pts_fs_info *fsi;
 	int error;
 
 	s = sget(fs_type, compare_init_pts_sb, set_anon_super, NULL);
@@ -458,7 +460,10 @@ static int get_init_pts_sb(struct file_system_type *fs_type, int flags,
 		}
 		s->s_flags |= MS_ACTIVE;
 	}
-	do_remount_sb(s, flags, data, 0);
+	fsi = DEVPTS_SB(s);
+	opts = &fsi->mount_opts;
+	parse_mount_options(data, PARSE_REMOUNT, opts);
+
 	simple_set_mnt(mnt, s);
 	return 0;
 }
-- 
1.5.2.5