Simplify devpts code

PATCH Superseded Sukadev Bhattiprolu 8 participants 23 messages Mar 7, 2009 – May 11, 2009

Share & tools

Sukadev Bhattiprolu

author

2009-03-07 18:09 UTC #0 lore raw

v2 cover Simplify devpts code

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 ?

Sukadev Bhattiprolu

author Acked-by Serge Hallyn on the v2 series

2009-03-07 18:12 UTC #1 lore raw

From: Sukadev Bhattiprolu <[email protected]>
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 <[email protected]>

Acked-by Serge Hallyn <[email protected]>

---
 fs/devpts/inode.c |    7 ++++++-
 1 files changed, 6 insertions(+), 1 deletions(-)

fs/devpts/inode.c +6 -1

index ad186b4..de15e73 100644
@@ -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;
 }

Signature · 1 line

1.5.2.5

Sukadev Bhattiprolu

author

2009-03-07 18:19 UTC #2 lore raw

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

Sukadev Bhattiprolu [[email protected]] wrote:

| From: Sukadev Bhattiprolu <[email protected]>
| 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 <[email protected]>
| Acked-by: Serge Hallyn <[email protected]>
| ---
|  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

Sukadev Bhattiprolu

author

2009-03-07 18:18 UTC #3 lore raw

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

In reply to Sukadev Bhattiprolu

From: Sukadev Bhattiprolu <[email protected]>
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 <[email protected]>

Acked-by Serge Hallyn <[email protected]>

---
 fs/devpts/inode.c |    7 ++++++-
 1 files changed, 6 insertions(+), 1 deletions(-)

fs/devpts/inode.c +6 -1

index ad186b4..de15e73 100644
@@ -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;
 }

Signature · 1 line

1.5.2.5

Sukadev Bhattiprolu

author

2009-03-07 18:12 UTC #4 lore raw

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

In reply to Sukadev Bhattiprolu

From: Sukadev Bhattiprolu <[email protected]>
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 <[email protected]>

---
 fs/devpts/inode.c |  100 ++++++++++++-----------------------------------------
 1 files changed, 22 insertions(+), 78 deletions(-)

fs/devpts/inode.c +22 ~~-78~~

index de15e73..9b24de8 100644
@@ -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

Signature · 1 line

1.5.2.5

Sukadev Bhattiprolu

author

2009-03-07 18:16 UTC #5 lore raw

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

In reply to Sukadev Bhattiprolu

From: Sukadev Bhattiprolu <[email protected]>
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 <[email protected]>

---
 fs/devpts/inode.c |  100 ++++++++++++-----------------------------------------
 1 files changed, 22 insertions(+), 78 deletions(-)

fs/devpts/inode.c +22 ~~-78~~

index de15e73..9b24de8 100644
@@ -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

Signature · 1 line

1.5.2.5

Sukadev Bhattiprolu

author

2009-03-07 18:13 UTC #6 lore raw

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

In reply to Sukadev Bhattiprolu

From: Sukadev Bhattiprolu <[email protected]>
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 <[email protected]>

Acked-by Serge Hallyn <[email protected]>

---
 fs/devpts/inode.c |   36 +++++++++++++++++-------------------
 1 files changed, 17 insertions(+), 19 deletions(-)

fs/devpts/inode.c +17 ~~-19~~

index 9b24de8..f7e887c 100644
@@ -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

Signature · 1 line

1.5.2.5

Sukadev Bhattiprolu

author

2009-03-07 18:13 UTC #7 lore raw

PATCH v3 4/5 Remove get_init_pts_sb()

In reply to Sukadev Bhattiprolu

From: Sukadev Bhattiprolu <[email protected]>
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 <[email protected]>

Acked-by Serge Hallyn <[email protected]>

---
 fs/devpts/inode.c |   25 ++++++-------------------
 1 files changed, 6 insertions(+), 19 deletions(-)

fs/devpts/inode.c +6 ~~-19~~

