[PATCH 0/10][v4]: Enable multiple devpts instances

[PATCH 0/10][v4]: Enable multiple devpts instances

[sukadev-r/Jw6+rmf7HQT0dZR+AlfA]

Enable multiple instances of devpts filesystem so each container can allocate
ptys independently.

User interface:

	Since supporting multiple mounts of devpts can break user-space, this
	feature is enabled only if:

		- CONFIG_DEVPTS_MULTIPLE_INSTANCES=y (new config token), and
		- new mount option, -o newinstance is specified

	If CONFIG_DEVPTS_MULTIPLE_INSTANCES=n, there should be no change in
	behavior.

	See [PATCH 10/10] - Documentation/filesystems/devpts.txt for detailed
	usage/compatibility information.

		[PATCH 01/10] Remove devpts_root global
		[PATCH 02/10] Per-mount allocated_ptys
		[PATCH 03/10] Per-mount 'config' object
		[PATCH 04/10] Extract option parsing to new function
		[PATCH 05/10] Add DEVPTS_MULTIPLE_INSTANCES config token
		[PATCH 06/10] Define mknod_ptmx()
		[PATCH 07/10] Update ptmx permissions during remount
		[PATCH 08/10] Define get_sb_ref()
		[PATCH 09/10] Enable multiple instances of devpts
		[PATCH 10/10] Document usage of multiple-instances of devpts
	
Implementation notes:

	1. To enable multiple mounts of /dev/pts, (most) devpts interfaces
	   need to know which instance of devpts is being accessed. This
	   patchset uses the 'struct inode' or 'struct tty_struct' of the
	   device being accessed to identify the appropriate devpts instance.
	   Hence the need for the /dev/pts/ptmx bind-mount.

	2. See comments in get_sb_ref() in fs/super.c (could not find
	   existing interfaces that accomplish it).

	3. Mount options must be parsed twice during mount (once to determine
	   the mode of mount (single/multi-instance) and once to actually
	   save the options. There does not seem to be an easy way to parse
	   once and reuse (See 'safe_process_mount_opts()' in [PATCH 9/10])


Changelog [v4]:

	- Port to 2008-09-04 ttydev tree (and drop patches that were merged in)
	- Add DEVPTS_MULTIPLE_INSTANCES config token (patch 5) and move new
	  behavior under #ifdef CONFIG_DEVPTS_MULTIPLE_INSTANCES. 
	- Create ptmx node even in initial mount 
	- Change default permissions of pts/ptmx node to 0000 (patch 6)
	- Cache ptmx dentry and use to update permissions of ptmx node on
	  remount so legacy mode can use the node with a simpler change to
	  /etc/fstab (patch 7)
	- Move get_sb_ref() helper code to separate patch (patch 8)
	- Add Documentation/filesystems/devpts.txt and describe usage info
	  in that file.
	  
Changelog [v3]:

	- Port to 2008-08-28 ttydev tree
	- Rename new mount options to 'ptmxmode' and 'newinstance'.
	- [Alan Cox] Use tty driver data to identify devpts (this is used to
	  cleanup get_node() in devpts_pty_kill()).
	- [H. Peter Anvin] get_node() cleanup in devpts (which was enabled by
	  the inode/tty parameter to devpts interfaces)
	- Bugfix in multi-mount mode (see Patch 11/11).
	- Executed pty tests in LTP (in both single-instance and multi-instance
	  mode)
	- Should be bisect-safe :-)

Changelog [v2]:

	- New mount option '-o newmnt' added (patch 8/8)
	- Support both single-mount and multi-mount semantics (patch 8/8)
	- Automatically create ptmx node when devpts is mounted (patch 7/8)
	- Extract option parsing code to new function (patch 6/8)
	- Make 'config' params per-mount variables (patch 5/8)
	- Slightly re-ordered existing patches in set (patches 1/8, 2/8)

TODO:
	- Do we need a '-o ptmxuid' and '-o ptmxgid' options as well ?
	- Update mount(8) man page
	- (Sometime in future) Remove even initial kernel mount of devpts 
	- Any other good test suites to test this (besides LTP, sshd).

[PATCH 01/10] Remove devpts_root global

[sukadev-r/Jw6+rmf7HQT0dZR+AlfA]

From 57b3c2e5e40a63c8de630f88f6862851acb6e6c4 Mon Sep 17 00:00:00 2001
From: Sukadev Bhattiprolu <sukadev-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>
Date: Tue, 9 Sep 2008 10:21:44 -0700
Subject: [PATCH 01/10] Remove devpts_root global

Remove the 'devpts_root' global variable and find the root dentry using
the super_block. The super-block can be found from the device inode, using
the new wrapper, pts_sb_from_inode().

Changelog: This patch is based on an earlier patchset from Serge Hallyn
	   and Matt Helsley.

Signed-off-by: Sukadev Bhattiprolu <sukadev-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>
---
 fs/devpts/inode.c |   29 ++++++++++++++++++++---------
 1 files changed, 20 insertions(+), 9 deletions(-)

diff --git a/fs/devpts/inode.c b/fs/devpts/inode.c
index a70d5d0..ec33833 100644
--- a/fs/devpts/inode.c
+++ b/fs/devpts/inode.c
@@ -34,7 +34,6 @@ static DEFINE_IDA(allocated_ptys);
 static DEFINE_MUTEX(allocated_ptys_lock);
 
 static struct vfsmount *devpts_mnt;
