[RFC][PATCH 0/6] Enable multiple mounts of devpts

[RFC][PATCH 0/6] Enable multiple mounts of devpts

[sukadev-r/Jw6+rmf7HQT0dZR+AlfA]

I thought I will send out the patches I mentioned to H. Peter Anvin
recently to get some feedback on the general direction. This version
of the patchset ducks the user-space issue, for now.

---

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

To enable multiple mounts (most) devpts interfaces need to know which
instance of devpts is being accessed. This patchset uses the 'struct
inode' of the device being accessed to identify the appropriate devpts
instance. It then uses get_sb_nodev() instead of get_sb_single() to
allow multiple mounts

	PATCH 1/6	Pass-in 'struct inode' to devpts interfaces
	PATCH 2/6	Remove 'devpts_root' global
	PATCH 3/6	Move 'allocated_ptys' to sb->s_s_fs_info
	PATCH 4/6	Allow mknod of ptmx and tty devices
	PATCH 5/6	Allow multiple mounts of devpts
	PATCH 6/6	Tweak in init_dev() /dev/tty

If devpts is mounted just once, this patchset should not change any behavior.

If devpts is mounted more than once, then '/dev/ptmx' must be a symlink
to '/dev/pts/ptmx' and in each new devpts mount we must create the
device node '/dev/pts/ptmx' [c, 5;2] by hand.

Have only done some basic testing with multiple mounts and sshd. May not
be bisect-safe.

Appreciate comments on overall approach of my mapping from the inode
to sb->s_fs_info to allocated_ptys and the hacky use of get_sb_nodev(),
and also on the tweak to init_dev() (patch 6).

Todo:
	User-space impact of /dev/ptmx symlink - Options are being
	discussed on mailing list (new mount option and config token,
	new fs name, etc)

	Remove even initial kernel mount of devpts ?

[RFC][PATCH 1/6] Pass-in 'struct inode' to devpts interfaces

[sukadev-r/Jw6+rmf7HQT0dZR+AlfA]

From: Sukadev Bhattiprolu <sukadev-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>
Subject: [RFC][PATCH 1/6] Pass-in 'struct inode' to devpts interfaces

Pass-in an 'inode' parameter to devpts interfaces.  The parameter
itself will be used in subsequent patches to identify the instance
of devpts mounted.

---
 drivers/char/pty.c        |    3 ++-
 drivers/char/tty_io.c     |   21 +++++++++++----------
 fs/devpts/inode.c         |   10 +++++-----
 include/linux/devpts_fs.h |   34 ++++++++++++++++++++++++----------
 4 files changed, 42 insertions(+), 26 deletions(-)

Index: linux-2.6.26-rc8-mm1/fs/devpts/inode.c
===================================================================
--- linux-2.6.26-rc8-mm1.orig/fs/devpts/inode.c	2008-08-04 02:07:25.000000000 -0700
+++ linux-2.6.26-rc8-mm1/fs/devpts/inode.c	2008-08-04 02:08:15.000000000 -0700
@@ -177,7 +177,7 @@ static struct dentry *get_node(int num)
 	return lookup_one_len(s, root, sprintf(s, "%d", num));
 }
 
-int devpts_new_index(void)
+int devpts_new_index(struct inode *inode)
 {
 	int index;
 	int idr_ret;
@@ -205,14 +205,14 @@ retry:
 	return index;
 }
 
-void devpts_kill_index(int idx)
+void devpts_kill_index(struct inode *inode, int idx)
 {
 	mutex_lock(&allocated_ptys_lock);
 	idr_remove(&allocated_ptys, idx);
 	mutex_unlock(&allocated_ptys_lock);
 }
 