index f7e887c..c821c9a 100644
@@ -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)
 {

Signature · 1 line

1.5.2.5

Sukadev Bhattiprolu

author

2009-03-07 18:14 UTC #8 lore raw

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

In reply to Sukadev Bhattiprolu

From: Sukadev Bhattiprolu <[email protected]>
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 <[email protected]>

Acked-by Serge Hallyn <[email protected]>

Tested-by Serge Hallyn <[email protected]>

---
 fs/devpts/inode.c |  115 ++++++++++++++++++----------------------------------
 1 files changed, 40 insertions(+), 75 deletions(-)

fs/devpts/inode.c +40 ~~-75~~

index c821c9a..c25fb34 100644
@@ -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;
 }

Signature · 1 line

1.5.2.5

Eric Paris

2009-05-07 20:36 UTC #9 lore raw

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

Quoted 3 lines · Sukadev Bhattiprolu

From: Sukadev Bhattiprolu <[email protected]>
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!

Quoted 184 lines

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 <[email protected]>

Acked-by Serge Hallyn <[email protected]>

Tested-by Serge Hallyn <[email protected]>

---
fs/devpts/inode.c | 115 ++++++++++++++++++----------------------------------

 1 files changed, 40 insertions(+), 75 deletions(-)

fs/devpts/inode.c +19 ~~-68~~

index c821c9a..c25fb34 100644
@@ -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;
+ }

fs/devpts/inode.c +6 -0

+
+       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);

fs/devpts/inode.c +4 -0

+
+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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Sukadev Bhattiprolu

author

2009-05-07 21:24 UTC #10 lore raw

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

Eric Paris [[email protected]] wrote:

| On Sat, Mar 7, 2009 at 2:12 PM, Sukadev Bhattiprolu
| <[email protected]> wrote:
| >
| > From: Sukadev Bhattiprolu <[email protected]>
| > 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

Eric Paris

2009-05-07 22:36 UTC #11 lore raw

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

Quoted 21 lines · Sukadev Bhattiprolu

Eric Paris [[email protected]] wrote:

| On Sat, Mar 7, 2009 at 2:12 PM, Sukadev Bhattiprolu
| <[email protected]> wrote:
| >
| > From: Sukadev Bhattiprolu <[email protected]>
| > 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!

Quoted 8 lines

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

Quoted 1 line

	$ 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

Quoted 1 line

	$ 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

Quoted 2 lines

Also, is CONFIG_DEVPTS_MULTIPLE_INSTANCES set in your .config ?

CONFIG_DEVPTS_MULTIPLE_INSTANCES=y

Quoted 1 line

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.....

Sukadev Bhattiprolu

author

2009-05-07 23:18 UTC #12 lore raw

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

Eric Paris [[email protected]] wrote:

| On Thu, 2009-05-07 at 14:24 -0700, Sukadev Bhattiprolu wrote: 
| > Eric Paris [[email protected]] wrote:
| > | On Sat, Mar 7, 2009 at 2:12 PM, Sukadev Bhattiprolu
| > | <[email protected]> wrote:
| > | >
| > | > From: Sukadev Bhattiprolu <[email protected]>
| > | > 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

Sukadev Bhattiprolu

author

2009-05-07 23:21 UTC #13 lore raw

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

| 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

Peter Staubach

2009-05-08 13:54 UTC #14 lore raw

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

Quoted 9 lines · 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:

Quoted 6 lines

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.

Marc Dionne

2009-05-09 03:20 UTC #15 lore raw

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

In reply to Sukadev Bhattiprolu

Quoted 4 lines · Sukadev Bhattiprolu

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

Sukadev Bhattiprolu

author

2009-05-11 22:15 UTC #16 lore raw

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

Quoted 12 lines · Marc Dionne [[email protected]] wrote:

Quoted 4 lines · Sukadev Bhattiprolu
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.

Quoted 8 lines

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,
---

Quoted 1 line

From 2f7746f4df78ff57125c4714f0cd64e739ccf804 Mon Sep 17 00:00:00 2001

From: Sukadev Bhattiprolu <[email protected]>
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 ([email protected])