-static struct dentry *devpts_root;
 
 static struct {
 	int setuid;
@@ -56,6 +55,14 @@ static match_table_t tokens = {
 	{Opt_err, NULL}
 };
 
+static inline struct super_block *pts_sb_from_inode(struct inode *inode)
+{
+	if (inode->i_sb->s_magic == DEVPTS_SUPER_MAGIC)
+		return inode->i_sb;
+
+	return devpts_mnt->mnt_sb;
+}
+
 static int devpts_remount(struct super_block *sb, int *flags, char *data)
 {
 	char *p;
@@ -142,7 +149,7 @@ devpts_fill_super(struct super_block *s, void *data, int silent)
 	inode->i_fop = &simple_dir_operations;
 	inode->i_nlink = 2;
 
-	devpts_root = s->s_root = d_alloc_root(inode);
+	s->s_root = d_alloc_root(inode);
 	if (s->s_root)
 		return 0;
 	
@@ -211,7 +218,9 @@ int devpts_pty_new(struct inode *ptmx_inode, struct tty_struct *tty)
 	struct tty_driver *driver = tty->driver;
 	dev_t device = MKDEV(driver->major, driver->minor_start+number);
 	struct dentry *dentry;
-	struct inode *inode = new_inode(devpts_mnt->mnt_sb);
+	struct super_block *sb = pts_sb_from_inode(ptmx_inode);
+	struct inode *inode = new_inode(sb);
+	struct dentry *root = sb->s_root;
 	char s[12];
 
 	/* We're supposed to be given the slave end of a pty */
@@ -231,15 +240,15 @@ int devpts_pty_new(struct inode *ptmx_inode, struct tty_struct *tty)
 
 	sprintf(s, "%d", number);
 
-	mutex_lock(&devpts_root->d_inode->i_mutex);
+	mutex_lock(&root->d_inode->i_mutex);
 
-	dentry = d_alloc_name(devpts_root, s);
+	dentry = d_alloc_name(root, s);
 	if (!IS_ERR(dentry)) {
 		d_add(dentry, inode);
-		fsnotify_create(devpts_root->d_inode, dentry);
+		fsnotify_create(root->d_inode, dentry);
 	}
 
-	mutex_unlock(&devpts_root->d_inode->i_mutex);
+	mutex_unlock(&root->d_inode->i_mutex);
 
 	return 0;
 }
@@ -256,11 +265,13 @@ struct tty_struct *devpts_get_tty(struct inode *pts_inode, int number)
 void devpts_pty_kill(struct tty_struct *tty)
 {
 	struct inode *inode = tty->driver_data;
+	struct super_block *sb = pts_sb_from_inode(inode);
+	struct dentry *root = sb->s_root;
 	struct dentry *dentry;
 
 	BUG_ON(inode->i_rdev == MKDEV(TTYAUX_MAJOR, PTMX_MINOR));
 
-	mutex_lock(&devpts_root->d_inode->i_mutex);
+	mutex_lock(&root->d_inode->i_mutex);
 
 	dentry = d_find_alias(inode);
 	if (dentry && !IS_ERR(dentry)) {
@@ -269,7 +280,7 @@ void devpts_pty_kill(struct tty_struct *tty)
 		dput(dentry);
 	}
 
-	mutex_unlock(&devpts_root->d_inode->i_mutex);
+	mutex_unlock(&root->d_inode->i_mutex);
 }
 
 static int __init init_devpts_fs(void)
-- 
1.5.2.5

Re: [PATCH 01/10] Remove devpts_root global

[Serge E. Hallyn]

Quoting sukadev-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org (sukadev-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org):
> 
> >From 57b3c2e5e40a63c8de630f88f6862851acb6e6c4 Mon Sep 17 00:00:00 2001
> From: Sukadev Bhattiprolu <sukadev-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>
> Date: Tue, 9 Sep 2008 10:21:44 -0700
> Subject: [PATCH 01/10] Remove devpts_root global
> 
> Remove the 'devpts_root' global variable and find the root dentry using
> the super_block. The super-block can be found from the device inode, using
> the new wrapper, pts_sb_from_inode().
> 
> Changelog: This patch is based on an earlier patchset from Serge Hallyn
> 	   and Matt Helsley.
> 
> Signed-off-by: Sukadev Bhattiprolu <sukadev-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>

Ok, I'll just have to go through these one by one until I figure out
why it's locking up my kernel...  This one absolutely looks clean.

Acked-by: Serge Hallyn <serue-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>

> ---
>  fs/devpts/inode.c |   29 ++++++++++++++++++++---------
>  1 files changed, 20 insertions(+), 9 deletions(-)
> 
> diff --git a/fs/devpts/inode.c b/fs/devpts/inode.c
> index a70d5d0..ec33833 100644
> --- a/fs/devpts/inode.c
> +++ b/fs/devpts/inode.c
> @@ -34,7 +34,6 @@ static DEFINE_IDA(allocated_ptys);
>  static DEFINE_MUTEX(allocated_ptys_lock);
> 
>  static struct vfsmount *devpts_mnt;
> -static struct dentry *devpts_root;
> 
>  static struct {
>  	int setuid;
> @@ -56,6 +55,14 @@ static match_table_t tokens = {
>  	{Opt_err, NULL}
>  };
> 
> +static inline struct super_block *pts_sb_from_inode(struct inode *inode)
> +{
> +	if (inode->i_sb->s_magic == DEVPTS_SUPER_MAGIC)
> +		return inode->i_sb;
> +
> +	return devpts_mnt->mnt_sb;
> +}
> +
>  static int devpts_remount(struct super_block *sb, int *flags, char *data)
>  {
>  	char *p;
> @@ -142,7 +149,7 @@ devpts_fill_super(struct super_block *s, void *data, int silent)
>  	inode->i_fop = &simple_dir_operations;
>  	inode->i_nlink = 2;
> 
> -	devpts_root = s->s_root = d_alloc_root(inode);
> +	s->s_root = d_alloc_root(inode);
>  	if (s->s_root)
>  		return 0;
>  	
> @@ -211,7 +218,9 @@ int devpts_pty_new(struct inode *ptmx_inode, struct tty_struct *tty)
>  	struct tty_driver *driver = tty->driver;
>  	dev_t device = MKDEV(driver->major, driver->minor_start+number);
>  	struct dentry *dentry;
> -	struct inode *inode = new_inode(devpts_mnt->mnt_sb);
> +	struct super_block *sb = pts_sb_from_inode(ptmx_inode);
> +	struct inode *inode = new_inode(sb);
> +	struct dentry *root = sb->s_root;
>  	char s[12];
> 
>  	/* We're supposed to be given the slave end of a pty */
> @@ -231,15 +240,15 @@ int devpts_pty_new(struct inode *ptmx_inode, struct tty_struct *tty)
> 
>  	sprintf(s, "%d", number);
> 
> -	mutex_lock(&devpts_root->d_inode->i_mutex);
> +	mutex_lock(&root->d_inode->i_mutex);
> 
> -	dentry = d_alloc_name(devpts_root, s);
> +	dentry = d_alloc_name(root, s);
>  	if (!IS_ERR(dentry)) {
>  		d_add(dentry, inode);
> -		fsnotify_create(devpts_root->d_inode, dentry);
> +		fsnotify_create(root->d_inode, dentry);
>  	}
> 
> -	mutex_unlock(&devpts_root->d_inode->i_mutex);
> +	mutex_unlock(&root->d_inode->i_mutex);
> 
>  	return 0;
>  }
> @@ -256,11 +265,13 @@ struct tty_struct *devpts_get_tty(struct inode *pts_inode, int number)
>  void devpts_pty_kill(struct tty_struct *tty)
>  {
>  	struct inode *inode = tty->driver_data;
> +	struct super_block *sb = pts_sb_from_inode(inode);
> +	struct dentry *root = sb->s_root;
>  	struct dentry *dentry;
> 
>  	BUG_ON(inode->i_rdev == MKDEV(TTYAUX_MAJOR, PTMX_MINOR));
> 
> -	mutex_lock(&devpts_root->d_inode->i_mutex);
> +	mutex_lock(&root->d_inode->i_mutex);
> 
>  	dentry = d_find_alias(inode);
>  	if (dentry && !IS_ERR(dentry)) {
> @@ -269,7 +280,7 @@ void devpts_pty_kill(struct tty_struct *tty)
>  		dput(dentry);
>  	}
> 
> -	mutex_unlock(&devpts_root->d_inode->i_mutex);
> +	mutex_unlock(&root->d_inode->i_mutex);
>  }
> 
>  static int __init init_devpts_fs(void)
> -- 
> 1.5.2.5

[PATCH 02/10] Per-mount allocated_ptys

[sukadev-r/Jw6+rmf7HQT0dZR+AlfA]

From bdba42b68e2b6765615618fce7eb7d4c6f75685d Mon Sep 17 00:00:00 2001
From: Sukadev Bhattiprolu <sukadev-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>
Date: Tue, 9 Sep 2008 10:22:11 -0700
Subject: [PATCH 02/10] Per-mount allocated_ptys

To enable multiple mounts of devpts, 'allocated_ptys' must be a per-mount
variable rather than a global variable.  Move 'allocated_ptys' into the
super_block's s_fs_info.

Changelog[v2]:
	Define and use DEVPTS_SB() wrapper.

Signed-off-by: Sukadev Bhattiprolu <sukadev-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>
---
 fs/devpts/inode.c |   55 ++++++++++++++++++++++++++++++++++++++++++++++------
 1 files changed, 48 insertions(+), 7 deletions(-)

diff --git a/fs/devpts/inode.c b/fs/devpts/inode.c
index ec33833..6e63db7 100644
--- a/fs/devpts/inode.c
+++ b/fs/devpts/inode.c
@@ -30,7 +30,6 @@
 #define PTMX_MINOR	2
 
 extern int pty_limit;			/* Config limit on Unix98 ptys */
-static DEFINE_IDA(allocated_ptys);
 static DEFINE_MUTEX(allocated_ptys_lock);
 
 static struct vfsmount *devpts_mnt;
@@ -55,6 +54,15 @@ static match_table_t tokens = {
 	{Opt_err, NULL}
 };
 
+struct pts_fs_info {
+	struct ida allocated_ptys;
+};
+
+static inline struct pts_fs_info *DEVPTS_SB(struct super_block *sb)
+{
+	return sb->s_fs_info;
+}
+
 static inline struct super_block *pts_sb_from_inode(struct inode *inode)
 {
 	if (inode->i_sb->s_magic == DEVPTS_SUPER_MAGIC)
@@ -126,6 +134,19 @@ static const struct super_operations devpts_sops = {
 	.show_options	= devpts_show_options,
 };
 
+static void *new_pts_fs_info(void)
+{
+	struct pts_fs_info *fsi;
+
+	fsi = kzalloc(sizeof(struct pts_fs_info), GFP_KERNEL);
+	if (!fsi)
+		return NULL;
+
+	ida_init(&fsi->allocated_ptys);
+
+	return fsi;
+}
+
 static int
 devpts_fill_super(struct super_block *s, void *data, int silent)
 {
@@ -137,9 +158,13 @@ devpts_fill_super(struct super_block *s, void *data, int silent)
 	s->s_op = &devpts_sops;
 	s->s_time_gran = 1;
 
+	s->s_fs_info = new_pts_fs_info();
+	if (!s->s_fs_info)
+		goto fail;
+
 	inode = new_inode(s);
 	if (!inode)
-		goto fail;
+		goto free_fsi;
 	inode->i_ino = 1;
 	inode->i_mtime = inode->i_atime = inode->i_ctime = CURRENT_TIME;
 	inode->i_blocks = 0;
@@ -155,6 +180,9 @@ devpts_fill_super(struct super_block *s, void *data, int silent)
 	
 	printk("devpts: get root dentry failed\n");
 	iput(inode);
+
+free_fsi:
+	kfree(s->s_fs_info);
 fail:
 	return -ENOMEM;
 }
@@ -165,11 +193,19 @@ static int devpts_get_sb(struct file_system_type *fs_type,
 	return get_sb_single(fs_type, flags, data, devpts_fill_super, mnt);
 }
 
+static void devpts_kill_sb(struct super_block *sb)
+{
+	struct pts_fs_info *fsi = DEVPTS_SB(sb);
+
+	kfree(fsi);
+	kill_anon_super(sb);
+}
+
 static struct file_system_type devpts_fs_type = {
 	.owner		= THIS_MODULE,
 	.name		= "devpts",
 	.get_sb		= devpts_get_sb,
-	.kill_sb	= kill_anon_super,
+	.kill_sb	= devpts_kill_sb,
 };
 
 /*
@@ -179,16 +215,18 @@ static struct file_system_type devpts_fs_type = {
 
 int devpts_new_index(struct inode *ptmx_inode)
 {
+	struct super_block *sb = pts_sb_from_inode(ptmx_inode);
+	struct pts_fs_info *fsi = DEVPTS_SB(sb);
 	int index;
 	int ida_ret;
 
 retry:
-	if (!ida_pre_get(&allocated_ptys, GFP_KERNEL)) {
+	if (!ida_pre_get(&fsi->allocated_ptys, GFP_KERNEL)) {
 		return -ENOMEM;
 	}
 
 	mutex_lock(&allocated_ptys_lock);
-	ida_ret = ida_get_new(&allocated_ptys, &index);
+	ida_ret = ida_get_new(&fsi->allocated_ptys, &index);
 	if (ida_ret < 0) {
 		mutex_unlock(&allocated_ptys_lock);
 		if (ida_ret == -EAGAIN)
@@ -197,7 +235,7 @@ retry:
 	}
 
 	if (index >= pty_limit) {
-		ida_remove(&allocated_ptys, index);
+		ida_remove(&fsi->allocated_ptys, index);
 		mutex_unlock(&allocated_ptys_lock);
 		return -EIO;
 	}
@@ -207,8 +245,11 @@ retry:
 
 void devpts_kill_index(struct inode *ptmx_inode, int idx)
 {
+	struct super_block *sb = pts_sb_from_inode(ptmx_inode);
+	struct pts_fs_info *fsi = DEVPTS_SB(sb);
+
 	mutex_lock(&allocated_ptys_lock);
-	ida_remove(&allocated_ptys, idx);
+	ida_remove(&fsi->allocated_ptys, idx);
 	mutex_unlock(&allocated_ptys_lock);
 }
 
-- 
1.5.2.5

Re: [PATCH 02/10] Per-mount allocated_ptys

[Serge E. Hallyn]

Quoting sukadev-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org (sukadev-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org):
> 
> >From bdba42b68e2b6765615618fce7eb7d4c6f75685d Mon Sep 17 00:00:00 2001
> From: Sukadev Bhattiprolu <sukadev-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>
> Date: Tue, 9 Sep 2008 10:22:11 -0700
> Subject: [PATCH 02/10] Per-mount allocated_ptys
> 
> To enable multiple mounts of devpts, 'allocated_ptys' must be a per-mount
> variable rather than a global variable.  Move 'allocated_ptys' into the
> super_block's s_fs_info.
> 
> Changelog[v2]:
> 	Define and use DEVPTS_SB() wrapper.
> 
> Signed-off-by: Sukadev Bhattiprolu <sukadev-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>
> ---
>  fs/devpts/inode.c |   55 ++++++++++++++++++++++++++++++++++++++++++++++------
>  1 files changed, 48 insertions(+), 7 deletions(-)
> 
> diff --git a/fs/devpts/inode.c b/fs/devpts/inode.c
> index ec33833..6e63db7 100644
> --- a/fs/devpts/inode.c
> +++ b/fs/devpts/inode.c
> @@ -30,7 +30,6 @@
>  #define PTMX_MINOR	2
> 
>  extern int pty_limit;			/* Config limit on Unix98 ptys */
> -static DEFINE_IDA(allocated_ptys);
>  static DEFINE_MUTEX(allocated_ptys_lock);
> 
>  static struct vfsmount *devpts_mnt;
> @@ -55,6 +54,15 @@ static match_table_t tokens = {
>  	{Opt_err, NULL}
>  };
> 
> +struct pts_fs_info {
> +	struct ida allocated_ptys;
> +};
> +
> +static inline struct pts_fs_info *DEVPTS_SB(struct super_block *sb)

This is an odd name...  how about DEVPTS_SBINFO or DEVPTS_SB_IDA?

> +{
> +	return sb->s_fs_info;
> +}
> +
>  static inline struct super_block *pts_sb_from_inode(struct inode *inode)
>  {
>  	if (inode->i_sb->s_magic == DEVPTS_SUPER_MAGIC)
> @@ -126,6 +134,19 @@ static const struct super_operations devpts_sops = {
>  	.show_options	= devpts_show_options,
>  };
> 
> +static void *new_pts_fs_info(void)
> +{
> +	struct pts_fs_info *fsi;
> +
> +	fsi = kzalloc(sizeof(struct pts_fs_info), GFP_KERNEL);
> +	if (!fsi)
> +		return NULL;
> +
> +	ida_init(&fsi->allocated_ptys);
> +
> +	return fsi;
> +}
> +
>  static int
>  devpts_fill_super(struct super_block *s, void *data, int silent)
>  {
> @@ -137,9 +158,13 @@ devpts_fill_super(struct super_block *s, void *data, int silent)
>  	s->s_op = &devpts_sops;
>  	s->s_time_gran = 1;
> 
> +	s->s_fs_info = new_pts_fs_info();
> +	if (!s->s_fs_info)
> +		goto fail;
> +
>  	inode = new_inode(s);
>  	if (!inode)
> -		goto fail;
> +		goto free_fsi;
>  	inode->i_ino = 1;
>  	inode->i_mtime = inode->i_atime = inode->i_ctime = CURRENT_TIME;
>  	inode->i_blocks = 0;
> @@ -155,6 +180,9 @@ devpts_fill_super(struct super_block *s, void *data, int silent)
>  	
>  	printk("devpts: get root dentry failed\n");
>  	iput(inode);
> +
> +free_fsi:
> +	kfree(s->s_fs_info);
>  fail:
>  	return -ENOMEM;
>  }
> @@ -165,11 +193,19 @@ static int devpts_get_sb(struct file_system_type *fs_type,
>  	return get_sb_single(fs_type, flags, data, devpts_fill_super, mnt);
>  }
> 
> +static void devpts_kill_sb(struct super_block *sb)
> +{
> +	struct pts_fs_info *fsi = DEVPTS_SB(sb);
> +

don't you need to ida_destroy(fsi->allocated_ptys) ?

> +	kfree(fsi);
> +	kill_anon_super(sb);
> +}
> +
>  static struct file_system_type devpts_fs_type = {
>  	.owner		= THIS_MODULE,
>  	.name		= "devpts",
>  	.get_sb		= devpts_get_sb,
> -	.kill_sb	= kill_anon_super,
> +	.kill_sb	= devpts_kill_sb,
>  };
> 
>  /*
> @@ -179,16 +215,18 @@ static struct file_system_type devpts_fs_type = {
> 
>  int devpts_new_index(struct inode *ptmx_inode)
>  {
> +	struct super_block *sb = pts_sb_from_inode(ptmx_inode);
> +	struct pts_fs_info *fsi = DEVPTS_SB(sb);
>  	int index;
>  	int ida_ret;
> 
>  retry:
> -	if (!ida_pre_get(&allocated_ptys, GFP_KERNEL)) {
> +	if (!ida_pre_get(&fsi->allocated_ptys, GFP_KERNEL)) {
>  		return -ENOMEM;
>  	}
> 
>  	mutex_lock(&allocated_ptys_lock);
> -	ida_ret = ida_get_new(&allocated_ptys, &index);
> +	ida_ret = ida_get_new(&fsi->allocated_ptys, &index);
>  	if (ida_ret < 0) {
>  		mutex_unlock(&allocated_ptys_lock);
>  		if (ida_ret == -EAGAIN)
> @@ -197,7 +235,7 @@ retry:
>  	}
> 
>  	if (index >= pty_limit) {
> -		ida_remove(&allocated_ptys, index);
> +		ida_remove(&fsi->allocated_ptys, index);
>  		mutex_unlock(&allocated_ptys_lock);
>  		return -EIO;
>  	}
> @@ -207,8 +245,11 @@ retry:
> 
>  void devpts_kill_index(struct inode *ptmx_inode, int idx)
>  {
> +	struct super_block *sb = pts_sb_from_inode(ptmx_inode);
> +	struct pts_fs_info *fsi = DEVPTS_SB(sb);
> +
>  	mutex_lock(&allocated_ptys_lock);
> -	ida_remove(&allocated_ptys, idx);
> +	ida_remove(&fsi->allocated_ptys, idx);
>  	mutex_unlock(&allocated_ptys_lock);
>  }
> 
> -- 
> 1.5.2.5

Re: [PATCH 02/10] Per-mount allocated_ptys

[sukadev-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8]

Serge E. Hallyn [serue-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org] wrote:
| Quoting sukadev-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org (sukadev-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org):
| > 
| > >From bdba42b68e2b6765615618fce7eb7d4c6f75685d Mon Sep 17 00:00:00 2001
| > From: Sukadev Bhattiprolu <sukadev-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>
| > Date: Tue, 9 Sep 2008 10:22:11 -0700
| > Subject: [PATCH 02/10] Per-mount allocated_ptys
| > 
| > To enable multiple mounts of devpts, 'allocated_ptys' must be a per-mount
| > variable rather than a global variable.  Move 'allocated_ptys' into the
| > super_block's s_fs_info.
| > 
| > Changelog[v2]:
| > 	Define and use DEVPTS_SB() wrapper.
| > 
| > Signed-off-by: Sukadev Bhattiprolu <sukadev-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>
| > ---
| >  fs/devpts/inode.c |   55 ++++++++++++++++++++++++++++++++++++++++++++++------
| >  1 files changed, 48 insertions(+), 7 deletions(-)
| > 
| > diff --git a/fs/devpts/inode.c b/fs/devpts/inode.c
| > index ec33833..6e63db7 100644
| > --- a/fs/devpts/inode.c
| > +++ b/fs/devpts/inode.c
| > @@ -30,7 +30,6 @@
| >  #define PTMX_MINOR	2
| > 
| >  extern int pty_limit;			/* Config limit on Unix98 ptys */
| > -static DEFINE_IDA(allocated_ptys);
| >  static DEFINE_MUTEX(allocated_ptys_lock);
| > 
| >  static struct vfsmount *devpts_mnt;
| > @@ -55,6 +54,15 @@ static match_table_t tokens = {
| >  	{Opt_err, NULL}
| >  };
| > 
| > +struct pts_fs_info {
| > +	struct ida allocated_ptys;
| > +};
| > +
| > +static inline struct pts_fs_info *DEVPTS_SB(struct super_block *sb)
| 
| This is an odd name...  how about DEVPTS_SBINFO or DEVPTS_SB_IDA?

I was just following established tradition :-)

	EXT4_SB(), EXT3_SB(), EXT2_SB(), REISERFS_SB(), NFS_SB()

I don't mind DEVPTS_SBINFO(), but since this will soon have more than
just an ida, DEVPTS_SB_IDA() may not work.

| 
| > +{
| > +	return sb->s_fs_info;
| > +}
| > +
| >  static inline struct super_block *pts_sb_from_inode(struct inode *inode)
| >  {
| >  	if (inode->i_sb->s_magic == DEVPTS_SUPER_MAGIC)
| > @@ -126,6 +134,19 @@ static const struct super_operations devpts_sops = {
| >  	.show_options	= devpts_show_options,
| >  };
| > 
| > +static void *new_pts_fs_info(void)
| > +{
| > +	struct pts_fs_info *fsi;
| > +
| > +	fsi = kzalloc(sizeof(struct pts_fs_info), GFP_KERNEL);
| > +	if (!fsi)
| > +		return NULL;
| > +
| > +	ida_init(&fsi->allocated_ptys);
| > +
| > +	return fsi;
| > +}
| > +
| >  static int
| >  devpts_fill_super(struct super_block *s, void *data, int silent)
| >  {
| > @@ -137,9 +158,13 @@ devpts_fill_super(struct super_block *s, void *data, int silent)
| >  	s->s_op = &devpts_sops;
| >  	s->s_time_gran = 1;
| > 
| > +	s->s_fs_info = new_pts_fs_info();
| > +	if (!s->s_fs_info)
| > +		goto fail;
| > +
| >  	inode = new_inode(s);
| >  	if (!inode)
| > -		goto fail;
| > +		goto free_fsi;
| >  	inode->i_ino = 1;
| >  	inode->i_mtime = inode->i_atime = inode->i_ctime = CURRENT_TIME;
| >  	inode->i_blocks = 0;
| > @@ -155,6 +180,9 @@ devpts_fill_super(struct super_block *s, void *data, int silent)
| >  	
| >  	printk("devpts: get root dentry failed\n");
| >  	iput(inode);
| > +
| > +free_fsi:
| > +	kfree(s->s_fs_info);
| >  fail:
| >  	return -ENOMEM;
| >  }
| > @@ -165,11 +193,19 @@ static int devpts_get_sb(struct file_system_type *fs_type,
| >  	return get_sb_single(fs_type, flags, data, devpts_fill_super, mnt);
| >  }
| > 
| > +static void devpts_kill_sb(struct super_block *sb)
| > +{
| > +	struct pts_fs_info *fsi = DEVPTS_SB(sb);
| > +
| 
| don't you need to ida_destroy(fsi->allocated_ptys) ?

I had it before, but since all ptys must be destroyed, and ida_remove()
frees the pages as necessary and ida_init() does not alloc memory, I had
commented it out and later removed it. Let me check again.

| 
| > +	kfree(fsi);
| > +	kill_anon_super(sb);
| > +}
| > +
| >  static struct file_system_type devpts_fs_type = {
| >  	.owner		= THIS_MODULE,
| >  	.name		= "devpts",
| >  	.get_sb		= devpts_get_sb,
| > -	.kill_sb	= kill_anon_super,
| > +	.kill_sb	= devpts_kill_sb,
| >  };
| > 
| >  /*
| > @@ -179,16 +215,18 @@ static struct file_system_type devpts_fs_type = {
| > 
| >  int devpts_new_index(struct inode *ptmx_inode)
| >  {
| > +	struct super_block *sb = pts_sb_from_inode(ptmx_inode);
| > +	struct pts_fs_info *fsi = DEVPTS_SB(sb);
| >  	int index;
| >  	int ida_ret;
| > 
| >  retry:
| > -	if (!ida_pre_get(&allocated_ptys, GFP_KERNEL)) {
| > +	if (!ida_pre_get(&fsi->allocated_ptys, GFP_KERNEL)) {
| >  		return -ENOMEM;
| >  	}
| > 
| >  	mutex_lock(&allocated_ptys_lock);
| > -	ida_ret = ida_get_new(&allocated_ptys, &index);
| > +	ida_ret = ida_get_new(&fsi->allocated_ptys, &index);
| >  	if (ida_ret < 0) {
| >  		mutex_unlock(&allocated_ptys_lock);
| >  		if (ida_ret == -EAGAIN)
| > @@ -197,7 +235,7 @@ retry:
| >  	}
| > 
| >  	if (index >= pty_limit) {
| > -		ida_remove(&allocated_ptys, index);
| > +		ida_remove(&fsi->allocated_ptys, index);
| >  		mutex_unlock(&allocated_ptys_lock);
| >  		return -EIO;
| >  	}
| > @@ -207,8 +245,11 @@ retry:
| > 
| >  void devpts_kill_index(struct inode *ptmx_inode, int idx)
| >  {
| > +	struct super_block *sb = pts_sb_from_inode(ptmx_inode);
| > +	struct pts_fs_info *fsi = DEVPTS_SB(sb);
| > +
| >  	mutex_lock(&allocated_ptys_lock);
| > -	ida_remove(&allocated_ptys, idx);
| > +	ida_remove(&fsi->allocated_ptys, idx);
| >  	mutex_unlock(&allocated_ptys_lock);
| >  }
| > 
| > -- 
| > 1.5.2.5

[PATCH 03/10] Per-mount 'config' object

[sukadev-r/Jw6+rmf7HQT0dZR+AlfA]

From 449652761e1d0c620a01577002ad655adac9a1c5 Mon Sep 17 00:00:00 2001
From: Sukadev Bhattiprolu <sukadev-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>
Date: Tue, 9 Sep 2008 10:22:53 -0700
Subject: [PATCH 03/10] Per-mount 'config' object

With support for multiple mounts of devpts, the 'config' structure really
represents per-mount options rather than config parameters. Rename 'config'
structure to 'pts_mount_opts' and store it in the super-block.

Signed-off-by: Sukadev Bhattiprolu <sukadev-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>
---
 fs/devpts/inode.c |   49 +++++++++++++++++++++++++++++--------------------
 1 files changed, 29 insertions(+), 20 deletions(-)

diff --git a/fs/devpts/inode.c b/fs/devpts/inode.c
index 6e63db7..e91c15c 100644
--- a/fs/devpts/inode.c
+++ b/fs/devpts/inode.c
@@ -34,13 +34,13 @@ static DEFINE_MUTEX(allocated_ptys_lock);
 
 static struct vfsmount *devpts_mnt;
 
-static struct {
+struct pts_mount_opts {
 	int setuid;
 	int setgid;
 	uid_t   uid;
 	gid_t   gid;
 	umode_t mode;
-} config = {.mode = DEVPTS_DEFAULT_MODE};
+};
 
 enum {
 	Opt_uid, Opt_gid, Opt_mode,
@@ -56,6 +56,7 @@ static match_table_t tokens = {
 
 struct pts_fs_info {
 	struct ida allocated_ptys;
+	struct pts_mount_opts mount_opts;
 };
 
 static inline struct pts_fs_info *DEVPTS_SB(struct super_block *sb)
@@ -74,12 +75,14 @@ static inline struct super_block *pts_sb_from_inode(struct inode *inode)
 static int devpts_remount(struct super_block *sb, int *flags, char *data)
 {
 	char *p;
+	struct pts_fs_info *fsi = DEVPTS_SB(sb);
+	struct pts_mount_opts *opts = &fsi->mount_opts;
 
-	config.setuid  = 0;
-	config.setgid  = 0;
-	config.uid     = 0;
-	config.gid     = 0;
-	config.mode    = DEVPTS_DEFAULT_MODE;
+	opts->setuid  = 0;
+	opts->setgid  = 0;
+	opts->uid     = 0;
+	opts->gid     = 0;
+	opts->mode    = DEVPTS_DEFAULT_MODE;
 
 	while ((p = strsep(&data, ",")) != NULL) {
 		substring_t args[MAX_OPT_ARGS];
@@ -94,19 +97,19 @@ static int devpts_remount(struct super_block *sb, int *flags, char *data)
 		case Opt_uid:
 			if (match_int(&args[0], &option))
 				return -EINVAL;
-			config.uid = option;
-			config.setuid = 1;
+			opts->uid = option;
+			opts->setuid = 1;
 			break;
 		case Opt_gid:
 			if (match_int(&args[0], &option))
 				return -EINVAL;
-			config.gid = option;
-			config.setgid = 1;
+			opts->gid = option;
+			opts->setgid = 1;
 			break;
 		case Opt_mode:
 			if (match_octal(&args[0], &option))
 				return -EINVAL;
-			config.mode = option & S_IALLUGO;
+			opts->mode = option & S_IALLUGO;
 			break;
 		default:
 			printk(KERN_ERR "devpts: called with bogus options\n");
@@ -119,11 +122,14 @@ static int devpts_remount(struct super_block *sb, int *flags, char *data)
 
 static int devpts_show_options(struct seq_file *seq, struct vfsmount *vfs)
 {
-	if (config.setuid)
-		seq_printf(seq, ",uid=%u", config.uid);
-	if (config.setgid)
-		seq_printf(seq, ",gid=%u", config.gid);
-	seq_printf(seq, ",mode=%03o", config.mode);
+	struct pts_fs_info *fsi = DEVPTS_SB(vfs->mnt_sb);
+	struct pts_mount_opts *opts = &fsi->mount_opts;
+
+	if (opts->setuid)
+		seq_printf(seq, ",uid=%u", opts->uid);
+	if (opts->setgid)
+		seq_printf(seq, ",gid=%u", opts->gid);
+	seq_printf(seq, ",mode=%03o", opts->mode);
 
 	return 0;
 }
@@ -143,6 +149,7 @@ static void *new_pts_fs_info(void)
 		return NULL;
 
 	ida_init(&fsi->allocated_ptys);
+	fsi->mount_opts.mode = DEVPTS_DEFAULT_MODE;
 
 	return fsi;
 }
@@ -262,6 +269,8 @@ int devpts_pty_new(struct inode *ptmx_inode, struct tty_struct *tty)
 	struct super_block *sb = pts_sb_from_inode(ptmx_inode);
 	struct inode *inode = new_inode(sb);
 	struct dentry *root = sb->s_root;
+	struct pts_fs_info *fsi = DEVPTS_SB(sb);
+	struct pts_mount_opts *opts = &fsi->mount_opts;
 	char s[12];
 
 	/* We're supposed to be given the slave end of a pty */
@@ -272,10 +281,10 @@ int devpts_pty_new(struct inode *ptmx_inode, struct tty_struct *tty)
 		return -ENOMEM;
 
 	inode->i_ino = number+2;
-	inode->i_uid = config.setuid ? config.uid : current->fsuid;
-	inode->i_gid = config.setgid ? config.gid : current->fsgid;
+	inode->i_uid = opts->setuid ? opts->uid : current->fsuid;
+	inode->i_gid = opts->setgid ? opts->gid : current->fsgid;
 	inode->i_mtime = inode->i_atime = inode->i_ctime = CURRENT_TIME;
-	init_special_inode(inode, S_IFCHR|config.mode, device);
+	init_special_inode(inode, S_IFCHR|opts->mode, device);
 	inode->i_private = tty;
 	tty->driver_data = inode;
 
-- 
1.5.2.5

Re: [PATCH 03/10] Per-mount 'config' object

[Serge E. Hallyn]

Quoting sukadev-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org (sukadev-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org):
> 
> >From 449652761e1d0c620a01577002ad655adac9a1c5 Mon Sep 17 00:00:00 2001
> From: Sukadev Bhattiprolu <sukadev-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>
> Date: Tue, 9 Sep 2008 10:22:53 -0700
> Subject: [PATCH 03/10] Per-mount 'config' object
> 
> With support for multiple mounts of devpts, the 'config' structure really
> represents per-mount options rather than config parameters. Rename 'config'
> structure to 'pts_mount_opts' and store it in the super-block.
> 
> Signed-off-by: Sukadev Bhattiprolu <sukadev-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>

Acked-by: Serge Hallyn <serue-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>

(except, of course, for wanting the DEVPTS_SB fn to be renamed)

> ---
>  fs/devpts/inode.c |   49 +++++++++++++++++++++++++++++--------------------
>  1 files changed, 29 insertions(+), 20 deletions(-)
> 
> diff --git a/fs/devpts/inode.c b/fs/devpts/inode.c
> index 6e63db7..e91c15c 100644
> --- a/fs/devpts/inode.c
> +++ b/fs/devpts/inode.c
> @@ -34,13 +34,13 @@ static DEFINE_MUTEX(allocated_ptys_lock);
> 
>  static struct vfsmount *devpts_mnt;
> 
> -static struct {
> +struct pts_mount_opts {
>  	int setuid;
>  	int setgid;
>  	uid_t   uid;
>  	gid_t   gid;
>  	umode_t mode;
> -} config = {.mode = DEVPTS_DEFAULT_MODE};
> +};
> 
>  enum {
>  	Opt_uid, Opt_gid, Opt_mode,
> @@ -56,6 +56,7 @@ static match_table_t tokens = {
> 
>  struct pts_fs_info {
>  	struct ida allocated_ptys;
> +	struct pts_mount_opts mount_opts;
>  };
> 
>  static inline struct pts_fs_info *DEVPTS_SB(struct super_block *sb)
> @@ -74,12 +75,14 @@ static inline struct super_block *pts_sb_from_inode(struct inode *inode)
>  static int devpts_remount(struct super_block *sb, int *flags, char *data)
>  {
>  	char *p;
> +	struct pts_fs_info *fsi = DEVPTS_SB(sb);
> +	struct pts_mount_opts *opts = &fsi->mount_opts;
> 
> -	config.setuid  = 0;
> -	config.setgid  = 0;
> -	config.uid     = 0;
> -	config.gid     = 0;
> -	config.mode    = DEVPTS_DEFAULT_MODE;
> +	opts->setuid  = 0;
> +	opts->setgid  = 0;
> +	opts->uid     = 0;
> +	opts->gid     = 0;
> +	opts->mode    = DEVPTS_DEFAULT_MODE;
> 
>  	while ((p = strsep(&data, ",")) != NULL) {
>  		substring_t args[MAX_OPT_ARGS];
> @@ -94,19 +97,19 @@ static int devpts_remount(struct super_block *sb, int *flags, char *data)
>  		case Opt_uid:
>  			if (match_int(&args[0], &option))
>  				return -EINVAL;
> -			config.uid = option;
> -			config.setuid = 1;
> +			opts->uid = option;
> +			opts->setuid = 1;
>  			break;
>  		case Opt_gid:
>  			if (match_int(&args[0], &option))
>  				return -EINVAL;
> -			config.gid = option;
> -			config.setgid = 1;
> +			opts->gid = option;
> +			opts->setgid = 1;
>  			break;
>  		case Opt_mode:
>  			if (match_octal(&args[0], &option))
>  				return -EINVAL;
> -			config.mode = option & S_IALLUGO;
> +			opts->mode = option & S_IALLUGO;
>  			break;
>  		default:
>  			printk(KERN_ERR "devpts: called with bogus options\n");
> @@ -119,11 +122,14 @@ static int devpts_remount(struct super_block *sb, int *flags, char *data)
> 
>  static int devpts_show_options(struct seq_file *seq, struct vfsmount *vfs)
>  {
> -	if (config.setuid)
> -		seq_printf(seq, ",uid=%u", config.uid);
> -	if (config.setgid)
> -		seq_printf(seq, ",gid=%u", config.gid);
> -	seq_printf(seq, ",mode=%03o", config.mode);
> +	struct pts_fs_info *fsi = DEVPTS_SB(vfs->mnt_sb);
> +	struct pts_mount_opts *opts = &fsi->mount_opts;
> +
> +	if (opts->setuid)
> +		seq_printf(seq, ",uid=%u", opts->uid);
> +	if (opts->setgid)
> +		seq_printf(seq, ",gid=%u", opts->gid);
> +	seq_printf(seq, ",mode=%03o", opts->mode);
> 
>  	return 0;
>  }
> @@ -143,6 +149,7 @@ static void *new_pts_fs_info(void)
>  		return NULL;
> 
>  	ida_init(&fsi->allocated_ptys);
> +	fsi->mount_opts.mode = DEVPTS_DEFAULT_MODE;
> 
>  	return fsi;
>  }
> @@ -262,6 +269,8 @@ int devpts_pty_new(struct inode *ptmx_inode, struct tty_struct *tty)
>  	struct super_block *sb = pts_sb_from_inode(ptmx_inode);
>  	struct inode *inode = new_inode(sb);
>  	struct dentry *root = sb->s_root;
> +	struct pts_fs_info *fsi = DEVPTS_SB(sb);
> +	struct pts_mount_opts *opts = &fsi->mount_opts;
>  	char s[12];
> 
>  	/* We're supposed to be given the slave end of a pty */
> @@ -272,10 +281,10 @@ int devpts_pty_new(struct inode *ptmx_inode, struct tty_struct *tty)
>  		return -ENOMEM;
> 
>  	inode->i_ino = number+2;
> -	inode->i_uid = config.setuid ? config.uid : current->fsuid;
> -	inode->i_gid = config.setgid ? config.gid : current->fsgid;
> +	inode->i_uid = opts->setuid ? opts->uid : current->fsuid;
> +	inode->i_gid = opts->setgid ? opts->gid : current->fsgid;
>  	inode->i_mtime = inode->i_atime = inode->i_ctime = CURRENT_TIME;
> -	init_special_inode(inode, S_IFCHR|config.mode, device);
> +	init_special_inode(inode, S_IFCHR|opts->mode, device);
>  	inode->i_private = tty;
>  	tty->driver_data = inode;
> 
> -- 
> 1.5.2.5

[PATCH 04/10] Extract option parsing to new function

[sukadev-r/Jw6+rmf7HQT0dZR+AlfA]

From 91a31d2286a22a2cfa03f9ddea44fd2eaf5fa576 Mon Sep 17 00:00:00 2001
From: Sukadev Bhattiprolu <sukadev-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>
Date: Tue, 9 Sep 2008 10:23:00 -0700
Subject: [PATCH 04/10] Extract option parsing to new function

Move code to parse mount options into a separate function so it can
(later) be shared between mount and remount operations.

Signed-off-by: Sukadev Bhattiprolu <sukadev-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>
---
 fs/devpts/inode.c |   12 +++++++++---
 1 files changed, 9 insertions(+), 3 deletions(-)

diff --git a/fs/devpts/inode.c b/fs/devpts/inode.c
index e91c15c..7ae60aa 100644
--- a/fs/devpts/inode.c
+++ b/fs/devpts/inode.c
@@ -72,11 +72,9 @@ static inline struct super_block *pts_sb_from_inode(struct inode *inode)
 	return devpts_mnt->mnt_sb;
 }
 
-static int devpts_remount(struct super_block *sb, int *flags, char *data)
+static int parse_mount_options(char *data, struct pts_mount_opts *opts)
 {
 	char *p;
-	struct pts_fs_info *fsi = DEVPTS_SB(sb);
-	struct pts_mount_opts *opts = &fsi->mount_opts;
 
 	opts->setuid  = 0;
 	opts->setgid  = 0;
@@ -120,6 +118,14 @@ static int devpts_remount(struct super_block *sb, int *flags, char *data)
 	return 0;
 }
 
+static int devpts_remount(struct super_block *sb, int *flags, char *data)
+{
+	struct pts_fs_info *fsi = DEVPTS_SB(sb);
+	struct pts_mount_opts *opts = &fsi->mount_opts;
+
+	return parse_mount_options(data, opts);
+}
+
 static int devpts_show_options(struct seq_file *seq, struct vfsmount *vfs)
 {
 	struct pts_fs_info *fsi = DEVPTS_SB(vfs->mnt_sb);
-- 
1.5.2.5

Re: [PATCH 04/10] Extract option parsing to new function

[Serge E. Hallyn]

Quoting sukadev-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org (sukadev-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org):
> 
> >From 91a31d2286a22a2cfa03f9ddea44fd2eaf5fa576 Mon Sep 17 00:00:00 2001
> From: Sukadev Bhattiprolu <sukadev-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>
> Date: Tue, 9 Sep 2008 10:23:00 -0700
> Subject: [PATCH 04/10] Extract option parsing to new function
> 
> Move code to parse mount options into a separate function so it can
> (later) be shared between mount and remount operations.
> 
> Signed-off-by: Sukadev Bhattiprolu <sukadev-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>

Acked-by: Serge Hallyn <serue-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>

> ---
>  fs/devpts/inode.c |   12 +++++++++---
>  1 files changed, 9 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/devpts/inode.c b/fs/devpts/inode.c
> index e91c15c..7ae60aa 100644
> --- a/fs/devpts/inode.c
> +++ b/fs/devpts/inode.c
> @@ -72,11 +72,9 @@ static inline struct super_block *pts_sb_from_inode(struct inode *inode)
>  	return devpts_mnt->mnt_sb;
>  }
> 
> -static int devpts_remount(struct super_block *sb, int *flags, char *data)
> +static int parse_mount_options(char *data, struct pts_mount_opts *opts)
>  {
>  	char *p;
> -	struct pts_fs_info *fsi = DEVPTS_SB(sb);
> -	struct pts_mount_opts *opts = &fsi->mount_opts;
> 
>  	opts->setuid  = 0;
>  	opts->setgid  = 0;
> @@ -120,6 +118,14 @@ static int devpts_remount(struct super_block *sb, int *flags, char *data)
>  	return 0;
>  }
> 
> +static int devpts_remount(struct super_block *sb, int *flags, char *data)
> +{
> +	struct pts_fs_info *fsi = DEVPTS_SB(sb);
> +	struct pts_mount_opts *opts = &fsi->mount_opts;
> +
> +	return parse_mount_options(data, opts);
> +}
> +
>  static int devpts_show_options(struct seq_file *seq, struct vfsmount *vfs)
>  {
>  	struct pts_fs_info *fsi = DEVPTS_SB(vfs->mnt_sb);
> -- 
> 1.5.2.5

[PATCH 05/10] Add DEVPTS_MULTIPLE_INSTANCES config token

[sukadev-r/Jw6+rmf7HQT0dZR+AlfA]

From d443597559ded0785804716ed58c990ee477d56b Mon Sep 17 00:00:00 2001
From: Sukadev Bhattiprolu <sukadev-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>
Date: Tue, 9 Sep 2008 12:00:51 -0700
Subject: [PATCH 05/10] Add DEVPTS_MULTIPLE_INSTANCES config token

---
 drivers/char/Kconfig |   11 +++++++++++
 1 files changed, 11 insertions(+), 0 deletions(-)

diff --git a/drivers/char/Kconfig b/drivers/char/Kconfig
index 700ff96..0d3ea89 100644
--- a/drivers/char/Kconfig
+++ b/drivers/char/Kconfig
@@ -443,6 +443,17 @@ config UNIX98_PTYS
 	  All modern Linux systems use the Unix98 ptys.  Say Y unless
 	  you're on an embedded system and want to conserve memory.
 
+config DEVPTS_MULTIPLE_INSTANCES
+	bool "Support multiple instances of devpts"
+	depends on UNIX98_PTYS
+	default n
+	---help---
+	  Enable support for multiple instances of devpts filesystem.
+	  If you want to have isolated PTY namespaces (eg: in containers),
+	  say Y here.  Otherwise, say N. If enabled, each mount of devpts
+	  filesystem with the '-o newinstance' option will create an
+	  independent PTY namespace.
+
 config LEGACY_PTYS
 	bool "Legacy (BSD) PTY support"
 	default y
-- 
1.5.2.5

Re: [PATCH 05/10] Add DEVPTS_MULTIPLE_INSTANCES config token

[Serge E. Hallyn]

Quoting sukadev-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org (sukadev-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org):
> 
> >From d443597559ded0785804716ed58c990ee477d56b Mon Sep 17 00:00:00 2001
> From: Sukadev Bhattiprolu <sukadev-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>
> Date: Tue, 9 Sep 2008 12:00:51 -0700
> Subject: [PATCH 05/10] Add DEVPTS_MULTIPLE_INSTANCES config token
> 

No sign-off?

Acked-by: Serge Hallyn <serue-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>

> ---
>  drivers/char/Kconfig |   11 +++++++++++
>  1 files changed, 11 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/char/Kconfig b/drivers/char/Kconfig
> index 700ff96..0d3ea89 100644
> --- a/drivers/char/Kconfig
> +++ b/drivers/char/Kconfig
> @@ -443,6 +443,17 @@ config UNIX98_PTYS
>  	  All modern Linux systems use the Unix98 ptys.  Say Y unless
>  	  you're on an embedded system and want to conserve memory.
> 
> +config DEVPTS_MULTIPLE_INSTANCES
> +	bool "Support multiple instances of devpts"
> +	depends on UNIX98_PTYS
> +	default n
> +	---help---
> +	  Enable support for multiple instances of devpts filesystem.
> +	  If you want to have isolated PTY namespaces (eg: in containers),
> +	  say Y here.  Otherwise, say N. If enabled, each mount of devpts
> +	  filesystem with the '-o newinstance' option will create an
> +	  independent PTY namespace.
> +
>  config LEGACY_PTYS
>  	bool "Legacy (BSD) PTY support"
>  	default y
> -- 
> 1.5.2.5

[PATCH 06/10] Define mknod_ptmx()

[sukadev-r/Jw6+rmf7HQT0dZR+AlfA]

From df5d5863f8601baa25237b60c3fcd614ee0b34aa Mon Sep 17 00:00:00 2001
From: Sukadev Bhattiprolu <sukadev-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>
Date: Tue, 9 Sep 2008 10:23:07 -0700
Subject: [PATCH 06/10] Define mknod_ptmx()

/dev/ptmx is closely tied to the devpts filesystem. An open of /dev/ptmx,
allocates the next pty index and the associated device shows up in the
devpts fs as /dev/pts/n.

Wih multiple instancs of devpts filesystem, during an open of /dev/ptmx
we would be unable to determine which instance of the devpts is being
accessed.

So we move the 'ptmx' node into /dev/pts and use the inode of the 'ptmx'
node to identify the superblock and hence the devpts instance.  This patch
adds ability for the kernel to internally create the [ptmx, c, 5:2] device
when mounting devpts filesystem.  Since the ptmx node in devpts is new and
may surprise some userspace scripts, the default permissions for the new
node is 0000.  These permissions can be changed either using chmod or by
remounting with the new '-o ptmxmode=0666' mount option.

Changelog[v4]:
	- Change default permissions of pts/ptmx node to 0000.
	- Move code for ptmxmode under #ifdef CONFIG_DEVPTS_MULTIPLE_INSTANCES.

Changelog[v3]:
	- Rename ptmx_mode to ptmxmode (for consistency with 'newinstance')

Changelog[v2]:
	- [H. Peter Anvin] Remove mknod() system call support and create the
	  ptmx node internally.

Changelog[v1]:
	- Earlier version of this patch enabled creating /dev/pts/tty as
	  well. As pointed out by Al Viro and H. Peter Anvin, that is not
	  really necessary.

Signed-off-by: Sukadev Bhattiprolu <sukadev-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>
---
 fs/devpts/inode.c |   76 +++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 files changed, 74 insertions(+), 2 deletions(-)

diff --git a/fs/devpts/inode.c b/fs/devpts/inode.c
index 7ae60aa..17e14f5 100644
--- a/fs/devpts/inode.c
+++ b/fs/devpts/inode.c
@@ -27,6 +27,13 @@
 #define DEVPTS_SUPER_MAGIC 0x1cd1
 
 #define DEVPTS_DEFAULT_MODE 0600
+/*
+ * ptmx is a new node in /dev/pts and will be unused in legacy (single-
+ * instance) mode. To prevent surprises in user space, set permissions of
+ * ptmx to 0. Use 'chmod' or remount with '-o ptmxmode' to set meaningful
+ * permissions.
+ */
+#define DEVPTS_DEFAULT_PTMX_MODE 0000
 #define PTMX_MINOR	2
 
 extern int pty_limit;			/* Config limit on Unix98 ptys */
@@ -40,10 +47,11 @@ struct pts_mount_opts {
 	uid_t   uid;
 	gid_t   gid;
 	umode_t mode;
+	umode_t ptmxmode;
 };
 
 enum {
-	Opt_uid, Opt_gid, Opt_mode,
+	Opt_uid, Opt_gid, Opt_mode, Opt_ptmxmode,
 	Opt_err
 };
 
@@ -51,6 +59,9 @@ static match_table_t tokens = {
 	{Opt_uid, "uid=%u"},
 	{Opt_gid, "gid=%u"},
 	{Opt_mode, "mode=%o"},
+#ifdef CONFIG_DEVPTS_MULTIPLE_INSTANCES
+	{Opt_ptmxmode, "ptmxmode=%o"},
+#endif
 	{Opt_err, NULL}
 };
 
@@ -81,6 +92,7 @@ static int parse_mount_options(char *data, struct pts_mount_opts *opts)
 	opts->uid     = 0;
 	opts->gid     = 0;
 	opts->mode    = DEVPTS_DEFAULT_MODE;
+	opts->ptmxmode = DEVPTS_DEFAULT_PTMX_MODE;
 
 	while ((p = strsep(&data, ",")) != NULL) {
 		substring_t args[MAX_OPT_ARGS];
@@ -109,6 +121,13 @@ static int parse_mount_options(char *data, struct pts_mount_opts *opts)
 				return -EINVAL;
 			opts->mode = option & S_IALLUGO;
 			break;
+#ifdef CONFIG_DEVPTS_MULTIPLE_INSTANCES
+		case Opt_ptmxmode:
+			if (match_octal(&args[0], &option))
+				return -EINVAL;
+			opts->ptmxmode = option & S_IALLUGO;
+			break;
+#endif
 		default:
 			printk(KERN_ERR "devpts: called with bogus options\n");
 			return -EINVAL;
@@ -118,6 +137,55 @@ static int parse_mount_options(char *data, struct pts_mount_opts *opts)
 	return 0;
 }
 
+#ifdef CONFIG_DEVPTS_MULTIPLE_INSTANCES
+static int mknod_ptmx(struct super_block *sb)
+{
+	struct dentry *root;
+	struct dentry *dentry;
+	struct inode *inode;
+	struct pts_fs_info *fsi = DEVPTS_SB(sb);
+	struct pts_mount_opts *opts = &fsi->mount_opts;
+	int mode;
+
+	root = sb->s_root;
+	dentry = lookup_one_len("ptmx", root, 4);
+	if (IS_ERR(dentry)) {
+		printk(KERN_ERR "Unable to alloc dentry for ptmx node\n");
+		return -ENOMEM;
+	}
+
+	if (dentry->d_inode) {
+		printk(KERN_ERR "'ptmx' (ino %lu) exists in this devpts\n",
+				dentry->d_inode->i_ino);
+		return 0;
+	}
+
+	/*
+	 * Create a new 'ptmx' node in this mount of devpts.
+	 */
+	inode = new_inode(sb);
+	if (!inode) {
+		printk(KERN_ERR "Unable to alloc inode for ptmx node\n");
+		dput(dentry);
+		return -ENOMEM;
+	}
+
+	inode->i_uid = inode->i_gid = 0;
+	inode->i_blocks = 0;
+	inode->i_mtime = inode->i_atime = inode->i_ctime = CURRENT_TIME;
+
+	mode = S_IFCHR|opts->ptmxmode;
+	init_special_inode(inode, mode, MKDEV(TTYAUX_MAJOR, 2));
+
+	d_add(dentry, inode);
+
+	printk(KERN_DEBUG "Created ptmx node in devpts ino %lu\n",
+			inode->i_ino);
+
+	return 0;
+}
+#endif
+
 static int devpts_remount(struct super_block *sb, int *flags, char *data)
 {
 	struct pts_fs_info *fsi = DEVPTS_SB(sb);
@@ -136,6 +204,9 @@ static int devpts_show_options(struct seq_file *seq, struct vfsmount *vfs)
 	if (opts->setgid)
 		seq_printf(seq, ",gid=%u", opts->gid);
 	seq_printf(seq, ",mode=%03o", opts->mode);
+#ifdef CONFIG_DEVPTS_MULTIPLE_INSTANCES
+	seq_printf(seq, ",ptmxmode=%03o", opts->ptmxmode);
+#endif
 
 	return 0;
 }
@@ -156,6 +227,7 @@ static void *new_pts_fs_info(void)
 
 	ida_init(&fsi->allocated_ptys);
 	fsi->mount_opts.mode = DEVPTS_DEFAULT_MODE;
+	fsi->mount_opts.ptmxmode = DEVPTS_DEFAULT_PTMX_MODE;
 
 	return fsi;
 }
@@ -211,7 +283,7 @@ static void devpts_kill_sb(struct super_block *sb)
 	struct pts_fs_info *fsi = DEVPTS_SB(sb);
 
 	kfree(fsi);
-	kill_anon_super(sb);
+	kill_litter_super(sb);
 }
 
 static struct file_system_type devpts_fs_type = {
-- 
1.5.2.5

Re: [PATCH 06/10] Define mknod_ptmx()

[Serge E. Hallyn]

Quoting sukadev-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org (sukadev-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org):
> 
> >From df5d5863f8601baa25237b60c3fcd614ee0b34aa Mon Sep 17 00:00:00 2001
> From: Sukadev Bhattiprolu <sukadev-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>
> Date: Tue, 9 Sep 2008 10:23:07 -0700
> Subject: [PATCH 06/10] Define mknod_ptmx()
> 
> /dev/ptmx is closely tied to the devpts filesystem. An open of /dev/ptmx,
> allocates the next pty index and the associated device shows up in the
> devpts fs as /dev/pts/n.
> 
> Wih multiple instancs of devpts filesystem, during an open of /dev/ptmx
> we would be unable to determine which instance of the devpts is being
> accessed.
> 
> So we move the 'ptmx' node into /dev/pts and use the inode of the 'ptmx'
> node to identify the superblock and hence the devpts instance.  This patch
> adds ability for the kernel to internally create the [ptmx, c, 5:2] device
> when mounting devpts filesystem.  Since the ptmx node in devpts is new and
> may surprise some userspace scripts, the default permissions for the new
> node is 0000.  These permissions can be changed either using chmod or by
> remounting with the new '-o ptmxmode=0666' mount option.
> 
> Changelog[v4]:
> 	- Change default permissions of pts/ptmx node to 0000.
> 	- Move code for ptmxmode under #ifdef CONFIG_DEVPTS_MULTIPLE_INSTANCES.
> 
> Changelog[v3]:
> 	- Rename ptmx_mode to ptmxmode (for consistency with 'newinstance')
> 
> Changelog[v2]:
> 	- [H. Peter Anvin] Remove mknod() system call support and create the
> 	  ptmx node internally.
> 
> Changelog[v1]:
> 	- Earlier version of this patch enabled creating /dev/pts/tty as
> 	  well. As pointed out by Al Viro and H. Peter Anvin, that is not
> 	  really necessary.
> 
> Signed-off-by: Sukadev Bhattiprolu <sukadev-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>
> ---
>  fs/devpts/inode.c |   76 +++++++++++++++++++++++++++++++++++++++++++++++++++-
>  1 files changed, 74 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/devpts/inode.c b/fs/devpts/inode.c
> index 7ae60aa..17e14f5 100644
> --- a/fs/devpts/inode.c
> +++ b/fs/devpts/inode.c
> @@ -27,6 +27,13 @@
>  #define DEVPTS_SUPER_MAGIC 0x1cd1
> 
>  #define DEVPTS_DEFAULT_MODE 0600
> +/*
> + * ptmx is a new node in /dev/pts and will be unused in legacy (single-
> + * instance) mode. To prevent surprises in user space, set permissions of
> + * ptmx to 0. Use 'chmod' or remount with '-o ptmxmode' to set meaningful
> + * permissions.
> + */
> +#define DEVPTS_DEFAULT_PTMX_MODE 0000
>  #define PTMX_MINOR	2
> 
>  extern int pty_limit;			/* Config limit on Unix98 ptys */
> @@ -40,10 +47,11 @@ struct pts_mount_opts {
>  	uid_t   uid;
>  	gid_t   gid;
>  	umode_t mode;
> +	umode_t ptmxmode;
>  };
> 
>  enum {
> -	Opt_uid, Opt_gid, Opt_mode,
> +	Opt_uid, Opt_gid, Opt_mode, Opt_ptmxmode,
>  	Opt_err
>  };
> 
> @@ -51,6 +59,9 @@ static match_table_t tokens = {
>  	{Opt_uid, "uid=%u"},
>  	{Opt_gid, "gid=%u"},
>  	{Opt_mode, "mode=%o"},
> +#ifdef CONFIG_DEVPTS_MULTIPLE_INSTANCES
> +	{Opt_ptmxmode, "ptmxmode=%o"},
> +#endif
>  	{Opt_err, NULL}
>  };
> 
> @@ -81,6 +92,7 @@ static int parse_mount_options(char *data, struct pts_mount_opts *opts)
>  	opts->uid     = 0;
>  	opts->gid     = 0;
>  	opts->mode    = DEVPTS_DEFAULT_MODE;
> +	opts->ptmxmode = DEVPTS_DEFAULT_PTMX_MODE;
> 
>  	while ((p = strsep(&data, ",")) != NULL) {
>  		substring_t args[MAX_OPT_ARGS];
> @@ -109,6 +121,13 @@ static int parse_mount_options(char *data, struct pts_mount_opts *opts)
>  				return -EINVAL;
>  			opts->mode = option & S_IALLUGO;
>  			break;
> +#ifdef CONFIG_DEVPTS_MULTIPLE_INSTANCES
> +		case Opt_ptmxmode:
> +			if (match_octal(&args[0], &option))
> +				return -EINVAL;
> +			opts->ptmxmode = option & S_IALLUGO;
> +			break;
> +#endif
>  		default:
>  			printk(KERN_ERR "devpts: called with bogus options\n");
>  			return -EINVAL;
> @@ -118,6 +137,55 @@ static int parse_mount_options(char *data, struct pts_mount_opts *opts)
>  	return 0;
>  }
> 
> +#ifdef CONFIG_DEVPTS_MULTIPLE_INSTANCES
> +static int mknod_ptmx(struct super_block *sb)
> +{
> +	struct dentry *root;
> +	struct dentry *dentry;
> +	struct inode *inode;
> +	struct pts_fs_info *fsi = DEVPTS_SB(sb);
> +	struct pts_mount_opts *opts = &fsi->mount_opts;
> +	int mode;
> +
> +	root = sb->s_root;
> +	dentry = lookup_one_len("ptmx", root, 4);

I realize you intend for this to only be used before mount is completed,
but it still seems like a good idea to have this grab the root inode
mutex.

> +	if (IS_ERR(dentry)) {
> +		printk(KERN_ERR "Unable to alloc dentry for ptmx node\n");
> +		return -ENOMEM;
> +	}
> +
> +	if (dentry->d_inode) {
> +		printk(KERN_ERR "'ptmx' (ino %lu) exists in this devpts\n",
> +				dentry->d_inode->i_ino);
> +		return 0;
> +	}
> +
> +	/*
> +	 * Create a new 'ptmx' node in this mount of devpts.
> +	 */
> +	inode = new_inode(sb);
> +	if (!inode) {
> +		printk(KERN_ERR "Unable to alloc inode for ptmx node\n");
> +		dput(dentry);
> +		return -ENOMEM;
> +	}
> +
> +	inode->i_uid = inode->i_gid = 0;

As we've discussed, you need to reserve inode number 2 for ptmx, set
inode->i_ino to 2 here, and then set inode->i_ino to the pty_index+3
when creating a new pty.

> +	inode->i_blocks = 0;
> +	inode->i_mtime = inode->i_atime = inode->i_ctime = CURRENT_TIME;
> +
> +	mode = S_IFCHR|opts->ptmxmode;
> +	init_special_inode(inode, mode, MKDEV(TTYAUX_MAJOR, 2));
> +
> +	d_add(dentry, inode);
> +
> +	printk(KERN_DEBUG "Created ptmx node in devpts ino %lu\n",
> +			inode->i_ino);
> +
> +	return 0;
> +}
> +#endif
> +
>  static int devpts_remount(struct super_block *sb, int *flags, char *data)
>  {
>  	struct pts_fs_info *fsi = DEVPTS_SB(sb);
> @@ -136,6 +204,9 @@ static int devpts_show_options(struct seq_file *seq, struct vfsmount *vfs)
>  	if (opts->setgid)
>  		seq_printf(seq, ",gid=%u", opts->gid);
>  	seq_printf(seq, ",mode=%03o", opts->mode);
> +#ifdef CONFIG_DEVPTS_MULTIPLE_INSTANCES
> +	seq_printf(seq, ",ptmxmode=%03o", opts->ptmxmode);
> +#endif
> 
>  	return 0;
>  }
> @@ -156,6 +227,7 @@ static void *new_pts_fs_info(void)
> 
>  	ida_init(&fsi->allocated_ptys);
>  	fsi->mount_opts.mode = DEVPTS_DEFAULT_MODE;
> +	fsi->mount_opts.ptmxmode = DEVPTS_DEFAULT_PTMX_MODE;
> 
>  	return fsi;
>  }
> @@ -211,7 +283,7 @@ static void devpts_kill_sb(struct super_block *sb)
>  	struct pts_fs_info *fsi = DEVPTS_SB(sb);
> 
>  	kfree(fsi);
> -	kill_anon_super(sb);
> +	kill_litter_super(sb);
>  }
> 
>  static struct file_system_type devpts_fs_type = {
> -- 
> 1.5.2.5

Re: [PATCH 06/10] Define mknod_ptmx()

[sukadev-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8]

Serge E. Hallyn [serue-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org] wrote:
| Quoting sukadev-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org (sukadev-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org):
| > 
| > >From df5d5863f8601baa25237b60c3fcd614ee0b34aa Mon Sep 17 00:00:00 2001
| > From: Sukadev Bhattiprolu <sukadev-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>
| > Date: Tue, 9 Sep 2008 10:23:07 -0700
| > Subject: [PATCH 06/10] Define mknod_ptmx()
| > 
| > /dev/ptmx is closely tied to the devpts filesystem. An open of /dev/ptmx,
| > allocates the next pty index and the associated device shows up in the
| > devpts fs as /dev/pts/n.
| > 
| > Wih multiple instancs of devpts filesystem, during an open of /dev/ptmx
| > we would be unable to determine which instance of the devpts is being
| > accessed.
| > 
| > So we move the 'ptmx' node into /dev/pts and use the inode of the 'ptmx'
| > node to identify the superblock and hence the devpts instance.  This patch
| > adds ability for the kernel to internally create the [ptmx, c, 5:2] device
| > when mounting devpts filesystem.  Since the ptmx node in devpts is new and
| > may surprise some userspace scripts, the default permissions for the new
| > node is 0000.  These permissions can be changed either using chmod or by
| > remounting with the new '-o ptmxmode=0666' mount option.
| > 
| > Changelog[v4]:
| > 	- Change default permissions of pts/ptmx node to 0000.
| > 	- Move code for ptmxmode under #ifdef CONFIG_DEVPTS_MULTIPLE_INSTANCES.
| > 
| > Changelog[v3]:
| > 	- Rename ptmx_mode to ptmxmode (for consistency with 'newinstance')
| > 
| > Changelog[v2]:
| > 	- [H. Peter Anvin] Remove mknod() system call support and create the
| > 	  ptmx node internally.
| > 
| > Changelog[v1]:
| > 	- Earlier version of this patch enabled creating /dev/pts/tty as
| > 	  well. As pointed out by Al Viro and H. Peter Anvin, that is not
| > 	  really necessary.
| > 
| > Signed-off-by: Sukadev Bhattiprolu <sukadev-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>
| > ---
| >  fs/devpts/inode.c |   76 +++++++++++++++++++++++++++++++++++++++++++++++++++-
| >  1 files changed, 74 insertions(+), 2 deletions(-)
| > 
| > diff --git a/fs/devpts/inode.c b/fs/devpts/inode.c
| > index 7ae60aa..17e14f5 100644
| > --- a/fs/devpts/inode.c
| > +++ b/fs/devpts/inode.c
| > @@ -27,6 +27,13 @@
| >  #define DEVPTS_SUPER_MAGIC 0x1cd1
| > 
| >  #define DEVPTS_DEFAULT_MODE 0600
| > +/*
| > + * ptmx is a new node in /dev/pts and will be unused in legacy (single-
| > + * instance) mode. To prevent surprises in user space, set permissions of
| > + * ptmx to 0. Use 'chmod' or remount with '-o ptmxmode' to set meaningful
| > + * permissions.
| > + */
| > +#define DEVPTS_DEFAULT_PTMX_MODE 0000
| >  #define PTMX_MINOR	2
| > 
| >  extern int pty_limit;			/* Config limit on Unix98 ptys */
| > @@ -40,10 +47,11 @@ struct pts_mount_opts {
| >  	uid_t   uid;
| >  	gid_t   gid;
| >  	umode_t mode;
| > +	umode_t ptmxmode;
| >  };
| > 
| >  enum {
| > -	Opt_uid, Opt_gid, Opt_mode,
| > +	Opt_uid, Opt_gid, Opt_mode, Opt_ptmxmode,
| >  	Opt_err
| >  };
| > 
| > @@ -51,6 +59,9 @@ static match_table_t tokens = {
| >  	{Opt_uid, "uid=%u"},
| >  	{Opt_gid, "gid=%u"},
| >  	{Opt_mode, "mode=%o"},
| > +#ifdef CONFIG_DEVPTS_MULTIPLE_INSTANCES
| > +	{Opt_ptmxmode, "ptmxmode=%o"},
| > +#endif
| >  	{Opt_err, NULL}
| >  };
| > 
| > @@ -81,6 +92,7 @@ static int parse_mount_options(char *data, struct pts_mount_opts *opts)
| >  	opts->uid     = 0;
| >  	opts->gid     = 0;
| >  	opts->mode    = DEVPTS_DEFAULT_MODE;
| > +	opts->ptmxmode = DEVPTS_DEFAULT_PTMX_MODE;
| > 
| >  	while ((p = strsep(&data, ",")) != NULL) {
| >  		substring_t args[MAX_OPT_ARGS];
| > @@ -109,6 +121,13 @@ static int parse_mount_options(char *data, struct pts_mount_opts *opts)
| >  				return -EINVAL;
| >  			opts->mode = option & S_IALLUGO;
| >  			break;
| > +#ifdef CONFIG_DEVPTS_MULTIPLE_INSTANCES
| > +		case Opt_ptmxmode:
| > +			if (match_octal(&args[0], &option))
| > +				return -EINVAL;
| > +			opts->ptmxmode = option & S_IALLUGO;
| > +			break;
| > +#endif
| >  		default:
| >  			printk(KERN_ERR "devpts: called with bogus options\n");
| >  			return -EINVAL;
| > @@ -118,6 +137,55 @@ static int parse_mount_options(char *data, struct pts_mount_opts *opts)
| >  	return 0;
| >  }
| > 
| > +#ifdef CONFIG_DEVPTS_MULTIPLE_INSTANCES
| > +static int mknod_ptmx(struct super_block *sb)
| > +{
| > +	struct dentry *root;
| > +	struct dentry *dentry;
| > +	struct inode *inode;
| > +	struct pts_fs_info *fsi = DEVPTS_SB(sb);
| > +	struct pts_mount_opts *opts = &fsi->mount_opts;
| > +	int mode;
| > +
| > +	root = sb->s_root;
| > +	dentry = lookup_one_len("ptmx", root, 4);
| 
| I realize you intend for this to only be used before mount is completed,
| but it still seems like a good idea to have this grab the root inode
| mutex.

Ok.

| 
| > +	if (IS_ERR(dentry)) {
| > +		printk(KERN_ERR "Unable to alloc dentry for ptmx node\n");
| > +		return -ENOMEM;
| > +	}
| > +
| > +	if (dentry->d_inode) {
| > +		printk(KERN_ERR "'ptmx' (ino %lu) exists in this devpts\n",
| > +				dentry->d_inode->i_ino);
| > +		return 0;
| > +	}
| > +
| > +	/*
| > +	 * Create a new 'ptmx' node in this mount of devpts.
| > +	 */
| > +	inode = new_inode(sb);
| > +	if (!inode) {
| > +		printk(KERN_ERR "Unable to alloc inode for ptmx node\n");
| > +		dput(dentry);
| > +		return -ENOMEM;
| > +	}
| > +
| > +	inode->i_uid = inode->i_gid = 0;
| 
| As we've discussed, you need to reserve inode number 2 for ptmx, set
| inode->i_ino to 2 here, and then set inode->i_ino to the pty_index+3
| when creating a new pty.

Yes. I agree. Had it that way before, but let new_inode() choose the
number. But obvoiusly the numbers can collide if we chose the inode
numbers for the ptys differently.

Re: [PATCH 06/10] Define mknod_ptmx()

[Serge E. Hallyn]

Quoting sukadev-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org (sukadev-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org):
> 
> >From df5d5863f8601baa25237b60c3fcd614ee0b34aa Mon Sep 17 00:00:00 2001
> From: Sukadev Bhattiprolu <sukadev-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>
> Date: Tue, 9 Sep 2008 10:23:07 -0700
> Subject: [PATCH 06/10] Define mknod_ptmx()
> 
> /dev/ptmx is closely tied to the devpts filesystem. An open of /dev/ptmx,
> allocates the next pty index and the associated device shows up in the
> devpts fs as /dev/pts/n.
> 
> Wih multiple instancs of devpts filesystem, during an open of /dev/ptmx
> we would be unable to determine which instance of the devpts is being
> accessed.
> 
> So we move the 'ptmx' node into /dev/pts and use the inode of the 'ptmx'
> node to identify the superblock and hence the devpts instance.  This patch
> adds ability for the kernel to internally create the [ptmx, c, 5:2] device
> when mounting devpts filesystem.  Since the ptmx node in devpts is new and
> may surprise some userspace scripts, the default permissions for the new
> node is 0000.  These permissions can be changed either using chmod or by
> remounting with the new '-o ptmxmode=0666' mount option.
> 
> Changelog[v4]:
> 	- Change default permissions of pts/ptmx node to 0000.
> 	- Move code for ptmxmode under #ifdef CONFIG_DEVPTS_MULTIPLE_INSTANCES.
> 
> Changelog[v3]:
> 	- Rename ptmx_mode to ptmxmode (for consistency with 'newinstance')
> 
> Changelog[v2]:
> 	- [H. Peter Anvin] Remove mknod() system call support and create the
> 	  ptmx node internally.
> 
> Changelog[v1]:
> 	- Earlier version of this patch enabled creating /dev/pts/tty as
> 	  well. As pointed out by Al Viro and H. Peter Anvin, that is not
> 	  really necessary.
> 
> Signed-off-by: Sukadev Bhattiprolu <sukadev-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>
> ---
>  fs/devpts/inode.c |   76 +++++++++++++++++++++++++++++++++++++++++++++++++++-
>  1 files changed, 74 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/devpts/inode.c b/fs/devpts/inode.c
> index 7ae60aa..17e14f5 100644
> --- a/fs/devpts/inode.c
> +++ b/fs/devpts/inode.c
> @@ -27,6 +27,13 @@
>  #define DEVPTS_SUPER_MAGIC 0x1cd1
> 
>  #define DEVPTS_DEFAULT_MODE 0600
> +/*
> + * ptmx is a new node in /dev/pts and will be unused in legacy (single-
> + * instance) mode. To prevent surprises in user space, set permissions of
> + * ptmx to 0. Use 'chmod' or remount with '-o ptmxmode' to set meaningful
> + * permissions.
> + */
> +#define DEVPTS_DEFAULT_PTMX_MODE 0000
>  #define PTMX_MINOR	2
> 
>  extern int pty_limit;			/* Config limit on Unix98 ptys */
> @@ -40,10 +47,11 @@ struct pts_mount_opts {
>  	uid_t   uid;
>  	gid_t   gid;
>  	umode_t mode;
> +	umode_t ptmxmode;
>  };
> 
>  enum {
> -	Opt_uid, Opt_gid, Opt_mode,
> +	Opt_uid, Opt_gid, Opt_mode, Opt_ptmxmode,
>  	Opt_err
>  };
> 
> @@ -51,6 +59,9 @@ static match_table_t tokens = {
>  	{Opt_uid, "uid=%u"},
>  	{Opt_gid, "gid=%u"},
>  	{Opt_mode, "mode=%o"},
> +#ifdef CONFIG_DEVPTS_MULTIPLE_INSTANCES
> +	{Opt_ptmxmode, "ptmxmode=%o"},
> +#endif
>  	{Opt_err, NULL}
>  };
> 
> @@ -81,6 +92,7 @@ static int parse_mount_options(char *data, struct pts_mount_opts *opts)
>  	opts->uid     = 0;
>  	opts->gid     = 0;
>  	opts->mode    = DEVPTS_DEFAULT_MODE;
> +	opts->ptmxmode = DEVPTS_DEFAULT_PTMX_MODE;
> 
>  	while ((p = strsep(&data, ",")) != NULL) {
>  		substring_t args[MAX_OPT_ARGS];
> @@ -109,6 +121,13 @@ static int parse_mount_options(char *data, struct pts_mount_opts *opts)
>  				return -EINVAL;
>  			opts->mode = option & S_IALLUGO;
>  			break;
> +#ifdef CONFIG_DEVPTS_MULTIPLE_INSTANCES
> +		case Opt_ptmxmode:
> +			if (match_octal(&args[0], &option))
> +				return -EINVAL;
> +			opts->ptmxmode = option & S_IALLUGO;
> +			break;
> +#endif
>  		default:
>  			printk(KERN_ERR "devpts: called with bogus options\n");
>  			return -EINVAL;
> @@ -118,6 +137,55 @@ static int parse_mount_options(char *data, struct pts_mount_opts *opts)
>  	return 0;
>  }
> 
> +#ifdef CONFIG_DEVPTS_MULTIPLE_INSTANCES
> +static int mknod_ptmx(struct super_block *sb)
> +{
> +	struct dentry *root;
> +	struct dentry *dentry;
> +	struct inode *inode;
> +	struct pts_fs_info *fsi = DEVPTS_SB(sb);
> +	struct pts_mount_opts *opts = &fsi->mount_opts;
> +	int mode;
> +
> +	root = sb->s_root;
> +	dentry = lookup_one_len("ptmx", root, 4);
> +	if (IS_ERR(dentry)) {
> +		printk(KERN_ERR "Unable to alloc dentry for ptmx node\n");
> +		return -ENOMEM;
> +	}
> +
> +	if (dentry->d_inode) {
> +		printk(KERN_ERR "'ptmx' (ino %lu) exists in this devpts\n",
> +				dentry->d_inode->i_ino);
> +		return 0;
> +	}
> +
> +	/*
> +	 * Create a new 'ptmx' node in this mount of devpts.
> +	 */
> +	inode = new_inode(sb);
> +	if (!inode) {
> +		printk(KERN_ERR "Unable to alloc inode for ptmx node\n");
> +		dput(dentry);
> +		return -ENOMEM;
> +	}
> +
> +	inode->i_uid = inode->i_gid = 0;
> +	inode->i_blocks = 0;
> +	inode->i_mtime = inode->i_atime = inode->i_ctime = CURRENT_TIME;
> +
> +	mode = S_IFCHR|opts->ptmxmode;
> +	init_special_inode(inode, mode, MKDEV(TTYAUX_MAJOR, 2));
> +
> +	d_add(dentry, inode);

Actually, you never do dput this dentry.

At the moment you need to dput it here, then once you cache it, you
don't dput it here, but dput the cache one at devpts_kill_sb.  Right?

> +	printk(KERN_DEBUG "Created ptmx node in devpts ino %lu\n",
> +			inode->i_ino);
> +
> +	return 0;
> +}
> +#endif
> +
>  static int devpts_remount(struct super_block *sb, int *flags, char *data)
>  {
>  	struct pts_fs_info *fsi = DEVPTS_SB(sb);
> @@ -136,6 +204,9 @@ static int devpts_show_options(struct seq_file *seq, struct vfsmount *vfs)
>  	if (opts->setgid)
>  		seq_printf(seq, ",gid=%u", opts->gid);
>  	seq_printf(seq, ",mode=%03o", opts->mode);
> +#ifdef CONFIG_DEVPTS_MULTIPLE_INSTANCES
> +	seq_printf(seq, ",ptmxmode=%03o", opts->ptmxmode);
> +#endif
> 
>  	return 0;
>  }
> @@ -156,6 +227,7 @@ static void *new_pts_fs_info(void)
> 
>  	ida_init(&fsi->allocated_ptys);
>  	fsi->mount_opts.mode = DEVPTS_DEFAULT_MODE;
> +	fsi->mount_opts.ptmxmode = DEVPTS_DEFAULT_PTMX_MODE;
> 
>  	return fsi;
>  }
> @@ -211,7 +283,7 @@ static void devpts_kill_sb(struct super_block *sb)
>  	struct pts_fs_info *fsi = DEVPTS_SB(sb);
> 
>  	kfree(fsi);
> -	kill_anon_super(sb);
> +	kill_litter_super(sb);
>  }
> 
>  static struct file_system_type devpts_fs_type = {
> -- 
> 1.5.2.5

Re: [PATCH 06/10] Define mknod_ptmx()

[sukadev-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8]

Serge E. Hallyn [serue-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org] wrote:
| > +	/*
| > +	 * Create a new 'ptmx' node in this mount of devpts.
| > +	 */
| > +	inode = new_inode(sb);
| > +	if (!inode) {
| > +		printk(KERN_ERR "Unable to alloc inode for ptmx node\n");
| > +		dput(dentry);
| > +		return -ENOMEM;
| > +	}
| > +
| > +	inode->i_uid = inode->i_gid = 0;
| > +	inode->i_blocks = 0;
| > +	inode->i_mtime = inode->i_atime = inode->i_ctime = CURRENT_TIME;
| > +
| > +	mode = S_IFCHR|opts->ptmxmode;
| > +	init_special_inode(inode, mode, MKDEV(TTYAUX_MAJOR, 2));
| > +
| > +	d_add(dentry, inode);
| 
| Actually, you never do dput this dentry.

kill_litter_super() should dput it right ? Earlier we were not caching
ptmx_dentry and so relied on kill_litter_super() to dput.

Now that I have ptmx_dentry cached, I could explicitly dput() in
devpts_kill_sb() and go back to kill_anon_super(). Would that be better
?

| 
| At the moment you need to dput it here, then once you cache it, you
| don't dput it here, but dput the cache one at devpts_kill_sb.  Right?
| 
| > +	printk(KERN_DEBUG "Created ptmx node in devpts ino %lu\n",
| > +			inode->i_ino);
| > +
| > +	return 0;
| > +}
| > +#endif
| > +
| >  static int devpts_remount(struct super_block *sb, int *flags, char *data)
| >  {
| >  	struct pts_fs_info *fsi = DEVPTS_SB(sb);
| > @@ -136,6 +204,9 @@ static int devpts_show_options(struct seq_file *seq, struct vfsmount *vfs)
| >  	if (opts->setgid)
| >  		seq_printf(seq, ",gid=%u", opts->gid);
| >  	seq_printf(seq, ",mode=%03o", opts->mode);
| > +#ifdef CONFIG_DEVPTS_MULTIPLE_INSTANCES
| > +	seq_printf(seq, ",ptmxmode=%03o", opts->ptmxmode);
| > +#endif
| > 
| >  	return 0;
| >  }
| > @@ -156,6 +227,7 @@ static void *new_pts_fs_info(void)
| > 
| >  	ida_init(&fsi->allocated_ptys);
| >  	fsi->mount_opts.mode = DEVPTS_DEFAULT_MODE;
| > +	fsi->mount_opts.ptmxmode = DEVPTS_DEFAULT_PTMX_MODE;
| > 
| >  	return fsi;
| >  }
| > @@ -211,7 +283,7 @@ static void devpts_kill_sb(struct super_block *sb)
| >  	struct pts_fs_info *fsi = DEVPTS_SB(sb);
| > 
| >  	kfree(fsi);
| > -	kill_anon_super(sb);
| > +	kill_litter_super(sb);
| >  }
| > 
| >  static struct file_system_type devpts_fs_type = {
| > -- 
| > 1.5.2.5

Re: [PATCH 06/10] Define mknod_ptmx()

[Serge E. Hallyn]

Quoting sukadev-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org (sukadev-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org):
> Serge E. Hallyn [serue-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org] wrote:
> | > +	/*
> | > +	 * Create a new 'ptmx' node in this mount of devpts.
> | > +	 */
> | > +	inode = new_inode(sb);
> | > +	if (!inode) {
> | > +		printk(KERN_ERR "Unable to alloc inode for ptmx node\n");
> | > +		dput(dentry);
> | > +		return -ENOMEM;
> | > +	}
> | > +
> | > +	inode->i_uid = inode->i_gid = 0;
> | > +	inode->i_blocks = 0;
> | > +	inode->i_mtime = inode->i_atime = inode->i_ctime = CURRENT_TIME;
> | > +
> | > +	mode = S_IFCHR|opts->ptmxmode;
> | > +	init_special_inode(inode, mode, MKDEV(TTYAUX_MAJOR, 2));
> | > +
> | > +	d_add(dentry, inode);
> | 
> | Actually, you never do dput this dentry.
> 
> kill_litter_super() should dput it right ? Earlier we were not caching
> ptmx_dentry and so relied on kill_litter_super() to dput.
> 
> Now that I have ptmx_dentry cached, I could explicitly dput() in
> devpts_kill_sb() and go back to kill_anon_super(). Would that be better
> ?

I think so, but unless someone backs me up on that, ignore me.

> | At the moment you need to dput it here, then once you cache it, you
> | don't dput it here, but dput the cache one at devpts_kill_sb.  Right?
> | 
> | > +	printk(KERN_DEBUG "Created ptmx node in devpts ino %lu\n",
> | > +			inode->i_ino);
> | > +
> | > +	return 0;
> | > +}
> | > +#endif
> | > +
> | >  static int devpts_remount(struct super_block *sb, int *flags, char *data)
> | >  {
> | >  	struct pts_fs_info *fsi = DEVPTS_SB(sb);
> | > @@ -136,6 +204,9 @@ static int devpts_show_options(struct seq_file *seq, struct vfsmount *vfs)
> | >  	if (opts->setgid)
> | >  		seq_printf(seq, ",gid=%u", opts->gid);
> | >  	seq_printf(seq, ",mode=%03o", opts->mode);
> | > +#ifdef CONFIG_DEVPTS_MULTIPLE_INSTANCES
> | > +	seq_printf(seq, ",ptmxmode=%03o", opts->ptmxmode);
> | > +#endif
> | > 
> | >  	return 0;
> | >  }
> | > @@ -156,6 +227,7 @@ static void *new_pts_fs_info(void)
> | > 
> | >  	ida_init(&fsi->allocated_ptys);
> | >  	fsi->mount_opts.mode = DEVPTS_DEFAULT_MODE;
> | > +	fsi->mount_opts.ptmxmode = DEVPTS_DEFAULT_PTMX_MODE;
> | > 
> | >  	return fsi;
> | >  }
> | > @@ -211,7 +283,7 @@ static void devpts_kill_sb(struct super_block *sb)
> | >  	struct pts_fs_info *fsi = DEVPTS_SB(sb);
> | > 
> | >  	kfree(fsi);
> | > -	kill_anon_super(sb);
> | > +	kill_litter_super(sb);
> | >  }
> | > 
> | >  static struct file_system_type devpts_fs_type = {
> | > -- 
> | > 1.5.2.5

[PATCH 07/10] Update ptmx permissions during remount

[sukadev-r/Jw6+rmf7HQT0dZR+AlfA]

From 8c20ae3e9d5e051791a99b9060326069432d7fff Mon Sep 17 00:00:00 2001
From: Sukadev Bhattiprolu <sukadev-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>
Date: Tue, 9 Sep 2008 18:37:55 -0700
Subject: [PATCH 07/10] Update ptmx permissions during remount

By default, /dev/pts/ptmx node starts out with 0000 permissions.  While
user's can chmod /dev/pts/ptmx, it maybe easier for legacy systems to
update /etc/fstab and change the permissions using ptmxmode option.

This patch caches the dentry for ptmx node and uses it to update permissions
of ptmx node during remount (code will be enabled in a follow-on patch).

Signed-off-by: Sukadev Bhattiprolu <sukadev-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>
---
 fs/devpts/inode.c |   28 +++++++++++++++++++++++++++-
 1 files changed, 27 insertions(+), 1 deletions(-)

diff --git a/fs/devpts/inode.c b/fs/devpts/inode.c
index 17e14f5..6b56255 100644
--- a/fs/devpts/inode.c
+++ b/fs/devpts/inode.c
@@ -68,6 +68,7 @@ static match_table_t tokens = {
 struct pts_fs_info {
 	struct ida allocated_ptys;
 	struct pts_mount_opts mount_opts;
+	struct dentry *ptmx_dentry;
 };
 
 static inline struct pts_fs_info *DEVPTS_SB(struct super_block *sb)
@@ -184,14 +185,39 @@ static int mknod_ptmx(struct super_block *sb)
 
 	return 0;
 }
+
+static void update_ptmx_mode(struct pts_fs_info *fsi)
+{
+	struct inode *inode;
+	if (fsi->ptmx_dentry) {
+		inode = fsi->ptmx_dentry->d_inode;
+		inode->i_mode = S_IFCHR|fsi->mount_opts.ptmxmode;
+	}
+}
+#else
+static inline void update_ptmx_mode(struct pts_fs_info *fsi)
+{
+	return;
+}
 #endif
 
 static int devpts_remount(struct super_block *sb, int *flags, char *data)
 {
+	int err;
 	struct pts_fs_info *fsi = DEVPTS_SB(sb);
 	struct pts_mount_opts *opts = &fsi->mount_opts;
 
-	return parse_mount_options(data, opts);
+	err = parse_mount_options(data, opts);
+
+	/*
+	 * parse_mount_options() restores options to default values
+	 * before parsing and may have changed ptmxmode. So, update the
+	 * mode in the inode too. Bogus options don't fail the remount,
+	 * so do this even on error return.
+	 */
+	update_ptmx_mode(fsi);
+
+	return err;
 }
 
 static int devpts_show_options(struct seq_file *seq, struct vfsmount *vfs)
-- 
1.5.2.5

Re: [PATCH 07/10] Update ptmx permissions during remount

[Serge E. Hallyn]

Quoting sukadev-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org (sukadev-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org):
> 
> >From 8c20ae3e9d5e051791a99b9060326069432d7fff Mon Sep 17 00:00:00 2001
> From: Sukadev Bhattiprolu <sukadev-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>
> Date: Tue, 9 Sep 2008 18:37:55 -0700
> Subject: [PATCH 07/10] Update ptmx permissions during remount
> 
> By default, /dev/pts/ptmx node starts out with 0000 permissions.  While
> user's can chmod /dev/pts/ptmx, it maybe easier for legacy systems to
> update /etc/fstab and change the permissions using ptmxmode option.
> 
> This patch caches the dentry for ptmx node and uses it to update permissions
> of ptmx node during remount (code will be enabled in a follow-on patch).
> 
> Signed-off-by: Sukadev Bhattiprolu <sukadev-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>

As an alternative, you could just lookup_one_len("ptmx", sb->s_root, 4)
to avoid caching.  Not sure whether that would be considered better
though, so

Acked-by: Serge Hallyn <serue-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>

-serge

> ---
>  fs/devpts/inode.c |   28 +++++++++++++++++++++++++++-
>  1 files changed, 27 insertions(+), 1 deletions(-)
> 
> diff --git a/fs/devpts/inode.c b/fs/devpts/inode.c
> index 17e14f5..6b56255 100644
> --- a/fs/devpts/inode.c
> +++ b/fs/devpts/inode.c
> @@ -68,6 +68,7 @@ static match_table_t tokens = {
>  struct pts_fs_info {
>  	struct ida allocated_ptys;
>  	struct pts_mount_opts mount_opts;
> +	struct dentry *ptmx_dentry;
>  };
> 
>  static inline struct pts_fs_info *DEVPTS_SB(struct super_block *sb)
> @@ -184,14 +185,39 @@ static int mknod_ptmx(struct super_block *sb)
> 
>  	return 0;
>  }
> +
> +static void update_ptmx_mode(struct pts_fs_info *fsi)
> +{
> +	struct inode *inode;
> +	if (fsi->ptmx_dentry) {
> +		inode = fsi->ptmx_dentry->d_inode;
> +		inode->i_mode = S_IFCHR|fsi->mount_opts.ptmxmode;
> +	}
> +}
> +#else
> +static inline void update_ptmx_mode(struct pts_fs_info *fsi)
> +{
> +	return;
> +}
>  #endif
> 
>  static int devpts_remount(struct super_block *sb, int *flags, char *data)
>  {
> +	int err;
>  	struct pts_fs_info *fsi = DEVPTS_SB(sb);
>  	struct pts_mount_opts *opts = &fsi->mount_opts;
> 
> -	return parse_mount_options(data, opts);
> +	err = parse_mount_options(data, opts);
> +
> +	/*
> +	 * parse_mount_options() restores options to default values
> +	 * before parsing and may have changed ptmxmode. So, update the
> +	 * mode in the inode too. Bogus options don't fail the remount,
> +	 * so do this even on error return.
> +	 */
> +	update_ptmx_mode(fsi);
> +
> +	return err;
>  }
> 
>  static int devpts_show_options(struct seq_file *seq, struct vfsmount *vfs)
> -- 
> 1.5.2.5

[PATCH 08/10] Define get_sb_ref()

[sukadev-r/Jw6+rmf7HQT0dZR+AlfA]

From 8886ef518c1855890be7fbc6bea853ce7feb263e Mon Sep 17 00:00:00 2001
From: Sukadev Bhattiprolu <sukadev-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>
Date: Tue, 9 Sep 2008 18:42:44 -0700
Subject: [PATCH 08/10] Define get_sb_ref()

See comments in fs/super.c (in patch below) for need for this interface.
This interface will be used in follow-on patch to enable multiple
instances of devpts.

Signed-off-by: Sukadev Bhattiprolu <sukadev-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>
---
 fs/super.c         |   37 +++++++++++++++++++++++++++++++++++++
 include/linux/fs.h |    2 ++
 2 files changed, 39 insertions(+), 0 deletions(-)

diff --git a/fs/super.c b/fs/super.c
index e931ae9..ba7059c 100644
--- a/fs/super.c
+++ b/fs/super.c
@@ -882,6 +882,43 @@ int get_sb_single(struct file_system_type *fs_type,
 }
 
 EXPORT_SYMBOL(get_sb_single);
+/*
+ * int get_sb_ref(struct super_block *sb, int flags, void *data,
+                struct vfsmount *mnt)
+ *
+ * 	This interface is needed to support multiple mounts in devpts while
+ * 	preserving backward compatibility of the current 'single-mount'
+ * 	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.
+ *
+ * 	But 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 is a simplified version of get_sb_single() and assumes that
+ * 	caller has a properly initialized @sb.
+ */
+int get_sb_ref(struct super_block *sb, int flags, void *data,
+                struct vfsmount *mnt)
+{
+        int err;
+
+        spin_lock(&sb_lock);
+
+        if (!grab_super(sb))
+                return -EAGAIN;
+
+        err = do_remount_sb(sb, flags, data, 0);
+        if (err) {
+                /* Don't deactivate_super() - its from initial pts mount */
+                up_write(&sb->s_umount);
+                return err;
+        }
+        return simple_set_mnt(mnt, sb);
+}
 
 struct vfsmount *
 vfs_kern_mount(struct file_system_type *type, int flags, const char *name, void *data)
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 580b513..3bda46d 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -1512,6 +1512,8 @@ extern int get_sb_single(struct file_system_type *fs_type,
 	int flags, void *data,
 	int (*fill_super)(struct super_block *, void *, int),
 	struct vfsmount *mnt);
+extern int get_sb_ref(struct super_block *sb, int flags, void *data,
+	struct vfsmount *mnt);
 extern int get_sb_nodev(struct file_system_type *fs_type,
 	int flags, void *data,
 	int (*fill_super)(struct super_block *, void *, int),
-- 
1.5.2.5

Re: [PATCH 08/10] Define get_sb_ref()

[Serge E. Hallyn]

Quoting sukadev-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org (sukadev-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org):
> 
> >From 8886ef518c1855890be7fbc6bea853ce7feb263e Mon Sep 17 00:00:00 2001
> From: Sukadev Bhattiprolu <sukadev-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>
> Date: Tue, 9 Sep 2008 18:42:44 -0700
> Subject: [PATCH 08/10] Define get_sb_ref()
> 
> See comments in fs/super.c (in patch below) for need for this interface.
> This interface will be used in follow-on patch to enable multiple
> instances of devpts.
> 
> Signed-off-by: Sukadev Bhattiprolu <sukadev-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>
> ---
>  fs/super.c         |   37 +++++++++++++++++++++++++++++++++++++
>  include/linux/fs.h |    2 ++
>  2 files changed, 39 insertions(+), 0 deletions(-)
> 
> diff --git a/fs/super.c b/fs/super.c
> index e931ae9..ba7059c 100644
> --- a/fs/super.c
> +++ b/fs/super.c
> @@ -882,6 +882,43 @@ int get_sb_single(struct file_system_type *fs_type,
>  }
> 
>  EXPORT_SYMBOL(get_sb_single);
> +/*
> + * int get_sb_ref(struct super_block *sb, int flags, void *data,
> +                struct vfsmount *mnt)
> + *
> + * 	This interface is needed to support multiple mounts in devpts while
> + * 	preserving backward compatibility of the current 'single-mount'
> + * 	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.
> + *
> + * 	But 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 is a simplified version of get_sb_single() and assumes that
> + * 	caller has a properly initialized @sb.
> + */
> +int get_sb_ref(struct super_block *sb, int flags, void *data,
> +                struct vfsmount *mnt)
> +{
> +        int err;
> +
> +        spin_lock(&sb_lock);
> +
> +        if (!grab_super(sb))
> +                return -EAGAIN;
> +
> +        err = do_remount_sb(sb, flags, data, 0);
> +        if (err) {
> +                /* Don't deactivate_super() - its from initial pts mount */

But you did increment sb->s_active, and you're not decrementing it?

> +                up_write(&sb->s_umount);
> +                return err;
> +        }
> +        return simple_set_mnt(mnt, sb);
> +}
> 
>  struct vfsmount *
>  vfs_kern_mount(struct file_system_type *type, int flags, const char *name, void *data)
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index 580b513..3bda46d 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -1512,6 +1512,8 @@ extern int get_sb_single(struct file_system_type *fs_type,
>  	int flags, void *data,
>  	int (*fill_super)(struct super_block *, void *, int),
>  	struct vfsmount *mnt);
> +extern int get_sb_ref(struct super_block *sb, int flags, void *data,
> +	struct vfsmount *mnt);
>  extern int get_sb_nodev(struct file_system_type *fs_type,
>  	int flags, void *data,
>  	int (*fill_super)(struct super_block *, void *, int),
> -- 
> 1.5.2.5

Re: [PATCH 08/10] Define get_sb_ref()

[Dave Hansen]

On Fri, 2008-09-12 at 10:53 -0700, sukadev-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org wrote:
> + *     But 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.

Can't you just override the test() function to get what you want here?

-- Dave

Re: [PATCH 08/10] Define get_sb_ref()

[sukadev-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8]

Dave Hansen [dave-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org] wrote:
| On Fri, 2008-09-12 at 10:53 -0700, sukadev-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org wrote:
| > + *     But 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.
| 
| Can't you just override the test() function to get what you want here?

get_sb_single() does not take a test() parameter and so I would still
need a get_sb_ref() or get_sb_special() interface right ? 

This special interface could call sget() with a custom-test function,
to get the super-block.  But in case of devpts, we already have the
super-block. So we don't need to call sget(). We just need get a reference
and remount.

Suka

Re: [PATCH 08/10] Define get_sb_ref()

[Dave Hansen]

On Fri, 2008-09-26 at 14:21 -0700, sukadev-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org wrote:
> Dave Hansen [dave-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org] wrote:
> | On Fri, 2008-09-12 at 10:53 -0700, sukadev-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org wrote:
> | > + *     But 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.
> | 
> | Can't you just override the test() function to get what you want here?
> 
> get_sb_single() does not take a test() parameter and so I would still
> need a get_sb_ref() or get_sb_special() interface right ? 
> 
> This special interface could call sget() with a custom-test function,
> to get the super-block.  But in case of devpts, we already have the
> super-block. So we don't need to call sget(). We just need get a reference
> and remount.

Well, you shouldn't be using get_sb_single() at all any more, right?

At this point, you're doing something super-specialized for devpts.  So,
why do this in super.c.  Just put it in place of devpts_get_sb()'s
current contents.

For me get_sb_ref() is a super-confusing name, especially when mixed
with all the other sb functions.  WTF is get_sb() doing if there's a
get_sb_ref()?

-- Dave

Re: [PATCH 08/10] Define get_sb_ref()

[sukadev-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8]

Dave Hansen [dave-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org] wrote:
| On Fri, 2008-09-26 at 14:21 -0700, sukadev-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org wrote:
| > Dave Hansen [dave-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org] wrote:
| > | On Fri, 2008-09-12 at 10:53 -0700, sukadev-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org wrote:
| > | > + *     But 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.
| > | 
| > | Can't you just override the test() function to get what you want here?
| > 
| > get_sb_single() does not take a test() parameter and so I would still
| > need a get_sb_ref() or get_sb_special() interface right ? 
| > 
| > This special interface could call sget() with a custom-test function,
| > to get the super-block.  But in case of devpts, we already have the
| > super-block. So we don't need to call sget(). We just need get a reference
| > and remount.
| 
| Well, you shouldn't be using get_sb_single() at all any more, right?

You mean define something like get_sb_multi_mode() and define a  new
test function ? I can try that.

| 
| At this point, you're doing something super-specialized for devpts.  So,
| why do this in super.c.  Just put it in place of devpts_get_sb()'s
| current contents.

grab_super() is static. I have to either externalize it or define the
new interface in fs/super.c.  I am not sure if mqueue ns or others have
similar semantics and if so, defining interface in fs/super.c has a better
chance of being found/generalized when there is another user.

| 
| For me get_sb_ref() is a super-confusing name, especially when mixed
| with all the other sb functions.  WTF is get_sb() doing if there's a
| get_sb_ref()?

I agree its not a great name. The 'get_sb's do several things some in
private interfaces, and I needed only a subset of them.

Do you have any preference over get_sb_multi_mode() over exporting 
grab_super() and moving the pts-specific stuff to into devpts/inode.c

Re: [PATCH 08/10] Define get_sb_ref()

[Cedric Le Goater]

sukadev-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org wrote:
> Dave Hansen [dave-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org] wrote:
> | On Fri, 2008-09-26 at 14:21 -0700, sukadev-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org wrote:
> | > Dave Hansen [dave-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org] wrote:
> | > | On Fri, 2008-09-12 at 10:53 -0700, sukadev-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org wrote:
> | > | > + *     But 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.
> | > | 
> | > | Can't you just override the test() function to get what you want here?
> | > 
> | > get_sb_single() does not take a test() parameter and so I would still
> | > need a get_sb_ref() or get_sb_special() interface right ? 
> | > 
> | > This special interface could call sget() with a custom-test function,
> | > to get the super-block.  But in case of devpts, we already have the
> | > super-block. So we don't need to call sget(). We just need get a reference
> | > and remount.
> | 
> | Well, you shouldn't be using get_sb_single() at all any more, right?
> 
> You mean define something like get_sb_multi_mode() and define a  new
> test function ? I can try that.
> 
> | 
> | At this point, you're doing something super-specialized for devpts.  So,
> | why do this in super.c.  Just put it in place of devpts_get_sb()'s
> | current contents.
> 
> grab_super() is static. I have to either externalize it or define the
> new interface in fs/super.c.  I am not sure if mqueue ns or others have
> similar semantics and if so, defining interface in fs/super.c has a better
> chance of being found/generalized when there is another user.

mqueue ns and proc have similar needs but there are those special little
things that make difficult to share a common routine.

Here's the get_sb_single_ns() we would like to share. 

C.
  
From: Cedric Le Goater <clg-NmTC/0ZBporQT0dZR+AlfA@public.gmane.org>

This patch defines the mqueuefs ->get_sb routine as follows:
  . when allocating a new super_block, the mq namespace is stored in that
    super_block s_fs_info attribute.
  . when looking for a super_block, a comparison is done against the
    s_fs_info attribute (it should be equal to the mq namespace that will
    be passed in as a data parameter).

Signed-off-by: Nadia Derbey <Nadia.Derbey-6ktuUTfB/bM@public.gmane.org>
Signed-off-by: Cedric Le Goater <clg-NmTC/0ZBporQT0dZR+AlfA@public.gmane.org>

---
 ipc/mqueue.c |   52 +++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 51 insertions(+), 1 deletion(-)

Index: linux-2.6.27-rc7/ipc/mqueue.c
===================================================================
--- linux-2.6.27-rc7.orig/ipc/mqueue.c	2008-09-25 15:03:58.000000000 +0200
+++ linux-2.6.27-rc7/ipc/mqueue.c	2008-09-25 16:28:13.000000000 +0200
@@ -201,6 +201,48 @@ static int mqueue_fill_super(struct supe
 	return 0;
 }

+static int compare_sb_single_ns(struct super_block *sb, void *data)
+{
+	return sb->s_fs_info == data;
+}
+
+static int set_sb_single_ns(struct super_block *sb, void *data)
+{
+	struct mq_namespace *mq_ns = (struct mq_namespace *) data;
+	int error;
+
+	sb->s_fs_info = get_mq_ns(mq_ns);
+	error = set_anon_super(sb, NULL);
+	if (error)
+		put_mq_ns(mq_ns);
+	return error;
+}
+
+static int get_sb_single_ns(struct file_system_type *fs_type,
+	int flags, void *data,
+	int (*fill_super)(struct super_block *, void *, int),
+	struct vfsmount *mnt)
+{
+	struct super_block *s;
+	int error;
+
+	s = sget(fs_type, compare_sb_single_ns, set_sb_single_ns, data);
+	if (IS_ERR(s))
+		return PTR_ERR(s);
+	if (!s->s_root) {
+		s->s_flags = flags;
+		error = 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;
+	}
+	do_remount_sb(s, flags, data, 0);
+	return simple_set_mnt(mnt, s);
+}
+
 static int mqueue_get_sb(struct file_system_type *fs_type,
 			 int flags, const char *dev_name,
 			 void *data, struct vfsmount *mnt)
@@ -208,6 +250,14 @@ static int mqueue_get_sb(struct file_sys
 	return get_sb_single(fs_type, flags, data, mqueue_fill_super, mnt);
 }

+static void mqueue_kill_sb(struct super_block *sb)
+{
+	struct mq_namespace *mq_ns = (struct mq_namespace *) sb->s_fs_info;
+
+	kill_litter_super(sb);
+	put_mq_ns(mq_ns);
+}
+
 static void init_once(void *foo)
 {
 	struct mqueue_inode_info *p = (struct mqueue_inode_info *) foo;

Re: [PATCH 08/10] Define get_sb_ref()

[Serge E. Hallyn]

Quoting Cedric Le Goater (clg-NmTC/0ZBporQT0dZR+AlfA@public.gmane.org):
> sukadev-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org wrote:
> > Dave Hansen [dave-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org] wrote:
> > | On Fri, 2008-09-26 at 14:21 -0700, sukadev-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org wrote:
> > | > Dave Hansen [dave-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org] wrote:
> > | > | On Fri, 2008-09-12 at 10:53 -0700, sukadev-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org wrote:
> > | > | > + *     But 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.
> > | > | 
> > | > | Can't you just override the test() function to get what you want here?
> > | > 
> > | > get_sb_single() does not take a test() parameter and so I would still
> > | > need a get_sb_ref() or get_sb_special() interface right ? 
> > | > 
> > | > This special interface could call sget() with a custom-test function,
> > | > to get the super-block.  But in case of devpts, we already have the
> > | > super-block. So we don't need to call sget(). We just need get a reference
> > | > and remount.
> > | 
> > | Well, you shouldn't be using get_sb_single() at all any more, right?
> > 
> > You mean define something like get_sb_multi_mode() and define a  new
> > test function ? I can try that.
> > 
> > | 
> > | At this point, you're doing something super-specialized for devpts.  So,
> > | why do this in super.c.  Just put it in place of devpts_get_sb()'s
> > | current contents.
> > 
> > grab_super() is static. I have to either externalize it or define the
> > new interface in fs/super.c.  I am not sure if mqueue ns or others have
> > similar semantics and if so, defining interface in fs/super.c has a better
> > chance of being found/generalized when there is another user.
> 
> mqueue ns and proc have similar needs but there are those special little
> things that make difficult to share a common routine.
> 
> Here's the get_sb_single_ns() we would like to share. 
> 
> C.
>   
> From: Cedric Le Goater <clg-NmTC/0ZBporQT0dZR+AlfA@public.gmane.org>
> 
> This patch defines the mqueuefs ->get_sb routine as follows:
>   . when allocating a new super_block, the mq namespace is stored in that
>     super_block s_fs_info attribute.
>   . when looking for a super_block, a comparison is done against the
>     s_fs_info attribute (it should be equal to the mq namespace that will
>     be passed in as a data parameter).
> 
> Signed-off-by: Nadia Derbey <Nadia.Derbey-6ktuUTfB/bM@public.gmane.org>
> Signed-off-by: Cedric Le Goater <clg-NmTC/0ZBporQT0dZR+AlfA@public.gmane.org>
> 
> ---
>  ipc/mqueue.c |   52 +++++++++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 51 insertions(+), 1 deletion(-)
> 
> Index: linux-2.6.27-rc7/ipc/mqueue.c
> ===================================================================
> --- linux-2.6.27-rc7.orig/ipc/mqueue.c	2008-09-25 15:03:58.000000000 +0200
> +++ linux-2.6.27-rc7/ipc/mqueue.c	2008-09-25 16:28:13.000000000 +0200
> @@ -201,6 +201,48 @@ static int mqueue_fill_super(struct supe
>  	return 0;
>  }
> 
> +static int compare_sb_single_ns(struct super_block *sb, void *data)
> +{
> +	return sb->s_fs_info == data;
> +}
> +
> +static int set_sb_single_ns(struct super_block *sb, void *data)
> +{
> +	struct mq_namespace *mq_ns = (struct mq_namespace *) data;
> +	int error;
> +
> +	sb->s_fs_info = get_mq_ns(mq_ns);
> +	error = set_anon_super(sb, NULL);
> +	if (error)
> +		put_mq_ns(mq_ns);

Hmm, how come fs/proc/root.c doesn't do a put_pid_ns if set_anon_super()
failed?  Does it need to do that, or is there some part of the error
path cleanup at a higher level that would cause that to happen anyway
(i.e. kill_sb ends up being called anyway)?

> +	return error;
> +}
> +
> +static int get_sb_single_ns(struct file_system_type *fs_type,
> +	int flags, void *data,
> +	int (*fill_super)(struct super_block *, void *, int),
> +	struct vfsmount *mnt)
> +{
> +	struct super_block *s;
> +	int error;
> +
> +	s = sget(fs_type, compare_sb_single_ns, set_sb_single_ns, data);
> +	if (IS_ERR(s))
> +		return PTR_ERR(s);
> +	if (!s->s_root) {
> +		s->s_flags = flags;
> +		error = 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;
> +	}
> +	do_remount_sb(s, flags, data, 0);
> +	return simple_set_mnt(mnt, s);
> +}
> +
>  static int mqueue_get_sb(struct file_system_type *fs_type,
>  			 int flags, const char *dev_name,
>  			 void *data, struct vfsmount *mnt)
> @@ -208,6 +250,14 @@ static int mqueue_get_sb(struct file_sys
>  	return get_sb_single(fs_type, flags, data, mqueue_fill_super, mnt);
>  }
> 
> +static void mqueue_kill_sb(struct super_block *sb)
> +{
> +	struct mq_namespace *mq_ns = (struct mq_namespace *) sb->s_fs_info;
> +
> +	kill_litter_super(sb);
> +	put_mq_ns(mq_ns);
> +}
> +
>  static void init_once(void *foo)
>  {
>  	struct mqueue_inode_info *p = (struct mqueue_inode_info *) foo;
> _______________________________________________
> Containers mailing list
> Containers-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org
> https://lists.linux-foundation.org/mailman/listinfo/containers

Re: [PATCH 08/10] Define get_sb_ref()

[Cedric Le Goater]

>> +static int compare_sb_single_ns(struct super_block *sb, void *data)
>> +{
>> +	return sb->s_fs_info == data;
>> +}
>> +
>> +static int set_sb_single_ns(struct super_block *sb, void *data)
>> +{
>> +	struct mq_namespace *mq_ns = (struct mq_namespace *) data;
>> +	int error;
>> +
>> +	sb->s_fs_info = get_mq_ns(mq_ns);
>> +	error = set_anon_super(sb, NULL);
>> +	if (error)
>> +		put_mq_ns(mq_ns);
> 
> Hmm, how come fs/proc/root.c doesn't do a put_pid_ns if set_anon_super()
> failed?  Does it need to do that, or is there some part of the error
> path cleanup at a higher level that would cause that to happen anyway
> (i.e. kill_sb ends up being called anyway)?

the kill_sb ops is only called on an active super_block which can not 
happen if sget() fails. This means that proc_set_super() fails to decrement 
the refcount on the newly created pid namespace if set_anon_super() fails.
That was spotted by Nadia on the mq namespace.

I'll add the following patch to the -lxc patchset to see how it behaves 
under test.

Thanks,

C.

commit f5ab821d6e4ca95bd19d79f7e5ba58a6fb63f6b0
Author: Cedric Le Goater <clg-NmTC/0ZBporQT0dZR+AlfA@public.gmane.org>
Date:   Wed Oct 1 14:34:57 2008 +0200

    proc_set_super() fails to decrement the refcount on the pid namespace
    if set_anon_super() fails.
    
    Signed-off-by: Cedric Le Goater <clg-NmTC/0ZBporQT0dZR+AlfA@public.gmane.org>

diff --git a/fs/proc/root.c b/fs/proc/root.c
index 9511753..55227d4 100644
--- a/fs/proc/root.c
+++ b/fs/proc/root.c
@@ -30,10 +30,14 @@ static int proc_test_super(struct super_block *sb, void *data)
 static int proc_set_super(struct super_block *sb, void *data)
 {
        struct pid_namespace *ns;
+       int error;
 
        ns = (struct pid_namespace *)data;
        sb->s_fs_info = get_pid_ns(ns);
-       return set_anon_super(sb, NULL);
+       error = set_anon_super(sb, NULL);
+       if (error)
+               put_pid_ns(ns);
+       return error;
 }
 
 static int proc_get_sb(struct file_system_type *fs_type,

Re: [PATCH 08/10] Define get_sb_ref()

[sukadev-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8]

Dave Hansen [dave-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org] wrote:
| On Fri, 2008-09-26 at 14:21 -0700, sukadev-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org wrote:
| > Dave Hansen [dave-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org] wrote:
| > | On Fri, 2008-09-12 at 10:53 -0700, sukadev-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org wrote:
| > | > + *     But 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.
| > | 
| > | Can't you just override the test() function to get what you want here?
| > 
| > get_sb_single() does not take a test() parameter and so I would still
| > need a get_sb_ref() or get_sb_special() interface right ? 
| > 
| > This special interface could call sget() with a custom-test function,
| > to get the super-block.  But in case of devpts, we already have the
| > super-block. So we don't need to call sget(). We just need get a reference
| > and remount.
| 
| Well, you shouldn't be using get_sb_single() at all any more, right?

How about something along these lines.

(applies on current devpts patchset.  Touch tested on 2.6.27-rc6+ttydev-0918)

Suka

---
From f021ad44e1b21bdc9f47c152830f40893fbaaf6a Mon Sep 17 00:00:00 2001
From: Sukadev Bhattiprolu <sukadev-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>
Date: Sat, 27 Sep 2008 11:22:48 -0700
Subject: [PATCH] Define/use get_sb_specific()

See function header of get_sb_specific() for details of new interface.

TODO: 	This is a quick/dirty patch and needs to be properly integrated into
  	the patchset, by:
		- replace patch that defines get_sb_ref() with a patch that
		  defines get_sb_specific()
		- update patch that uses get_sb_ref() to use get_sb_specific()
		- extract common code in get_sb_specific() and get_sb_single()
		  into a new function ?
---
 fs/devpts/inode.c  |   20 +++++++++------
 fs/super.c         |   67 +++++++++++++++++++++++++++++++++++++++++++++++++++-
 include/linux/fs.h |    5 ++++
 3 files changed, 83 insertions(+), 9 deletions(-)

diff --git a/fs/devpts/inode.c b/fs/devpts/inode.c
index c54b010..2f6bfb9 100644
--- a/fs/devpts/inode.c
+++ b/fs/devpts/inode.c
@@ -416,22 +416,26 @@ static int init_pts_mount(struct file_system_type *fs_type, int flags,
 		void *data, struct vfsmount *mnt)
 {
 	int err;
+	struct super_block *test_sb;
 
-	if (!devpts_mnt) {
-		err = get_sb_single(fs_type, flags, data, devpts_fill_super,
-				mnt);
+	test_sb = NULL;
+	if (devpts_mnt)
+		test_sb = devpts_mnt->mnt_sb;
 
+	err = get_sb_specific(fs_type, flags, data, devpts_fill_super,
+			test_sb, mnt);
+	if (err)
+		return err;
+
+	if (!devpts_mnt) {
 		err = mknod_ptmx(mnt->mnt_sb);
 		if (err) {
 			dput(mnt->mnt_sb->s_root);
 			deactivate_super(mnt->mnt_sb);
-		} else
-			devpts_mnt = mnt;
-
-		return err;
+		}
 	}
 
-	return get_sb_ref(devpts_mnt->mnt_sb, flags, data, mnt);
+	return err;
 }
 
 static int devpts_get_sb(struct file_system_type *fs_type,
diff --git a/fs/super.c b/fs/super.c
index ba7059c..e766bac 100644
--- a/fs/super.c
+++ b/fs/super.c
@@ -882,9 +882,74 @@ int get_sb_single(struct file_system_type *fs_type,
 }
 
 EXPORT_SYMBOL(get_sb_single);
+
+static int compare_specific(struct super_block *s, void *test_sb)
+{
+	return s == test_sb;
+}
+
+/*
+ * int get_sb_specific(fs_type, flags, *data, fill_super, test_sb, mnt)
+ *
+ * 	If super_block @test_sb exists, get it and remount it Otherwise,
+ * 	allocate a new super-block and mount it.
+ *
+ * 	This interface is needed to support multiple mounts in devpts while
+ * 	preserving backward compatibility of the current 'single-mount'
+ * 	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.
+ *
+ * 	But 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.
+ *
+ * TODO:
+ * 	Except for the sget() call and test_sb parameter, this
+ * 	function is identical to get_sb_single(). Extract common
+ * 	code to a separate function.
+ */
+
+ */
+int get_sb_specific(struct file_system_type *fs_type,
+	int flags, void *data,
+	int (*fill_super)(struct super_block *, void *, int),
+	struct super_block *test_sb,
+	struct vfsmount *mnt)
+{
+	struct super_block *s;
+	int error;
+
+	/*
+	 * Unlike other get_sb_* functions, the 'data' used for comparison
+	 * is different from the options data.
+	 *
+	 * We use set_anon_super() which ignores 'data' parameter, so
+	 * 'test_sb' being different from 'data' should not matter ?
+	 */
+	s = sget(fs_type, compare_specific, set_anon_super, test_sb);
+	if (IS_ERR(s))
+		return PTR_ERR(s);
+	if (!s->s_root) {
+		s->s_flags = flags;
+		error = 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;
+	}
+	do_remount_sb(s, flags, data, 0);
+	return simple_set_mnt(mnt, s);
+}
+
+EXPORT_SYMBOL(get_sb_specific);
 /*
  * int get_sb_ref(struct super_block *sb, int flags, void *data,
-                struct vfsmount *mnt)
+ * 	struct vfsmount *mnt)
  *
  * 	This interface is needed to support multiple mounts in devpts while
  * 	preserving backward compatibility of the current 'single-mount'
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 3bda46d..72fdfd5 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -1512,6 +1512,11 @@ extern int get_sb_single(struct file_system_type *fs_type,
 	int flags, void *data,
 	int (*fill_super)(struct super_block *, void *, int),
 	struct vfsmount *mnt);
+extern int get_sb_specific(struct file_system_type *fs_type,
+	int flags, void *data,
+	int (*fill_super)(struct super_block *, void *, int),
+	struct super_block *test_sb,
+	struct vfsmount *mnt);
 extern int get_sb_ref(struct super_block *sb, int flags, void *data,
 	struct vfsmount *mnt);
 extern int get_sb_nodev(struct file_system_type *fs_type,
-- 
1.5.2.5

Re: [PATCH 08/10] Define get_sb_ref()

[sukadev-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8]

sukadev-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org [sukadev-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org] wrote:
| Dave Hansen [dave-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org] wrote:
| | On Fri, 2008-09-26 at 14:21 -0700, sukadev-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org wrote:
| | > Dave Hansen [dave-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org] wrote:
| | > | On Fri, 2008-09-12 at 10:53 -0700, sukadev-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org wrote:
| | > | > + *     But 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.
| | > | 
| | > | Can't you just override the test() function to get what you want here?
| | > 
| | > get_sb_single() does not take a test() parameter and so I would still
| | > need a get_sb_ref() or get_sb_special() interface right ? 
| | > 
| | > This special interface could call sget() with a custom-test function,
| | > to get the super-block.  But in case of devpts, we already have the
| | > super-block. So we don't need to call sget(). We just need get a reference
| | > and remount.
| | 
| | Well, you shouldn't be using get_sb_single() at all any more, right?
| 

Since mqueue_ns is using a different approach, and devpts is the only one
needing this new interface, we can move this into fs/devpts/inode.c.

This patch again applies on current ptsns patchset.

---

From fc983c743daad24c599215500a2e06e7eff82239 Mon Sep 17 00:00:00 2001
From: Sukadev Bhattiprolu <sukadev-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>
Date: Fri, 3 Oct 2008 19:33:46 -0700
Subject: [PATCH] Define/use get_init_pts_sb()

get_init_pts_sb() is identical to get_sb_single() except that it
consistently picks up the super-block from initial kernel mount.

get_sb_single() fails to pick up the initial kernel mount if there
has been an 'newinstance' mount of devpts since the initial kernel
mount.

Touch-tested

TODO:
	This patch leaves the 'get_sb_ref() function unused which can be
	deleted.

	If this makes sense, integrate this patch into ptsns patchset.
---
 fs/devpts/inode.c |   53 +++++++++++++++++++++++++++++++++++++++++------------
 1 files changed, 41 insertions(+), 12 deletions(-)

diff --git a/fs/devpts/inode.c b/fs/devpts/inode.c
index 941a3ce..27aa6d3 100644
--- a/fs/devpts/inode.c
+++ b/fs/devpts/inode.c
@@ -408,6 +408,38 @@ static int is_new_instance_mount(void *data)
 	return rc;
 }
 
+static int compare_init_pts_sb(struct super_block *s, void *p)
+{
+	if (devpts_mnt)
+		return devpts_mnt->mnt_sb == s;
+
+	return 0;
+}
+
+static int get_init_pts_sb(struct file_system_type *fs_type, int flags,
+		void *data, struct vfsmount *mnt)
+{
+        struct super_block *s;
+        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;
+        }
+        do_remount_sb(s, flags, data, 0);
+        return simple_set_mnt(mnt, s);
+}
+
 /*
  * Mount or remount the initial kernel mount of devpts. This type of
  * mount maintains the legacy, single-instance semantics, while the
@@ -418,21 +450,18 @@ static int init_pts_mount(struct file_system_type *fs_type, int flags,
 {
 	int err;
 
-	if (!devpts_mnt) {
-		err = get_sb_single(fs_type, flags, data, devpts_fill_super,
-				mnt);
-
-		err = mknod_ptmx(mnt->mnt_sb);
-		if (err) {
-			dput(mnt->mnt_sb->s_root);
-			deactivate_super(mnt->mnt_sb);
-		} else
-			devpts_mnt = mnt;
-
+	err = get_init_pts_sb(fs_type, flags, data, mnt);
+	if (err || devpts_mnt)
 		return err;
+
+	/* first mount of devpts - make ptmx node */
+	err = mknod_ptmx(mnt->mnt_sb);
+	if (err) {
+		dput(mnt->mnt_sb->s_root);
+		deactivate_super(mnt->mnt_sb);
 	}
 
-	return get_sb_ref(devpts_mnt->mnt_sb, flags, data, mnt);
+	return err;
 }
 
 static int devpts_get_sb(struct file_system_type *fs_type,
-- 
1.5.2.5

[PATCH 09/10] Enable multiple instances of devpts

[sukadev-r/Jw6+rmf7HQT0dZR+AlfA]

From 4567a37856205a04cc0617e3fcc8ede36b25bcf5 Mon Sep 17 00:00:00 2001
From: Sukadev Bhattiprolu <sukadev-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>
Date: Tue, 9 Sep 2008 18:52:56 -0700
Subject: [PATCH 09/10] Enable multiple instances of devpts

To support containers, allow multiple instances of devpts filesystem, such
that indices of ptys allocated in one instance are independent of ptys
allocated in other instances of devpts.

But to preserve backward compatibility, enable this support for multiple
instances only if:

	- CONFIG_DEVPTS_MULTIPLE_INSTANCES is set to Y, and
	- '-o newinstance' mount option is specified while mounting devpts

See Documentation/fs/devpts.txt (next patch in series) for details.

To use multi-instance mount, a container startup script could:

	$ ns_exec -cm /bin/bash
	$ umount /dev/pts
	$ mount -t devpts -o newinstance lxcpts /dev/pts
	$ mount -o bind /dev/pts/ptmx /dev/ptmx
	$ sshd -p 1234

where 'ns_exec -cm /bin/bash' is calls clone() with CLONE_NEWNS flag and execs
/bin/bash in the child process. A pty created by the sshd is not visible in
the original mount of /dev/pts.

USER-SPACE-IMPACT:

	See Documentation/fs/devpts.txt (included in next patch) for user-space
	impact in multi-instance and mixed-mode operation.
TODO:
	- Update mount(8), pts(4) man pages. Highlight impact of not
	  redirecting /dev/ptmx to /dev/pts/ptmx after a multi-instance mount.

Implementation note:

	See comments in new get_sb_ref() function in fs/super.c on why
	get_sb_single() cannot be directly used.

Changelog[v5]:
	- Move get_sb_ref() definition to earlier patch

	- Move usage info to Documentation/filesystems/devpts.txt (next patch)

	- Make ptmx node even in init_pts_ns, now that default mode is 0000
	  (defined in earlier patch, enabled here).

	- Cache ptmx dentry and use to update mode during remount
	  (defined in earlier patch, enabled here).

	- Bugfix: explicitly ignore newinstance on remount (if newinstance was
	  specified on remount of initial mount, it would be ignored but
	  /proc/mounts would imply that the option was set)

Changelog[v4]:

	- Update patch description to address H. Peter Anvin's comments

	- Consolidate multi-instance mode code under new config token,
	  CONFIG_DEVPTS_MULTIPLE_INSTANCE.

	- Move usage-details from patch description to
	  Documentation/fs/devpts.txt

Changelog[v3]:
	- Rename new mount option to 'newinstance'

	- Create ptmx nodes only in 'newinstance' mounts

	- Bugfix: parse_mount_options() modifies @data but since we need to
	  parse the @data twice (once in devpts_get_sb() and once during
	  do_remount_sb()), parse a local copy of @data in devpts_get_sb().
	  (restructured code in devpts_get_sb() to fix this)

Changelog[v2]:
	- Support both single-mount and multiple-mount semantics and
	  provide '-onewmnt' option to select the semantics.

Signed-off-by: Sukadev Bhattiprolu <sukadev-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>
---
 fs/devpts/inode.c |  168 +++++++++++++++++++++++++++++++++++++++++++++++++++--
 1 files changed, 163 insertions(+), 5 deletions(-)

diff --git a/fs/devpts/inode.c b/fs/devpts/inode.c
index 6b56255..c54b010 100644
--- a/fs/devpts/inode.c
+++ b/fs/devpts/inode.c
@@ -48,10 +48,11 @@ struct pts_mount_opts {
 	gid_t   gid;
 	umode_t mode;
 	umode_t ptmxmode;
+	int newinstance;
 };
 
 enum {
-	Opt_uid, Opt_gid, Opt_mode, Opt_ptmxmode,
+	Opt_uid, Opt_gid, Opt_mode, Opt_ptmxmode, Opt_newinstance,
 	Opt_err
 };
 
@@ -61,6 +62,7 @@ static match_table_t tokens = {
 	{Opt_mode, "mode=%o"},
 #ifdef CONFIG_DEVPTS_MULTIPLE_INSTANCES
 	{Opt_ptmxmode, "ptmxmode=%o"},
+	{Opt_newinstance, "newinstance"},
 #endif
 	{Opt_err, NULL}
 };
@@ -78,13 +80,15 @@ static inline struct pts_fs_info *DEVPTS_SB(struct super_block *sb)
 
 static inline struct super_block *pts_sb_from_inode(struct inode *inode)
 {
+#ifdef CONFIG_DEVPTS_MULTIPLE_INSTANCES
 	if (inode->i_sb->s_magic == DEVPTS_SUPER_MAGIC)
 		return inode->i_sb;
-
+#endif
 	return devpts_mnt->mnt_sb;
 }
 
-static int parse_mount_options(char *data, struct pts_mount_opts *opts)
+static int parse_mount_options(char *data, int remount,
+		struct pts_mount_opts *opts)
 {
 	char *p;
 
@@ -95,6 +99,10 @@ static int parse_mount_options(char *data, struct pts_mount_opts *opts)
 	opts->mode    = DEVPTS_DEFAULT_MODE;
 	opts->ptmxmode = DEVPTS_DEFAULT_PTMX_MODE;
 
+	/* ignore newinstance on remount to avoid confusing show_options */
+	if (!remount)
+		opts->newinstance = 0;
+
 	while ((p = strsep(&data, ",")) != NULL) {
 		substring_t args[MAX_OPT_ARGS];
 		int token;
@@ -128,6 +136,10 @@ static int parse_mount_options(char *data, struct pts_mount_opts *opts)
 				return -EINVAL;
 			opts->ptmxmode = option & S_IALLUGO;
 			break;
+		case Opt_newinstance:
+			if (!remount)
+				opts->newinstance = 1;
+			break;
 #endif
 		default:
 			printk(KERN_ERR "devpts: called with bogus options\n");
@@ -180,6 +192,8 @@ static int mknod_ptmx(struct super_block *sb)
 
 	d_add(dentry, inode);
 
+	fsi->ptmx_dentry = dentry;
+
 	printk(KERN_DEBUG "Created ptmx node in devpts ino %lu\n",
 			inode->i_ino);
 
@@ -207,7 +221,7 @@ static int devpts_remount(struct super_block *sb, int *flags, char *data)
 	struct pts_fs_info *fsi = DEVPTS_SB(sb);
 	struct pts_mount_opts *opts = &fsi->mount_opts;
 
-	err = parse_mount_options(data, opts);
+	err = parse_mount_options(data, 1, opts);
 
 	/*
 	 * parse_mount_options() restores options to default values
@@ -232,6 +246,8 @@ static int devpts_show_options(struct seq_file *seq, struct vfsmount *vfs)
 	seq_printf(seq, ",mode=%03o", opts->mode);
 #ifdef CONFIG_DEVPTS_MULTIPLE_INSTANCES
 	seq_printf(seq, ",ptmxmode=%03o", opts->ptmxmode);
+	if (opts->newinstance)
+		seq_printf(seq, ",newinstance");
 #endif
 
 	return 0;
@@ -298,12 +314,153 @@ fail:
 	return -ENOMEM;
 }
 
+#ifdef CONFIG_DEVPTS_MULTIPLE_INSTANCES
+/*
+ * 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, 0, 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)
+{
+	int err;
+	struct pts_fs_info *fsi;
+	struct pts_mount_opts *opts;
+
+	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);
+	opts = &fsi->mount_opts;
+
+	err = parse_mount_options(data, 0, opts);
+	if (err)
+		goto fail;
+
+	err = mknod_ptmx(mnt->mnt_sb);
+	if (err)
+		goto fail;
+
+	return 0;
+
+fail:
+	dput(mnt->mnt_sb->s_root);
+	deactivate_super(mnt->mnt_sb);
+	return err;
+}
+
+/*
+ * 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;
+}
+
+/*
+ * 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 vfsmount *mnt)
+{
+	int err;
+
+	if (!devpts_mnt) {
+		err = get_sb_single(fs_type, flags, data, devpts_fill_super,
+				mnt);
+
+		err = mknod_ptmx(mnt->mnt_sb);
+		if (err) {
+			dput(mnt->mnt_sb->s_root);
+			deactivate_super(mnt->mnt_sb);
+		} else
+			devpts_mnt = mnt;
+
+		return err;
+	}
+
+	return get_sb_ref(devpts_mnt->mnt_sb, flags, data, mnt);
+}
+
 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;
+
+	if (new)
+		return new_pts_mount(fs_type, flags, data, mnt);
+
+	return init_pts_mount(fs_type, flags, data, mnt);
+}
+#else
+/*
+ * This supports only the legacy single-instance semantics (no
+ * multiple-instance semantics)
+ */
+static int devpts_get_sb(struct file_system_type *fs_type, int flags,
+		const char *dev_name, void *data, struct vfsmount *mnt)
+{
 	return get_sb_single(fs_type, flags, data, devpts_fill_super, mnt);
 }
 
+#endif
+
 static void devpts_kill_sb(struct super_block *sb)
 {
 	struct pts_fs_info *fsi = DEVPTS_SB(sb);
@@ -431,8 +588,9 @@ void devpts_pty_kill(struct tty_struct *tty)
 	if (dentry && !IS_ERR(dentry)) {
 		inode->i_nlink--;
 		d_delete(dentry);
-		dput(dentry);
+		dput(dentry);		// d_lookup in devpts_pty_new
 	}
+	dput(dentry);			// d_find_alias above
 
 	mutex_unlock(&root->d_inode->i_mutex);
 }
-- 
1.5.2.5

Re: [PATCH 09/10] Enable multiple instances of devpts

[Serge E. Hallyn]

Quoting sukadev-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org (sukadev-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org):
> 
> >From 4567a37856205a04cc0617e3fcc8ede36b25bcf5 Mon Sep 17 00:00:00 2001
> From: Sukadev Bhattiprolu <sukadev-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>
> Date: Tue, 9 Sep 2008 18:52:56 -0700
> Subject: [PATCH 09/10] Enable multiple instances of devpts
> 
> To support containers, allow multiple instances of devpts filesystem, such
> that indices of ptys allocated in one instance are independent of ptys
> allocated in other instances of devpts.
> 
> But to preserve backward compatibility, enable this support for multiple
> instances only if:
> 
> 	- CONFIG_DEVPTS_MULTIPLE_INSTANCES is set to Y, and
> 	- '-o newinstance' mount option is specified while mounting devpts
> 
> See Documentation/fs/devpts.txt (next patch in series) for details.
> 
> To use multi-instance mount, a container startup script could:
> 
> 	$ ns_exec -cm /bin/bash
> 	$ umount /dev/pts
> 	$ mount -t devpts -o newinstance lxcpts /dev/pts
> 	$ mount -o bind /dev/pts/ptmx /dev/ptmx
> 	$ sshd -p 1234
> 
> where 'ns_exec -cm /bin/bash' is calls clone() with CLONE_NEWNS flag and execs
> /bin/bash in the child process. A pty created by the sshd is not visible in
> the original mount of /dev/pts.
> 
> USER-SPACE-IMPACT:
> 
> 	See Documentation/fs/devpts.txt (included in next patch) for user-space
> 	impact in multi-instance and mixed-mode operation.
> TODO:
> 	- Update mount(8), pts(4) man pages. Highlight impact of not
> 	  redirecting /dev/ptmx to /dev/pts/ptmx after a multi-instance mount.
> 
> Implementation note:
> 
> 	See comments in new get_sb_ref() function in fs/super.c on why
> 	get_sb_single() cannot be directly used.
> 
> Changelog[v5]:
> 	- Move get_sb_ref() definition to earlier patch
> 
> 	- Move usage info to Documentation/filesystems/devpts.txt (next patch)
> 
> 	- Make ptmx node even in init_pts_ns, now that default mode is 0000
> 	  (defined in earlier patch, enabled here).
> 
> 	- Cache ptmx dentry and use to update mode during remount
> 	  (defined in earlier patch, enabled here).
> 
> 	- Bugfix: explicitly ignore newinstance on remount (if newinstance was
> 	  specified on remount of initial mount, it would be ignored but
> 	  /proc/mounts would imply that the option was set)
> 
> Changelog[v4]:
> 
> 	- Update patch description to address H. Peter Anvin's comments
> 
> 	- Consolidate multi-instance mode code under new config token,
> 	  CONFIG_DEVPTS_MULTIPLE_INSTANCE.
> 
> 	- Move usage-details from patch description to
> 	  Documentation/fs/devpts.txt
> 
> Changelog[v3]:
> 	- Rename new mount option to 'newinstance'
> 
> 	- Create ptmx nodes only in 'newinstance' mounts
> 
> 	- Bugfix: parse_mount_options() modifies @data but since we need to
> 	  parse the @data twice (once in devpts_get_sb() and once during
> 	  do_remount_sb()), parse a local copy of @data in devpts_get_sb().
> 	  (restructured code in devpts_get_sb() to fix this)
> 
> Changelog[v2]:
> 	- Support both single-mount and multiple-mount semantics and
> 	  provide '-onewmnt' option to select the semantics.
> 
> Signed-off-by: Sukadev Bhattiprolu <sukadev-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>
> ---
>  fs/devpts/inode.c |  168 +++++++++++++++++++++++++++++++++++++++++++++++++++--
>  1 files changed, 163 insertions(+), 5 deletions(-)
> 
> diff --git a/fs/devpts/inode.c b/fs/devpts/inode.c
> index 6b56255..c54b010 100644
> --- a/fs/devpts/inode.c
> +++ b/fs/devpts/inode.c
> @@ -48,10 +48,11 @@ struct pts_mount_opts {
>  	gid_t   gid;
>  	umode_t mode;
>  	umode_t ptmxmode;
> +	int newinstance;
>  };
> 
>  enum {
> -	Opt_uid, Opt_gid, Opt_mode, Opt_ptmxmode,
> +	Opt_uid, Opt_gid, Opt_mode, Opt_ptmxmode, Opt_newinstance,
>  	Opt_err
>  };
> 
> @@ -61,6 +62,7 @@ static match_table_t tokens = {
>  	{Opt_mode, "mode=%o"},
>  #ifdef CONFIG_DEVPTS_MULTIPLE_INSTANCES
>  	{Opt_ptmxmode, "ptmxmode=%o"},
> +	{Opt_newinstance, "newinstance"},
>  #endif
>  	{Opt_err, NULL}
>  };
> @@ -78,13 +80,15 @@ static inline struct pts_fs_info *DEVPTS_SB(struct super_block *sb)
> 
>  static inline struct super_block *pts_sb_from_inode(struct inode *inode)
>  {
> +#ifdef CONFIG_DEVPTS_MULTIPLE_INSTANCES
>  	if (inode->i_sb->s_magic == DEVPTS_SUPER_MAGIC)
>  		return inode->i_sb;
> -
> +#endif
>  	return devpts_mnt->mnt_sb;
>  }
> 
> -static int parse_mount_options(char *data, struct pts_mount_opts *opts)
> +static int parse_mount_options(char *data, int remount,
> +		struct pts_mount_opts *opts)
>  {
>  	char *p;
> 
> @@ -95,6 +99,10 @@ static int parse_mount_options(char *data, struct pts_mount_opts *opts)
>  	opts->mode    = DEVPTS_DEFAULT_MODE;
>  	opts->ptmxmode = DEVPTS_DEFAULT_PTMX_MODE;

So does this mean that if I do a remount, the mode and ptmxmode will get
reset to the defaults unless I specify them again?

> 
> +	/* ignore newinstance on remount to avoid confusing show_options */
> +	if (!remount)
> +		opts->newinstance = 0;
> +
>  	while ((p = strsep(&data, ",")) != NULL) {
>  		substring_t args[MAX_OPT_ARGS];
>  		int token;
> @@ -128,6 +136,10 @@ static int parse_mount_options(char *data, struct pts_mount_opts *opts)
>  				return -EINVAL;
>  			opts->ptmxmode = option & S_IALLUGO;
>  			break;
> +		case Opt_newinstance:
> +			if (!remount)
> +				opts->newinstance = 1;
> +			break;
>  #endif
>  		default:
>  			printk(KERN_ERR "devpts: called with bogus options\n");
> @@ -180,6 +192,8 @@ static int mknod_ptmx(struct super_block *sb)
> 
>  	d_add(dentry, inode);
> 
> +	fsi->ptmx_dentry = dentry;
> +
>  	printk(KERN_DEBUG "Created ptmx node in devpts ino %lu\n",
>  			inode->i_ino);
> 
> @@ -207,7 +221,7 @@ static int devpts_remount(struct super_block *sb, int *flags, char *data)
>  	struct pts_fs_info *fsi = DEVPTS_SB(sb);
>  	struct pts_mount_opts *opts = &fsi->mount_opts;
> 
> -	err = parse_mount_options(data, opts);
> +	err = parse_mount_options(data, 1, opts);

A guess a #define rather than '1' would be better here.

> 
>  	/*
>  	 * parse_mount_options() restores options to default values
> @@ -232,6 +246,8 @@ static int devpts_show_options(struct seq_file *seq, struct vfsmount *vfs)
>  	seq_printf(seq, ",mode=%03o", opts->mode);
>  #ifdef CONFIG_DEVPTS_MULTIPLE_INSTANCES
>  	seq_printf(seq, ",ptmxmode=%03o", opts->ptmxmode);
> +	if (opts->newinstance)
> +		seq_printf(seq, ",newinstance");

Is actually that something we want to show?  It doesn't seem
informative.

>  #endif
> 
>  	return 0;
> @@ -298,12 +314,153 @@ fail:
>  	return -ENOMEM;
>  }
> 
> +#ifdef CONFIG_DEVPTS_MULTIPLE_INSTANCES
> +/*
> + * 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, 0, 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)
> +{
> +	int err;
> +	struct pts_fs_info *fsi;
> +	struct pts_mount_opts *opts;
> +
> +	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);
> +	opts = &fsi->mount_opts;
> +
> +	err = parse_mount_options(data, 0, opts);
> +	if (err)
> +		goto fail;
> +
> +	err = mknod_ptmx(mnt->mnt_sb);
> +	if (err)
> +		goto fail;
> +
> +	return 0;
> +
> +fail:
> +	dput(mnt->mnt_sb->s_root);
> +	deactivate_super(mnt->mnt_sb);
> +	return err;
> +}
> +
> +/*
> + * 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;
> +}
> +
> +/*
> + * 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 vfsmount *mnt)
> +{
> +	int err;
> +
> +	if (!devpts_mnt) {
> +		err = get_sb_single(fs_type, flags, data, devpts_fill_super,
> +				mnt);
> +
> +		err = mknod_ptmx(mnt->mnt_sb);
> +		if (err) {
> +			dput(mnt->mnt_sb->s_root);
> +			deactivate_super(mnt->mnt_sb);
> +		} else
> +			devpts_mnt = mnt;
> +
> +		return err;

There is no locking here, so in early-userspace two competing processes
could both try to set devpts_mnt, right?

> +	}
> +
> +	return get_sb_ref(devpts_mnt->mnt_sb, flags, data, mnt);
> +}
> +
>  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;
> +
> +	if (new)
> +		return new_pts_mount(fs_type, flags, data, mnt);
> +
> +	return init_pts_mount(fs_type, flags, data, mnt);

Wait a sec - so if a container does

	mount -t devpts -o newinstance none /dev/pts
	and then later on just does
	mount -t devpts none /dev/pts

it'll get the init_pts_ns, not the one it had created?

Yup, just confirmed, using:

	ns_exec -cmiup /bin/sh
	   mount -t devpts -o newinstance -n none /dev/pts
	   mount -t devpts -n none /mnt
	   ls /dev/pts
	     ptmx
	   ls /mnt
	     0 ptmx
That's weird.

> +}
> +#else
> +/*
> + * This supports only the legacy single-instance semantics (no
> + * multiple-instance semantics)
> + */
> +static int devpts_get_sb(struct file_system_type *fs_type, int flags,
> +		const char *dev_name, void *data, struct vfsmount *mnt)
> +{
>  	return get_sb_single(fs_type, flags, data, devpts_fill_super, mnt);
>  }
> 
> +#endif
> +
>  static void devpts_kill_sb(struct super_block *sb)
>  {
>  	struct pts_fs_info *fsi = DEVPTS_SB(sb);
> @@ -431,8 +588,9 @@ void devpts_pty_kill(struct tty_struct *tty)
>  	if (dentry && !IS_ERR(dentry)) {
>  		inode->i_nlink--;
>  		d_delete(dentry);
> -		dput(dentry);
> +		dput(dentry);		// d_lookup in devpts_pty_new
>  	}
> +	dput(dentry);			// d_find_alias above
> 
>  	mutex_unlock(&root->d_inode->i_mutex);
>  }
> -- 
> 1.5.2.5

Re: [PATCH 09/10] Enable multiple instances of devpts

[sukadev-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8]

Thanks for detailed review Serge.

Sorry was unexpectedly off most of this week. Have some responses below
will review/address other comments over the next few days.


Serge E. Hallyn [serue-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org] wrote:
| Quoting sukadev-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org (sukadev-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org):
| > 
| > >From 4567a37856205a04cc0617e3fcc8ede36b25bcf5 Mon Sep 17 00:00:00 2001
| > From: Sukadev Bhattiprolu <sukadev-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>
| > Date: Tue, 9 Sep 2008 18:52:56 -0700
| > Subject: [PATCH 09/10] Enable multiple instances of devpts
| > 
| > To support containers, allow multiple instances of devpts filesystem, such
| > that indices of ptys allocated in one instance are independent of ptys
| > allocated in other instances of devpts.
| > 
| > But to preserve backward compatibility, enable this support for multiple
| > instances only if:
| > 
| > 	- CONFIG_DEVPTS_MULTIPLE_INSTANCES is set to Y, and
| > 	- '-o newinstance' mount option is specified while mounting devpts
| > 
| > See Documentation/fs/devpts.txt (next patch in series) for details.
| > 
| > To use multi-instance mount, a container startup script could:
| > 
| > 	$ ns_exec -cm /bin/bash
| > 	$ umount /dev/pts
| > 	$ mount -t devpts -o newinstance lxcpts /dev/pts
| > 	$ mount -o bind /dev/pts/ptmx /dev/ptmx
| > 	$ sshd -p 1234
| > 
| > where 'ns_exec -cm /bin/bash' is calls clone() with CLONE_NEWNS flag and execs
| > /bin/bash in the child process. A pty created by the sshd is not visible in
| > the original mount of /dev/pts.
| > 
| > USER-SPACE-IMPACT:
| > 
| > 	See Documentation/fs/devpts.txt (included in next patch) for user-space
| > 	impact in multi-instance and mixed-mode operation.
| > TODO:
| > 	- Update mount(8), pts(4) man pages. Highlight impact of not
| > 	  redirecting /dev/ptmx to /dev/pts/ptmx after a multi-instance mount.
| > 
| > Implementation note:
| > 
| > 	See comments in new get_sb_ref() function in fs/super.c on why
| > 	get_sb_single() cannot be directly used.
| > 
| > Changelog[v5]:
| > 	- Move get_sb_ref() definition to earlier patch
| > 
| > 	- Move usage info to Documentation/filesystems/devpts.txt (next patch)
| > 
| > 	- Make ptmx node even in init_pts_ns, now that default mode is 0000
| > 	  (defined in earlier patch, enabled here).
| > 
| > 	- Cache ptmx dentry and use to update mode during remount
| > 	  (defined in earlier patch, enabled here).
| > 
| > 	- Bugfix: explicitly ignore newinstance on remount (if newinstance was
| > 	  specified on remount of initial mount, it would be ignored but
| > 	  /proc/mounts would imply that the option was set)
| > 
| > Changelog[v4]:
| > 
| > 	- Update patch description to address H. Peter Anvin's comments
| > 
| > 	- Consolidate multi-instance mode code under new config token,
| > 	  CONFIG_DEVPTS_MULTIPLE_INSTANCE.
| > 
| > 	- Move usage-details from patch description to
| > 	  Documentation/fs/devpts.txt
| > 
| > Changelog[v3]:
| > 	- Rename new mount option to 'newinstance'
| > 
| > 	- Create ptmx nodes only in 'newinstance' mounts
| > 
| > 	- Bugfix: parse_mount_options() modifies @data but since we need to
| > 	  parse the @data twice (once in devpts_get_sb() and once during
| > 	  do_remount_sb()), parse a local copy of @data in devpts_get_sb().
| > 	  (restructured code in devpts_get_sb() to fix this)
| > 
| > Changelog[v2]:
| > 	- Support both single-mount and multiple-mount semantics and
| > 	  provide '-onewmnt' option to select the semantics.
| > 
| > Signed-off-by: Sukadev Bhattiprolu <sukadev-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>
| > ---
| >  fs/devpts/inode.c |  168 +++++++++++++++++++++++++++++++++++++++++++++++++++--
| >  1 files changed, 163 insertions(+), 5 deletions(-)
| > 
| > diff --git a/fs/devpts/inode.c b/fs/devpts/inode.c
| > index 6b56255..c54b010 100644
| > --- a/fs/devpts/inode.c
| > +++ b/fs/devpts/inode.c
| > @@ -48,10 +48,11 @@ struct pts_mount_opts {
| >  	gid_t   gid;
| >  	umode_t mode;
| >  	umode_t ptmxmode;
| > +	int newinstance;
| >  };
| > 
| >  enum {
| > -	Opt_uid, Opt_gid, Opt_mode, Opt_ptmxmode,
| > +	Opt_uid, Opt_gid, Opt_mode, Opt_ptmxmode, Opt_newinstance,
| >  	Opt_err
| >  };
| > 
| > @@ -61,6 +62,7 @@ static match_table_t tokens = {
| >  	{Opt_mode, "mode=%o"},
| >  #ifdef CONFIG_DEVPTS_MULTIPLE_INSTANCES
| >  	{Opt_ptmxmode, "ptmxmode=%o"},
| > +	{Opt_newinstance, "newinstance"},
| >  #endif
| >  	{Opt_err, NULL}
| >  };
| > @@ -78,13 +80,15 @@ static inline struct pts_fs_info *DEVPTS_SB(struct super_block *sb)
| > 
| >  static inline struct super_block *pts_sb_from_inode(struct inode *inode)
| >  {
| > +#ifdef CONFIG_DEVPTS_MULTIPLE_INSTANCES
| >  	if (inode->i_sb->s_magic == DEVPTS_SUPER_MAGIC)
| >  		return inode->i_sb;
| > -
| > +#endif
| >  	return devpts_mnt->mnt_sb;
| >  }
| > 
| > -static int parse_mount_options(char *data, struct pts_mount_opts *opts)
| > +static int parse_mount_options(char *data, int remount,
| > +		struct pts_mount_opts *opts)
| >  {
| >  	char *p;
| > 
| > @@ -95,6 +99,10 @@ static int parse_mount_options(char *data, struct pts_mount_opts *opts)
| >  	opts->mode    = DEVPTS_DEFAULT_MODE;
| >  	opts->ptmxmode = DEVPTS_DEFAULT_PTMX_MODE;
| 
| So does this mean that if I do a remount, the mode and ptmxmode will get
| reset to the defaults unless I specify them again?

Yes.

I think I pointed out this behavior earlier but don't think I received any
comments and stuck to current behavior (which for instance resets the mode
to default).

Besides, this behavior may help in the transition - initial kernel mount
would set ptmx-mode to 0 (default) and admin can update /etc/fstab to set
ptmx mode meaningfully.

| 
| > 
| > +	/* ignore newinstance on remount to avoid confusing show_options */
| > +	if (!remount)
| > +		opts->newinstance = 0;
| > +
| >  	while ((p = strsep(&data, ",")) != NULL) {
| >  		substring_t args[MAX_OPT_ARGS];
| >  		int token;
| > @@ -128,6 +136,10 @@ static int parse_mount_options(char *data, struct pts_mount_opts *opts)
| >  				return -EINVAL;
| >  			opts->ptmxmode = option & S_IALLUGO;
| >  			break;
| > +		case Opt_newinstance:
| > +			if (!remount)
| > +				opts->newinstance = 1;
| > +			break;
| >  #endif
| >  		default:
| >  			printk(KERN_ERR "devpts: called with bogus options\n");
| > @@ -180,6 +192,8 @@ static int mknod_ptmx(struct super_block *sb)
| > 
| >  	d_add(dentry, inode);
| > 
| > +	fsi->ptmx_dentry = dentry;
| > +
| >  	printk(KERN_DEBUG "Created ptmx node in devpts ino %lu\n",
| >  			inode->i_ino);
| > 
| > @@ -207,7 +221,7 @@ static int devpts_remount(struct super_block *sb, int *flags, char *data)
| >  	struct pts_fs_info *fsi = DEVPTS_SB(sb);
| >  	struct pts_mount_opts *opts = &fsi->mount_opts;
| > 
| > -	err = parse_mount_options(data, opts);
| > +	err = parse_mount_options(data, 1, opts);
| 
| A guess a #define rather than '1' would be better here.

Ok.

| 
| > 
| >  	/*
| >  	 * parse_mount_options() restores options to default values
| > @@ -232,6 +246,8 @@ static int devpts_show_options(struct seq_file *seq, struct vfsmount *vfs)
| >  	seq_printf(seq, ",mode=%03o", opts->mode);
| >  #ifdef CONFIG_DEVPTS_MULTIPLE_INSTANCES
| >  	seq_printf(seq, ",ptmxmode=%03o", opts->ptmxmode);
| > +	if (opts->newinstance)
| > +		seq_printf(seq, ",newinstance");
| 
| Is actually that something we want to show?  It doesn't seem
| informative.

Without this users have no easy way of knowing whether they have a 
private mount specially if they mounted from command line ?

Does it mislead or confuse ?

| 
| >  #endif
| > 
| >  	return 0;
| > @@ -298,12 +314,153 @@ fail:
| >  	return -ENOMEM;
| >  }
| > 
| > +#ifdef CONFIG_DEVPTS_MULTIPLE_INSTANCES
| > +/*
| > + * 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, 0, 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)
| > +{
| > +	int err;
| > +	struct pts_fs_info *fsi;
| > +	struct pts_mount_opts *opts;
| > +
| > +	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);
| > +	opts = &fsi->mount_opts;
| > +
| > +	err = parse_mount_options(data, 0, opts);
| > +	if (err)
| > +		goto fail;
| > +
| > +	err = mknod_ptmx(mnt->mnt_sb);
| > +	if (err)
| > +		goto fail;
| > +
| > +	return 0;
| > +
| > +fail:
| > +	dput(mnt->mnt_sb->s_root);
| > +	deactivate_super(mnt->mnt_sb);
| > +	return err;
| > +}
| > +
| > +/*
| > + * 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;
| > +}
| > +
| > +/*
| > + * 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 vfsmount *mnt)
| > +{
| > +	int err;
| > +
| > +	if (!devpts_mnt) {
| > +		err = get_sb_single(fs_type, flags, data, devpts_fill_super,
| > +				mnt);
| > +
| > +		err = mknod_ptmx(mnt->mnt_sb);
| > +		if (err) {
| > +			dput(mnt->mnt_sb->s_root);
| > +			deactivate_super(mnt->mnt_sb);
| > +		} else
| > +			devpts_mnt = mnt;
| > +
| > +		return err;
| 
| There is no locking here, so in early-userspace two competing processes
| could both try to set devpts_mnt, right?

Hmm. I was thinking there would be only one thread calling the
vfs_kern_mount() in init_devpts_fs.

| 
| > +	}
| > +
| > +	return get_sb_ref(devpts_mnt->mnt_sb, flags, data, mnt);
| > +}
| > +
| >  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;
| > +
| > +	if (new)
| > +		return new_pts_mount(fs_type, flags, data, mnt);
| > +
| > +	return init_pts_mount(fs_type, flags, data, mnt);
| 
| Wait a sec - so if a container does
| 
| 	mount -t devpts -o newinstance none /dev/pts
| 	and then later on just does
| 	mount -t devpts none /dev/pts
| 
| it'll get the init_pts_ns, not the one it had created?

Yes.  Should we treat the latter as remount of the private instance ?
If so, user could add '-oremount' ?

The logic seems simple: With newinstance create a private namespace.
Without newinstance, bind to initial ns.

| 
| Yup, just confirmed, using:
| 
| 	ns_exec -cmiup /bin/sh
| 	   mount -t devpts -o newinstance -n none /dev/pts
| 	   mount -t devpts -n none /mnt
| 	   ls /dev/pts
| 	     ptmx
| 	   ls /mnt
| 	     0 ptmx
| That's weird.
| > +}
| > +#else
| > +/*
| > + * This supports only the legacy single-instance semantics (no
| > + * multiple-instance semantics)
| > + */
| > +static int devpts_get_sb(struct file_system_type *fs_type, int flags,
| > +		const char *dev_name, void *data, struct vfsmount *mnt)
| > +{
| >  	return get_sb_single(fs_type, flags, data, devpts_fill_super, mnt);
| >  }
| > 
| > +#endif
| > +
| >  static void devpts_kill_sb(struct super_block *sb)
| >  {
| >  	struct pts_fs_info *fsi = DEVPTS_SB(sb);
| > @@ -431,8 +588,9 @@ void devpts_pty_kill(struct tty_struct *tty)
| >  	if (dentry && !IS_ERR(dentry)) {
| >  		inode->i_nlink--;
| >  		d_delete(dentry);
| > -		dput(dentry);
| > +		dput(dentry);		// d_lookup in devpts_pty_new
| >  	}
| > +	dput(dentry);			// d_find_alias above
| > 
| >  	mutex_unlock(&root->d_inode->i_mutex);
| >  }
| > -- 
| > 1.5.2.5

Re: [PATCH 09/10] Enable multiple instances of devpts

[Serge E. Hallyn]

Quoting sukadev-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org (sukadev-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org):
> Thanks for detailed review Serge.
> 
> Sorry was unexpectedly off most of this week. Have some responses below
> will review/address other comments over the next few days.
> 
> 
> Serge E. Hallyn [serue-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org] wrote:
> | Quoting sukadev-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org (sukadev-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org):
> | > 
> | > >From 4567a37856205a04cc0617e3fcc8ede36b25bcf5 Mon Sep 17 00:00:00 2001
> | > From: Sukadev Bhattiprolu <sukadev-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>
> | > Date: Tue, 9 Sep 2008 18:52:56 -0700
> | > Subject: [PATCH 09/10] Enable multiple instances of devpts
> | > 
> | > To support containers, allow multiple instances of devpts filesystem, such
> | > that indices of ptys allocated in one instance are independent of ptys
> | > allocated in other instances of devpts.
> | > 
> | > But to preserve backward compatibility, enable this support for multiple
> | > instances only if:
> | > 
> | > 	- CONFIG_DEVPTS_MULTIPLE_INSTANCES is set to Y, and
> | > 	- '-o newinstance' mount option is specified while mounting devpts
> | > 
> | > See Documentation/fs/devpts.txt (next patch in series) for details.
> | > 
> | > To use multi-instance mount, a container startup script could:
> | > 
> | > 	$ ns_exec -cm /bin/bash
> | > 	$ umount /dev/pts
> | > 	$ mount -t devpts -o newinstance lxcpts /dev/pts
> | > 	$ mount -o bind /dev/pts/ptmx /dev/ptmx
> | > 	$ sshd -p 1234
> | > 
> | > where 'ns_exec -cm /bin/bash' is calls clone() with CLONE_NEWNS flag and execs
> | > /bin/bash in the child process. A pty created by the sshd is not visible in
> | > the original mount of /dev/pts.
> | > 
> | > USER-SPACE-IMPACT:
> | > 
> | > 	See Documentation/fs/devpts.txt (included in next patch) for user-space
> | > 	impact in multi-instance and mixed-mode operation.
> | > TODO:
> | > 	- Update mount(8), pts(4) man pages. Highlight impact of not
> | > 	  redirecting /dev/ptmx to /dev/pts/ptmx after a multi-instance mount.
> | > 
> | > Implementation note:
> | > 
> | > 	See comments in new get_sb_ref() function in fs/super.c on why
> | > 	get_sb_single() cannot be directly used.
> | > 
> | > Changelog[v5]:
> | > 	- Move get_sb_ref() definition to earlier patch
> | > 
> | > 	- Move usage info to Documentation/filesystems/devpts.txt (next patch)
> | > 
> | > 	- Make ptmx node even in init_pts_ns, now that default mode is 0000
> | > 	  (defined in earlier patch, enabled here).
> | > 
> | > 	- Cache ptmx dentry and use to update mode during remount
> | > 	  (defined in earlier patch, enabled here).
> | > 
> | > 	- Bugfix: explicitly ignore newinstance on remount (if newinstance was
> | > 	  specified on remount of initial mount, it would be ignored but
> | > 	  /proc/mounts would imply that the option was set)
> | > 
> | > Changelog[v4]:
> | > 
> | > 	- Update patch description to address H. Peter Anvin's comments
> | > 
> | > 	- Consolidate multi-instance mode code under new config token,
> | > 	  CONFIG_DEVPTS_MULTIPLE_INSTANCE.
> | > 
> | > 	- Move usage-details from patch description to
> | > 	  Documentation/fs/devpts.txt
> | > 
> | > Changelog[v3]:
> | > 	- Rename new mount option to 'newinstance'
> | > 
> | > 	- Create ptmx nodes only in 'newinstance' mounts
> | > 
> | > 	- Bugfix: parse_mount_options() modifies @data but since we need to
> | > 	  parse the @data twice (once in devpts_get_sb() and once during
> | > 	  do_remount_sb()), parse a local copy of @data in devpts_get_sb().
> | > 	  (restructured code in devpts_get_sb() to fix this)
> | > 
> | > Changelog[v2]:
> | > 	- Support both single-mount and multiple-mount semantics and
> | > 	  provide '-onewmnt' option to select the semantics.
> | > 
> | > Signed-off-by: Sukadev Bhattiprolu <sukadev-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>
> | > ---
> | >  fs/devpts/inode.c |  168 +++++++++++++++++++++++++++++++++++++++++++++++++++--
> | >  1 files changed, 163 insertions(+), 5 deletions(-)
> | > 
> | > diff --git a/fs/devpts/inode.c b/fs/devpts/inode.c
> | > index 6b56255..c54b010 100644
> | > --- a/fs/devpts/inode.c
> | > +++ b/fs/devpts/inode.c
> | > @@ -48,10 +48,11 @@ struct pts_mount_opts {
> | >  	gid_t   gid;
> | >  	umode_t mode;
> | >  	umode_t ptmxmode;
> | > +	int newinstance;
> | >  };
> | > 
> | >  enum {
> | > -	Opt_uid, Opt_gid, Opt_mode, Opt_ptmxmode,
> | > +	Opt_uid, Opt_gid, Opt_mode, Opt_ptmxmode, Opt_newinstance,
> | >  	Opt_err
> | >  };
> | > 
> | > @@ -61,6 +62,7 @@ static match_table_t tokens = {
> | >  	{Opt_mode, "mode=%o"},
> | >  #ifdef CONFIG_DEVPTS_MULTIPLE_INSTANCES
> | >  	{Opt_ptmxmode, "ptmxmode=%o"},
> | > +	{Opt_newinstance, "newinstance"},
> | >  #endif
> | >  	{Opt_err, NULL}
> | >  };
> | > @@ -78,13 +80,15 @@ static inline struct pts_fs_info *DEVPTS_SB(struct super_block *sb)
> | > 
> | >  static inline struct super_block *pts_sb_from_inode(struct inode *inode)
> | >  {
> | > +#ifdef CONFIG_DEVPTS_MULTIPLE_INSTANCES
> | >  	if (inode->i_sb->s_magic == DEVPTS_SUPER_MAGIC)
> | >  		return inode->i_sb;
> | > -
> | > +#endif
> | >  	return devpts_mnt->mnt_sb;
> | >  }
> | > 
> | > -static int parse_mount_options(char *data, struct pts_mount_opts *opts)
> | > +static int parse_mount_options(char *data, int remount,
> | > +		struct pts_mount_opts *opts)
> | >  {
> | >  	char *p;
> | > 
> | > @@ -95,6 +99,10 @@ static int parse_mount_options(char *data, struct pts_mount_opts *opts)
> | >  	opts->mode    = DEVPTS_DEFAULT_MODE;
> | >  	opts->ptmxmode = DEVPTS_DEFAULT_PTMX_MODE;
> | 
> | So does this mean that if I do a remount, the mode and ptmxmode will get
> | reset to the defaults unless I specify them again?
> 
> Yes.
> 
> I think I pointed out this behavior earlier but don't think I received any
> comments and stuck to current behavior (which for instance resets the mode
> to default).
> 
> Besides, this behavior may help in the transition - initial kernel mount
> would set ptmx-mode to 0 (default) and admin can update /etc/fstab to set
> ptmx mode meaningfully.
> 
> | 
> | > 
> | > +	/* ignore newinstance on remount to avoid confusing show_options */
> | > +	if (!remount)
> | > +		opts->newinstance = 0;
> | > +
> | >  	while ((p = strsep(&data, ",")) != NULL) {
> | >  		substring_t args[MAX_OPT_ARGS];
> | >  		int token;
> | > @@ -128,6 +136,10 @@ static int parse_mount_options(char *data, struct pts_mount_opts *opts)
> | >  				return -EINVAL;
> | >  			opts->ptmxmode = option & S_IALLUGO;
> | >  			break;
> | > +		case Opt_newinstance:
> | > +			if (!remount)
> | > +				opts->newinstance = 1;
> | > +			break;
> | >  #endif
> | >  		default:
> | >  			printk(KERN_ERR "devpts: called with bogus options\n");
> | > @@ -180,6 +192,8 @@ static int mknod_ptmx(struct super_block *sb)
> | > 
> | >  	d_add(dentry, inode);
> | > 
> | > +	fsi->ptmx_dentry = dentry;
> | > +
> | >  	printk(KERN_DEBUG "Created ptmx node in devpts ino %lu\n",
> | >  			inode->i_ino);
> | > 
> | > @@ -207,7 +221,7 @@ static int devpts_remount(struct super_block *sb, int *flags, char *data)
> | >  	struct pts_fs_info *fsi = DEVPTS_SB(sb);
> | >  	struct pts_mount_opts *opts = &fsi->mount_opts;
> | > 
> | > -	err = parse_mount_options(data, opts);
> | > +	err = parse_mount_options(data, 1, opts);
> | 
> | A guess a #define rather than '1' would be better here.
> 
> Ok.
> 
> | 
> | > 
> | >  	/*
> | >  	 * parse_mount_options() restores options to default values
> | > @@ -232,6 +246,8 @@ static int devpts_show_options(struct seq_file *seq, struct vfsmount *vfs)
> | >  	seq_printf(seq, ",mode=%03o", opts->mode);
> | >  #ifdef CONFIG_DEVPTS_MULTIPLE_INSTANCES
> | >  	seq_printf(seq, ",ptmxmode=%03o", opts->ptmxmode);
> | > +	if (opts->newinstance)
> | > +		seq_printf(seq, ",newinstance");
> | 
> | Is actually that something we want to show?  It doesn't seem
> | informative.
> 
> Without this users have no easy way of knowing whether they have a 
> private mount specially if they mounted from command line ?

If they were in a container to begin with, then they still don't know.

Now if you were to keep a unique per-instance id and have show_options
list 'instance=%x', that would be helpful.  Either that or just
dropping the info altogether make sense.  This 'newinstance' listing
is meaningless.

> Does it mislead or confuse ?
> 
> | 
> | >  #endif
> | > 
> | >  	return 0;
> | > @@ -298,12 +314,153 @@ fail:
> | >  	return -ENOMEM;
> | >  }
> | > 
> | > +#ifdef CONFIG_DEVPTS_MULTIPLE_INSTANCES
> | > +/*
> | > + * 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, 0, 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)
> | > +{
> | > +	int err;
> | > +	struct pts_fs_info *fsi;
> | > +	struct pts_mount_opts *opts;
> | > +
> | > +	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);
> | > +	opts = &fsi->mount_opts;
> | > +
> | > +	err = parse_mount_options(data, 0, opts);
> | > +	if (err)
> | > +		goto fail;
> | > +
> | > +	err = mknod_ptmx(mnt->mnt_sb);
> | > +	if (err)
> | > +		goto fail;
> | > +
> | > +	return 0;
> | > +
> | > +fail:
> | > +	dput(mnt->mnt_sb->s_root);
> | > +	deactivate_super(mnt->mnt_sb);
> | > +	return err;
> | > +}
> | > +
> | > +/*
> | > + * 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;
> | > +}
> | > +
> | > +/*
> | > + * 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 vfsmount *mnt)
> | > +{
> | > +	int err;
> | > +
> | > +	if (!devpts_mnt) {
> | > +		err = get_sb_single(fs_type, flags, data, devpts_fill_super,
> | > +				mnt);
> | > +
> | > +		err = mknod_ptmx(mnt->mnt_sb);
> | > +		if (err) {
> | > +			dput(mnt->mnt_sb->s_root);
> | > +			deactivate_super(mnt->mnt_sb);
> | > +		} else
> | > +			devpts_mnt = mnt;
> | > +
> | > +		return err;
> | 
> | There is no locking here, so in early-userspace two competing processes
> | could both try to set devpts_mnt, right?
> 
> Hmm. I was thinking there would be only one thread calling the
> vfs_kern_mount() in init_devpts_fs.

But what if init happens to (perhaps mistakenly) lead to 2 racing ones?

Sure it's just a small memory leak, but why not just prevent it.

> | 
> | > +	}
> | > +
> | > +	return get_sb_ref(devpts_mnt->mnt_sb, flags, data, mnt);
> | > +}
> | > +
> | >  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;
> | > +
> | > +	if (new)
> | > +		return new_pts_mount(fs_type, flags, data, mnt);
> | > +
> | > +	return init_pts_mount(fs_type, flags, data, mnt);
> | 
> | Wait a sec - so if a container does
> | 
> | 	mount -t devpts -o newinstance none /dev/pts
> | 	and then later on just does
> | 	mount -t devpts none /dev/pts
> | 
> | it'll get the init_pts_ns, not the one it had created?
> 
> Yes.  Should we treat the latter as remount of the private instance ?
> If so, user could add '-oremount' ?
> 
> The logic seems simple: With newinstance create a private namespace.
> Without newinstance, bind to initial ns.

But if I'm in a container in a new mounts ns and somehow managed 
to umount -l /dev/pts, shouldn't i be able to remount my container's
devpts by just doing 'mount -t devpts devpts /dev/pts'?

> | 
> | Yup, just confirmed, using:
> | 
> | 	ns_exec -cmiup /bin/sh
> | 	   mount -t devpts -o newinstance -n none /dev/pts
> | 	   mount -t devpts -n none /mnt
> | 	   ls /dev/pts
> | 	     ptmx
> | 	   ls /mnt
> | 	     0 ptmx
> | That's weird.
> | > +}
> | > +#else
> | > +/*
> | > + * This supports only the legacy single-instance semantics (no
> | > + * multiple-instance semantics)
> | > + */
> | > +static int devpts_get_sb(struct file_system_type *fs_type, int flags,
> | > +		const char *dev_name, void *data, struct vfsmount *mnt)
> | > +{
> | >  	return get_sb_single(fs_type, flags, data, devpts_fill_super, mnt);
> | >  }
> | > 
> | > +#endif
> | > +
> | >  static void devpts_kill_sb(struct super_block *sb)
> | >  {
> | >  	struct pts_fs_info *fsi = DEVPTS_SB(sb);
> | > @@ -431,8 +588,9 @@ void devpts_pty_kill(struct tty_struct *tty)
> | >  	if (dentry && !IS_ERR(dentry)) {
> | >  		inode->i_nlink--;
> | >  		d_delete(dentry);
> | > -		dput(dentry);
> | > +		dput(dentry);		// d_lookup in devpts_pty_new
> | >  	}
> | > +	dput(dentry);			// d_find_alias above
> | > 
> | >  	mutex_unlock(&root->d_inode->i_mutex);
> | >  }
> | > -- 
> | > 1.5.2.5

Re: [PATCH 09/10] Enable multiple instances of devpts

[sukadev-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8]

Serge E. Hallyn [serue-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org] wrote:
| Quoting sukadev-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org (sukadev-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org):
| > | > @@ -232,6 +246,8 @@ static int devpts_show_options(struct seq_file *seq, struct vfsmount *vfs)
| > | >  	seq_printf(seq, ",mode=%03o", opts->mode);
| > | >  #ifdef CONFIG_DEVPTS_MULTIPLE_INSTANCES
| > | >  	seq_printf(seq, ",ptmxmode=%03o", opts->ptmxmode);
| > | > +	if (opts->newinstance)
| > | > +		seq_printf(seq, ",newinstance");
| > | 
| > | Is actually that something we want to show?  It doesn't seem
| > | informative.
| > 
| > Without this users have no easy way of knowing whether they have a 
| > private mount specially if they mounted from command line ?

You mean in a nested container ? I agree that it does not help then.

| 
| If they were in a container to begin with, then they still don't know.
| 
| Now if you were to keep a unique per-instance id and have show_options
| list 'instance=%x', that would be helpful.  Either that or just
| dropping the info altogether make sense.  This 'newinstance' listing
| is meaningless.
| 

Another way to look at it is that it is a mount option that was specified
and we just report it. It may not be useful always but might help in some
cases. But I am fine either way.

<snip>

| > | > +
| > | > +		err = mknod_ptmx(mnt->mnt_sb);
| > | > +		if (err) {
| > | > +			dput(mnt->mnt_sb->s_root);
| > | > +			deactivate_super(mnt->mnt_sb);
| > | > +		} else
| > | > +			devpts_mnt = mnt;
| > | > +
| > | > +		return err;
| > | 
| > | There is no locking here, so in early-userspace two competing processes
| > | could both try to set devpts_mnt, right?
| > 
| > Hmm. I was thinking there would be only one thread calling the
| > vfs_kern_mount() in init_devpts_fs.
| 
| But what if init happens to (perhaps mistakenly) lead to 2 racing ones?
| 
| Sure it's just a small memory leak, but why not just prevent it.

Ok.

| 
| > | 
| > | > +	}
| > | > +
| > | > +	return get_sb_ref(devpts_mnt->mnt_sb, flags, data, mnt);
| > | > +}
| > | > +
| > | >  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;
| > | > +
| > | > +	if (new)
| > | > +		return new_pts_mount(fs_type, flags, data, mnt);
| > | > +
| > | > +	return init_pts_mount(fs_type, flags, data, mnt);
| > | 
| > | Wait a sec - so if a container does
| > | 
| > | 	mount -t devpts -o newinstance none /dev/pts
| > | 	and then later on just does
| > | 	mount -t devpts none /dev/pts
| > | 
| > | it'll get the init_pts_ns, not the one it had created?
| > 
| > Yes.  Should we treat the latter as remount of the private instance ?
| > If so, user could add '-oremount' ?
| > 
| > The logic seems simple: With newinstance create a private namespace.
| > Without newinstance, bind to initial ns.
| 
| But if I'm in a container in a new mounts ns and somehow managed 
| to umount -l /dev/pts, shouldn't i be able to remount my container's
| devpts by just doing 'mount -t devpts devpts /dev/pts'?

Now wouldn't that require us to associate the devpts mount with some
notion of a container ? (a namespace object in nsproxy of container-init 
like we do with /proc).

Yes, after 'umount -l'  we have lost _that_ devpts ns and we may have to
'redo' the relevant container-init parts

Re: [PATCH 09/10] Enable multiple instances of devpts

[Serge E. Hallyn]

Quoting sukadev-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org (sukadev-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org):
> Serge E. Hallyn [serue-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org] wrote:
> | Quoting sukadev-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org (sukadev-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org):
> | > | > @@ -232,6 +246,8 @@ static int devpts_show_options(struct seq_file *seq, struct vfsmount *vfs)
> | > | >  	seq_printf(seq, ",mode=%03o", opts->mode);
> | > | >  #ifdef CONFIG_DEVPTS_MULTIPLE_INSTANCES
> | > | >  	seq_printf(seq, ",ptmxmode=%03o", opts->ptmxmode);
> | > | > +	if (opts->newinstance)
> | > | > +		seq_printf(seq, ",newinstance");
> | > | 
> | > | Is actually that something we want to show?  It doesn't seem
> | > | informative.
> | > 
> | > Without this users have no easy way of knowing whether they have a 
> | > private mount specially if they mounted from command line ?
> 
> You mean in a nested container ? I agree that it does not help then.
> 
> | 
> | If they were in a container to begin with, then they still don't know.
> | 
> | Now if you were to keep a unique per-instance id and have show_options
> | list 'instance=%x', that would be helpful.  Either that or just
> | dropping the info altogether make sense.  This 'newinstance' listing
> | is meaningless.
> | 
> 
> Another way to look at it is that it is a mount option that was specified
> and we just report it. It may not be useful always but might help in some
> cases. But I am fine either way.
> 
> <snip>
> 
> | > | > +
> | > | > +		err = mknod_ptmx(mnt->mnt_sb);
> | > | > +		if (err) {
> | > | > +			dput(mnt->mnt_sb->s_root);
> | > | > +			deactivate_super(mnt->mnt_sb);
> | > | > +		} else
> | > | > +			devpts_mnt = mnt;
> | > | > +
> | > | > +		return err;
> | > | 
> | > | There is no locking here, so in early-userspace two competing processes
> | > | could both try to set devpts_mnt, right?
> | > 
> | > Hmm. I was thinking there would be only one thread calling the
> | > vfs_kern_mount() in init_devpts_fs.
> | 
> | But what if init happens to (perhaps mistakenly) lead to 2 racing ones?
> | 
> | Sure it's just a small memory leak, but why not just prevent it.
> 
> Ok.
> 
> | 
> | > | 
> | > | > +	}
> | > | > +
> | > | > +	return get_sb_ref(devpts_mnt->mnt_sb, flags, data, mnt);
> | > | > +}
> | > | > +
> | > | >  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;
> | > | > +
> | > | > +	if (new)
> | > | > +		return new_pts_mount(fs_type, flags, data, mnt);
> | > | > +
> | > | > +	return init_pts_mount(fs_type, flags, data, mnt);
> | > | 
> | > | Wait a sec - so if a container does
> | > | 
> | > | 	mount -t devpts -o newinstance none /dev/pts
> | > | 	and then later on just does
> | > | 	mount -t devpts none /dev/pts
> | > | 
> | > | it'll get the init_pts_ns, not the one it had created?
> | > 
> | > Yes.  Should we treat the latter as remount of the private instance ?
> | > If so, user could add '-oremount' ?
> | > 
> | > The logic seems simple: With newinstance create a private namespace.
> | > Without newinstance, bind to initial ns.
> | 
> | But if I'm in a container in a new mounts ns and somehow managed 
> | to umount -l /dev/pts, shouldn't i be able to remount my container's
> | devpts by just doing 'mount -t devpts devpts /dev/pts'?
> 
> Now wouldn't that require us to associate the devpts mount with some
> notion of a container ? (a namespace object in nsproxy of container-init 
> like we do with /proc).

Yes.

> Yes, after 'umount -l'  we have lost _that_ devpts ns and we may have to
> 'redo' the relevant container-init parts

It all just feels fragile.

I realize this is an attempt to do the 'pure fs approach' you were asked
to do.  I just don't like the resulting semantics.

-serge

Re: [PATCH 09/10] Enable multiple instances of devpts

[Cedric Le Goater]

>> | > The logic seems simple: With newinstance create a private namespace.
>> | > Without newinstance, bind to initial ns.
>> | 
>> | But if I'm in a container in a new mounts ns and somehow managed 
>> | to umount -l /dev/pts, shouldn't i be able to remount my container's
>> | devpts by just doing 'mount -t devpts devpts /dev/pts'?

yes. That's the track we are following with the mq namespace.

>> Now wouldn't that require us to associate the devpts mount with some
>> notion of a container ? (a namespace object in nsproxy of container-init 
>> like we do with /proc).
> 
> Yes.

and it gets a little bit ugly because you need to maintain an internal
mount associated with the namespace to be able to remount the same
superblock when a :

	$ umount /dev/pts
	$ mount -t devpts devpts /dev/pts'?

is done.

So if you're using the mount option 'newinstance' to unshare the 
namespace, you end up doing the internal mount and the user mount 
under the same call, which is a bit weird, indeed.

C.

>> Yes, after 'umount -l'  we have lost _that_ devpts ns and we may have to
>> 'redo' the relevant container-init parts
> 
> It all just feels fragile.
> 
> I realize this is an attempt to do the 'pure fs approach' you were asked
> to do.  I just don't like the resulting 

Re: [PATCH 09/10] Enable multiple instances of devpts

[Cedric Le Goater]

[ ... ]

> | >  	/*
> | >  	 * parse_mount_options() restores options to default values
> | > @@ -232,6 +246,8 @@ static int devpts_show_options(struct seq_file *seq, struct vfsmount *vfs)
> | >  	seq_printf(seq, ",mode=%03o", opts->mode);
> | >  #ifdef CONFIG_DEVPTS_MULTIPLE_INSTANCES
> | >  	seq_printf(seq, ",ptmxmode=%03o", opts->ptmxmode);
> | > +	if (opts->newinstance)
> | > +		seq_printf(seq, ",newinstance");
> | 
> | Is actually that something we want to show?  It doesn't seem
> | informative.
> 
> Without this users have no easy way of knowing whether they have a 
> private mount specially if they mounted from command line ?
> 
> Does it mislead or confuse ?

the mount options are already shown in /proc/self/mounts. I don't think you need
a show_options() op.

C.

[PATCH 10/10] Document usage of multiple-instances of devpts

[sukadev-r/Jw6+rmf7HQT0dZR+AlfA]

From 6d2fe9386880157f871667077db3180e9f0083a1 Mon Sep 17 00:00:00 2001
From: Sukadev Bhattiprolu <sukadev-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>
Date: Tue, 9 Sep 2008 10:43:50 -0700
Subject: [PATCH 10/10] Document usage of multiple-instances of devpts

Signed-off-by: Sukadev Bhattiprolu <sukadev-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>
---
 Documentation/filesystems/devpts.txt |  122 ++++++++++++++++++++++++++++++++++
 1 files changed, 122 insertions(+), 0 deletions(-)
 create mode 100644 Documentation/filesystems/devpts.txt

diff --git a/Documentation/filesystems/devpts.txt b/Documentation/filesystems/devpts.txt
new file mode 100644
index 0000000..93d9d01
--- /dev/null
+++ b/Documentation/filesystems/devpts.txt
@@ -0,0 +1,122 @@
+
+To support containers, we now allow multiple instances of devpts filesystem,
+such that indices of ptys allocated in one instance are independent of indices
+allocated in other instances of devpts.
+
+To preserve backward compatibility, this support for multiple instances is
+enabled only if:
+
+	- CONFIG_DEVPTS_MULTIPLE_INSTANCES=y, and
+	- '-o newinstance' mount option is specified while mounting devpts
+
+IOW, devpts now supports both single-instance and multi-instance semantics.
+
+If CONFIG_DEVPTS_MULTIPLE_INSTANCES=n, there is no change in behavior and
+this referred to as the "legacy" mode. In this mode, the new mount options
+(-o newinstance and -o ptmxmode) will be ignored with a 'bogus option' message
+on console.
+
+If CONFIG_DEVPTS_MULTIPLE_INSTANCES=y and devpts is mounted without the
+'newinstance' option (as in current start-up scripts) the new mount binds
+to the initial kernel mount of devpts. This mode is referred to as the
+'single-instance' mode and the current, single-instance semantics are
+preserved, i.e PTYs are common across the system.
+
+The only difference between this single-instance mode and the legacy mode
+is the presence of new, '/dev/pts/ptmx' node with permissions 0000, which
+can safely be ignored.
+
+If CONFIG_DEVPTS_MULTIPLE_INSTANCES=y and 'newinstance' option is specified,
+the mount is considered to be in the multi-instance mode and a new instance
+of the devpts fs is created. Any ptys created in this instance are independent
+of ptys in other instances of devpts. Like in the single-instance mode, the
+/dev/pts/ptmx node is present. To effectively use the multi-instance mode,
+open of /dev/ptmx must be a redirected to '/dev/pts/ptmx' using a symlink or
+bind-mount.
+
+Eg: A container startup script could do the following:
+
+	$ chmod 0666 /dev/pts/ptmx
+	$ rm /dev/ptmx
+	$ ln -s pts/ptmx /dev/ptmx
+	$ ns_exec -cm /bin/bash
+
+	# We are now in new container
+
+	$ umount /dev/pts
+	$ mount -t devpts -o newinstance lxcpts /dev/pts
+	$ sshd -p 1234
+
+where 'ns_exec -cm /bin/bash' calls clone() with CLONE_NEWNS flag and execs
+/bin/bash in the child process.  A pty created by the sshd is not visible in
+the original mount of /dev/pts.
+
+User-space changes
+------------------
+
+In multi-instance mode (i.e '-o newinstance' mount option is specified at least
+once), following user-space issues should be noted.
+
+1. If -o newinstance mount option is never used, /dev/pts/ptmx can be ignored
+   and no change is needed to system-startup scripts.
+
+2. To effectively use multi-instance mode (i.e -o newinstance is specified)
+   administrators or startup scripts should "redirect" open of /dev/ptmx to
+   /dev/pts/ptmx using either a bind mount or symlink.
+
+	$ mount -t devpts -o newinstance devpts /dev/pts
+
+   followed by either
+
+	$ rm /dev/ptmx
+	$ ln -s pts/ptmx /dev/ptmx
+	$ chmod 666 /dev/pts/ptmx
+   or
+	$ mount -o bind /dev/pts/ptmx /dev/ptmx
+
+3. The '/dev/ptmx -> pts/ptmx' symlink is the preferred method since it
+   enables better error-reporting and treats both single-instance and
+   multi-instance mounts similarly.
+
+   But this method requires that system-startup scripts set the mode of
+   /dev/pts/ptmx correctly (default mode is 0000). The scripts can set the
+   mode by, either
+
+   	- adding ptmxmode mount option to devpts entry in /etc/fstab, or
+	- using 'chmod 0666 /dev/pts/ptmx'
+
+4. If multi-instance mode mount is needed for containers, but the system
+   startup scripts have not yet been updated, container-startup scripts
+   should bind mount /dev/ptmx to /dev/pts/ptmx to avoid breaking single-
+   instance mounts.
+
+   Or, in general, container-startup scripts should use:
+
+	mount -t devpts -o newinstance -o ptmxmode 0666 devpts /dev/pts
+	if [ ! -L /dev/ptmx ]; then
+		mount -o bind /dev/pts/ptmx /dev/ptmx
+	fi
+
+   When all devpts mounts are multi-instance, /dev/ptmx can permanently be
+   a symlink to pts/ptmx and the bind mount can be ignored.
+
+5. A multi-instance mount that is not accompanied by the /dev/ptmx to
+   /dev/pts/ptmx redirection would result in an unusable/unreachable pty.
+
+	mount -t devpts -o newinstance lxcpts /dev/pts
+
+   immediately followed by:
+
+	open("/dev/ptmx")
+
+    would create a pty, say /dev/pts/7, in the initial kernel mount.
+    But /dev/pts/7 would be invisible in the new mount.
+
+6. The permissions for /dev/pts/ptmx node should be specified when mounting
+   /dev/pts, using the '-o ptmxmode=%o' mount option (default is 0000).
+
+	mount -t devpts -o newinstance -o ptmxmode=0644 devpts /dev/pts
+
+   The permissions can be later be changed as usual with 'chmod'.
+
+	chmod 666 /dev/pts/ptmx
-- 
1.5.2.5

Re: [PATCH 10/10] Document usage of multiple-instances of devpts

[Alan Cox]

Ok I'm happy with this patch set. It appears correct as far as the tty
side is concerned, it looks sensible in terms of interface with the
devpts layer.

Really depends what everyone else thinks about the vfs bits and the API

Re: [PATCH 10/10] Document usage of multiple-instances of devpts

[H. Peter Anvin]

Alan Cox wrote:
> Ok I'm happy with this patch set. It appears correct as far as the tty
> side is concerned, it looks sensible in terms of interface with the
> devpts layer.
> 
> Really depends what everyone else thinks about the vfs bits and the API

The last version looks fine to me.  To be fair, I have only reviewed it, 
not actually tested it.

Acked-by: H. Peter Anvin <hpa-YMNOUZJC4hwAvxtiuMwx3w@public.gmane.org>

	-hpa

Re: [PATCH 10/10] Document usage of multiple-instances of devpts

[sukadev-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8]

Alan Cox [alan-qBU/x9rampVanCEyBjwyrvXRex20P6io@public.gmane.org] wrote:
| Ok I'm happy with this patch set. It appears correct as far as the tty
| side is concerned, it looks sensible in terms of interface with the
| devpts layer.

Alan, Peter: Thanks for review and feedback.

| 
| Really depends what everyone else thinks about the vfs bits and the API

Maybe I should post to fsdevel, manpages, and lkml for the vfs/api changes. 

Serge, Eric, Pavel: Any comments on the patchset before I do that ?

Sukadev

Re: [PATCH 10/10] Document usage of multiple-instances of devpts

[Serge E. Hallyn]

Quoting sukadev-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org (sukadev-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org):
> Alan Cox [alan-qBU/x9rampVanCEyBjwyrvXRex20P6io@public.gmane.org] wrote:
> | Ok I'm happy with this patch set. It appears correct as far as the tty
> | side is concerned, it looks sensible in terms of interface with the
> | devpts layer.
> 
> Alan, Peter: Thanks for review and feedback.
> 
> | 
> | Really depends what everyone else thinks about the vfs bits and the API
> 
> Maybe I should post to fsdevel, manpages, and lkml for the vfs/api changes. 
> 
> Serge, Eric, Pavel: Any comments on the patchset before I do that ?

I didn't have any comments on the code, it looked fine.  But the
kernel locked up for me trying to ssh into a container last night.
Complete lockup so haven't yet figured out why.

Oh, actually I did have a comment on code - finding that config
variable was dreadful :)  Now obviously it makes sense on the
one hand, but finding that plus CONFIG_NET_NS plus the other
namespace variables is getting ugly.

-serge

Re: [PATCH 10/10] Document usage of multiple-instances of devpts

[Serge E. Hallyn]

Quoting Serge E. Hallyn (serue-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org):
> Quoting sukadev-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org (sukadev-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org):
> > Alan Cox [alan-qBU/x9rampVanCEyBjwyrvXRex20P6io@public.gmane.org] wrote:
> > | Ok I'm happy with this patch set. It appears correct as far as the tty
> > | side is concerned, it looks sensible in terms of interface with the
> > | devpts layer.
> > 
> > Alan, Peter: Thanks for review and feedback.
> > 
> > | 
> > | Really depends what everyone else thinks about the vfs bits and the API
> > 
> > Maybe I should post to fsdevel, manpages, and lkml for the vfs/api changes. 
> > 
> > Serge, Eric, Pavel: Any comments on the patchset before I do that ?
> 
> I didn't have any comments on the code, it looked fine.  But the
> kernel locked up for me trying to ssh into a container last night.
> Complete lockup so haven't yet figured out why.

Sigh, sorry, was having a hard time using sysrq through qemu.  Here is a
stack trace:

Starting udev:
telnet> send break
SysRq : Show State

=================================
[ INFO: inconsistent lock state ]
2.6.27-rc7-00093-g32a1662-dirty #243
---------------------------------
inconsistent {hardirq-on-W} -> {in-hardirq-R} usage.
udevtrigger/1043 [HC1[1]:SC0[0]:HE0:SE1] takes:
 (tasklist_lock){--?-}, at: [<c011c1a7>] show_state_filter+0x19/0x7d
{hardirq-on-W} state was registered at:
  [<c0139cc2>] mark_held_locks+0x42/0x58
  [<c03ea9b3>] _spin_unlock_irq+0x20/0x23
  [<c0139e19>] trace_hardirqs_on_caller+0xc8/0x112
  [<c03ea9b3>] _spin_unlock_irq+0x20/0x23
  [<c0266045>] proc_clear_tty+0x3a/0x43
  [<c012a530>] sys_setsid+0x67/0x7c
  [<c0102f2e>] syscall_call+0x7/0xb
  [<ffffffff>] 0xffffffff
irq event stamp: 176180
hardirqs last  enabled at (176179): [<c022fe94>] trace_hardirqs_on_thunk+0xc/0x10
hardirqs last disabled at (176180): [<c022fea4>] trace_hardirqs_off_thunk+0xc/0x10
softirqs last  enabled at (176178): [<c01230d3>] do_softirq+0x37/0x4d
softirqs last disabled at (176173): [<c01230d3>] do_softirq+0x37/0x4d

other info that might help us debug this:
4 locks held by udevtrigger/1043:
 #0:  (rcu_read_lock){..--}, at: [<c0180b40>] __d_lookup+0x0/0x116
 #1:  (&i->lock){++..}, at: [<c028140c>] serial8250_interrupt+0x16/0xd4
 #2:  (&port_lock_key){+...}, at: [<c02811aa>] serial8250_handle_port+0xe/0x25a
 #3:  (sysrq_key_table_lock){+...}, at: [<c0275f45>] __handle_sysrq+0x17/0xfa

stack backtrace:
Pid: 1043, comm: udevtrigger Not tainted 2.6.27-rc7-00093-g32a1662-dirty #243
 [<c0138d53>] print_usage_bug+0x13e/0x147
 [<c0139884>] mark_lock+0x3af/0x7ab
 [<c013b22c>] __lock_acquire+0x403/0x665
 [<c013b4d7>] lock_acquire+0x49/0x61
 [<c011c1a7>] show_state_filter+0x19/0x7d
 [<c03ea899>] _read_lock+0x19/0x24
 [<c011c1a7>] show_state_filter+0x19/0x7d
 [<c011c1a7>] show_state_filter+0x19/0x7d
 [<c0275fb1>] __handle_sysrq+0x83/0xfa
 [<c02812ba>] serial8250_handle_port+0x11e/0x25a
 [<c028143c>] serial8250_interrupt+0x46/0xd4
 [<c014f03e>] handle_IRQ_event+0x13/0x3d
 [<c014ffc5>] handle_edge_irq+0xa1/0x102
 [<c01055c4>] do_IRQ+0x69/0x7d
 [<c01038d8>] common_interrupt+0x28/0x30
 [<c01800d8>] __shrink_dcache_sb+0x7/0x256
 [<c0180c24>] __d_lookup+0xe4/0x116
 [<c01788f4>] do_lookup+0x28/0x158
 [<c017a1ce>] __link_path_walk+0x702/0xac9
 [<c017a5cc>] path_walk+0x37/0x70
 [<c017a76b>] do_path_lookup+0xe3/0x144
 [<c017af4a>] user_path_at+0x37/0x5f
 [<c01754f0>] vfs_stat_fd+0x15/0x3b
 [<c01755cb>] sys_stat64+0xf/0x24
 [<c0184d7b>] mntput_no_expire+0x18/0xdb
 [<c022fe94>] trace_hardirqs_on_thunk+0xc/0x10
 [<c0139e2f>] trace_hardirqs_on_caller+0xde/0x112
 [<c022fe94>] trace_hardirqs_on_thunk+0xc/0x10
 [<c0102f2e>] syscall_call+0x7/0xb
 =======================
init          S c05ab00c     0     1      0
       c7aee000 00000046 00000001 c05ab00c c05ae200 c05ae200 c7820000 c7820154
       c11a9200 00000000 00000000 00000000 c79a4428 ffffffff c7820000 00000000
       00000000 00000000 7fffffff c7829f9c 00000040 00000000 c03e8f02 c03e956b
Call Trace:
 [<c03e8f02>] schedule_timeout+0x13/0x86
 [<c03e956b>] __mutex_unlock_slowpath+0xd9/0xe0
 [<c0139e2f>] trace_hardirqs_on_caller+0xde/0x112
 [<c01964b2>] inotify_poll+0x45/0x4b
 [<c017d0eb>] do_select+0x391/0x3db
 [<c017d626>] __pollwait+0x0/0xac
 [<c0119ee4>] default_wake_function+0x0/0x8
 [<c0119ee4>] default_wake_function+0x0/0x8
 [<c0119ee4>] default_wake_function+0x0/0x8
 [<c013b427>] __lock_acquire+0x5fe/0x665
 [<c013b427>] __lock_acquire+0x5fe/0x665
 [<c013b427>] __lock_acquire+0x5fe/0x665
 [<c013b427>] __lock_acquire+0x5fe/0x665
 [<c013b427>] __lock_acquire+0x5fe/0x665
 [<c017d33e>] core_sys_select+0x209/0x2f2
 [<c013b427>] __lock_acquire+0x5fe/0x665
 [<c015e856>] __do_fault+0x1ff/0x329
 [<c015e941>] __do_fault+0x2ea/0x329
 [<c013b427>] __lock_acquire+0x5fe/0x665
 [<c015fba2>] handle_mm_fault+0x282/0x538
 [<c017d76c>] sys_select+0x9a/0x161
 [<c022fe94>] trace_hardirqs_on_thunk+0xc/0x10
 [<c0113eb4>] do_page_fault+0x0/0x578
 [<c0139e2f>] trace_hardirqs_on_caller+0xde/0x112
 [<c0102f2e>] syscall_call+0x7/0xb
 =======================
kthreadd      S c05ab00c     0     2      0
       c05281a0 00000046 00000001 c05ab00c c05ae200 c05ae200 c7820890 c78209e4
       c11a9200 00000000 00000002 00000000 00000000 c012ea49 00000246 00000246
       c052cc4c 000003d9 c7829d14 000003d9 c052cc44 00000000 c012e98d ffffffff
Call Trace:
 [<c012ea49>] kthreadd+0x128/0x149
 [<c012e98d>] kthreadd+0x6c/0x149
 [<c012e921>] kthreadd+0x0/0x149
 [<c0103b6b>] kernel_thread_helper+0x7/0x10
 =======================
migration/0   S c05ab00c     0     3      2
       c05281a0 00000046 00000001 c05ab00c c05ae200 c05ae200 c7821120 c7821274
       c11a9200 00000000 00000002 00000001 00000000 c011bc8c c7821120 c03ea9b3
       00000000 c0139e2f c11a9648 c11a9200 00000000 00000000 c011bd93 c05ae200
Call Trace:
 [<c011bc8c>] migration_thread+0x43/0x1ce
 [<c03ea9b3>] _spin_unlock_irq+0x20/0x23
 [<c0139e2f>] trace_hardirqs_on_caller+0xde/0x112
 [<c011bd93>] migration_thread+0x14a/0x1ce
 [<c011bc49>] migration_thread+0x0/0x1ce
 [<c012eb4d>] kthread+0x38/0x5f
 [<c012eb15>] kthread+0x0/0x5f
 [<c0103b6b>] kernel_thread_helper+0x7/0x10
 =======================
ksoftirqd/0   S c05ab00c     0     4      2
       c7aee900 00000046 c11a600c c05ab00c c05ae200 c05ae200 c78219b0 c7821b04
       c11a9200 00000000 00000001 00000046 c012304a c05ae080 c78219b0 c01230e5
       00000000 c0139e2f c05ae080 c05ae080 00000000 c05ae080 c0123267 00000000
Call Trace:
 [<c012304a>] __do_softirq+0x9d/0xef
 [<c01230e5>] do_softirq+0x49/0x4d
 [<c0139e2f>] trace_hardirqs_on_caller+0xde/0x112
 [<c0123267>] ksoftirqd+0x35/0xb7
 [<c0123232>] ksoftirqd+0x0/0xb7
 [<c012eb4d>] kthread+0x38/0x5f
 [<c012eb15>] kthread+0x0/0x5f
 [<c0103b6b>] kernel_thread_helper+0x7/0x10
 =======================
watchdog/0    R running      0     5      2
       c05281a0 00000046 3f505b51 c05ab00c c05ae200 c05ae200 c7822240 c7822394
       c11a9200 00000000 c0139e2f 00000000 00000246 00000246 c05ab000 00bfb000
       00000000 00000000 00000000 c014eca1 00000000 00000000 c014ecdb 00000000
Call Trace:
 [<c0139e2f>] trace_hardirqs_on_caller+0xde/0x112
 [<c014eca1>] watchdog+0x0/0x1bb
 [<c014ecdb>] watchdog+0x3a/0x1bb
 [<c014eca1>] watchdog+0x0/0x1bb
 [<c012eb4d>] kthread+0x38/0x5f
 [<c012eb15>] kthread+0x0/0x5f
 [<c0103b6b>] kernel_thread_helper+0x7/0x10
 =======================
events/0      R running      0     6      2
       c05281a0 00000046 00000000 c05ab00c c05ae200 c05ae200 c7822ad0 c7822c24
       c11a9200 00000000 00000000 c03eaa02 00000000 ffffffff c03eaaa0 00000000
       00000000 00000000 c7814880 c012c7a9 c78148a4 00000000 c012c830 00000000
Call Trace:
 [<c03eaa02>] _spin_lock_irqsave+0x29/0x31
 [<c03eaaa0>] _spin_unlock_irqrestore+0x34/0x39
 [<c012c7a9>] worker_thread+0x0/0xbf
 [<c012c830>] worker_thread+0x87/0xbf
 [<c012ec00>] autoremove_wake_function+0x0/0x2b
 [<c012c7a9>] worker_thread+0x0/0xbf
 [<c012eb4d>] kthread+0x38/0x5f
 [<c012eb15>] kthread+0x0/0x5f
 [<c0103b6b>] kernel_thread_helper+0x7/0x10
 =======================
khelper       S c05ab00c     0     7      2
       c05281a0 00000046 00000000 c05ab00c c05ae200 c05ae200 c7823360 c78234b4
       c11a9200 00000000 00000000 c03eaa02 00000000 c7823360 c03eaaa0 c7814924
       c0139e2f c7814924 c7814900 c012c7a9 c7814924 00000000 c012c830 00000000
Call Trace:
 [<c03eaa02>] _spin_lock_irqsave+0x29/0x31
 [<c03eaaa0>] _spin_unlock_irqrestore+0x34/0x39
 [<c0139e2f>] trace_hardirqs_on_caller+0xde/0x112
 [<c012c7a9>] worker_thread+0x0/0xbf
 [<c012c830>] worker_thread+0x87/0xbf
 [<c012ec00>] autoremove_wake_function+0x0/0x2b
 [<c012c7a9>] worker_thread+0x0/0xbf
 [<c012eb4d>] kthread+0x38/0x5f
 [<c012eb15>] kthread+0x0/0x5f
 [<c0103b6b>] kernel_thread_helper+0x7/0x10
 =======================
kblockd/0     S c05ab00c     0    80      2
       c7aee900 00000046 00000000 c05ab00c c05ae200 c05ae200 c7904d10 c7904e64
       c11a9200 00000000 00000000 c03eaa02 00000000 c7904d10 c03eaaa0 c783dd24
       c0139e2f c783dd24 c783dd00 c012c7a9 c783dd24 00000000 c012c830 00000000
Call Trace:
 [<c03eaa02>] _spin_lock_irqsave+0x29/0x31
 [<c03eaaa0>] _spin_unlock_irqrestore+0x34/0x39
 [<c0139e2f>] trace_hardirqs_on_caller+0xde/0x112
 [<c012c7a9>] worker_thread+0x0/0xbf
 [<c012c830>] worker_thread+0x87/0xbf
 [<c012ec00>] autoremove_wake_function+0x0/0x2b
 [<c012c7a9>] worker_thread+0x0/0xbf
 [<c012eb4d>] kthread+0x38/0x5f
 [<c012eb15>] kthread+0x0/0x5f
 [<c0103b6b>] kernel_thread_helper+0x7/0x10
 =======================
kacpid        S c05ab00c     0    81      2
       c05281a0 00000046 00000000 c05ab00c c05ae200 c05ae200 c79055a0 c79056f4
       c11a9200 00000000 00000000 c03eaa02 00000000 c79055a0 c03eaaa0 c783dda4
       c0139e2f c783dda4 c783dd80 c012c7a9 c783dda4 00000000 c012c830 00000000
Call Trace:
 [<c03eaa02>] _spin_lock_irqsave+0x29/0x31
 [<c03eaaa0>] _spin_unlock_irqrestore+0x34/0x39
 [<c0139e2f>] trace_hardirqs_on_caller+0xde/0x112
 [<c012c7a9>] worker_thread+0x0/0xbf
 [<c012c830>] worker_thread+0x87/0xbf
 [<c012ec00>] autoremove_wake_function+0x0/0x2b
 [<c012c7a9>] worker_thread+0x0/0xbf
 [<c012eb4d>] kthread+0x38/0x5f
 [<c012eb15>] kthread+0x0/0x5f
 [<c0103b6b>] kernel_thread_helper+0x7/0x10
 =======================
kacpi_notify  S c05ab00c     0    82      2
       c05281a0 00000046 00000000 c05ab00c c05ae200 c05ae200 c7905e30 c7905f84
       c11a9200 00000000 00000000 c03eaa02 00000000 c7905e30 c03eaaa0 c783de24
       c0139e2f c783de24 c783de00 c012c7a9 c783de24 00000000 c012c830 00000000
Call Trace:
 [<c03eaa02>] _spin_lock_irqsave+0x29/0x31
 [<c03eaaa0>] _spin_unlock_irqrestore+0x34/0x39
 [<c0139e2f>] trace_hardirqs_on_caller+0xde/0x112
 [<c012c7a9>] worker_thread+0x0/0xbf
 [<c012c830>] worker_thread+0x87/0xbf
 [<c012ec00>] autoremove_wake_function+0x0/0x2b
 [<c012c7a9>] worker_thread+0x0/0xbf
 [<c012eb4d>] kthread+0x38/0x5f
 [<c012eb15>] kthread+0x0/0x5f
 [<c0103b6b>] kernel_thread_helper+0x7/0x10
 =======================
ata/0         S c05ab00c     0   122      2
       c05281a0 00000046 00000000 c05ab00c c05ae200 c05ae200 c78266c0 c7826814
       c11a9200 00000000 00000000 c03eaa02 00000000 c78266c0 c03eaaa0 c78430a4
       c0139e2f c78430a4 c7843080 c012c7a9 c78430a4 00000000 c012c830 00000000
Call Trace:
 [<c03eaa02>] _spin_lock_irqsave+0x29/0x31
 [<c03eaaa0>] _spin_unlock_irqrestore+0x34/0x39
 [<c0139e2f>] trace_hardirqs_on_caller+0xde/0x112
 [<c012c7a9>] worker_thread+0x0/0xbf
 [<c012c830>] worker_thread+0x87/0xbf
 [<c012ec00>] autoremove_wake_function+0x0/0x2b
 [<c012c7a9>] worker_thread+0x0/0xbf
 [<c012eb4d>] kthread+0x38/0x5f
 [<c012eb15>] kthread+0x0/0x5f
 [<c0103b6b>] kernel_thread_helper+0x7/0x10
 =======================
ata_aux       S c05ab00c     0   123      2
       c05281a0 00000046 00000000 c05ab00c c05ae200 c05ae200 c7824480 c78245d4
       c11a9200 00000000 00000000 c03eaa02 00000000 c7824480 c03eaaa0 c7843124
       c0139e2f c7843124 c7843100 c012c7a9 c7843124 00000000 c012c830 00000000
Call Trace:
 [<c03eaa02>] _spin_lock_irqsave+0x29/0x31
 [<c03eaaa0>] _spin_unlock_irqrestore+0x34/0x39
 [<c0139e2f>] trace_hardirqs_on_caller+0xde/0x112
 [<c012c7a9>] worker_thread+0x0/0xbf
 [<c012c830>] worker_thread+0x87/0xbf
 [<c012ec00>] autoremove_wake_function+0x0/0x2b
 [<c012c7a9>] worker_thread+0x0/0xbf
 [<c012eb4d>] kthread+0x38/0x5f
 [<c012eb15>] kthread+0x0/0x5f
 [<c0103b6b>] kernel_thread_helper+0x7/0x10
 =======================
ksuspend_usbd S c05ab00c     0   124      2
       c05281a0 00000046 00000000 c05ab00c c05ae200 c05ae200 c7823bf0 c7823d44
       c11a9200 00000000 00000000 c03eaa02 00000000 c7823bf0 c03eaaa0 c78431a4
       c0139e2f c78431a4 c7843180 c012c7a9 c78431a4 00000000 c012c830 00000000
Call Trace:
 [<c03eaa02>] _spin_lock_irqsave+0x29/0x31
 [<c03eaaa0>] _spin_unlock_irqrestore+0x34/0x39
 [<c0139e2f>] trace_hardirqs_on_caller+0xde/0x112
 [<c012c7a9>] worker_thread+0x0/0xbf
 [<c012c830>] worker_thread+0x87/0xbf
 [<c012ec00>] autoremove_wake_function+0x0/0x2b
 [<c012c7a9>] worker_thread+0x0/0xbf
 [<c012eb4d>] kthread+0x38/0x5f
 [<c012eb15>] kthread+0x0/0x5f
 [<c0103b6b>] kernel_thread_helper+0x7/0x10
 =======================
khubd         S c05ab00c     0   129      2
       c05281a0 00000046 00000000 c05ab00c c05ae200 c05ae200 c78819b0 c7881b04
       c11a9200 00000000 00000000 c03eaa02 00000000 c78819b0 c03eaaa0 c788ffbc
       c0139e2f c0544be8 c0544be0 c0313c5d c788ffbc 00000000 c03146be c788ffa8
Call Trace:
 [<c03eaa02>] _spin_lock_irqsave+0x29/0x31
 [<c03eaaa0>] _spin_unlock_irqrestore+0x34/0x39
 [<c0139e2f>] trace_hardirqs_on_caller+0xde/0x112
 [<c0313c5d>] hub_thread+0x0/0xaeb
 [<c03146be>] hub_thread+0xa61/0xaeb
 [<c03ea9b3>] _spin_unlock_irq+0x20/0x23
 [<c011ae44>] finish_task_switch+0x41/0x8e
 [<c011ae03>] finish_task_switch+0x0/0x8e
 [<c03e8c4b>] schedule+0x5b1/0x603
 [<c012ec00>] autoremove_wake_function+0x0/0x2b
 [<c0139e2f>] trace_hardirqs_on_caller+0xde/0x112
 [<c0313c5d>] hub_thread+0x0/0xaeb
 [<c012eb4d>] kthread+0x38/0x5f
 [<c012eb15>] kthread+0x0/0x5f
 [<c0103b6b>] kernel_thread_helper+0x7/0x10
 =======================
kseriod       S c05ab00c     0   132      2
       c05281a0 00000046 00000246 c05ab00c c05ae200 c05ae200 c7883360 c78834b4
       c11a9200 00000000 00000246 c03eaa02 00000000 ffffffff c03eaaa0 00000000
       00000000 00000000 c790f7e0 c790f7e0 c786bfc8 00000246 c032fa02 c786bfb4
Call Trace:
 [<c03eaa02>] _spin_lock_irqsave+0x29/0x31
 [<c03eaaa0>] _spin_unlock_irqrestore+0x34/0x39
 [<c032fa02>] serio_thread+0x27b/0x2e1
 [<c012ec00>] autoremove_wake_function+0x0/0x2b
 [<c032f787>] serio_thread+0x0/0x2e1
 [<c012eb4d>] kthread+0x38/0x5f
 [<c012eb15>] kthread+0x0/0x5f
 [<c0103b6b>] kernel_thread_helper+0x7/0x10
 =======================
pdflush       S c05ab00c     0   170      2
       c05281a0 00000046 00000001 c05ab00c c05ae200 c05ae200 c7880890 c78809e4
       c11a9200 00000000 00000002 00000001 00000000 c01593c5 c7880890 c03ea9b3
       00000000 c0139e2f c7941fc4 c0159355 00000000 00000000 c0159418 c7880890
Call Trace:
 [<c01593c5>] pdflush+0x70/0x1b5
 [<c03ea9b3>] _spin_unlock_irq+0x20/0x23
 [<c0139e2f>] trace_hardirqs_on_caller+0xde/0x112
 [<c0159355>] pdflush+0x0/0x1b5
 [<c0159418>] pdflush+0xc3/0x1b5
 [<c0139e2f>] trace_hardirqs_on_caller+0xde/0x112
 [<c012eb4d>] kthread+0x38/0x5f
 [<c012eb15>] kthread+0x0/0x5f
 [<c0103b6b>] kernel_thread_helper+0x7/0x10
 =======================
pdflush       R running      0   171      2
       c05281a0 00000046 00000001 c05ab00c c05ae200 c05ae200 c7881120 c7881274
       c11a9200 00000000 00000002 00000001 00000000 c01593c5 c7881120 c03ea9b3
       00000000 c0139e2f c7943fc4 c0159355 00000000 00000000 c0159418 c7881120
Call Trace:
 [<c01593c5>] pdflush+0x70/0x1b5
 [<c03ea9b3>] _spin_unlock_irq+0x20/0x23
 [<c0139e2f>] trace_hardirqs_on_caller+0xde/0x112
 [<c0159355>] pdflush+0x0/0x1b5
 [<c0159418>] pdflush+0xc3/0x1b5
 [<c0158f97>] wb_kupdate+0x0/0xde
 [<c012eb4d>] kthread+0x38/0x5f
 [<c012eb15>] kthread+0x0/0x5f
 [<c0103b6b>] kernel_thread_helper+0x7/0x10
 =======================
kswapd0       S c05ab00c     0   172      2
       c05281a0 00000046 c0550900 c05ab00c c05ae200 c05ae200 c7882240 c7882394
       c11a9200 00000000 c0550900 c03eaa02 00000000 c7882240 c03eaaa0 c7945fb4
       c0139e2f c0555948 c7882240 c015c11a c7945fb4 c0550900 c015c1e4 c05ab14c
Call Trace:
 [<c03eaa02>] _spin_lock_irqsave+0x29/0x31
 [<c03eaaa0>] _spin_unlock_irqrestore+0x34/0x39
 [<c0139e2f>] trace_hardirqs_on_caller+0xde/0x112
 [<c015c11a>] kswapd+0x0/0x3ee
 [<c015c1e4>] kswapd+0xca/0x3ee
 [<c011ae44>] finish_task_switch+0x41/0x8e
 [<c011ae03>] finish_task_switch+0x0/0x8e
 [<c03e8c4b>] schedule+0x5b1/0x603
 [<c012ec00>] autoremove_wake_function+0x0/0x2b
 [<c03eaaa0>] _spin_unlock_irqrestore+0x34/0x39
 [<c0139e2f>] trace_hardirqs_on_caller+0xde/0x112
 [<c015c11a>] kswapd+0x0/0x3ee
 [<c012eb4d>] kthread+0x38/0x5f
 [<c012eb15>] kthread+0x0/0x5f
 [<c0103b6b>] kernel_thread_helper+0x7/0x10
 =======================
aio/0         S c05ab00c     0   219      2
       c05281a0 00000046 00000000 c05ab00c c05ae200 c05ae200 c798aad0 c798ac24
       c11a9200 00000000 00000000 c03eaa02 00000000 c798aad0 c03eaaa0 c7867ca4
       c0139e2f c7867ca4 c7867c80 c012c7a9 c7867ca4 00000000 c012c830 00000000
Call Trace:
 [<c03eaa02>] _spin_lock_irqsave+0x29/0x31
 [<c03eaaa0>] _spin_unlock_irqrestore+0x34/0x39
 [<c0139e2f>] trace_hardirqs_on_caller+0xde/0x112
 [<c012c7a9>] worker_thread+0x0/0xbf
 [<c012c830>] worker_thread+0x87/0xbf
 [<c012ec00>] autoremove_wake_function+0x0/0x2b
 [<c012c7a9>] worker_thread+0x0/0xbf
 [<c012eb4d>] kthread+0x38/0x5f
 [<c012eb15>] kthread+0x0/0x5f
 [<c0103b6b>] kernel_thread_helper+0x7/0x10
 =======================
nfsiod        S c05ab00c     0   231      2
       c05281a0 00000046 00000000 c05ab00c c05ae200 c05ae200 c7824d10 c7824e64
       c11a9200 00000000 00000000 c03eaa02 00000000 c7824d10 c03eaaa0 c7867ea4
       c0139e2f c7867ea4 c7867e80 c012c7a9 c7867ea4 00000000 c012c830 00000000
Call Trace:
 [<c03eaa02>] _spin_lock_irqsave+0x29/0x31
 [<c03eaaa0>] _spin_unlock_irqrestore+0x34/0x39
 [<c0139e2f>] trace_hardirqs_on_caller+0xde/0x112
 [<c012c7a9>] worker_thread+0x0/0xbf
 [<c012c830>] worker_thread+0x87/0xbf
 [<c012ec00>] autoremove_wake_function+0x0/0x2b
 [<c012c7a9>] worker_thread+0x0/0xbf
 [<c012eb4d>] kthread+0x38/0x5f
 [<c012eb15>] kthread+0x0/0x5f
 [<c0103b6b>] kernel_thread_helper+0x7/0x10
 =======================
khpsbpkt      S c05ab00c     0   936      2
       c05281a0 00000046 00000001 c05ab00c c05ae200 c05ae200 c798bbf0 c798bd44
       c11a9200 00000000 00000002 00000001 00000000 c0300343 c798bbf0 c03ea9b3
       c7ba1fc4 c0139e2f c0543c18 c7ba1fa8 c7ba1fc4 00000000 c0300360 c7ba1fc4
Call Trace:
 [<c0300343>] hpsbpkt_thread+0xa5/0xe3
 [<c03ea9b3>] _spin_unlock_irq+0x20/0x23
 [<c0139e2f>] trace_hardirqs_on_caller+0xde/0x112
 [<c0300360>] hpsbpkt_thread+0xc2/0xe3
 [<c030029e>] hpsbpkt_thread+0x0/0xe3
 [<c012eb4d>] kthread+0x38/0x5f
 [<c012eb15>] kthread+0x0/0x5f
 [<c0103b6b>] kernel_thread_helper+0x7/0x10
 =======================
kpsmoused     S c05ab00c     0   963      2
       c05281a0 00000046 00000000 c05ab00c c05ae200 c05ae200 c7b4a240 c7b4a394
       c11a9200 00000000 00000000 c03eaa02 00000000 c7b4a240 c03eaaa0 c797a024
       c0139e2f c797a024 c797a000 c012c7a9 c797a024 00000000 c012c830 00000000
Call Trace:
 [<c03eaa02>] _spin_lock_irqsave+0x29/0x31
 [<c03eaaa0>] _spin_unlock_irqrestore+0x34/0x39
 [<c0139e2f>] trace_hardirqs_on_caller+0xde/0x112
 [<c012c7a9>] worker_thread+0x0/0xbf
 [<c012c830>] worker_thread+0x87/0xbf
 [<c012ec00>] autoremove_wake_function+0x0/0x2b
 [<c012c7a9>] worker_thread+0x0/0xbf
 [<c012eb4d>] kthread+0x38/0x5f
 [<c012eb15>] kthread+0x0/0x5f
 [<c0103b6b>] kernel_thread_helper+0x7/0x10
 =======================
kstriped      S c05ab00c     0   965      2
       c05281a0 00000046 00000000 c05ab00c c05ae200 c05ae200 c7b4bbf0 c7b4bd44
       c11a9200 00000000 00000000 c03eaa02 00000000 c7b4bbf0 c03eaaa0 c797a0a4
       c0139e2f c797a0a4 c797a080 c012c7a9 c797a0a4 00000000 c012c830 00000000
Call Trace:
 [<c03eaa02>] _spin_lock_irqsave+0x29/0x31
 [<c03eaaa0>] _spin_unlock_irqrestore+0x34/0x39
 [<c0139e2f>] trace_hardirqs_on_caller+0xde/0x112
 [<c012c7a9>] worker_thread+0x0/0xbf
 [<c012c830>] worker_thread+0x87/0xbf
 [<c012ec00>] autoremove_wake_function+0x0/0x2b
 [<c012c7a9>] worker_thread+0x0/0xbf
 [<c012eb4d>] kthread+0x38/0x5f
 [<c012eb15>] kthread+0x0/0x5f
 [<c0103b6b>] kernel_thread_helper+0x7/0x10
 =======================
kondemand/0   S c05ab00c     0   969      2
       c05281a0 00000046 00000000 c05ab00c c05ae200 c05ae200 c7b4c480 c7b4c5d4
       c11a9200 00000000 00000000 c03eaa02 00000000 c7b4c480 c03eaaa0 c797a124
       c0139e2f c797a124 c797a100 c012c7a9 c797a124 00000000 c012c830 00000000
Call Trace:
 [<c03eaa02>] _spin_lock_irqsave+0x29/0x31
 [<c03eaaa0>] _spin_unlock_irqrestore+0x34/0x39
 [<c0139e2f>] trace_hardirqs_on_caller+0xde/0x112
 [<c012c7a9>] worker_thread+0x0/0xbf
 [<c012c830>] worker_thread+0x87/0xbf
 [<c012ec00>] autoremove_wake_function+0x0/0x2b
 [<c012c7a9>] worker_thread+0x0/0xbf
 [<c012eb4d>] kthread+0x38/0x5f
 [<c012eb15>] kthread+0x0/0x5f
 [<c0103b6b>] kernel_thread_helper+0x7/0x10
 =======================
rpciod/0      S c05ab00c     0   977      2
       c05281a0 00000046 00000000 c05ab00c c05ae200 c05ae200 c7b4ef50 c7b4f0a4
       c11a9200 00000000 00000000 c03eaa02 00000000 c7b4ef50 c03eaaa0 c7913624
       c0139e2f c7913624 c7913600 c012c7a9 c7913624 00000000 c012c830 00000000
Call Trace:
 [<c03eaa02>] _spin_lock_irqsave+0x29/0x31
 [<c03eaaa0>] _spin_unlock_irqrestore+0x34/0x39
 [<c0139e2f>] trace_hardirqs_on_caller+0xde/0x112
 [<c012c7a9>] worker_thread+0x0/0xbf
 [<c012c830>] worker_thread+0x87/0xbf
 [<c012ec00>] autoremove_wake_function+0x0/0x2b
 [<c012c7a9>] worker_thread+0x0/0xbf
 [<c012eb4d>] kthread+0x38/0x5f
 [<c012eb15>] kthread+0x0/0x5f
 [<c0103b6b>] kernel_thread_helper+0x7/0x10
 =======================
kjournald     S c05ab00c     0   985      2
       c05281a0 00000046 c7861414 c05ab00c c05ae200 c05ae200 c7b48000 c7b48154
       c11a9200 00000000 00000000 c7861414 c0139cc2 c7b48000 00000246 00000246
       c7861414 00000000 c7861400 00000000 c7861510 c7861414 c01dbe30 c788bfb4
Call Trace:
 [<c0139cc2>] mark_held_locks+0x42/0x58
 [<c01dbe30>] kjournald+0x176/0x1e8
 [<c012ec00>] autoremove_wake_function+0x0/0x2b
 [<c01dbcba>] kjournald+0x0/0x1e8
 [<c012eb4d>] kthread+0x38/0x5f
 [<c012eb15>] kthread+0x0/0x5f
 [<c0103b6b>] kernel_thread_helper+0x7/0x10
 =======================
sh            S c05ab00c     0   986      1
       c7aee480 00000046 00000000 c05ab00c c05ae200 c05ae200 c7b4cd10 c7b4ce64
       c11a9200 00000000 00000002 c7b4e6c0 00000000 c7b4cd10 00000246 00000246
       c055b980 00000000 c7b4cd08 00000000 c7b4cd10 00000004 c01215f6 00000014
Call Trace:
 [<c01215f6>] do_wait+0x1d0/0x2c7
 [<c0119ee4>] default_wake_function+0x0/0x8
 [<c0121769>] sys_wait4+0x7c/0x8f
 [<c012178f>] sys_waitpid+0x13/0x17
 [<c0102f2e>] syscall_call+0x7/0xb
 =======================
rc.sysinit    S c05ab00c     0   988    986
       c7aee240 00000046 00000000 c05ab00c c05ae200 c05ae200 c7b4e6c0 c7b4e814
       c11a9200 00000000 00000002 c7b4d5a0 00000000 ffffffff 00000246 00000000
       00000000 00000000 c7b4e6b8 00000000 c7b4e6c0 0000000e c01215f6 00000014
Call Trace:
 [<c01215f6>] do_wait+0x1d0/0x2c7
 [<c0119ee4>] default_wake_function+0x0/0x8
 [<c0121769>] sys_wait4+0x7c/0x8f
 [<c012178f>] sys_waitpid+0x13/0x17
 [<c0102f2e>] syscall_call+0x7/0xb
 =======================
start_udev    S c05ab00c     0  1008    988
       c7aeed80 00000046 00000000 c05ab00c c05ae200 c05ae200 c7b4d5a0 c7b4d6f4
       c11a9200 00000000 00000002 c7880000 00000000 c7b4d5a0 00000246 00000246
       c055b980 00000000 c7b4d598 00000000 c7b4d5a0 00000004 c01215f6 00000014
Call Trace:
 [<c01215f6>] do_wait+0x1d0/0x2c7
 [<c0119ee4>] default_wake_function+0x0/0x8
 [<c0121769>] sys_wait4+0x7c/0x8f
 [<c012178f>] sys_waitpid+0x13/0x17
 [<c0102f2e>] syscall_call+0x7/0xb
 =======================
udevd         R running      0  1040      1
       c7aee6c0 00000046 00000000 c05ab00c c05ae200 c05ae200 c78866c0 c7886818
       c11a9200 00000000 c7aee6c4 00000246 c78866c0 c022fe94 01200011 00000000
       b7cf4968 00000046 01200011 00000000 b7cf4968 c7b38000 c0102fbe 01200011
Call Trace:
 [<c022fe94>] trace_hardirqs_on_thunk+0xc/0x10
 [<c0102fbe>] work_resched+0x5/0x2a
 =======================
udevtrigger   R running      0  1043   1008
       bfa5b398 c01755cb 00000246 c79ee0e8 c79ee080 bfa5a5e8 c0184d7b c7bce000
       0000001f 00000200 bfa5a5e8 c7bce000 c7880000 c022fe94 bfa5a5e8 c0139e2f
       bfa5a7e8 bfa5a7e8 c022fe94 c79ee080 00000000 bfa5ac88 c0102f2e bfa5ac88
Call Trace:
 [<c01755cb>] sys_stat64+0xf/0x24
 [<c0184d7b>] mntput_no_expire+0x18/0xdb
 [<c022fe94>] trace_hardirqs_on_thunk+0xc/0x10
 [<c0139e2f>] trace_hardirqs_on_caller+0xde/0x112
 [<c022fe94>] trace_hardirqs_on_thunk+0xc/0x10
 [<c0102f2e>] syscall_call+0x7/0xb
 =======================
udevd         R running      0  1052   1040
       c7aeeb40 00000046 00000286 c05ab00c c05ae200 c05ae200 c7882ad0 c7882c28
       c11a9200 00000000 c055b980 bfcffa70 00000000 00000000 c03ea6d0 00000000
       c012ad66 00000046 00000000 bfcffa70 bfcffa70 c78b4000 c0102fbe 00000000
Call Trace:
 [<c03ea6d0>] _read_unlock+0x14/0x1c
 [<c012ad66>] sys_setpriority+0x19c/0x1a4
 [<c0102fbe>] work_resched+0x5/0x2a
 =======================
BUG: soft lockup - CPU#0 stuck for 61s! [udevtrigger:1043]
Modules linked in:
irq event stamp: 176180
hardirqs last  enabled at (176179): [<c022fe94>] trace_hardirqs_on_thunk+0xc/0x10
hardirqs last disabled at (176180): [<c022fea4>] trace_hardirqs_off_thunk+0xc/0x10
softirqs last  enabled at (176178): [<c01230d3>] do_softirq+0x37/0x4d
softirqs last disabled at (176173): [<c01230d3>] do_softirq+0x37/0x4d

Pid: 1043, comm: udevtrigger Not tainted (2.6.27-rc7-00093-g32a1662-dirty #243)
EIP: 0060:[<c0180c19>] EFLAGS: 00000207 CPU: 0
EIP is at __d_lookup+0xd9/0x116
EAX: d7c6bb9d EBX: c7404da8 ECX: c7880390 EDX: 00000001
ESI: c11cfeb4 EDI: c7404dd0 EBP: c75c5d10 ESP: c7bcfe08
 DS: 007b ES: 007b FS: 00d8 GS: 0033 SS: 0068
CR0: 8005003b CR2: b7dcfa50 CR3: 07b3a000 CR4: 000006d0
DR0: 00000000 DR1: 00000000 DR2: 00000000 DR3: 00000000
DR6: ffff0ff0 DR7: 00000400
 [<c01788f4>] ? do_lookup+0x28/0x158
 [<c017a1ce>] ? __link_path_walk+0x702/0xac9
 [<c017a5cc>] ? path_walk+0x37/0x70
 [<c017a76b>] ? do_path_lookup+0xe3/0x144
 [<c017af4a>] ? user_path_at+0x37/0x5f
 [<c01754f0>] ? vfs_stat_fd+0x15/0x3b
 [<c01755cb>] ? sys_stat64+0xf/0x24
 [<c0184d7b>] ? mntput_no_expire+0x18/0xdb
 [<c022fe94>] ? trace_hardirqs_on_thunk+0xc/0x10
 [<c0139e2f>] ? trace_hardirqs_on_caller+0xde/0x112
 [<c022fe94>] ? trace_hardirqs_on_thunk+0xc/0x10
 [<c0102f2e>] ? syscall_call+0x7/0xb
 =======================
BUG: soft lockup - CPU#0 stuck for 61s! [udevtrigger:1043]
Modules linked in:
irq event stamp: 176180
hardirqs last  enabled at (176179): [<c022fe94>] trace_hardirqs_on_thunk+0xc/0x10
hardirqs last disabled at (176180): [<c022fea4>] trace_hardirqs_off_thunk+0xc/0x10
softirqs last  enabled at (176178): [<c01230d3>] do_softirq+0x37/0x4d
softirqs last disabled at (176173): [<c01230d3>] do_softirq+0x37/0x4d

Pid: 1043, comm: udevtrigger Not tainted (2.6.27-rc7-00093-g32a1662-dirty #243)
EIP: 0060:[<c0180c24>] EFLAGS: 00000282 CPU: 0
EIP is at __d_lookup+0xe4/0x116
EAX: c7404dd0 EBX: c7404da8 ECX: c7880390 EDX: 00000001
ESI: c11cfeb4 EDI: c7404dd0 EBP: c75c5d10 ESP: c7bcfe08
 DS: 007b ES: 007b FS: 00d8 GS: 0033 SS: 0068
CR0: 8005003b CR2: b7dcfa50 CR3: 07b3a000 CR4: 000006d0
DR0: 00000000 DR1: 00000000 DR2: 00000000 DR3: 00000000
DR6: ffff0ff0 DR7: 00000400
 [<c01788f4>] ? do_lookup+0x28/0x158
 [<c017a1ce>] ? __link_path_walk+0x702/0xac9
 [<c017a5cc>] ? path_walk+0x37/0x70
 [<c017a76b>] ? do_path_lookup+0xe3/0x144
 [<c017af4a>] ? user_path_at+0x37/0x5f
 [<c01754f0>] ? vfs_stat_fd+0x15/0x3b
 [<c01755cb>] ? sys_stat64+0xf/0x24
 [<c0184d7b>] ? mntput_no_expire+0x18/0xdb
 [<c022fe94>] ? trace_hardirqs_on_thunk+0xc/0x10
 [<c0139e2f>] ? trace_hardirqs_on_caller+0xde/0x112
 [<c022fe94>] ? trace_hardirqs_on_thunk+0xc/0x10
 [<c0102f2e>] ? syscall_call+0x7/0xb
 =======================

Re: [PATCH 10/10] Document usage of multiple-instances of devpts

[Serge E. Hallyn]

Quoting sukadev-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org (sukadev-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org):
> Alan Cox [alan-qBU/x9rampVanCEyBjwyrvXRex20P6io@public.gmane.org] wrote:
> | Ok I'm happy with this patch set. It appears correct as far as the tty
> | side is concerned, it looks sensible in terms of interface with the
> | devpts layer.
> 
> Alan, Peter: Thanks for review and feedback.
> 
> | 
> | Really depends what everyone else thinks about the vfs bits and the API
> 
> Maybe I should post to fsdevel, manpages, and lkml for the vfs/api changes. 
> 
> Serge, Eric, Pavel: Any comments on the patchset before I do that ?

No luck yet finding my random lockup (it *should* be obvious), but do
notice that you ignore the return value of get_sb_single() in
init_pts_mount().  Certainly that should be fixed.

-serge

Re: [PATCH 10/10] Document usage of multiple-instances of devpts

[sukadev-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8]

Serge E. Hallyn [serue-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org] wrote:

| No luck yet finding my random lockup (it *should* be obvious), but do
| notice that you ignore the return value of get_sb_single() in
| init_pts_mount().  Certainly that should be fixed.

Good catch. Will fix it. Thanks,

Suka

Re: [PATCH 10/10] Document usage of multiple-instances of devpts

[Serge E. Hallyn]

Quoting sukadev-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org (sukadev-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org):
> 
> >From 6d2fe9386880157f871667077db3180e9f0083a1 Mon Sep 17 00:00:00 2001
> From: Sukadev Bhattiprolu <sukadev-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org>
> Date: Tue, 9 Sep 2008 10:43:50 -0700
> Subject: [PATCH 10/10] Document usage of multiple-instances of devpts
> 
> Signed-off-by: Sukadev Bhattiprolu <sukadev-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>
> ---
>  Documentation/filesystems/devpts.txt |  122 ++++++++++++++++++++++++++++++++++
>  1 files changed, 122 insertions(+), 0 deletions(-)
>  create mode 100644 Documentation/filesystems/devpts.txt
> 
> diff --git a/Documentation/filesystems/devpts.txt b/Documentation/filesystems/devpts.txt
> new file mode 100644
> index 0000000..93d9d01
> --- /dev/null
> +++ b/Documentation/filesystems/devpts.txt
> @@ -0,0 +1,122 @@
> +
> +To support containers, we now allow multiple instances of devpts filesystem,
> +such that indices of ptys allocated in one instance are independent of indices
> +allocated in other instances of devpts.
> +
> +To preserve backward compatibility, this support for multiple instances is
> +enabled only if:
> +
> +	- CONFIG_DEVPTS_MULTIPLE_INSTANCES=y, and
> +	- '-o newinstance' mount option is specified while mounting devpts
> +
> +IOW, devpts now supports both single-instance and multi-instance semantics.
> +
> +If CONFIG_DEVPTS_MULTIPLE_INSTANCES=n, there is no change in behavior and
> +this referred to as the "legacy" mode. In this mode, the new mount options
> +(-o newinstance and -o ptmxmode) will be ignored with a 'bogus option' message
> +on console.
> +
> +If CONFIG_DEVPTS_MULTIPLE_INSTANCES=y and devpts is mounted without the
> +'newinstance' option (as in current start-up scripts) the new mount binds
> +to the initial kernel mount of devpts. This mode is referred to as the
> +'single-instance' mode and the current, single-instance semantics are
> +preserved, i.e PTYs are common across the system.
> +
> +The only difference between this single-instance mode and the legacy mode
> +is the presence of new, '/dev/pts/ptmx' node with permissions 0000, which
> +can safely be ignored.
> +
> +If CONFIG_DEVPTS_MULTIPLE_INSTANCES=y and 'newinstance' option is specified,
> +the mount is considered to be in the multi-instance mode and a new instance
> +of the devpts fs is created. Any ptys created in this instance are independent
> +of ptys in other instances of devpts. Like in the single-instance mode, the
> +/dev/pts/ptmx node is present. To effectively use the multi-instance mode,
> +open of /dev/ptmx must be a redirected to '/dev/pts/ptmx' using a symlink or
> +bind-mount.
> +
> +Eg: A container startup script could do the following:
> +
> +	$ chmod 0666 /dev/pts/ptmx
> +	$ rm /dev/ptmx
> +	$ ln -s pts/ptmx /dev/ptmx
> +	$ ns_exec -cm /bin/bash
> +
> +	# We are now in new container
> +
> +	$ umount /dev/pts
> +	$ mount -t devpts -o newinstance lxcpts /dev/pts
> +	$ sshd -p 1234
> +
> +where 'ns_exec -cm /bin/bash' calls clone() with CLONE_NEWNS flag and execs
> +/bin/bash in the child process.  A pty created by the sshd is not visible in
> +the original mount of /dev/pts.
> +
> +User-space changes
> +------------------
> +
> +In multi-instance mode (i.e '-o newinstance' mount option is specified at least
> +once), following user-space issues should be noted.
> +
> +1. If -o newinstance mount option is never used, /dev/pts/ptmx can be ignored
> +   and no change is needed to system-startup scripts.
> +
> +2. To effectively use multi-instance mode (i.e -o newinstance is specified)
> +   administrators or startup scripts should "redirect" open of /dev/ptmx to
> +   /dev/pts/ptmx using either a bind mount or symlink.
> +
> +	$ mount -t devpts -o newinstance devpts /dev/pts
> +
> +   followed by either
> +
> +	$ rm /dev/ptmx
> +	$ ln -s pts/ptmx /dev/ptmx
> +	$ chmod 666 /dev/pts/ptmx
> +   or
> +	$ mount -o bind /dev/pts/ptmx /dev/ptmx
> +
> +3. The '/dev/ptmx -> pts/ptmx' symlink is the preferred method since it
> +   enables better error-reporting and treats both single-instance and
> +   multi-instance mounts similarly.
> +
> +   But this method requires that system-startup scripts set the mode of
> +   /dev/pts/ptmx correctly (default mode is 0000). The scripts can set the
> +   mode by, either
> +
> +   	- adding ptmxmode mount option to devpts entry in /etc/fstab, or
> +	- using 'chmod 0666 /dev/pts/ptmx'
> +
> +4. If multi-instance mode mount is needed for containers, but the system
> +   startup scripts have not yet been updated, container-startup scripts
> +   should bind mount /dev/ptmx to /dev/pts/ptmx to avoid breaking single-
> +   instance mounts.
> +
> +   Or, in general, container-startup scripts should use:
> +
> +	mount -t devpts -o newinstance -o ptmxmode 0666 devpts /dev/pts

ptmxmode=0666

That is, of course, crucial, this being the documentation :)

> +	if [ ! -L /dev/ptmx ]; then
> +		mount -o bind /dev/pts/ptmx /dev/ptmx
> +	fi
> +
> +   When all devpts mounts are multi-instance, /dev/ptmx can permanently be
> +   a symlink to pts/ptmx and the bind mount can be ignored.
> +
> +5. A multi-instance mount that is not accompanied by the /dev/ptmx to
> +   /dev/pts/ptmx redirection would result in an unusable/unreachable pty.
> +
> +	mount -t devpts -o newinstance lxcpts /dev/pts
> +
> +   immediately followed by:
> +
> +	open("/dev/ptmx")
> +
> +    would create a pty, say /dev/pts/7, in the initial kernel mount.
> +    But /dev/pts/7 would be invisible in the new mount.
> +
> +6. The permissions for /dev/pts/ptmx node should be specified when mounting
> +   /dev/pts, using the '-o ptmxmode=%o' mount option (default is 0000).
> +
> +	mount -t devpts -o newinstance -o ptmxmode=0644 devpts /dev/pts
> +
> +   The permissions can be later be changed as usual with 'chmod'.
> +
> +	chmod 666 /dev/pts/ptmx
> -- 
> 1.5.2.5

Re: [PATCH 10/10] Document usage of multiple-instances of devpts

[sukadev-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8]

Serge E. Hallyn [serue-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org] wrote:
| > +   Or, in general, container-startup scripts should use:
| > +
| > +	mount -t devpts -o newinstance -o ptmxmode 0666 devpts /dev/pts
| 
| ptmxmode=0666
| 
| That is, of course, crucial, this being the documentation :)

Agree :-)