-int devpts_pty_new(struct tty_struct *tty)
+int devpts_pty_new(struct inode *ptmx_inode, struct tty_struct *tty)
 {
 	int number = tty->index; /* tty layer puts index from devpts_new_index() in here */
 	struct tty_driver *driver = tty->driver;
@@ -245,7 +245,7 @@ int devpts_pty_new(struct tty_struct *tt
 	return 0;
 }
 
-struct tty_struct *devpts_get_tty(int number)
+struct tty_struct *devpts_get_tty(struct inode *inode, int number)
 {
 	struct dentry *dentry = get_node(number);
 	struct tty_struct *tty;
@@ -262,7 +262,7 @@ struct tty_struct *devpts_get_tty(int nu
 	return tty;
 }
 
-void devpts_pty_kill(int number)
+void devpts_pty_kill(struct inode *inode, int number)
 {
 	struct dentry *dentry = get_node(number);
 
Index: linux-2.6.26-rc8-mm1/include/linux/devpts_fs.h
===================================================================
--- linux-2.6.26-rc8-mm1.orig/include/linux/devpts_fs.h	2008-08-04 02:07:24.000000000 -0700
+++ linux-2.6.26-rc8-mm1/include/linux/devpts_fs.h	2008-08-04 02:07:27.000000000 -0700
@@ -17,20 +17,34 @@
 
 #ifdef CONFIG_UNIX98_PTYS
 
-int devpts_new_index(void);
-void devpts_kill_index(int idx);
-int devpts_pty_new(struct tty_struct *tty);      /* mknod in devpts */
-struct tty_struct *devpts_get_tty(int number);	 /* get tty structure */
-void devpts_pty_kill(int number);		 /* unlink */
+int devpts_new_index(struct inode *inode);
+void devpts_kill_index(struct inode *inode, int idx);
+
+/* mknod in devpts */
+int devpts_pty_new(struct inode *inode, struct tty_struct *tty);
+
+/* get tty structure */
+struct tty_struct *devpts_get_tty(struct inode *inode, int number);
+
+/* unlink */
+void devpts_pty_kill(struct inode *inode, int number);
 
 #else
 
 /* Dummy stubs in the no-pty case */
-static inline int devpts_new_index(void) { return -EINVAL; }
-static inline void devpts_kill_index(int idx) { }
-static inline int devpts_pty_new(struct tty_struct *tty) { return -EINVAL; }
-static inline struct tty_struct *devpts_get_tty(int number) { return NULL; }
-static inline void devpts_pty_kill(int number) { }
+static inline int devpts_new_index(struct inode *inode) { return -EINVAL; }
+static inline void devpts_kill_index(struct inode *inode, int idx) { }
+
+static inline int devpts_pty_new(struc inode *inode, struct tty_struct *tty)
+{
+	return -EINVAL;
+}
+
+static inline struct tty_struct *devpts_get_tty(struct inode *inode, int number)
+{
+	return NULL;
+}
+static inline void devpts_pty_kill(struc inode *inode, int number) { }
 
 #endif
 
Index: linux-2.6.26-rc8-mm1/drivers/char/pty.c
===================================================================
--- linux-2.6.26-rc8-mm1.orig/drivers/char/pty.c	2008-08-04 02:07:24.000000000 -0700
+++ linux-2.6.26-rc8-mm1/drivers/char/pty.c	2008-08-04 02:07:27.000000000 -0700
@@ -59,7 +59,8 @@ static void pty_close(struct tty_struct 
 		set_bit(TTY_OTHER_CLOSED, &tty->flags);
 #ifdef CONFIG_UNIX98_PTYS
 		if (tty->driver == ptm_driver)
-			devpts_pty_kill(tty->index);
+			devpts_pty_kill(filp->f_path.dentry->d_inode,
+					tty->index);
 #endif
 		tty_vhangup(tty->link);
 	}
Index: linux-2.6.26-rc8-mm1/drivers/char/tty_io.c
===================================================================
--- linux-2.6.26-rc8-mm1.orig/drivers/char/tty_io.c	2008-08-04 02:07:24.000000000 -0700
+++ linux-2.6.26-rc8-mm1/drivers/char/tty_io.c	2008-08-04 02:07:55.000000000 -0700
@@ -2056,7 +2056,7 @@ static void tty_line_name(struct tty_dri
  * relaxed for the (most common) case of reopening a tty.
  */
 
-static int init_dev(struct tty_driver *driver, int idx,
+static int init_dev(struct tty_driver *driver, struct inode *inode,  int idx,
 	struct tty_struct **ret_tty)
 {
 	struct tty_struct *tty, *o_tty;
@@ -2066,7 +2066,7 @@ static int init_dev(struct tty_driver *d
 
 	/* check whether we're reopening an existing tty */
 	if (driver->flags & TTY_DRIVER_DEVPTS_MEM) {
-		tty = devpts_get_tty(idx);
+		tty = devpts_get_tty(inode, idx);
 		/*
 		 * If we don't have a tty here on a slave open, it's because
 		 * the master already started the close process and there's
@@ -2370,10 +2370,11 @@ static void release_dev(struct file *fil
 	int	idx;
 	char	buf[64];
 	unsigned long flags;
+	struct inode *inode;
 
+	inode = filp->f_path.dentry->d_inode;
 	tty = (struct tty_struct *)filp->private_data;
-	if (tty_paranoia_check(tty, filp->f_path.dentry->d_inode,
-							"release_dev"))
+	if (tty_paranoia_check(tty, inode, "release_dev"))
 		return;
 
 	check_tty_count(tty, "release_dev");
@@ -2628,7 +2629,7 @@ static void release_dev(struct file *fil
 
 	/* Make this pty number available for reallocation */
 	if (devpts)
-		devpts_kill_index(idx);
+		devpts_kill_index(inode, idx);
 }
 
 /**
@@ -2709,7 +2710,7 @@ retry_open:
 		return -ENODEV;
 	}
 got_driver:
-	retval = init_dev(driver, index, &tty);
+	retval = init_dev(driver, inode, index, &tty);
 	mutex_unlock(&tty_mutex);
 	if (retval)
 		return retval;
@@ -2801,12 +2802,12 @@ static int __ptmx_open(struct inode *ino
 	nonseekable_open(inode, filp);
 
 	/* find a device that is not in use. */
-	index = devpts_new_index();
+	index = devpts_new_index(inode);
 	if (index < 0)
 		return index;
 
 	mutex_lock(&tty_mutex);
-	retval = init_dev(ptm_driver, index, &tty);
+	retval = init_dev(ptm_driver, inode, index, &tty);
 	mutex_unlock(&tty_mutex);
 
 	if (retval)
@@ -2816,7 +2817,7 @@ static int __ptmx_open(struct inode *ino
 	filp->private_data = tty;
 	file_move(filp, &tty->tty_files);
 
-	retval = devpts_pty_new(tty->link);
+	retval = devpts_pty_new(inode, tty->link);
 	if (retval)
 		goto out1;
 
@@ -2828,7 +2829,7 @@ out1:
 	release_dev(filp);
 	return retval;
 out:
-	devpts_kill_index(index);
+	devpts_kill_index(inode, index);
 	return retval;
 }

[RFC][PATCH 2/6] Remove 'devpts_root' global

[sukadev-r/Jw6+rmf7HQT0dZR+AlfA]

From: Sukadev Bhattiprolu <sukadev-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>
Subject: [RFC][PATCH 2/6] Remove 'devpts_root' global

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

---
 fs/devpts/inode.c |   36 ++++++++++++++++++++++++------------
 1 file changed, 24 insertions(+), 12 deletions(-)

Index: linux-2.6.26-rc8-mm1/fs/devpts/inode.c
===================================================================
--- linux-2.6.26-rc8-mm1.orig/fs/devpts/inode.c	2008-08-04 02:08:15.000000000 -0700
+++ linux-2.6.26-rc8-mm1/fs/devpts/inode.c	2008-08-04 02:08:43.000000000 -0700
@@ -33,7 +33,14 @@ static DEFINE_IDR(allocated_ptys);
 static DEFINE_MUTEX(allocated_ptys_lock);
 
 static struct vfsmount *devpts_mnt;
-static struct dentry *devpts_root;
+
+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 struct {
 	int setuid;
@@ -141,7 +148,7 @@ devpts_fill_super(struct super_block *s,
 	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;
 	
@@ -169,10 +176,9 @@ static struct file_system_type devpts_fs
  * to the System V naming convention
  */
 
-static struct dentry *get_node(int num)
+static struct dentry *get_node(struct dentry *root, int num)
 {
 	char s[12];
-	struct dentry *root = devpts_root;
 	mutex_lock(&root->d_inode->i_mutex);
 	return lookup_one_len(s, root, sprintf(s, "%d", num));
 }
@@ -218,7 +224,9 @@ int devpts_pty_new(struct inode *ptmx_in
 	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;
 
 	/* We're supposed to be given the slave end of a pty */
 	BUG_ON(driver->type != TTY_DRIVER_TYPE_PTY);
@@ -234,20 +242,22 @@ int devpts_pty_new(struct inode *ptmx_in
 	init_special_inode(inode, S_IFCHR|config.mode, device);
 	inode->i_private = tty;
 
-	dentry = get_node(number);
+	dentry = get_node(root, number);
 	if (!IS_ERR(dentry) && !dentry->d_inode) {
 		d_instantiate(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;
 }
 
 struct tty_struct *devpts_get_tty(struct inode *inode, int number)
 {
-	struct dentry *dentry = get_node(number);
+	struct super_block *sb = pts_sb_from_inode(inode);
+	struct dentry *root = sb->s_root;
+	struct dentry *dentry = get_node(root, number);
 	struct tty_struct *tty;
 
 	tty = NULL;
@@ -257,14 +267,16 @@ struct tty_struct *devpts_get_tty(struct
 		dput(dentry);
 	}
 
-	mutex_unlock(&devpts_root->d_inode->i_mutex);
+	mutex_unlock(&root->d_inode->i_mutex);
 
 	return tty;
 }
 
 void devpts_pty_kill(struct inode *inode, int number)
 {
-	struct dentry *dentry = get_node(number);
+	struct super_block *sb = pts_sb_from_inode(inode);
+	struct dentry *root = sb->s_root;
+	struct dentry *dentry = get_node(root, number);
 
 	if (!IS_ERR(dentry)) {
 		struct inode *inode = dentry->d_inode;
@@ -275,7 +287,7 @@ void devpts_pty_kill(struct inode *inode
 		}
 		dput(dentry);
 	}
-	mutex_unlock(&devpts_root->d_inode->i_mutex);
+	mutex_unlock(&root->d_inode->i_mutex);
 }
 
 static int __init init_devpts_fs(void)

[RFC][PATCH 3/6] Move 'allocated_ptys' to sb->s_s_fs_info

[sukadev-r/Jw6+rmf7HQT0dZR+AlfA]

From: Sukadev Bhattiprolu <sukadev-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>
Subject: [RFC][PATCH 3/6] Move 'allocated_ptys' to sb->s_s_fs_info

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

---
 fs/devpts/inode.c |   53 ++++++++++++++++++++++++++++++++++++++++++++++-------
 1 file changed, 46 insertions(+), 7 deletions(-)

Index: linux-2.6.26-rc8-mm1/fs/devpts/inode.c
===================================================================
--- linux-2.6.26-rc8-mm1.orig/fs/devpts/inode.c	2008-08-04 02:08:43.000000000 -0700
+++ linux-2.6.26-rc8-mm1/fs/devpts/inode.c	2008-08-04 02:08:50.000000000 -0700
@@ -28,8 +28,11 @@
 
 #define DEVPTS_DEFAULT_MODE 0600
 
+struct pts_fs_info {
+	struct idr allocated_ptys;
+};
+
 extern int pty_limit;			/* Config limit on Unix98 ptys */
-static DEFINE_IDR(allocated_ptys);
 static DEFINE_MUTEX(allocated_ptys_lock);
 
 static struct vfsmount *devpts_mnt;
@@ -125,6 +128,19 @@ static const struct super_operations dev
 	.show_options	= devpts_show_options,
 };
 
+static void *new_pts_fs_info(void)
+{
+	struct pts_fs_info *fsi;
+
+	fsi = kmalloc(sizeof(struct pts_fs_info), GFP_KERNEL);
+	if (fsi) {
+		idr_init(&fsi->allocated_ptys);
+	}
+	printk(KERN_ERR "new_pts_fs_info(): Returning fsi %p\n", fsi);
+	return fsi;
+}
+
+
 static int
 devpts_fill_super(struct super_block *s, void *data, int silent)
 {
@@ -135,10 +151,14 @@ devpts_fill_super(struct super_block *s,
 	s->s_magic = DEVPTS_SUPER_MAGIC;
 	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;
@@ -154,6 +174,9 @@ devpts_fill_super(struct super_block *s,
 	
 	printk("devpts: get root dentry failed\n");
 	iput(inode);
+
+free_fsi:
+	kfree(s->s_fs_info);
 fail:
 	return -ENOMEM;
 }
@@ -164,11 +187,22 @@ static int devpts_get_sb(struct file_sys
 	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 = sb->s_fs_info;	// rcu ?
+
+	//idr_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,
 };
 
 /*
@@ -187,14 +221,16 @@ int devpts_new_index(struct inode *inode
 {
 	int index;
 	int idr_ret;
+	struct super_block *sb = pts_sb_from_inode(inode);
+	struct pts_fs_info *fsi = sb->s_fs_info; 	// need rcu ?
 
 retry:
-	if (!idr_pre_get(&allocated_ptys, GFP_KERNEL)) {
+	if (!idr_pre_get(&fsi->allocated_ptys, GFP_KERNEL)) {
 		return -ENOMEM;
 	}
 
 	mutex_lock(&allocated_ptys_lock);
-	idr_ret = idr_get_new(&allocated_ptys, NULL, &index);
+	idr_ret = idr_get_new(&fsi->allocated_ptys, NULL, &index);
 	if (idr_ret < 0) {
 		mutex_unlock(&allocated_ptys_lock);
 		if (idr_ret == -EAGAIN)
@@ -203,7 +239,7 @@ retry:
 	}
 
 	if (index >= pty_limit) {
-		idr_remove(&allocated_ptys, index);
+		idr_remove(&fsi->allocated_ptys, index);
 		mutex_unlock(&allocated_ptys_lock);
 		return -EIO;
 	}
@@ -213,8 +249,11 @@ retry:
 
 void devpts_kill_index(struct inode *inode, int idx)
 {
+	struct super_block *sb = pts_sb_from_inode(inode);
+	struct pts_fs_info *fsi = sb->s_fs_info; 	// need rcu ?
+
 	mutex_lock(&allocated_ptys_lock);
-	idr_remove(&allocated_ptys, idx);
+	idr_remove(&fsi->allocated_ptys, idx);
 	mutex_unlock(&allocated_ptys_lock);
 }

[RFC][PATCH 4/6]: Allow mknod of ptmx in devpts

[sukadev-r/Jw6+rmf7HQT0dZR+AlfA]

From: Sukadev Bhattiprolu <sukadev-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>
Subject: [RFC][PATCH 4/6]: Allow mknod of ptmx in devpts

/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 mounts of devpts filesystem, an open of /dev/ptmx would be
unable to determine which instance of the devpts is being accessed.

One solution for this would be to create make /dev/ptmx a symlink to
/dev/pts/ptmx and create the device node, ptmx, in each instance of
devpts.  When /dev/ptmx is opened, we can use the inode of /dev/pts/ptmx
to identify the specific devpts instance.

(This solution has an impact on the 'startup scripts', and that is 
being discussed separately).

This patch merely enables creating the [c, 5:2] (ptmx) device in devpts
filesystem.

TODO:
	- Ability to unlink the /dev/pts/ptmx
	- Remove traces of '/dev/pts/tty' node

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

---
 fs/devpts/inode.c |   56 +++++++++++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 53 insertions(+), 3 deletions(-)

Index: linux-2.6.26-rc8-mm1/fs/devpts/inode.c
===================================================================
--- linux-2.6.26-rc8-mm1.orig/fs/devpts/inode.c	2008-08-04 02:08:50.000000000 -0700
+++ linux-2.6.26-rc8-mm1/fs/devpts/inode.c	2008-08-04 17:26:26.000000000 -0700
@@ -141,6 +141,56 @@ static void *new_pts_fs_info(void)
 }
 
 
+
+static int devpts_mknod(struct inode *dir, struct dentry *dentry,
+			int mode, dev_t rdev)
+{
+	int inum;
+	struct inode *inode;
+	struct super_block *sb = dir->i_sb;
+
+	if (dentry->d_inode)
+		return -EEXIST;
+
+	if (!S_ISCHR(mode))
+		return -EPERM;
+
+	if (rdev == MKDEV(TTYAUX_MAJOR, 2))
+		inum = 2;
+#if 0
+	else if (rdev == MKDEV(TTYAUX_MAJOR, 0))
+		inum = 3;
+#endif
+	else
+		return -EPERM;
+
+	inode = new_inode(sb);
+	if (!inode)
+		return -ENOMEM;
+
+	inode->i_ino = inum;
+	inode->i_uid = inode->i_gid = 0;
+	inode->i_blocks = 0;
+	inode->i_mtime = inode->i_atime = inode->i_ctime = CURRENT_TIME;
+
+	init_special_inode(inode, mode, rdev);
+
+	d_instantiate(dentry, inode);
+	/*
+	 * Get a reference to the dentry so the device-nodes persist
+	 * even when there are no active references to them. We use
+	 * kill_litter_super() to remove this entry when unmounting
+	 * devpts.
+	 */
+	dget(dentry);
+	return 0;
+}
+
+const struct inode_operations devpts_dir_inode_operations = {
+	.lookup         = simple_lookup,
+	.mknod		= devpts_mknod,
+};
+
 static int
 devpts_fill_super(struct super_block *s, void *data, int silent)
 {
@@ -164,7 +214,7 @@ devpts_fill_super(struct super_block *s,
 	inode->i_blocks = 0;
 	inode->i_uid = inode->i_gid = 0;
 	inode->i_mode = S_IFDIR | S_IRUGO | S_IXUGO | S_IWUSR;
-	inode->i_op = &simple_dir_inode_operations;
+	inode->i_op = &devpts_dir_inode_operations;
 	inode->i_fop = &simple_dir_operations;
 	inode->i_nlink = 2;
 
@@ -195,7 +245,7 @@ static void devpts_kill_sb(struct super_
 	//idr_destroy(&fsi->allocated_ptys);
 	kfree(fsi);
 
-	kill_anon_super(sb);
+	kill_litter_super(sb);
 }
 
 static struct file_system_type devpts_fs_type = {
@@ -274,7 +324,7 @@ int devpts_pty_new(struct inode *ptmx_in
 	if (!inode)
 		return -ENOMEM;
 
-	inode->i_ino = number+2;
+	inode->i_ino = number+4;
 	inode->i_uid = config.setuid ? config.uid : current->fsuid;
 	inode->i_gid = config.setgid ? config.gid : current->fsgid;
 	inode->i_mtime = inode->i_atime = inode->i_ctime = CURRENT_TIME;

[RFC][PATCH 5/6] Allow multiple mounts of devpts

[sukadev-r/Jw6+rmf7HQT0dZR+AlfA]

From: Sukadev Bhattiprolu <sukadev-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>
Subject: [RFC][PATCH 5/6] Allow multiple mounts of devpts

Can we simply enable multiple mounts using get_sb_nodev(), now that we
don't have any pts_namespace/'data' to be saved ?

(quick/dirty - does not prevent multiple mounts of devpts within
a single 'container')

---
 fs/devpts/inode.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Index: linux-2.6.26-rc8-mm1/fs/devpts/inode.c
===================================================================
--- linux-2.6.26-rc8-mm1.orig/fs/devpts/inode.c	2008-08-04 17:26:26.000000000 -0700
+++ linux-2.6.26-rc8-mm1/fs/devpts/inode.c	2008-08-04 17:26:31.000000000 -0700
@@ -234,7 +234,7 @@ fail:
 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);
+	return get_sb_nodev(fs_type, flags, data, devpts_fill_super, mnt);
 }

[RFC][PATCH 6/6]: /dev/tty tweak in init_dev()

[sukadev-r/Jw6+rmf7HQT0dZR+AlfA]

From: Sukadev Bhattiprolu <sukadev-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>
Subject: [RFC][PATCH 6/6]: /dev/tty tweak in init_dev()

When opening /dev/tty, __tty_open() finds the tty using get_current_tty().
When __tty_open() calls init_dev(), init_dev() tries to 'find' the tty
again from devpts.  Is that really necessary ?

The problem with asking devpts again is that with multiple mounts, devpts
cannot find the tty without knowing the specific mount instance. We can't
find the mount instance of devpts, since the inode of /dev/tty is in a
different filesystem.

---
 drivers/char/tty_io.c |    5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

Index: linux-2.6.26-rc8-mm1/drivers/char/tty_io.c
===================================================================
--- linux-2.6.26-rc8-mm1.orig/drivers/char/tty_io.c	2008-08-04 17:25:20.000000000 -0700
+++ linux-2.6.26-rc8-mm1/drivers/char/tty_io.c	2008-08-04 17:26:34.000000000 -0700
@@ -2066,7 +2066,10 @@ static int init_dev(struct tty_driver *d
 
 	/* check whether we're reopening an existing tty */
 	if (driver->flags & TTY_DRIVER_DEVPTS_MEM) {
-		tty = devpts_get_tty(inode, idx);
+		if (inode->i_rdev == MKDEV(TTYAUX_MAJOR, 0))
+			tty = *ret_tty;
+		else
+			tty = devpts_get_tty(inode, idx);
 		/*
 		 * If we don't have a tty here on a slave open, it's because
 		 * the master already started the close process and there's

Re: [RFC][PATCH 0/6] Enable multiple mounts of devpts

[H. Peter Anvin]

sukadev-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org wrote:
> 
> If devpts is mounted more than once, then '/dev/ptmx' must be a symlink
> to '/dev/pts/ptmx' and in each new devpts mount we must create the
> device node '/dev/pts/ptmx' [c, 5;2] by hand.
> 

This should be auto-created.  That also eliminates any need to support 
the mknod system call.

> Appreciate comments on overall approach of my mapping from the inode
> to sb->s_fs_info to allocated_ptys and the hacky use of get_sb_nodev(),
> and also on the tweak to init_dev() (patch 6).
> 
> Todo:
> 	User-space impact of /dev/ptmx symlink - Options are being
> 	discussed on mailing list (new mount option and config token,
> 	new fs name, etc)
> 
> 	Remove even initial kernel mount of devpts ?

The initial kernel mount of devpts should be removed, since that 
instance will never be accessible.

	-hpa

Re: [RFC][PATCH 0/6] Enable multiple mounts of devpts

[sukadev-r/Jw6+rmf7HQT0dZR+AlfA]

H. Peter Anvin [hpa-YMNOUZJC4hwAvxtiuMwx3w@public.gmane.org] wrote:
> sukadev-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org wrote:
>> If devpts is mounted more than once, then '/dev/ptmx' must be a symlink
>> to '/dev/pts/ptmx' and in each new devpts mount we must create the
>> device node '/dev/pts/ptmx' [c, 5;2] by hand.
>
> This should be auto-created.  That also eliminates any need to support the 
> mknod system call.

Ok. But was wondering if we can pass the ptmx symlink burden to the
'container-startup sripts' since they are the ones that need the second
or subsequent mount of devpts.

So, initially and for systems that don't need multiple mounts of devpts,
existing behavior can continue (/dev/ptmx is a node).

Container startup scripts have to anyway remount /dev/pts and mknod
/dev/pts/ptmx. These scripts could additionally check if /dev/ptmx is
a node and make it a symlink. The container script would have to do
this check while it still has access to the first mount of devpts
and mknod in the first devpts mnt.

But then again, the first mount is still special in the kernel.

>
>> Appreciate comments on overall approach of my mapping from the inode
>> to sb->s_fs_info to allocated_ptys and the hacky use of get_sb_nodev(),
>> and also on the tweak to init_dev() (patch 6).
>> Todo:
>> 	User-space impact of /dev/ptmx symlink - Options are being
>> 	discussed on mailing list (new mount option and config token,
>> 	new fs name, etc)
>> 	Remove even initial kernel mount of devpts ?
>
> The initial kernel mount of devpts should be removed, since that instance 
> will never be accessible.
>
> 	-hpa

Re: [RFC][PATCH 0/6] Enable multiple mounts of devpts

[H. Peter Anvin]

sukadev-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org wrote:
> 
> Appreciate comments on overall approach of my mapping from the inode
> to sb->s_fs_info to allocated_ptys and the hacky use of get_sb_nodev(),
> and also on the tweak to init_dev() (patch 6).
> 

First of all, thanks for taking this on :)  It's always delightful to 
spout some ideas and have patches appear as a result :)

Once you have the notion of the device nodes tied to a specific devpts 
filesystem, a lot of the operations can be trivialized; for example, the 
whole devpts_get_tty() mechanism can be reduced to:

	if (inode->i_sb->sb_magic != DEVPTS_SUPER_MAGIC) {
		/* do cleanup */
		return -ENXIO;
	}
	tty = inode->i_private;

This is part of what makes this whole approach so desirable: it actually 
allows for some dramatic simplifications of the existing code.

One can even bind special operations to both the ptmx node and slave 
nodes, to bypass most of the character device and tty dispatch.  That 
might require too much hacking at the tty core to be worth it, though.

	-hpa

Re: [RFC][PATCH 0/6] Enable multiple mounts of devpts

[H. Peter Anvin]

sukadev-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org wrote:
> 
> Ok. But was wondering if we can pass the ptmx symlink burden to the
> 'container-startup sripts' since they are the ones that need the second
> or subsequent mount of devpts.
> 
> So, initially and for systems that don't need multiple mounts of devpts,
> existing behavior can continue (/dev/ptmx is a node).
> 
> Container startup scripts have to anyway remount /dev/pts and mknod
> /dev/pts/ptmx. These scripts could additionally check if /dev/ptmx is
> a node and make it a symlink. The container script would have to do
> this check while it still has access to the first mount of devpts
> and mknod in the first devpts mnt.
> 
> But then again, the first mount is still special in the kernel.
> 

You're right, I think we can do this and still retain most of the 
advantages, at least for a transition period.

The idea would be that you'd have a mount option, that if you do not 
specify it, you get a bind to the in-kernel mount; otherwise you get a 
new instance.  ptmx, if not invoked from inside a devpts filesystem, 
would default to the kernel-mounted instance.

Unfortunately I believe that means parsing the command options in 
getpts_get_sb() to know if we do have the "multi" option, but that isn't 
really all that difficult; it just means breaking the parser out as a 
separate subroutine.

	-hpa

Re: [RFC][PATCH 0/6] Enable multiple mounts of devpts

[Kyle Moffett]

On Mon, Aug 4, 2008 at 10:07 PM, H. Peter Anvin <hpa-YMNOUZJC4hwAvxtiuMwx3w@public.gmane.org> wrote:
> You're right, I think we can do this and still retain most of the
> advantages, at least for a transition period.
>
> The idea would be that you'd have a mount option, that if you do not specify
> it, you get a bind to the in-kernel mount; otherwise you get a new instance.
>  ptmx, if not invoked from inside a devpts filesystem, would default to the
> kernel-mounted instance.
>
> Unfortunately I believe that means parsing the command options in
> getpts_get_sb() to know if we do have the "multi" option, but that isn't
> really all that difficult; it just means breaking the parser out as a
> separate subroutine.

There definitely needs to be a mount option (and possibly a config
option to forcibly enable the mount option).  I personally have 5 or 6
different custom scripts that depend on being able to unmount and
remount devpts without losing access to the TTYs therein.  Eventually
I will need to port those over to use "mount --move", but it would be
bad to have a random kernel upgrade just break my imaging/cloning
system.

Cheers,
Kyle Moffett

Re: [RFC][PATCH 0/6] Enable multiple mounts of devpts

[Eric W. Biederman]

> There definitely needs to be a mount option (and possibly a config
> option to forcibly enable the mount option).  I personally have 5 or 6
> different custom scripts that depend on being able to unmount and
> remount devpts without losing access to the TTYs therein.  Eventually
> I will need to port those over to use "mount --move", but it would be
> bad to have a random kernel upgrade just break my imaging/cloning
> system.

An interesting point.  What should the semantics be.  If we unmount /dev/pts
and people still have ptys open.  -EBUSY?  Except for lazy unmounts?

Eric

Re: [RFC][PATCH 0/6] Enable multiple mounts of devpts

[Kyle Moffett]

On Tue, Aug 5, 2008 at 2:15 AM, Eric W. Biederman <ebiederm-aS9lmoZGLiVWk0Htik3J/w@public.gmane.org> wrote:
>
>> There definitely needs to be a mount option (and possibly a config
>> option to forcibly enable the mount option).  I personally have 5 or 6
>> different custom scripts that depend on being able to unmount and
>> remount devpts without losing access to the TTYs therein.  Eventually
>> I will need to port those over to use "mount --move", but it would be
>> bad to have a random kernel upgrade just break my imaging/cloning
>> system.
>
> An interesting point.  What should the semantics be.  If we unmount /dev/pts
> and people still have ptys open.  -EBUSY?  Except for lazy unmounts?

Well, even if it's unmounted you can still access your pty with
/dev/tty.  As it stands right now it's possible to "umount /dev/pts"
from an SSH login and still have a mostly-functional system.  The only
failure will be when somebody needs a pseudo-TTY and you have devpts
unmounted and UNIX98 ptys turned off.

So for the legacy case, the behavior should be exactly as it is now.
In the CONFIG_DEVPTY_FORCE_PERMOUNT/"permount"-option case, I agree
that you could easily go either way.

Cheers,
Kyle Moffett

Re: [RFC][PATCH 6/6]: /dev/tty tweak in init_dev()

[Serge E. Hallyn]

Quoting sukadev-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org (sukadev-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org):
> 
> From: Sukadev Bhattiprolu <sukadev-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>
> Subject: [RFC][PATCH 6/6]: /dev/tty tweak in init_dev()
> 
> When opening /dev/tty, __tty_open() finds the tty using get_current_tty().
> When __tty_open() calls init_dev(), init_dev() tries to 'find' the tty
> again from devpts.  Is that really necessary ?
> 
> The problem with asking devpts again is that with multiple mounts, devpts
> cannot find the tty without knowing the specific mount instance. We can't
> find the mount instance of devpts, since the inode of /dev/tty is in a
> different filesystem.
> 
> ---
>  drivers/char/tty_io.c |    5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> Index: linux-2.6.26-rc8-mm1/drivers/char/tty_io.c
> ===================================================================
> --- linux-2.6.26-rc8-mm1.orig/drivers/char/tty_io.c	2008-08-04 17:25:20.000000000 -0700
> +++ linux-2.6.26-rc8-mm1/drivers/char/tty_io.c	2008-08-04 17:26:34.000000000 -0700
> @@ -2066,7 +2066,10 @@ static int init_dev(struct tty_driver *d
> 
>  	/* check whether we're reopening an existing tty */
>  	if (driver->flags & TTY_DRIVER_DEVPTS_MEM) {
> -		tty = devpts_get_tty(inode, idx);
> +		if (inode->i_rdev == MKDEV(TTYAUX_MAJOR, 0))
> +			tty = *ret_tty;

I don't understand this.  ret_tty is for returning tty.  For instance in
_ptmx_open() it's passed in completely uninitialized.  Isn't it
dereferenced later in init_dev?

And what you're doing here doesn't really match your patch intro, but
the patch intro doesn't really say what the patch does, it just lists a
problem.  So I'm 100% unclear on what your intent here is.

> +		else
> +			tty = devpts_get_tty(inode, idx);
>  		/*
>  		 * If we don't have a tty here on a slave open, it's because
>  		 * the master already started the close process and there's

Re: [RFC][PATCH 6/6]: /dev/tty tweak in init_dev()

[sukadev-r/Jw6+rmf7HQT0dZR+AlfA]

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: Sukadev Bhattiprolu <sukadev-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>
| > Subject: [RFC][PATCH 6/6]: /dev/tty tweak in init_dev()
| > 
| > When opening /dev/tty, __tty_open() finds the tty using get_current_tty().
| > When __tty_open() calls init_dev(), init_dev() tries to 'find' the tty
| > again from devpts.  Is that really necessary ?

How about I change the second sentence to:

	When __tty_open() later calls init_dev() it passes in this
	'current-tty' to _tty_open() in *ret_tty. init_dev() ignores
	this and calls devpts again to find the tty.
| > 
| > The problem with asking devpts again is that with multiple mounts, devpts
| > cannot find the tty without knowing the specific mount instance. We can't
| > find the mount instance of devpts, since the inode of /dev/tty is in a
| > different filesystem.
| > 
| > ---
| >  drivers/char/tty_io.c |    5 ++++-
| >  1 file changed, 4 insertions(+), 1 deletion(-)
| > 
| > Index: linux-2.6.26-rc8-mm1/drivers/char/tty_io.c
| > ===================================================================
| > --- linux-2.6.26-rc8-mm1.orig/drivers/char/tty_io.c	2008-08-04 17:25:20.000000000 -0700
| > +++ linux-2.6.26-rc8-mm1/drivers/char/tty_io.c	2008-08-04 17:26:34.000000000 -0700
| > @@ -2066,7 +2066,10 @@ static int init_dev(struct tty_driver *d
| > 
| >  	/* check whether we're reopening an existing tty */
| >  	if (driver->flags & TTY_DRIVER_DEVPTS_MEM) {
| > -		tty = devpts_get_tty(inode, idx);
| > +		if (inode->i_rdev == MKDEV(TTYAUX_MAJOR, 0))
| > +			tty = *ret_tty;
| 
| I don't understand this.  ret_tty is for returning tty.  For instance in
| _ptmx_open() it's passed in completely uninitialized.  Isn't it
| dereferenced later in init_dev?

Yes, it is a quick/dirty fix.  When called from _ptmx_open() inode->i_rdev
would be [5,2] so, the uninitialized *ret_tty would not be used. 

Yes, it should be cleaner. I have been thinking of calling get_current_ty()
again here or renaming the parameter.

| 
| And what you're doing here doesn't really match your patch intro, but
| the patch intro doesn't really say what the patch does, it just lists a
| problem.  So I'm 100% unclear on what your intent here is.

I can update the intro as pointed above.  And will add a comment here.

The point is devpts does not have sufficient information to find the tty
struct for /dev/tty.  Fortunately, we already have the tty struct in this
case and don't need to ask devpts




| 
| > +		else
| > +			tty = devpts_get_tty(inode, idx);
| >  		/*
| >  		 * If we don't have a tty here on a slave open, it's because
| >  		 * the master already started the close process and there's

Re: [RFC][PATCH 6/6]: /dev/tty tweak in init_dev()

[Serge E. Hallyn]

Quoting sukadev-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org (sukadev-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org):
> 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: Sukadev Bhattiprolu <sukadev-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>
> | > Subject: [RFC][PATCH 6/6]: /dev/tty tweak in init_dev()
> | > 
> | > When opening /dev/tty, __tty_open() finds the tty using get_current_tty().
> | > When __tty_open() calls init_dev(), init_dev() tries to 'find' the tty
> | > again from devpts.  Is that really necessary ?
> 
> How about I change the second sentence to:
> 
> 	When __tty_open() later calls init_dev() it passes in this
> 	'current-tty' to _tty_open() in *ret_tty. init_dev() ignores
> 	this and calls devpts again to find the tty.

Ah, thank you, that makes sense.

-serge

> | > The problem with asking devpts again is that with multiple mounts, devpts
> | > cannot find the tty without knowing the specific mount instance. We can't
> | > find the mount instance of devpts, since the inode of /dev/tty is in a
> | > different filesystem.
> | > 
> | > ---
> | >  drivers/char/tty_io.c |    5 ++++-
> | >  1 file changed, 4 insertions(+), 1 deletion(-)
> | > 
> | > Index: linux-2.6.26-rc8-mm1/drivers/char/tty_io.c
> | > ===================================================================
> | > --- linux-2.6.26-rc8-mm1.orig/drivers/char/tty_io.c	2008-08-04 17:25:20.000000000 -0700
> | > +++ linux-2.6.26-rc8-mm1/drivers/char/tty_io.c	2008-08-04 17:26:34.000000000 -0700
> | > @@ -2066,7 +2066,10 @@ static int init_dev(struct tty_driver *d
> | > 
> | >  	/* check whether we're reopening an existing tty */
> | >  	if (driver->flags & TTY_DRIVER_DEVPTS_MEM) {
> | > -		tty = devpts_get_tty(inode, idx);
> | > +		if (inode->i_rdev == MKDEV(TTYAUX_MAJOR, 0))
> | > +			tty = *ret_tty;
> | 
> | I don't understand this.  ret_tty is for returning tty.  For instance in
> | _ptmx_open() it's passed in completely uninitialized.  Isn't it
> | dereferenced later in init_dev?
> 
> Yes, it is a quick/dirty fix.  When called from _ptmx_open() inode->i_rdev
> would be [5,2] so, the uninitialized *ret_tty would not be used. 
> 
> Yes, it should be cleaner. I have been thinking of calling get_current_ty()
> again here or renaming the parameter.
> 
> | 
> | And what you're doing here doesn't really match your patch intro, but
> | the patch intro doesn't really say what the patch does, it just lists a
> | problem.  So I'm 100% unclear on what your intent here is.
> 
> I can update the intro as pointed above.  And will add a comment here.
> 
> The point is devpts does not have sufficient information to find the tty
> struct for /dev/tty.  Fortunately, we already have the tty struct in this
> case and don't need to ask devpts
> 
> 
> 
> 
> | 
> | > +		else
> | > +			tty = devpts_get_tty(inode, idx);
> | >  		/*
> | >  		 * If we don't have a tty here on a slave open, it's because
> | >  		 * the master already started the close process and there's

Re: [RFC][PATCH 0/6] Enable multiple mounts of devpts

[H. Peter Anvin]

Kyle Moffett wrote:
> On Tue, Aug 5, 2008 at 2:15 AM, Eric W. Biederman <ebiederm-aS9lmoZGLiVWk0Htik3J/w@public.gmane.org> wrote:
>>> There definitely needs to be a mount option (and possibly a config
>>> option to forcibly enable the mount option).  I personally have 5 or 6
>>> different custom scripts that depend on being able to unmount and
>>> remount devpts without losing access to the TTYs therein.  Eventually
>>> I will need to port those over to use "mount --move", but it would be
>>> bad to have a random kernel upgrade just break my imaging/cloning
>>> system.
>> An interesting point.  What should the semantics be.  If we unmount /dev/pts
>> and people still have ptys open.  -EBUSY?  Except for lazy unmounts?
> 
> Well, even if it's unmounted you can still access your pty with
> /dev/tty.  As it stands right now it's possible to "umount /dev/pts"
> from an SSH login and still have a mostly-functional system.

This is only because there is always an instance in the kernel.

> The only
> failure will be when somebody needs a pseudo-TTY and you have devpts
> unmounted and UNIX98 ptys turned off.
> 
> So for the legacy case, the behavior should be exactly as it is now.
> In the CONFIG_DEVPTY_FORCE_PERMOUNT/"permount"-option case, I agree
> that you could easily go either way.

In the multimount case, we should refuse umounts (except, obviously, 
lazy umounts) if there are ptys open, or ptys used as current terminals, 
in that filesystem.  Simple.

	-hpa