---
 fs/devpts/inode.c |   18 ++++++++++++------
 1 files changed, 12 insertions(+), 6 deletions(-)

fs/devpts/inode.c +12 -6

index 63a4a59..b7a954e 100644
@@ -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);

H. Peter Anvin

2009-05-11 22:21 UTC #17 lore raw

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

Quoted 5 lines · Sukadev Bhattiprolu

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

Serge E. Hallyn

Acked-by

2009-05-11 22:37 UTC #18 lore raw

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

In reply to Sukadev Bhattiprolu

Quoting Sukadev Bhattiprolu ([email protected]):
...

Quoted 11 lines

Quoted 1 line
From 2f7746f4df78ff57125c4714f0cd64e739ccf804 Mon Sep 17 00:00:00 2001
From: Sukadev Bhattiprolu <[email protected]>
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.

Quoted 4 lines

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: ? :)

Quoted 2 lines

Signed-off-by Sukadev Bhattiprolu ([email protected])

Looks good.

Acked-by Serge Hallyn <[email protected]>

Quoted 40 lines

---
 fs/devpts/inode.c |   18 ++++++++++++------
 1 files changed, 12 insertions(+), 6 deletions(-)

fs/devpts/inode.c +12 -6

index 63a4a59..b7a954e 100644
@@ -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);

H. Peter Anvin

Reviewed-by

2009-05-11 22:54 UTC #19 lore raw

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

Quoted 17 lines · Serge E. Hallyn

Quoting Sukadev Bhattiprolu ([email protected]):
...
Quoted 14 lines
Quoted 1 line
From 2f7746f4df78ff57125c4714f0cd64e739ccf804 Mon Sep 17 00:00:00 2001
From: Sukadev Bhattiprolu <[email protected]>
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.

Quoted 9 lines

Quoted 1 line
Bug reported by Eric Paris: http://lkml.org/lkml/2009/5/7/448.
Would you almost say... Reported-by: ? :)
Quoted 1 line
Signed-off-by Sukadev Bhattiprolu ([email protected])
Looks good.
Acked-by Serge Hallyn <[email protected]>

Reviewed-by H. Peter Anvin <[email protected]>

-hpa

Andrew Morton

2009-05-11 22:47 UTC #20 lore raw

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

In reply to Sukadev Bhattiprolu

Quoted 22 lines · On Mon, 11 May 2009 15:15:02 -0700 Sukadev Bhattiprolu…

Quoted 12 lines · Marc Dionne [[email protected]] wrote:
Quoted 4 lines · Sukadev Bhattiprolu
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.
Quoted 8 lines
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.

Quoted 7 lines

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 ;)

Quoted 11 lines

Quoted 1 line
From 2f7746f4df78ff57125c4714f0cd64e739ccf804 Mon Sep 17 00:00:00 2001
From: Sukadev Bhattiprolu <[email protected]>
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.

Quoted 4 lines

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?

Quoted 1 line

Signed-off-by Sukadev Bhattiprolu ([email protected])

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?

Eric Paris

2009-05-11 23:11 UTC #21 lore raw

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

Quoted 46 lines · Andrew Morton

Quoted 22 lines · On Mon, 11 May 2009 15:15:02 -0700 Sukadev Bhattiprolu…
Quoted 12 lines · Marc Dionne [[email protected]] wrote:
Quoted 4 lines · Sukadev Bhattiprolu
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.
Quoted 8 lines
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.
Quoted 7 lines
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 ;)
Quoted 11 lines
Quoted 1 line
From 2f7746f4df78ff57125c4714f0cd64e739ccf804 Mon Sep 17 00:00:00 2001
From: Sukadev Bhattiprolu <[email protected]>
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.

Quoted 6 lines

Quoted 4 lines
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.

Quoted 7 lines

Quoted 1 line
Signed-off-by Sukadev Bhattiprolu ([email protected])
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

Marc Dionne

2009-05-11 23:00 UTC #22 lore raw

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

In reply to Sukadev Bhattiprolu

Quoted 9 lines · Sukadev Bhattiprolu

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