Devpts namespace

From: Sukadev Bhattiprolu <sukadev-r/[email protected]>
Subject: [RFC][PATCH 3/4]: Enable multiple mounts of /dev/pts

To support multiple PTY namespaces, we should be allow multiple mounts of /dev/pts, once within each PTY namespace.

This patch removes the get_sb_single() in devpts_get_sb() and uses test and set sb interfaces to allow remounting /dev/pts. The patch also removes the globals, 'devpts_root' and uses current_pts_mnt() to access 'devpts_mnt'

Changelog:
- Version 0: Based on earlier versions from Serge Hallyn and Matt Helsley.

Signed-off-by Sukadev Bhattiprolu <sukadev-r/[email protected]>

---
 fs/devpts/inode.c |  120 +++++++++++++++++++++++++++++++++++++++++++++---------
 1 file changed, 101 insertions(+), 19 deletions(-)

Index: linux-2.6.24/fs/devpts/inode.c
===================================================================

fs/devpts/inode.c +101 -19

@@ -34,7 +34,10 @@ static inline struct idr *current_pts_ns
 }
 
 static struct vfsmount *devpts_mnt;
-static struct dentry *devpts_root;
+static inline struct vfsmount *current_pts_ns_mnt(void)
+{
+	return devpts_mnt;
+}
 
 static struct {
 	int setuid;

@@ -130,7 +133,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;
 

@@ -140,10 +143,53 @@ fail:
 	return -ENOMEM;
 }
 
+/*
+ * We use test and set super-block operations to help determine whether we
+ * need a new super-block for this namespace. get_sb() walks the list of
+ * existing devpts supers, comparing them with the @data ptr. Since we
+ * passed 'current's namespace as the @data pointer we can compare the
+ * namespace pointer in the super-block's 's_fs_info'.  If the test is
+ * TRUE then get_sb() returns a new active reference to the super block.
+ * Otherwise, it helps us build an active reference to a new one.
+ */
+
+static int devpts_test_sb(struct super_block *sb, void *data)
+{
+	return sb->s_fs_info == data;
+}
+
+static int devpts_set_sb(struct super_block *sb, void *data)
+{
+	sb->s_fs_info = data;
+	return set_anon_super(sb, NULL);
+}
+
 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);
+	struct super_block *sb;
+	int err;
+
+	/* hereafter we're very simlar to get_sb_nodev */
+	sb = sget(fs_type, devpts_test_sb, devpts_set_sb, data);
+	if (IS_ERR(sb))
+		return PTR_ERR(sb);
+
+	if (sb->s_root)
+		return simple_set_mnt(mnt, sb);
+
+	sb->s_flags = flags;
+	err = devpts_fill_super(sb, data, flags & MS_SILENT ? 1 : 0);
+	if (err) {
+		up_write(&sb->s_umount);
+		deactivate_super(sb);
+		return err;
+	}
+
+	sb->s_flags |= MS_ACTIVE;
+	devpts_mnt = mnt;
+
+	return simple_set_mnt(mnt, sb);
 }
 
 static struct file_system_type devpts_fs_type = {

@@ -158,10 +204,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));
 }

@@ -207,12 +252,28 @@ int devpts_pty_new(struct tty_struct *tt
 	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 dentry *root;
+	struct vfsmount *mnt;
+	struct inode *inode;
+
 
 	/* We're supposed to be given the slave end of a pty */
 	BUG_ON(driver->type != TTY_DRIVER_TYPE_PTY);
 	BUG_ON(driver->subtype != PTY_TYPE_SLAVE);
 
+	mnt = current_pts_ns_mnt();
+	if (!mnt)
+		return -ENOSYS;
+	root = mnt->mnt_root;
+
+	mutex_lock(&root->d_inode->i_mutex);
+	inode = idr_find(current_pts_ns_allocated_ptys(), number);
+	mutex_unlock(&root->d_inode->i_mutex);
+
+	if (inode && !IS_ERR(inode))
+		return -EEXIST;
+
+	inode = new_inode(mnt->mnt_sb);
 	if (!inode)
 		return -ENOMEM;
 

@@ -222,23 +283,31 @@ int devpts_pty_new(struct tty_struct *tt
 	inode->i_mtime = inode->i_atime = inode->i_ctime = CURRENT_TIME;
 	init_special_inode(inode, S_IFCHR|config.mode, device);
 	inode->i_private = tty;
+	idr_replace(current_pts_ns_allocated_ptys(), inode, number);
 
-	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(int number)
 {
-	struct dentry *dentry = get_node(number);
+	struct vfsmount *mnt;
+	struct dentry *dentry;
 	struct tty_struct *tty;
 
+	mnt = current_pts_ns_mnt();
+	if (!mnt)
+		return NULL;
+
+	dentry = get_node(mnt->mnt_root, number);
+
 	tty = NULL;
 	if (!IS_ERR(dentry)) {
 		if (dentry->d_inode)

@@ -246,14 +315,21 @@ struct tty_struct *devpts_get_tty(int nu
 		dput(dentry);
 	}
 
-	mutex_unlock(&devpts_root->d_inode->i_mutex);
+	mutex_unlock(&mnt->mnt_root->d_inode->i_mutex);
 
 	return tty;
 }
 
 void devpts_pty_kill(int number)
 {
-	struct dentry *dentry = get_node(number);
+	struct dentry *dentry;
+	struct dentry *root;
+	struct vfsmount *mnt;
+
+	mnt = current_pts_ns_mnt();
+	root = mnt->mnt_root;
+
+	dentry = get_node(root, number);
 
 	if (!IS_ERR(dentry)) {
 		struct inode *inode = dentry->d_inode;

@@ -264,17 +340,23 @@ void devpts_pty_kill(int number)
 		}
 		dput(dentry);
 	}
-	mutex_unlock(&devpts_root->d_inode->i_mutex);
+	mutex_unlock(&root->d_inode->i_mutex);
 }
 
 static int __init init_devpts_fs(void)
 {
-	int err = register_filesystem(&devpts_fs_type);
-	if (!err) {
-		devpts_mnt = kern_mount(&devpts_fs_type);
-		if (IS_ERR(devpts_mnt))
-			err = PTR_ERR(devpts_mnt);
-	}
+	struct vfsmount *mnt;
+	int err;
+
+	err = register_filesystem(&devpts_fs_type);
+	if (err)
+		return err;
+
+	mnt = kern_mount_data(&devpts_fs_type, NULL);
+	if (IS_ERR(mnt))
+		err = PTR_ERR(mnt);
+	else
+		devpts_mnt = mnt;
 	return err;
 }
 

_______________________________________________
Containers mailing list Containers-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org
https://lists.linux-foundation.org/mailman/listinfo/containers

From: Sukadev Bhattiprolu <sukadev-r/[email protected]>
Subject: [RFC][PATCH 3/4]: Enable multiple mounts of /dev/pts

To support multiple PTY namespaces, we should be allow multiple mounts of /dev/pts, once within each PTY namespace.

This patch removes the get_sb_single() in devpts_get_sb() and uses test and set sb interfaces to allow remounting /dev/pts. The patch also removes the globals, 'devpts_root' and uses current_pts_mnt() to access 'devpts_mnt'

Changelog:
- Version 0: Based on earlier versions from Serge Hallyn and Matt Helsley.

Signed-off-by Sukadev Bhattiprolu <sukadev-r/[email protected]>

---
 fs/devpts/inode.c |  120 +++++++++++++++++++++++++++++++++++++++++++++---------
 1 file changed, 101 insertions(+), 19 deletions(-)

Index: linux-2.6.24/fs/devpts/inode.c
===================================================================

fs/devpts/inode.c +35 -3

@@ -34,7 +34,10 @@ static inline struct idr *current_pts_ns
 }
 
 static struct vfsmount *devpts_mnt;
-static struct dentry *devpts_root;
+static inline struct vfsmount *current_pts_ns_mnt(void)
+{
+	return devpts_mnt;
+}
 
 static struct {
 	int setuid;

@@ -130,7 +133,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;
 

@@ -140,10 +143,53 @@ fail:
 	return -ENOMEM;
 }
 
+/*
+ * We use test and set super-block operations to help determine whether we
+ * need a new super-block for this namespace. get_sb() walks the list of
+ * existing devpts supers, comparing them with the @data ptr. Since we
+ * passed 'current's namespace as the @data pointer we can compare the
+ * namespace pointer in the super-block's 's_fs_info'.  If the test is
+ * TRUE then get_sb() returns a new active reference to the super block.
+ * Otherwise, it helps us build an active reference to a new one.
+ */
+
+static int devpts_test_sb(struct super_block *sb, void *data)
+{
+	return sb->s_fs_info == data;
+}
+
+static int devpts_set_sb(struct super_block *sb, void *data)
+{
+	sb->s_fs_info = data;
+	return set_anon_super(sb, NULL);
+}
+
 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);
+	struct super_block *sb;
+	int err;
+
+	/* hereafter we're very simlar to get_sb_nodev */
+	sb = sget(fs_type, devpts_test_sb, devpts_set_sb, data);
+	if (IS_ERR(sb))
+		return PTR_ERR(sb);
+
+	if (sb->s_root)

fs/devpts/inode.c +56 -16

+	sb->s_flags |= MS_ACTIVE;
+	devpts_mnt = mnt;
+
+	return simple_set_mnt(mnt, sb);
 }
 
 static struct file_system_type devpts_fs_type = {

@@ -158,10 +204,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));
 }

@@ -207,12 +252,28 @@ int devpts_pty_new(struct tty_struct *tt
 	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 dentry *root;
+	struct vfsmount *mnt;
+	struct inode *inode;
+
 
 	/* We're supposed to be given the slave end of a pty */
 	BUG_ON(driver->type != TTY_DRIVER_TYPE_PTY);
 	BUG_ON(driver->subtype != PTY_TYPE_SLAVE);
 
+	mnt = current_pts_ns_mnt();
+	if (!mnt)
+		return -ENOSYS;
+	root = mnt->mnt_root;
+
+	mutex_lock(&root->d_inode->i_mutex);
+	inode = idr_find(current_pts_ns_allocated_ptys(), number);
+	mutex_unlock(&root->d_inode->i_mutex);
+
+	if (inode && !IS_ERR(inode))
+		return -EEXIST;
+
+	inode = new_inode(mnt->mnt_sb);
 	if (!inode)
 		return -ENOMEM;
 

@@ -222,23 +283,31 @@ int devpts_pty_new(struct tty_struct *tt
 	inode->i_mtime = inode->i_atime = inode->i_ctime = CURRENT_TIME;
 	init_special_inode(inode, S_IFCHR|config.mode, device);
 	inode->i_private = tty;
+	idr_replace(current_pts_ns_allocated_ptys(), inode, number);
 
-	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(int number)
 {
-	struct dentry *dentry = get_node(number);
+	struct vfsmount *mnt;
+	struct dentry *dentry;
 	struct tty_struct *tty;
 
+	mnt = current_pts_ns_mnt();
+	if (!mnt)
+		return NULL;
+
+	dentry = get_node(mnt->mnt_root, number);
+
 	tty = NULL;
 	if (!IS_ERR(dentry)) {
 		if (dentry->d_inode)

@@ -246,14 +315,21 @@ struct tty_struct *devpts_get_tty(int nu
 		dput(dentry);
 	}
 
-	mutex_unlock(&devpts_root->d_inode->i_mutex);
+	mutex_unlock(&mnt->mnt_root->d_inode->i_mutex);
 
 	return tty;
 }
 
 void devpts_pty_kill(int number)
 {
-	struct dentry *dentry = get_node(number);
+	struct dentry *dentry;
+	struct dentry *root;
+	struct vfsmount *mnt;
+
+	mnt = current_pts_ns_mnt();
+	root = mnt->mnt_root;
+
+	dentry = get_node(root, number);
 
 	if (!IS_ERR(dentry)) {
 		struct inode *inode = dentry->d_inode;

@@ -264,17 +340,23 @@ void devpts_pty_kill(int number)
 		}
 		dput(dentry);
 	}
-	mutex_unlock(&devpts_root->d_inode->i_mutex);
+	mutex_unlock(&root->d_inode->i_mutex);
 }
 
 static int __init init_devpts_fs(void)
 {
-	int err = register_filesystem(&devpts_fs_type);
-	if (!err) {
-		devpts_mnt = kern_mount(&devpts_fs_type);
-		if (IS_ERR(devpts_mnt))
-			err = PTR_ERR(devpts_mnt);
-	}
+	struct vfsmount *mnt;
+	int err;
+
+	err = register_filesystem(&devpts_fs_type);
+	if (err)
+		return err;
+
+	mnt = kern_mount_data(&devpts_fs_type, NULL);
+	if (IS_ERR(mnt))
+		err = PTR_ERR(mnt);
+	else
+		devpts_mnt = mnt;
 	return err;
 }
 

_______________________________________________
Containers mailing list Containers-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org
https://lists.linux-foundation.org/mailman/listinfo/containers

_______________________________________________
Devel mailing list
[email protected]
https://openvz.org/mailman/listinfo/devel

Quoted 87 lines · sukadev-r/[email protected] wrote:

From: Sukadev Bhattiprolu <sukadev-r/[email protected]>
Subject: [RFC][PATCH 3/4]: Enable multiple mounts of /dev/pts

To support multiple PTY namespaces, we should be allow multiple mounts of /dev/pts, once within each PTY namespace.

This patch removes the get_sb_single() in devpts_get_sb() and uses test and set sb interfaces to allow remounting /dev/pts. The patch also removes the globals, 'devpts_root' and uses current_pts_mnt() to access 'devpts_mnt'

Changelog:
- Version 0: Based on earlier versions from Serge Hallyn and Matt Helsley.

Signed-off-by Sukadev Bhattiprolu <sukadev-r/[email protected]>

---
 fs/devpts/inode.c |  120 +++++++++++++++++++++++++++++++++++++++++++++---------
 1 file changed, 101 insertions(+), 19 deletions(-)

Index: linux-2.6.24/fs/devpts/inode.c
===================================================================

fs/devpts/inode.c +40 -3

@@ -34,7 +34,10 @@ static inline struct idr *current_pts_ns
 }
 
 static struct vfsmount *devpts_mnt;
-static struct dentry *devpts_root;
+static inline struct vfsmount *current_pts_ns_mnt(void)
+{
+	return devpts_mnt;
+}
 
 static struct {
 	int setuid;

@@ -130,7 +133,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;
 

@@ -140,10 +143,53 @@ fail:
 	return -ENOMEM;
 }
 
+/*
+ * We use test and set super-block operations to help determine whether we
+ * need a new super-block for this namespace. get_sb() walks the list of
+ * existing devpts supers, comparing them with the @data ptr. Since we
+ * passed 'current's namespace as the @data pointer we can compare the
+ * namespace pointer in the super-block's 's_fs_info'.  If the test is
+ * TRUE then get_sb() returns a new active reference to the super block.
+ * Otherwise, it helps us build an active reference to a new one.
+ */
+
+static int devpts_test_sb(struct super_block *sb, void *data)
+{
+	return sb->s_fs_info == data;
+}
+
+static int devpts_set_sb(struct super_block *sb, void *data)
+{
+	sb->s_fs_info = data;
+	return set_anon_super(sb, NULL);
+}
+
 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);
+	struct super_block *sb;
+	int err;
+
+	/* hereafter we're very simlar to get_sb_nodev */
+	sb = sget(fs_type, devpts_test_sb, devpts_set_sb, data);
+	if (IS_ERR(sb))
+		return PTR_ERR(sb);
+
+	if (sb->s_root)
+		return simple_set_mnt(mnt, sb);
+
+	sb->s_flags = flags;
+	err = devpts_fill_super(sb, data, flags & MS_SILENT ? 1 : 0);
+	if (err) {

fs/devpts/inode.c fail: · line 188

Quoted 5 lines

fs/devpts/inode.c +5 -0

+		up_write(&sb->s_umount);
+		deactivate_super(sb);
+		return err;
+	}
+

That stuff becomes very very similar to that in proc :) Makes sense to consolidate. Maybe...

Quoted 146 lines

fs/devpts/inode.c +56 -16

+	sb->s_flags |= MS_ACTIVE;
+	devpts_mnt = mnt;
+
+	return simple_set_mnt(mnt, sb);
 }
 
 static struct file_system_type devpts_fs_type = {

@@ -158,10 +204,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));
 }

@@ -207,12 +252,28 @@ int devpts_pty_new(struct tty_struct *tt
 	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 dentry *root;
+	struct vfsmount *mnt;
+	struct inode *inode;
+
 
 	/* We're supposed to be given the slave end of a pty */
 	BUG_ON(driver->type != TTY_DRIVER_TYPE_PTY);
 	BUG_ON(driver->subtype != PTY_TYPE_SLAVE);
 
+	mnt = current_pts_ns_mnt();
+	if (!mnt)
+		return -ENOSYS;
+	root = mnt->mnt_root;
+
+	mutex_lock(&root->d_inode->i_mutex);
+	inode = idr_find(current_pts_ns_allocated_ptys(), number);
+	mutex_unlock(&root->d_inode->i_mutex);
+
+	if (inode && !IS_ERR(inode))
+		return -EEXIST;
+
+	inode = new_inode(mnt->mnt_sb);
 	if (!inode)
 		return -ENOMEM;
 

@@ -222,23 +283,31 @@ int devpts_pty_new(struct tty_struct *tt
 	inode->i_mtime = inode->i_atime = inode->i_ctime = CURRENT_TIME;
 	init_special_inode(inode, S_IFCHR|config.mode, device);
 	inode->i_private = tty;
+	idr_replace(current_pts_ns_allocated_ptys(), inode, number);
 
-	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(int number)
 {
-	struct dentry *dentry = get_node(number);
+	struct vfsmount *mnt;
+	struct dentry *dentry;
 	struct tty_struct *tty;
 
+	mnt = current_pts_ns_mnt();
+	if (!mnt)
+		return NULL;
+
+	dentry = get_node(mnt->mnt_root, number);
+
 	tty = NULL;
 	if (!IS_ERR(dentry)) {
 		if (dentry->d_inode)

@@ -246,14 +315,21 @@ struct tty_struct *devpts_get_tty(int nu
 		dput(dentry);
 	}
 
-	mutex_unlock(&devpts_root->d_inode->i_mutex);
+	mutex_unlock(&mnt->mnt_root->d_inode->i_mutex);
 
 	return tty;
 }
 
 void devpts_pty_kill(int number)
 {
-	struct dentry *dentry = get_node(number);
+	struct dentry *dentry;
+	struct dentry *root;
+	struct vfsmount *mnt;
+
+	mnt = current_pts_ns_mnt();
+	root = mnt->mnt_root;
+
+	dentry = get_node(root, number);
 
 	if (!IS_ERR(dentry)) {
 		struct inode *inode = dentry->d_inode;

@@ -264,17 +340,23 @@ void devpts_pty_kill(int number)
 		}
 		dput(dentry);
 	}
-	mutex_unlock(&devpts_root->d_inode->i_mutex);
+	mutex_unlock(&root->d_inode->i_mutex);
 }
 
 static int __init init_devpts_fs(void)
 {
-	int err = register_filesystem(&devpts_fs_type);
-	if (!err) {
-		devpts_mnt = kern_mount(&devpts_fs_type);
-		if (IS_ERR(devpts_mnt))
-			err = PTR_ERR(devpts_mnt);
-	}
+	struct vfsmount *mnt;
+	int err;
+
+	err = register_filesystem(&devpts_fs_type);
+	if (err)
+		return err;
+
+	mnt = kern_mount_data(&devpts_fs_type, NULL);
+	if (IS_ERR(mnt))
+		err = PTR_ERR(mnt);
+	else
+		devpts_mnt = mnt;
 	return err;
 }
 

_______________________________________________
Containers mailing list Containers-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org
https://lists.linux-foundation.org/mailman/listinfo/containers

Quoted 5 lines

_______________________________________________
Devel mailing list
[email protected]
https://openvz.org/mailman/listinfo/devel

_______________________________________________
Containers mailing list Containers-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org
https://lists.linux-foundation.org/mailman/listinfo/containers

Quoted 87 lines · Pavel Emelyanov

Quoted 86 lines · sukadev-r/[email protected] wrote:

From: Sukadev Bhattiprolu <sukadev-r/[email protected]>
Subject: [RFC][PATCH 3/4]: Enable multiple mounts of /dev/pts

To support multiple PTY namespaces, we should be allow multiple mounts of /dev/pts, once within each PTY namespace.

This patch removes the get_sb_single() in devpts_get_sb() and uses test and set sb interfaces to allow remounting /dev/pts. The patch also removes the globals, 'devpts_root' and uses current_pts_mnt() to access 'devpts_mnt'

Changelog:
- Version 0: Based on earlier versions from Serge Hallyn and Matt Helsley.

Signed-off-by Sukadev Bhattiprolu <sukadev-r/[email protected]>

---
 fs/devpts/inode.c |  120 +++++++++++++++++++++++++++++++++++++++++++++---------
 1 file changed, 101 insertions(+), 19 deletions(-)

Index: linux-2.6.24/fs/devpts/inode.c
===================================================================

fs/devpts/inode.c +39 -3

@@ -34,7 +34,10 @@ static inline struct idr *current_pts_ns
 }
 
 static struct vfsmount *devpts_mnt;
-static struct dentry *devpts_root;
+static inline struct vfsmount *current_pts_ns_mnt(void)
+{
+	return devpts_mnt;
+}
 
 static struct {
 	int setuid;

@@ -130,7 +133,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;
 

@@ -140,10 +143,53 @@ fail:
 	return -ENOMEM;
 }
 
+/*
+ * We use test and set super-block operations to help determine whether we
+ * need a new super-block for this namespace. get_sb() walks the list of
+ * existing devpts supers, comparing them with the @data ptr. Since we
+ * passed 'current's namespace as the @data pointer we can compare the
+ * namespace pointer in the super-block's 's_fs_info'.  If the test is
+ * TRUE then get_sb() returns a new active reference to the super block.
+ * Otherwise, it helps us build an active reference to a new one.
+ */
+
+static int devpts_test_sb(struct super_block *sb, void *data)
+{
+	return sb->s_fs_info == data;
+}
+
+static int devpts_set_sb(struct super_block *sb, void *data)
+{
+	sb->s_fs_info = data;
+	return set_anon_super(sb, NULL);
+}
+
 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);
+	struct super_block *sb;
+	int err;
+
+	/* hereafter we're very simlar to get_sb_nodev */
+	sb = sget(fs_type, devpts_test_sb, devpts_set_sb, data);
+	if (IS_ERR(sb))
+		return PTR_ERR(sb);
+
+	if (sb->s_root)
+		return simple_set_mnt(mnt, sb);
+
+	sb->s_flags = flags;
+	err = devpts_fill_super(sb, data, flags & MS_SILENT ? 1 : 0);

Quoted 8 lines

fs/devpts/inode.c fail: · line 188

Quoted 6 lines

fs/devpts/inode.c +6 -0

+	if (err) {
+		up_write(&sb->s_umount);
+		deactivate_super(sb);
+		return err;
+	}
+

That stuff becomes very very similar to that in proc :) Makes sense to consolidate. Maybe...

Yeah, and the mqns that Cedric sent too. I think Cedric said he'd started an a patch implementing a helper. Cedric?

Pavel, not long ago you said you were starting to look at tty and pty stuff - did you have any different ideas on devpts virtualization, or are you ok with this minus your comments thus far?

Quoted 154 lines

Quoted 145 lines

fs/devpts/inode.c +56 -16

+	sb->s_flags |= MS_ACTIVE;
+	devpts_mnt = mnt;
+
+	return simple_set_mnt(mnt, sb);
 }
 
 static struct file_system_type devpts_fs_type = {

@@ -158,10 +204,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));
 }

@@ -207,12 +252,28 @@ int devpts_pty_new(struct tty_struct *tt
 	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 dentry *root;
+	struct vfsmount *mnt;
+	struct inode *inode;
+
 
 	/* We're supposed to be given the slave end of a pty */
 	BUG_ON(driver->type != TTY_DRIVER_TYPE_PTY);
 	BUG_ON(driver->subtype != PTY_TYPE_SLAVE);
 
+	mnt = current_pts_ns_mnt();
+	if (!mnt)
+		return -ENOSYS;
+	root = mnt->mnt_root;
+
+	mutex_lock(&root->d_inode->i_mutex);
+	inode = idr_find(current_pts_ns_allocated_ptys(), number);
+	mutex_unlock(&root->d_inode->i_mutex);
+
+	if (inode && !IS_ERR(inode))
+		return -EEXIST;
+
+	inode = new_inode(mnt->mnt_sb);
 	if (!inode)
 		return -ENOMEM;
 

@@ -222,23 +283,31 @@ int devpts_pty_new(struct tty_struct *tt
 	inode->i_mtime = inode->i_atime = inode->i_ctime = CURRENT_TIME;
 	init_special_inode(inode, S_IFCHR|config.mode, device);
 	inode->i_private = tty;
+	idr_replace(current_pts_ns_allocated_ptys(), inode, number);
 
-	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(int number)
 {
-	struct dentry *dentry = get_node(number);
+	struct vfsmount *mnt;
+	struct dentry *dentry;
 	struct tty_struct *tty;
 
+	mnt = current_pts_ns_mnt();
+	if (!mnt)
+		return NULL;
+
+	dentry = get_node(mnt->mnt_root, number);
+
 	tty = NULL;
 	if (!IS_ERR(dentry)) {
 		if (dentry->d_inode)

@@ -246,14 +315,21 @@ struct tty_struct *devpts_get_tty(int nu
 		dput(dentry);
 	}
 
-	mutex_unlock(&devpts_root->d_inode->i_mutex);
+	mutex_unlock(&mnt->mnt_root->d_inode->i_mutex);
 
 	return tty;
 }
 
 void devpts_pty_kill(int number)
 {
-	struct dentry *dentry = get_node(number);
+	struct dentry *dentry;
+	struct dentry *root;
+	struct vfsmount *mnt;
+
+	mnt = current_pts_ns_mnt();
+	root = mnt->mnt_root;
+
+	dentry = get_node(root, number);
 
 	if (!IS_ERR(dentry)) {
 		struct inode *inode = dentry->d_inode;

@@ -264,17 +340,23 @@ void devpts_pty_kill(int number)
 		}
 		dput(dentry);
 	}
-	mutex_unlock(&devpts_root->d_inode->i_mutex);
+	mutex_unlock(&root->d_inode->i_mutex);
 }
 
 static int __init init_devpts_fs(void)
 {
-	int err = register_filesystem(&devpts_fs_type);
-	if (!err) {
-		devpts_mnt = kern_mount(&devpts_fs_type);
-		if (IS_ERR(devpts_mnt))
-			err = PTR_ERR(devpts_mnt);
-	}
+	struct vfsmount *mnt;
+	int err;
+
+	err = register_filesystem(&devpts_fs_type);
+	if (err)
+		return err;
+
+	mnt = kern_mount_data(&devpts_fs_type, NULL);
+	if (IS_ERR(mnt))
+		err = PTR_ERR(mnt);
+	else
+		devpts_mnt = mnt;
 	return err;
 }
 

_______________________________________________
Containers mailing list Containers-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org
https://lists.linux-foundation.org/mailman/listinfo/containers

Quoted 5 lines

_______________________________________________
Devel mailing list
[email protected]
https://openvz.org/mailman/listinfo/devel

_______________________________________________
Containers mailing list Containers-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org
https://lists.linux-foundation.org/mailman/listinfo/containers

Quoted 97 lines · Serge E. Hallyn

Quoted 87 lines · Pavel Emelyanov

Quoted 86 lines · sukadev-r/[email protected] wrote:

From: Sukadev Bhattiprolu <sukadev-r/[email protected]>
Subject: [RFC][PATCH 3/4]: Enable multiple mounts of /dev/pts

To support multiple PTY namespaces, we should be allow multiple mounts of /dev/pts, once within each PTY namespace.

This patch removes the get_sb_single() in devpts_get_sb() and uses test and set sb interfaces to allow remounting /dev/pts. The patch also removes the globals, 'devpts_root' and uses current_pts_mnt() to access 'devpts_mnt'

Changelog:
- Version 0: Based on earlier versions from Serge Hallyn and Matt Helsley.

Signed-off-by Sukadev Bhattiprolu <sukadev-r/[email protected]>

---
 fs/devpts/inode.c |  120 +++++++++++++++++++++++++++++++++++++++++++++---------
 1 file changed, 101 insertions(+), 19 deletions(-)

Index: linux-2.6.24/fs/devpts/inode.c
===================================================================

fs/devpts/inode.c +39 -3

@@ -34,7 +34,10 @@ static inline struct idr *current_pts_ns
 }
 
 static struct vfsmount *devpts_mnt;
-static struct dentry *devpts_root;
+static inline struct vfsmount *current_pts_ns_mnt(void)
+{
+	return devpts_mnt;
+}
 
 static struct {
 	int setuid;

@@ -130,7 +133,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;
 

@@ -140,10 +143,53 @@ fail:
 	return -ENOMEM;
 }
 
+/*
+ * We use test and set super-block operations to help determine whether we
+ * need a new super-block for this namespace. get_sb() walks the list of
+ * existing devpts supers, comparing them with the @data ptr. Since we
+ * passed 'current's namespace as the @data pointer we can compare the
+ * namespace pointer in the super-block's 's_fs_info'.  If the test is
+ * TRUE then get_sb() returns a new active reference to the super block.
+ * Otherwise, it helps us build an active reference to a new one.
+ */
+
+static int devpts_test_sb(struct super_block *sb, void *data)
+{
+	return sb->s_fs_info == data;
+}
+
+static int devpts_set_sb(struct super_block *sb, void *data)
+{
+	sb->s_fs_info = data;
+	return set_anon_super(sb, NULL);
+}
+
 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);
+	struct super_block *sb;
+	int err;
+
+	/* hereafter we're very simlar to get_sb_nodev */
+	sb = sget(fs_type, devpts_test_sb, devpts_set_sb, data);
+	if (IS_ERR(sb))
+		return PTR_ERR(sb);
+
+	if (sb->s_root)
+		return simple_set_mnt(mnt, sb);
+
+	sb->s_flags = flags;
+	err = devpts_fill_super(sb, data, flags & MS_SILENT ? 1 : 0);

Quoted 8 lines

fs/devpts/inode.c fail: · line 188

Quoted 6 lines

fs/devpts/inode.c +6 -0

+	if (err) {
+		up_write(&sb->s_umount);
+		deactivate_super(sb);
+		return err;
+	}
+

That stuff becomes very very similar to that in proc :) Makes sense to consolidate. Maybe...

Quoted 2 lines

Yeah, and the mqns that Cedric sent too. I think Cedric said he'd started an a patch implementing a helper. Cedric?

Mmm. I wanted to send one small objection to Cedric's patches with mqns, but the thread was abandoned by the time I decided to do-it-right-now.

So I can put it here: forcing the CLONE_NEWNS is not very good, since this makes impossible to push a bind mount inside a new namespace, which may operate in some chroot environment. But this ability is heavily

exploited in OpenVZ, so if we can somehow avoid forcing the NEWNS flag that would be very very good :) See my next comment about this issue.

Quoted 3 lines

Pavel, not long ago you said you were starting to look at tty and pty stuff - did you have any different ideas on devpts virtualization, or are you ok with this minus your comments thus far?

I have a similar idea of how to implement this, but I didn't thought about the details. As far as this issue is concerned, I see no reasons why we need a kern_mount-ed devtpsfs instance. If we don't make such, we may safely hold the ptsns from the superblock and be happy. The same seems applicable to the mqns, no?

The reason I have the kern_mount-ed instance of proc for pid namespaces is that I need a vfsmount to flush task entries from, but allowing it to be NULL (i.e. no kern_mount, but optional user mounts) means handing all the possible races, which is too heavy. But do we actually need the vfsmount for devpts and mqns if no user-space mounts exist?

Besides, I planned to include legacy ptys virtualization and console virtualizatin in this namespace, but it seems, that it is not present in this particular one.

Quoted 155 lines

Quoted 154 lines

Quoted 145 lines

fs/devpts/inode.c +56 -16

+	sb->s_flags |= MS_ACTIVE;
+	devpts_mnt = mnt;
+
+	return simple_set_mnt(mnt, sb);
 }
 
 static struct file_system_type devpts_fs_type = {

@@ -158,10 +204,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));
 }

@@ -207,12 +252,28 @@ int devpts_pty_new(struct tty_struct *tt
 	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 dentry *root;
+	struct vfsmount *mnt;
+	struct inode *inode;
+
 
 	/* We're supposed to be given the slave end of a pty */
 	BUG_ON(driver->type != TTY_DRIVER_TYPE_PTY);
 	BUG_ON(driver->subtype != PTY_TYPE_SLAVE);
 
+	mnt = current_pts_ns_mnt();
+	if (!mnt)
+		return -ENOSYS;
+	root = mnt->mnt_root;
+
+	mutex_lock(&root->d_inode->i_mutex);
+	inode = idr_find(current_pts_ns_allocated_ptys(), number);
+	mutex_unlock(&root->d_inode->i_mutex);
+
+	if (inode && !IS_ERR(inode))
+		return -EEXIST;
+
+	inode = new_inode(mnt->mnt_sb);
 	if (!inode)
 		return -ENOMEM;
 

@@ -222,23 +283,31 @@ int devpts_pty_new(struct tty_struct *tt
 	inode->i_mtime = inode->i_atime = inode->i_ctime = CURRENT_TIME;
 	init_special_inode(inode, S_IFCHR|config.mode, device);
 	inode->i_private = tty;
+	idr_replace(current_pts_ns_allocated_ptys(), inode, number);
 
-	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(int number)
 {
-	struct dentry *dentry = get_node(number);
+	struct vfsmount *mnt;
+	struct dentry *dentry;
 	struct tty_struct *tty;
 
+	mnt = current_pts_ns_mnt();
+	if (!mnt)
+		return NULL;
+
+	dentry = get_node(mnt->mnt_root, number);
+
 	tty = NULL;
 	if (!IS_ERR(dentry)) {
 		if (dentry->d_inode)

@@ -246,14 +315,21 @@ struct tty_struct *devpts_get_tty(int nu
 		dput(dentry);
 	}
 
-	mutex_unlock(&devpts_root->d_inode->i_mutex);
+	mutex_unlock(&mnt->mnt_root->d_inode->i_mutex);
 
 	return tty;
 }
 
 void devpts_pty_kill(int number)
 {
-	struct dentry *dentry = get_node(number);
+	struct dentry *dentry;
+	struct dentry *root;
+	struct vfsmount *mnt;
+
+	mnt = current_pts_ns_mnt();
+	root = mnt->mnt_root;
+
+	dentry = get_node(root, number);
 
 	if (!IS_ERR(dentry)) {
 		struct inode *inode = dentry->d_inode;

@@ -264,17 +340,23 @@ void devpts_pty_kill(int number)
 		}
 		dput(dentry);
 	}
-	mutex_unlock(&devpts_root->d_inode->i_mutex);
+	mutex_unlock(&root->d_inode->i_mutex);
 }
 
 static int __init init_devpts_fs(void)
 {
-	int err = register_filesystem(&devpts_fs_type);
-	if (!err) {
-		devpts_mnt = kern_mount(&devpts_fs_type);
-		if (IS_ERR(devpts_mnt))
-			err = PTR_ERR(devpts_mnt);
-	}
+	struct vfsmount *mnt;
+	int err;
+
+	err = register_filesystem(&devpts_fs_type);
+	if (err)
+		return err;
+
+	mnt = kern_mount_data(&devpts_fs_type, NULL);
+	if (IS_ERR(mnt))
+		err = PTR_ERR(mnt);
+	else
+		devpts_mnt = mnt;
 	return err;
 }
 

_______________________________________________
Containers mailing list Containers-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org
https://lists.linux-foundation.org/mailman/listinfo/containers

Quoted 5 lines

_______________________________________________
Devel mailing list
[email protected]
https://openvz.org/mailman/listinfo/devel

_______________________________________________
Containers mailing list Containers-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org
https://lists.linux-foundation.org/mailman/listinfo/containers

Quoted 6 lines

Mmm. I wanted to send one small objection to Cedric's patches with mqns, but the thread was abandoned by the time I decided to do-it-right-now.

So I can put it here: forcing the CLONE_NEWNS is not very good, since this makes impossible to push a bind mount inside a new namespace, which may operate in some chroot environment. But this ability is heavily

Which direction do you want to go? I'm wondering whether mounts propagation can address it.

Though really, I think you're right - we shouldn't break the kernel doing CLONE_NEWMQ or CLONE_NEWPTS without CLONE_NEWNS, so we shouldn't force the combination.

Quoted 6 lines

exploited in OpenVZ, so if we can somehow avoid forcing the NEWNS flag that would be very very good :) See my next comment about this issue.

Quoted 3 lines

Pavel, not long ago you said you were starting to look at tty and pty stuff - did you have any different ideas on devpts virtualization, or are you ok with this minus your comments thus far?

Quoted 5 lines

I have a similar idea of how to implement this, but I didn't thought about the details. As far as this issue is concerned, I see no reasons why we need a kern_mount-ed devtpsfs instance. If we don't make such, we may safely hold the ptsns from the superblock and be happy. The same seems applicable to the mqns, no?

But the current->nsproxy->devpts->mnt is used in several functions in patch 3.

Quoted 6 lines

The reason I have the kern_mount-ed instance of proc for pid namespaces is that I need a vfsmount to flush task entries from, but allowing it to be NULL (i.e. no kern_mount, but optional user mounts) means handing all the possible races, which is too heavy. But do we actually need the vfsmount for devpts and mqns if no user-space mounts exist?

Quoted 3 lines

Besides, I planned to include legacy ptys virtualization and console virtualizatin in this namespace, but it seems, that it is not present in this particular one.

I had been thinking the consoles would have their own ns, since there's really nothing linking them, but there really is no good reason why userspace should ever want them separate. So I'm fine with combining them.

[snip]

Quoted 7 lines

Quoted 6 lines

Mmm. I wanted to send one small objection to Cedric's patches with mqns, but the thread was abandoned by the time I decided to do-it-right-now.

So I can put it here: forcing the CLONE_NEWNS is not very good, since this makes impossible to push a bind mount inside a new namespace, which may operate in some chroot environment. But this ability is heavily

Quoted 2 lines

Which direction do you want to go? I'm wondering whether mounts propagation can address it.

Hardly. AFAIS there's no way to let the chroot-ed tasks see parts of vfs tree, that left behind them after chroot, unless they are in the same mntns as you, and you bind mount this parts to their tree. No?

Quoted 11 lines

Though really, I think you're right - we shouldn't break the kernel doing CLONE_NEWMQ or CLONE_NEWPTS without CLONE_NEWNS, so we shouldn't force the combination.

Quoted 6 lines

exploited in OpenVZ, so if we can somehow avoid forcing the NEWNS flag that would be very very good :) See my next comment about this issue.

Quoted 3 lines

Pavel, not long ago you said you were starting to look at tty and pty stuff - did you have any different ideas on devpts virtualization, or are you ok with this minus your comments thus far?

Quoted 8 lines

Quoted 5 lines

I have a similar idea of how to implement this, but I didn't thought about the details. As far as this issue is concerned, I see no reasons why we need a kern_mount-ed devtpsfs instance. If we don't make such, we may safely hold the ptsns from the superblock and be happy. The same seems applicable to the mqns, no?

But the current->nsproxy->devpts->mnt is used in several functions in patch 3.

Indeed. I overlooked this. Then we're in a deep ... problem here.

Breaking this circle was not that easy with pid namespaces, so I put the strut in proc_flush_task - when the last task from the namespace exits the kern-mount-ed vfsmnt is dropped, but we can't do the same stuff with devpts.

I do not remember now what the problem was and it's already quite late in Moscow, so if you don't mind I'll revisit the issue tomorrow.

Off-topic: does any of you know whether Andrew is willing to accept new features in the nearest future? The problem is that I have a device visibility controller fixed and pending to send, but I can't guess a good time for it :)

Quoted 6 lines

Quoted 6 lines

The reason I have the kern_mount-ed instance of proc for pid namespaces is that I need a vfsmount to flush task entries from, but allowing it to be NULL (i.e. no kern_mount, but optional user mounts) means handing all the possible races, which is too heavy. But do we actually need the vfsmount for devpts and mqns if no user-space mounts exist?

Quoted 8 lines

Quoted 3 lines

Besides, I planned to include legacy ptys virtualization and console virtualizatin in this namespace, but it seems, that it is not present in this particular one.

I had been thinking the consoles would have their own ns, since there's really nothing linking them, but there really is no good reason why userspace should ever want them separate. So I'm fine with combining them.

OK.

Quoted 5 lines · Pavel Emelyanov

[snip]

Quoted 3 lines

Quoted 3 lines

Mmm. I wanted to send one small objection to Cedric's patches with mqns, but the thread was abandoned by the time I decided to do-it-right-now.

Quoted 8 lines

Quoted 5 lines

Quoted 3 lines

So I can put it here: forcing the CLONE_NEWNS is not very good, since this makes impossible to push a bind mount inside a new namespace, which may operate in some chroot environment. But this ability is heavily

Which direction do you want to go? I'm wondering whether mounts propagation can address it.

Hardly. AFAIS there's no way to let the chroot-ed tasks see parts of vfs tree, that left behind them after chroot, unless they are in the same mntns as you, and you bind mount this parts to their tree. No?

Well no, but I suspect I'm just not understanding what you want to do. But if the chroot is under /jail1, and you've done, say,

	mkdir -p /share/pts
	mkdir -p /jail1/share
	mount --bind /share /share
	mount --make-shared /share
	mount --bind /share /jail1/share
	mount --make-slave /jail1/share

before the chroot-ed tasks were cloned with CLONE_NEWNS, then when you do

	mount --bind /dev/pts /share/pts

from the parent mntns (not that I know why you'd want to do *that* :) then the chroot'ed tasks will see the original mntns's /dev/pts under /jail1/share.

Quoted 15 lines

Quoted 15 lines

Though really, I think you're right - we shouldn't break the kernel doing CLONE_NEWMQ or CLONE_NEWPTS without CLONE_NEWNS, so we shouldn't force the combination.

Quoted 11 lines

exploited in OpenVZ, so if we can somehow avoid forcing the NEWNS flag that would be very very good :) See my next comment about this issue.

Quoted 3 lines

Pavel, not long ago you said you were starting to look at tty and pty stuff - did you have any different ideas on devpts virtualization, or are you ok with this minus your comments thus far?

I have a similar idea of how to implement this, but I didn't thought about the details. As far as this issue is concerned, I see no reasons why we need a kern_mount-ed devtpsfs instance. If we don't make such, we may safely hold the ptsns from the superblock and be happy. The same seems applicable to the mqns, no?

Quoted 8 lines

Quoted 2 lines

But the current->nsproxy->devpts->mnt is used in several functions in patch 3.

Indeed. I overlooked this. Then we're in a deep ... problem here.

Breaking this circle was not that easy with pid namespaces, so I put the strut in proc_flush_task - when the last task from the namespace exits the kern-mount-ed vfsmnt is dropped, but we can't do the same stuff with devpts.

But I still don't see what the problem is with my proposal? So long as you agree that if there are no tasks remaining in the devptsns, then any task which has its devpts mounted should see an empty directory (due to sb->s_info being NULL), I think it works.

Quoted 2 lines

I do not remember now what the problem was and it's already quite late in Moscow, so if you don't mind I'll revisit the issue tomorrow.

Ok, that's fine. I'll let it sit until then too :) Good night.

Quoted 4 lines

Off-topic: does any of you know whether Andrew is willing to accept new features in the nearest future? The problem is that I have a device visibility controller fixed and pending to send, but I can't guess a good time for it :)

Well even if Andrew won't take it I'd like to see it, so I'd appreciate a resend.

Quoted 14 lines

Quoted 6 lines

Quoted 6 lines

The reason I have the kern_mount-ed instance of proc for pid namespaces is that I need a vfsmount to flush task entries from, but allowing it to be NULL (i.e. no kern_mount, but optional user mounts) means handing all the possible races, which is too heavy. But do we actually need the vfsmount for devpts and mqns if no user-space mounts exist?

Quoted 7 lines

Quoted 3 lines

Besides, I planned to include legacy ptys virtualization and console virtualizatin in this namespace, but it seems, that it is not present in this particular one.

I had been thinking the consoles would have their own ns, since there's really nothing linking them, but there really is no good reason why userspace should ever want them separate. So I'm fine with combining them.

OK.

Quoted 5 lines

Quoted 5 lines

Breaking this circle was not that easy with pid namespaces, so I put the strut in proc_flush_task - when the last task from the namespace exits the kern-mount-ed vfsmnt is dropped, but we can't do the same stuff with devpts.

Quoted 4 lines

But I still don't see what the problem is with my proposal? So long as you agree that if there are no tasks remaining in the devptsns, then any task which has its devpts mounted should see an empty directory (due to sb->s_info being NULL), I think it works.

Well, if we _do_ can handle the races with ns->devpts_mnt switch from not NULL to NULL, then I'm fine with this approach.

I just remember, that with pid namespaces this caused a complicated locking and performance degradation. This is the problem I couldn't remember yesterday.

Quoted 25 lines · Serge E. Hallyn

Quoted 5 lines · Pavel Emelyanov

[snip]

Quoted 3 lines

Quoted 3 lines

Mmm. I wanted to send one small objection to Cedric's patches with mqns, but the thread was abandoned by the time I decided to do-it-right-now.

Quoted 8 lines

Quoted 5 lines

Quoted 3 lines

So I can put it here: forcing the CLONE_NEWNS is not very good, since this makes impossible to push a bind mount inside a new namespace, which may operate in some chroot environment. But this ability is heavily

Which direction do you want to go? I'm wondering whether mounts propagation can address it.

Hardly. AFAIS there's no way to let the chroot-ed tasks see parts of vfs tree, that left behind them after chroot, unless they are in the same mntns as you, and you bind mount this parts to their tree. No?

Well no, but I suspect I'm just not understanding what you want to do. But if the chroot is under /jail1, and you've done, say,

	mkdir -p /share/pts
	mkdir -p /jail1/share
	mount --bind /share /share
	mount --make-shared /share
	mount --bind /share /jail1/share
	mount --make-slave /jail1/share

Quoted 8 lines

before the chroot-ed tasks were cloned with CLONE_NEWNS, then when you do

	mount --bind /dev/pts /share/pts

from the parent mntns (not that I know why you'd want to do *that* :) then the chroot'ed tasks will see the original mntns's /dev/pts under /jail1/share.

I haven't yet tried that, but :( this function

	static inline int check_mnt(struct vfsmount *mnt)
	{
	        return mnt->mnt_ns == current->nsproxy->mnt_ns;
	}

and this code in do_loopback

        if (!check_mnt(nd->mnt) || !check_mnt(old_nd.mnt))
                goto out;

makes me think that trying to bind a mount from another mntns ot _to_ another is prohibited... Do I miss something?

Quoted 24 lines

Quoted 15 lines

Quoted 15 lines

Though really, I think you're right - we shouldn't break the kernel doing CLONE_NEWMQ or CLONE_NEWPTS without CLONE_NEWNS, so we shouldn't force the combination.

Quoted 11 lines

exploited in OpenVZ, so if we can somehow avoid forcing the NEWNS flag that would be very very good :) See my next comment about this issue.

Quoted 3 lines

Pavel, not long ago you said you were starting to look at tty and pty stuff - did you have any different ideas on devpts virtualization, or are you ok with this minus your comments thus far?

I have a similar idea of how to implement this, but I didn't thought about the details. As far as this issue is concerned, I see no reasons why we need a kern_mount-ed devtpsfs instance. If we don't make such, we may safely hold the ptsns from the superblock and be happy. The same seems applicable to the mqns, no?

Quoted 8 lines

Quoted 2 lines

But the current->nsproxy->devpts->mnt is used in several functions in patch 3.

Indeed. I overlooked this. Then we're in a deep ... problem here.

Breaking this circle was not that easy with pid namespaces, so I put the strut in proc_flush_task - when the last task from the namespace exits the kern-mount-ed vfsmnt is dropped, but we can't do the same stuff with devpts.

Quoted 4 lines

But I still don't see what the problem is with my proposal? So long as you agree that if there are no tasks remaining in the devptsns, then any task which has its devpts mounted should see an empty directory (due to sb->s_info being NULL), I think it works.

Well, if we _do_ can handle the races with ns->devpts_mnt switch from not NULL to NULL, then I'm fine with this approach.

I just remember, that with pid namespaces this caused a complicated locking and performance degradation. This is the problem I couldn't remember yesterday.

Quoted 28 lines

Quoted 2 lines

I do not remember now what the problem was and it's already quite late in Moscow, so if you don't mind I'll revisit the issue tomorrow.

Ok, that's fine. I'll let it sit until then too :) Good night.

Quoted 4 lines

Off-topic: does any of you know whether Andrew is willing to accept new features in the nearest future? The problem is that I have a device visibility controller fixed and pending to send, but I can't guess a good time for it :)

Well even if Andrew won't take it I'd like to see it, so I'd appreciate a resend.

Quoted 14 lines

Quoted 6 lines

Quoted 6 lines

The reason I have the kern_mount-ed instance of proc for pid namespaces is that I need a vfsmount to flush task entries from, but allowing it to be NULL (i.e. no kern_mount, but optional user mounts) means handing all the possible races, which is too heavy. But do we actually need the vfsmount for devpts and mqns if no user-space mounts exist?

Quoted 7 lines

Quoted 3 lines

Besides, I planned to include legacy ptys virtualization and console virtualizatin in this namespace, but it seems, that it is not present in this particular one.

I had been thinking the consoles would have their own ns, since there's really nothing linking them, but there really is no good reason why userspace should ever want them separate. So I'm fine with combining them.

OK.

Pavel Emelianov [[email protected]] wrote:

| Serge E. Hallyn wrote:
| > Quoting Pavel Emelyanov ([email protected]):
| >> [snip]
| >>
| >>>> Mmm. I wanted to send one small objection to Cedric's patches with mqns,
| >>>> but the thread was abandoned by the time I decided to do-it-right-now.
| >>>>
| >>>> So I can put it here: forcing the CLONE_NEWNS is not very good, since
| >>>> this makes impossible to push a bind mount inside a new namespace, which
| >>>> may operate in some chroot environment. But this ability is heavily
| >>> Which direction do you want to go?  I'm wondering whether mounts
| >>> propagation can address it.
| >> Hardly. AFAIS there's no way to let the chroot-ed tasks see parts of
| >> vfs tree, that left behind them after chroot, unless they are in the 
| >> same mntns as you, and you bind mount this parts to their tree. No?
| > 
| > Well no, but I suspect I'm just not understanding what you want to do.
| > But if the chroot is under /jail1, and you've done, say,
| > 
| > 	mkdir -p /share/pts
| > 	mkdir -p /jail1/share
| > 	mount --bind /share /share
| > 	mount --make-shared /share
| > 	mount --bind /share /jail1/share
| > 	mount --make-slave /jail1/share
| > 
| > before the chroot-ed tasks were cloned with CLONE_NEWNS, then when you
| > do
| > 
| > 	mount --bind /dev/pts /share/pts
| > 
| > from the parent mntns (not that I know why you'd want to do *that* :)
| > then the chroot'ed tasks will see the original mntns's /dev/pts under
| > /jail1/share.
| 
| I haven't yet tried that, but :( this function
| 
| 	static inline int check_mnt(struct vfsmount *mnt)
| 	{
| 	        return mnt->mnt_ns == current->nsproxy->mnt_ns;
| 	}
| 
| and this code in do_loopback
| 
|         if (!check_mnt(nd->mnt) || !check_mnt(old_nd.mnt))
|                 goto out;
| 
| makes me think that trying to bind a mount from another mntns
| ot _to_ another is prohibited... Do I miss something?
| 
| >>> Though really, I think you're right - we shouldn't break the kernel
| >>> doing CLONE_NEWMQ or CLONE_NEWPTS without CLONE_NEWNS, so we shouldn't
| >>> force the combination.
| >>>
| >>>> exploited in OpenVZ, so if we can somehow avoid forcing the NEWNS flag
| >>>> that would be very very good :) See my next comment about this issue.
| >>>>
| >>>>> Pavel, not long ago you said you were starting to look at tty and pty
| >>>>> stuff - did you have any different ideas on devpts virtualization, or
| >>>>> are you ok with this minus your comments thus far?
| >>>> I have a similar idea of how to implement this, but I didn't thought
| >>>> about the details. As far as this issue is concerned, I see no reasons
| >>>> why we need a kern_mount-ed devtpsfs instance. If we don't make such,
| >>>> we may safely hold the ptsns from the superblock and be happy. The
| >>>> same seems applicable to the mqns, no?
| >>> But the current->nsproxy->devpts->mnt is used in several functions in
| >>> patch 3.
| >> Indeed. I overlooked this. Then we're in a deep ... problem here.
| >>
| >> Breaking this circle was not that easy with pid namespaces, so
| >> I put the strut in proc_flush_task - when the last task from the
| >> namespace exits the kern-mount-ed vfsmnt is dropped, but we can't
| >> do the same stuff with devpts.
| > 
| > But I still don't see what the problem is with my proposal?  So long as
| > you agree that if there are no tasks remaining in the devptsns,
| > then any task which has its devpts mounted should see an empty directory
| > (due to sb->s_info being NULL), I think it works.
| 
| Well, if we _do_ can handle the races with ns->devpts_mnt switch
| from not NULL to NULL, then I'm fine with this approach.
| 
| I just remember, that with pid namespaces this caused a complicated
| locking and performance degradation. This is the problem I couldn't
| remember yesterday.

Well, iirc, one problem with pid namespaces was that we need to keep the task and pid_namespace association until the task was waited on (for instance the wait() call from parent needs the pid_t of the child which is tied to the pid ns in struct upid).

For this reason, we don't drop the mnt reference in free_pid_ns() but hold the reference till proc_flush_task().

With devpts, can't we simply drop the reference in free_pts_ns() so that when the last task using the pts_ns exits, we can unmount and release the mnt ?

IOW, do you suspect that the circular reference leads to leaking vfsmnts ?

Quoted 4 lines

Off-topic: does any of you know whether Andrew is willing to accept new features in the nearest future? The problem is that I have a device visibility controller fixed and pending to send, but I can't guess a good time for it :)

Well even if Andrew won't take it I'd like to see it, so I'd appreciate a resend.

Quoted 4 lines

Quoted 4 lines

Off-topic: does any of you know whether Andrew is willing to accept new features in the nearest future? The problem is that I have a device visibility controller fixed and pending to send, but I can't guess a good time for it :)

I have the clone64/unshare64 syscalls ready for most common arches :

	x86, x86_64, x86_64(32), ppc64, ppc64(32), s390x, s390x(31)

do you care to review or shall I send directly to andrew ?

There's a freezer patchset also that I need to resend ...

C.

Quoted 2 lines

Well even if Andrew won't take it I'd like to see it, so I'd appreciate a resend.

Quoted 5 lines · Pavel Emelyanov

Quoted 4 lines · Serge E. Hallyn

Quoted 3 lines · Pavel Emelyanov

Quoted 2 lines · sukadev-r/[email protected] wrote:

From: Sukadev Bhattiprolu <sukadev-r/[email protected]>
Subject: [RFC][PATCH 3/4]: Enable multiple mounts of /dev/pts

Quoted 10 lines

Quoted 4 lines

Quoted 2 lines

That stuff becomes very very similar to that in proc :) Makes sense to consolidate. Maybe...

Yeah, and the mqns that Cedric sent too. I think Cedric said he'd started an a patch implementing a helper. Cedric?

Mmm. I wanted to send one small objection to Cedric's patches with mqns, but the thread was abandoned by the time I decided to do-it-right-now.

So I can put it here: forcing the CLONE_NEWNS is not very good, since this makes impossible to push a bind mount inside a new namespace, which may operate in some chroot environment. But this ability is heavily

Which direction do you want to go? I'm wondering whether mounts propagation can address it.

Though really, I think you're right - we shouldn't break the kernel doing CLONE_NEWMQ or CLONE_NEWPTS without CLONE_NEWNS, so we shouldn't force the combination.

Quoted 11 lines

exploited in OpenVZ, so if we can somehow avoid forcing the NEWNS flag that would be very very good :) See my next comment about this issue.

Quoted 3 lines

Pavel, not long ago you said you were starting to look at tty and pty stuff - did you have any different ideas on devpts virtualization, or are you ok with this minus your comments thus far?

I have a similar idea of how to implement this, but I didn't thought about the details. As far as this issue is concerned, I see no reasons why we need a kern_mount-ed devtpsfs instance. If we don't make such, we may safely hold the ptsns from the superblock and be happy. The same seems applicable to the mqns, no?

But the current->nsproxy->devpts->mnt is used in several functions in patch 3.

Quoted 6 lines

The reason I have the kern_mount-ed instance of proc for pid namespaces is that I need a vfsmount to flush task entries from, but allowing it to be NULL (i.e. no kern_mount, but optional user mounts) means handing all the possible races, which is too heavy. But do we actually need the vfsmount for devpts and mqns if no user-space mounts exist?

Quoted 3 lines

Besides, I planned to include legacy ptys virtualization and console virtualizatin in this namespace, but it seems, that it is not present in this particular one.

I had been thinking the consoles would have their own ns, since there's really nothing linking them, but there really is no good reason why userspace should ever want them separate. So I'm fine with combining them.

Quoted 6 lines · Serge E. Hallyn

Quoted 5 lines · Pavel Emelyanov

Quoted 4 lines · Serge E. Hallyn

Quoted 3 lines · Pavel Emelyanov

Quoted 2 lines · sukadev-r/[email protected] wrote:

From: Sukadev Bhattiprolu <sukadev-r/[email protected]>
Subject: [RFC][PATCH 3/4]: Enable multiple mounts of /dev/pts

[SNIP]

Quoted 37 lines

Quoted 10 lines

Quoted 4 lines

Quoted 2 lines

That stuff becomes very very similar to that in proc :) Makes sense to consolidate. Maybe...

Yeah, and the mqns that Cedric sent too. I think Cedric said he'd started an a patch implementing a helper. Cedric?

Mmm. I wanted to send one small objection to Cedric's patches with mqns, but the thread was abandoned by the time I decided to do-it-right-now.

So I can put it here: forcing the CLONE_NEWNS is not very good, since this makes impossible to push a bind mount inside a new namespace, which may operate in some chroot environment. But this ability is heavily

Which direction do you want to go? I'm wondering whether mounts propagation can address it.
Though really, I think you're right - we shouldn't break the kernel doing CLONE_NEWMQ or CLONE_NEWPTS without CLONE_NEWNS, so we shouldn't force the combination.

Quoted 11 lines

exploited in OpenVZ, so if we can somehow avoid forcing the NEWNS flag that would be very very good :) See my next comment about this issue.

Quoted 3 lines

Pavel, not long ago you said you were starting to look at tty and pty stuff - did you have any different ideas on devpts virtualization, or are you ok with this minus your comments thus far?

I have a similar idea of how to implement this, but I didn't thought about the details. As far as this issue is concerned, I see no reasons why we need a kern_mount-ed devtpsfs instance. If we don't make such, we may safely hold the ptsns from the superblock and be happy. The same seems applicable to the mqns, no?

But the current->nsproxy->devpts->mnt is used in several functions in patch 3.

Quoted 9 lines

The reason I have the kern_mount-ed instance of proc for pid namespaces is that I need a vfsmount to flush task entries from, but allowing it to be NULL (i.e. no kern_mount, but optional user mounts) means handing all the possible races, which is too heavy. But do we actually need the vfsmount for devpts and mqns if no user-space mounts exist?

Besides, I planned to include legacy ptys virtualization and console virtualizatin in this namespace, but it seems, that it is not present in this particular one.

Quoted 4 lines

I had been thinking the consoles would have their own ns, since there's really nothing linking them, but there really is no good reason why userspace should ever want them separate. So I'm fine with combining them.

If you want to run something like an X server inside each container (eg each container holds a desktop session of a different user), then you need a separate virtual-console namespace for each container.

(yes, X per-se needs to provide remote display as opposed to use local hardware; see http://www.ncl.cs.columbia.edu/research/thinc/)

Quoted 51 lines · Oren Laadan

Quoted 6 lines · Serge E. Hallyn

Quoted 5 lines · Pavel Emelyanov

Quoted 4 lines · Serge E. Hallyn

Quoted 3 lines · Pavel Emelyanov

Quoted 2 lines · sukadev-r/[email protected] wrote:

From: Sukadev Bhattiprolu <sukadev-r/[email protected]>
Subject: [RFC][PATCH 3/4]: Enable multiple mounts of /dev/pts

[SNIP]

Quoted 41 lines

Quoted 10 lines

Quoted 4 lines

Quoted 2 lines

That stuff becomes very very similar to that in proc :) Makes sense to consolidate. Maybe...

Yeah, and the mqns that Cedric sent too. I think Cedric said he'd started an a patch implementing a helper. Cedric?

Mmm. I wanted to send one small objection to Cedric's patches with mqns, but the thread was abandoned by the time I decided to do-it-right-now.

So I can put it here: forcing the CLONE_NEWNS is not very good, since this makes impossible to push a bind mount inside a new namespace, which may operate in some chroot environment. But this ability is heavily

Which direction do you want to go? I'm wondering whether mounts propagation can address it.
Though really, I think you're right - we shouldn't break the kernel doing CLONE_NEWMQ or CLONE_NEWPTS without CLONE_NEWNS, so we shouldn't force the combination.

Quoted 11 lines

exploited in OpenVZ, so if we can somehow avoid forcing the NEWNS flag that would be very very good :) See my next comment about this issue.

Quoted 3 lines

Pavel, not long ago you said you were starting to look at tty and pty stuff - did you have any different ideas on devpts virtualization, or are you ok with this minus your comments thus far?

I have a similar idea of how to implement this, but I didn't thought about the details. As far as this issue is concerned, I see no reasons why we need a kern_mount-ed devtpsfs instance. If we don't make such, we may safely hold the ptsns from the superblock and be happy. The same seems applicable to the mqns, no?

But the current->nsproxy->devpts->mnt is used in several functions in patch 3.

Quoted 9 lines

The reason I have the kern_mount-ed instance of proc for pid namespaces is that I need a vfsmount to flush task entries from, but allowing it to be NULL (i.e. no kern_mount, but optional user mounts) means handing all the possible races, which is too heavy. But do we actually need the vfsmount for devpts and mqns if no user-space mounts exist?

Besides, I planned to include legacy ptys virtualization and console virtualizatin in this namespace, but it seems, that it is not present in this particular one.

I had been thinking the consoles would have their own ns, since there's really nothing linking them, but there really is no good reason why userspace should ever want them separate. So I'm fine with combining them.

Quoted 3 lines

If you want to run something like an X server inside each container (eg each container holds a desktop session of a different user), then you need a separate virtual-console namespace for each container.

Ok, but whether the consoles and devpts are unshared with the same cloneflag or not isn't an issue, right?

Quoted 3 lines

(yes, X per-se needs to provide remote display as opposed to use local hardware; see http://www.ncl.cs.columbia.edu/research/thinc/)

-serge

Quoted 57 lines · Serge E. Hallyn

Quoted 56 lines · Oren Laadan

Quoted 6 lines · Serge E. Hallyn

Quoted 5 lines · Pavel Emelyanov

Quoted 4 lines · Serge E. Hallyn

Quoted 3 lines · Pavel Emelyanov

Quoted 2 lines · sukadev-r/[email protected] wrote:

From: Sukadev Bhattiprolu <sukadev-r/[email protected]>
Subject: [RFC][PATCH 3/4]: Enable multiple mounts of /dev/pts

[SNIP]

Quoted 36 lines

Quoted 4 lines

Quoted 4 lines

Quoted 2 lines

That stuff becomes very very similar to that in proc :) Makes sense to consolidate. Maybe...

Yeah, and the mqns that Cedric sent too. I think Cedric said he'd started an a patch implementing a helper. Cedric?

Quoted 8 lines

Mmm. I wanted to send one small objection to Cedric's patches with mqns,
but the thread was abandoned by the time I decided to do-it-right-now.

So I can put it here: forcing the CLONE_NEWNS is not very good, since this makes impossible to push a bind mount inside a new namespace, which
may operate in some chroot environment. But this ability is heavily

Which direction do you want to go? I'm wondering whether mounts propagation can address it.
Though really, I think you're right - we shouldn't break the kernel doing CLONE_NEWMQ or CLONE_NEWPTS without CLONE_NEWNS, so we shouldn't force the combination.

Quoted 11 lines

exploited in OpenVZ, so if we can somehow avoid forcing the NEWNS flag that would be very very good :) See my next comment about this issue.

Quoted 3 lines

Pavel, not long ago you said you were starting to look at tty and pty stuff - did you have any different ideas on devpts virtualization, or are you ok with this minus your comments thus far?

I have a similar idea of how to implement this, but I didn't thought about the details. As far as this issue is concerned, I see no reasons why we need a kern_mount-ed devtpsfs instance. If we don't make such, we may safely hold the ptsns from the superblock and be happy. The same seems applicable to the mqns, no?

But the current->nsproxy->devpts->mnt is used in several functions in patch 3.

Quoted 6 lines

The reason I have the kern_mount-ed instance of proc for pid namespaces is that I need a vfsmount to flush task entries from, but allowing it to be NULL (i.e. no kern_mount, but optional user mounts) means handing all the possible races, which is too heavy. But do we actually need the vfsmount for devpts and mqns if no user-space mounts exist?

Quoted 7 lines

Quoted 3 lines

Besides, I planned to include legacy ptys virtualization and console virtualizatin in this namespace, but it seems, that it is not present in this particular one.

I had been thinking the consoles would have their own ns, since there's really nothing linking them, but there really is no good reason why userspace should ever want them separate. So I'm fine with combining them.

If you want to run something like an X server inside each container (eg each container holds a desktop session of a different user), then you need a separate virtual-console namespace for each container.

Quoted 2 lines

Ok, but whether the consoles and devpts are unshared with the same cloneflag or not isn't an issue, right?

true. (I misread your comment.)
(
modulo that we are additional-clone-flags-challenged ...)

Quoted 2 lines

Quoted 2 lines

(yes, X per-se needs to provide remote display as opposed to use local hardware; see http://www.ncl.cs.columbia.edu/research/thinc/)

Quoted 1 line

-serge

Quoted 4 lines · Oren Laadan

Quoted 2 lines · Serge E. Hallyn

Quoted 1 line · Oren Laadan

Serge E. Hallyn wrote:

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Quoted 6 lines · Pavel Emelyanov

Quoted 5 lines

Quoted 4 lines · Serge E. Hallyn

Quoted 3 lines · Pavel Emelyanov

Quoted 2 lines · sukadev-r/[email protected] wrote:

From: Sukadev Bhattiprolu <sukadev-r/[email protected]>
Subject: [RFC][PATCH 3/4]: Enable multiple mounts of /dev/pts

[SNIP]

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

Quoted 3 lines

Besides, I planned to include legacy ptys virtualization and console virtualizatin in this namespace, but it seems, that it is not present in this particular one.

I had been thinking the consoles would have their own ns, since there's really nothing linking them, but there really is no good reason why userspace should ever want them separate. So I'm fine with combining them.

Quoted 8 lines

Quoted 5 lines

Quoted 3 lines

If you want to run something like an X server inside each container (eg each container holds a desktop session of a different user), then you need a separate virtual-console namespace for each container.

Ok, but whether the consoles and devpts are unshared with the same cloneflag or not isn't an issue, right?

true. (I misread your comment.)
(
modulo that we are additional-clone-flags-challenged ...)

Right, plus the fact that the number of clone flags involved becomes almost obscene. Let's see if Pavel and Suka have a preference, since one of them seems likely to end up coding it :)

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

(yes, X per-se needs to provide remote display as opposed to use local hardware; see http://www.ncl.cs.columbia.edu/research/thinc/)

Nice, by the way :)

Quoted 1 line

Quoted 1 line

-serge

thanks,
-serge

Quoted 87 lines · Pavel Emelyanov

Quoted 86 lines · sukadev-r/[email protected] wrote:

From: Sukadev Bhattiprolu <sukadev-r/[email protected]>
Subject: [RFC][PATCH 3/4]: Enable multiple mounts of /dev/pts

To support multiple PTY namespaces, we should be allow multiple mounts of /dev/pts, once within each PTY namespace.

This patch removes the get_sb_single() in devpts_get_sb() and uses test and set sb interfaces to allow remounting /dev/pts. The patch also removes the globals, 'devpts_root' and uses current_pts_mnt() to access 'devpts_mnt'

Changelog:
- Version 0: Based on earlier versions from Serge Hallyn and Matt Helsley.

Signed-off-by Sukadev Bhattiprolu <sukadev-r/[email protected]>

---
 fs/devpts/inode.c |  120 +++++++++++++++++++++++++++++++++++++++++++++---------
 1 file changed, 101 insertions(+), 19 deletions(-)

Index: linux-2.6.24/fs/devpts/inode.c
===================================================================

fs/devpts/inode.c +39 -3

@@ -34,7 +34,10 @@ static inline struct idr *current_pts_ns
 }
 
 static struct vfsmount *devpts_mnt;
-static struct dentry *devpts_root;
+static inline struct vfsmount *current_pts_ns_mnt(void)
+{
+	return devpts_mnt;
+}
 
 static struct {
 	int setuid;

@@ -130,7 +133,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;
 

@@ -140,10 +143,53 @@ fail:
 	return -ENOMEM;
 }
 
+/*
+ * We use test and set super-block operations to help determine whether we
+ * need a new super-block for this namespace. get_sb() walks the list of
+ * existing devpts supers, comparing them with the @data ptr. Since we
+ * passed 'current's namespace as the @data pointer we can compare the
+ * namespace pointer in the super-block's 's_fs_info'.  If the test is
+ * TRUE then get_sb() returns a new active reference to the super block.
+ * Otherwise, it helps us build an active reference to a new one.
+ */
+
+static int devpts_test_sb(struct super_block *sb, void *data)
+{
+	return sb->s_fs_info == data;
+}
+
+static int devpts_set_sb(struct super_block *sb, void *data)
+{
+	sb->s_fs_info = data;
+	return set_anon_super(sb, NULL);
+}
+
 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);
+	struct super_block *sb;
+	int err;
+
+	/* hereafter we're very simlar to get_sb_nodev */
+	sb = sget(fs_type, devpts_test_sb, devpts_set_sb, data);
+	if (IS_ERR(sb))
+		return PTR_ERR(sb);
+
+	if (sb->s_root)
+		return simple_set_mnt(mnt, sb);
+
+	sb->s_flags = flags;
+	err = devpts_fill_super(sb, data, flags & MS_SILENT ? 1 : 0);

Quoted 8 lines

fs/devpts/inode.c fail: · line 188

Quoted 6 lines

fs/devpts/inode.c +6 -0

+	if (err) {
+		up_write(&sb->s_umount);
+		deactivate_super(sb);
+		return err;
+	}
+

That stuff becomes very very similar to that in proc :) Makes sense to consolidate. Maybe...

Yeah, and the mqns that Cedric sent too. I think Cedric said he'd started an a patch implementing a helper. Cedric?

From: Sukadev Bhattiprolu <sukadev-r/[email protected]>
Subject: [RFC][PATCH 4/4]: Enable cloning PTY namespaces

Enable cloning PTY namespaces.

TODO:
This version temporarily uses the clone flag '0x80000000' which is unused in mainline atm, but used for CLONE_IO in -mm. While we must extend clone() (urgently) to solve this, it hopefully does not affect review of the rest of this patchset.

Changelog:
- Version 0: Based on earlier versions from Serge Hallyn and Matt Helsley.

Signed-off-by Sukadev Bhattiprolu <sukadev-r/[email protected]>

---
 fs/devpts/inode.c         |   84 +++++++++++++++++++++++++++++++++++++++-------
 include/linux/devpts_fs.h |   52 ++++++++++++++++++++++++++++
 include/linux/init_task.h |    1 
 include/linux/nsproxy.h   |    2 +
 include/linux/sched.h     |    2 +
 kernel/fork.c             |    2 -
 kernel/nsproxy.c          |   17 ++++++++-
 7 files changed, 146 insertions(+), 14 deletions(-)

Index: linux-2.6.24/fs/devpts/inode.c
===================================================================

fs/devpts/inode.c +11 -4

@@ -25,18 +25,25 @@
 #define DEVPTS_SUPER_MAGIC 0x1cd1
 
 extern int pty_limit;		/* Config limit on Unix98 ptys */
-static DEFINE_IDR(allocated_ptys);
 static DECLARE_MUTEX(allocated_ptys_lock);
+static struct file_system_type devpts_fs_type;
+
+struct pts_namespace init_pts_ns = {
+	.kref = {
+		.refcount = ATOMIC_INIT(2),
+	},
+	.allocated_ptys = IDR_INIT(init_pts_ns.allocated_ptys),
+	.mnt = NULL,
+};
 
 static inline struct idr *current_pts_ns_allocated_ptys(void)
 {
-	return &allocated_ptys;
+	return &current->nsproxy->pts_ns->allocated_ptys;
 }
 
-static struct vfsmount *devpts_mnt;
 static inline struct vfsmount *current_pts_ns_mnt(void)
 {
-	return devpts_mnt;
+	return current->nsproxy->pts_ns->mnt;
 }
 
 static struct {

@@ -59,6 +66,42 @@ static match_table_t tokens = {
 	{Opt_err, NULL}
 };

fs/devpts/inode.c +53 -7

+	if (IS_ERR(ns->mnt)) {
+		kfree(ns);
+		return ERR_PTR(PTR_ERR(ns->mnt));
+	}
+
+	idr_init(&ns->allocated_ptys);
+	kref_init(&ns->kref);
+
+	return ns;
+}
+
+void free_pts_ns(struct kref *ns_kref)
+{
+	struct pts_namespace *ns;
+
+	ns = container_of(ns_kref, struct pts_namespace, kref);
+	BUG_ON(ns == &init_pts_ns);
+
+	mntput(ns->mnt);
+	/*
+	 * TODO:
+	 *      idr_remove_all(&ns->allocated_ptys); introduced in 2.6.23
+	 */
+	idr_destroy(&ns->allocated_ptys);
+	kfree(ns);
+}
+
 static int devpts_remount(struct super_block *sb, int *flags, char *data)
 {
 	char *p;

@@ -160,18 +203,27 @@ static int devpts_test_sb(struct super_b
 
 static int devpts_set_sb(struct super_block *sb, void *data)
 {
-	sb->s_fs_info = data;
+	struct pts_namespace *ns = data;
+
+	sb->s_fs_info = get_pts_ns(ns);
 	return set_anon_super(sb, NULL);
 }
 
 static int devpts_get_sb(struct file_system_type *fs_type,
 	int flags, const char *dev_name, void *data, struct vfsmount *mnt)
 {
+	struct pts_namespace *ns;
 	struct super_block *sb;
 	int err;
 
+	/* hereafter we're very similar to proc_get_sb */
+	if (flags & MS_KERNMOUNT)
+		ns = data;
+	else
+		ns = current->nsproxy->pts_ns;
+
 	/* hereafter we're very simlar to get_sb_nodev */
-	sb = sget(fs_type, devpts_test_sb, devpts_set_sb, data);
+	sb = sget(fs_type, devpts_test_sb, devpts_set_sb, ns);
 	if (IS_ERR(sb))
 		return PTR_ERR(sb);
 

@@ -187,16 +239,25 @@ static int devpts_get_sb(struct file_sys
 	}
 
 	sb->s_flags |= MS_ACTIVE;
-	devpts_mnt = mnt;
+	ns->mnt = mnt;
 
 	return simple_set_mnt(mnt, sb);
 }
 
+static void devpts_kill_sb(struct super_block *sb)
+{
+       struct pts_namespace *ns;
+
+       ns = sb->s_fs_info;
+       kill_anon_super(sb);
+       put_pts_ns(ns);
+}
+
 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,
 };
 
 /*

@@ -352,18 +413,19 @@ static int __init init_devpts_fs(void)
 	if (err)
 		return err;
 
-	mnt = kern_mount_data(&devpts_fs_type, NULL);
+	mnt = kern_mount_data(&devpts_fs_type, &init_pts_ns);
 	if (IS_ERR(mnt))
 		err = PTR_ERR(mnt);
 	else
-		devpts_mnt = mnt;
+		init_pts_ns.mnt = mnt;
 	return err;
 }
 
 static void __exit exit_devpts_fs(void)
 {
 	unregister_filesystem(&devpts_fs_type);
-	mntput(devpts_mnt);
+	mntput(init_pts_ns.mnt);
+	init_pts_ns.mnt = NULL;
 }
 
 module_init(init_devpts_fs)

Index: linux-2.6.24/include/linux/devpts_fs.h
===================================================================

include/linux/devpts_fs.h +52 -0

@@ -14,9 +14,45 @@
 #define _LINUX_DEVPTS_FS_H
 
 #include <linux/errno.h>
+#include <linux/nsproxy.h>
+#include <linux/kref.h>
+#include <linux/idr.h>
+
+struct pts_namespace {
+	struct kref kref;
+	struct idr allocated_ptys;
+	struct vfsmount *mnt;
+};
+
+extern struct pts_namespace init_pts_ns;
 
 #ifdef CONFIG_UNIX98_PTYS
 
+extern struct pts_namespace *new_pts_ns(void);
+extern void free_pts_ns(struct kref *kref);
+
+static inline struct pts_namespace *get_pts_ns(struct pts_namespace *ns)
+{
+	if (ns)
+		kref_get(&ns->kref);
+	return ns;
+}
+
+static inline void put_pts_ns(struct pts_namespace *ns)
+{
+	if (ns)
+		kref_put(&ns->kref, free_pts_ns);
+}
+
+static inline struct pts_namespace *copy_pts_ns(unsigned long flags,
+		struct pts_namespace *old_ns)
+{
+	if (flags & CLONE_NEWPTS)
+		return new_pts_ns();
+	else
+		return get_pts_ns(old_ns);
+}
+
 int devpts_new_index(void);
 void devpts_kill_index(int idx);
 int devpts_pty_new(struct tty_struct *tty);      /* mknod in devpts */

@@ -26,6 +62,22 @@ void devpts_pty_kill(int number);		 /* u
 #else
 
 /* Dummy stubs in the no-pty case */
+
+static inline struct pts_namespace *get_pts_ns(struct pts_namespace *ns)
+{
+	return &init_pts_ns;
+}
+
+static inline void put_pts_ns(struct pts_namespace *ns) { }
+
+static inline struct pts_namespace *copy_pts_ns(unsigned long flags,
+		struct pts_namespace *old_ns)
+{
+	if (flags & CLONE_NEWPTS)
+		return ERR_PTR(-EINVAL);
+	return old_ns;
+}
+
 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; }

Index: linux-2.6.24/include/linux/init_task.h
===================================================================

include/linux/init_task.h +1 -0

@@ -77,6 +77,7 @@ extern struct nsproxy init_nsproxy;
 	.mnt_ns		= NULL,						\
 	INIT_NET_NS(net_ns)                                             \
 	INIT_IPC_NS(ipc_ns)						\
+	.pts_ns		= &init_pts_ns,					\
 	.user_ns	= &init_user_ns,				\
 }
 

Index: linux-2.6.24/include/linux/nsproxy.h
===================================================================

include/linux/nsproxy.h +2 -0

@@ -8,6 +8,7 @@ struct mnt_namespace;
 struct uts_namespace;
 struct ipc_namespace;
 struct pid_namespace;
+struct pts_namespace;
 
 /*
  * A structure to contain pointers to all per-process

@@ -29,6 +30,7 @@ struct nsproxy {
 	struct pid_namespace *pid_ns;
 	struct user_namespace *user_ns;
 	struct net 	     *net_ns;
+	struct pts_namespace *pts_ns;
 };
 extern struct nsproxy init_nsproxy;
 

Index: linux-2.6.24/include/linux/sched.h
===================================================================

include/linux/sched.h +0 -0

 /*
  * Scheduling policies

Index: linux-2.6.24/kernel/fork.c
===================================================================

kernel/fork.c +1 -1

@@ -1655,7 +1655,7 @@ asmlinkage long sys_unshare(unsigned lon
 	if (unshare_flags & ~(CLONE_THREAD|CLONE_FS|CLONE_NEWNS|CLONE_SIGHAND|
 				CLONE_VM|CLONE_FILES|CLONE_SYSVSEM|
 				CLONE_NEWUTS|CLONE_NEWIPC|CLONE_NEWUSER|
-				CLONE_NEWNET))
+				CLONE_NEWNET|CLONE_NEWPTS))
 		goto bad_unshare_out;
 
 	if ((err = unshare_thread(unshare_flags)))

Index: linux-2.6.24/kernel/nsproxy.c
===================================================================

kernel/nsproxy.c +15 -2

@@ -21,6 +21,7 @@
 #include <linux/utsname.h>
 #include <linux/pid_namespace.h>
 #include <net/net_namespace.h>
+#include <linux/devpts_fs.h>
 
 static struct kmem_cache *nsproxy_cachep;
 

@@ -92,8 +93,17 @@ static struct nsproxy *create_new_namesp
 		goto out_net;
 	}
 
+	new_nsp->pts_ns = copy_pts_ns(flags, tsk->nsproxy->pts_ns);
+	if (IS_ERR(new_nsp->pts_ns)) {
+		err = PTR_ERR(new_nsp->pts_ns);
+		goto out_pts;
+	}
+
 	return new_nsp;
 
+out_pts:
+	if (new_nsp->net_ns)
+		put_net(new_nsp->net_ns);
 out_net:
 	if (new_nsp->user_ns)
 		put_user_ns(new_nsp->user_ns);

@@ -130,7 +140,8 @@ int copy_namespaces(unsigned long flags,
 	get_nsproxy(old_ns);
 
 	if (!(flags & (CLONE_NEWNS | CLONE_NEWUTS | CLONE_NEWIPC |
-				CLONE_NEWUSER | CLONE_NEWPID | CLONE_NEWNET)))
+				CLONE_NEWUSER | CLONE_NEWPID | CLONE_NEWNET |
+				CLONE_NEWPTS)))
 		return 0;
 
 	if (!capable(CAP_SYS_ADMIN)) {

@@ -169,6 +180,8 @@ void free_nsproxy(struct nsproxy *ns)
 		put_pid_ns(ns->pid_ns);
 	if (ns->user_ns)
 		put_user_ns(ns->user_ns);
+	if (ns->pts_ns)
+		put_pts_ns(ns->pts_ns);
 	put_net(ns->net_ns);
 	kmem_cache_free(nsproxy_cachep, ns);
 }

@@ -183,7 +196,7 @@ int unshare_nsproxy_namespaces(unsigned 
 	int err = 0;
 
 	if (!(unshare_flags & (CLONE_NEWNS | CLONE_NEWUTS | CLONE_NEWIPC |
-			       CLONE_NEWUSER | CLONE_NEWNET)))
+			       CLONE_NEWUSER | CLONE_NEWNET | CLONE_NEWPTS)))
 		return 0;
 
 	if (!capable(CAP_SYS_ADMIN))

_______________________________________________
Containers mailing list Containers-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org
https://lists.linux-foundation.org/mailman/listinfo/containers

_______________________________________________
Devel mailing list
[email protected]
https://openvz.org/mailman/listinfo/devel

Quoted 64 lines · sukadev-r/[email protected] wrote:

From: Sukadev Bhattiprolu <sukadev-r/[email protected]>
Subject: [RFC][PATCH 4/4]: Enable cloning PTY namespaces

Enable cloning PTY namespaces.

TODO:
This version temporarily uses the clone flag '0x80000000' which is unused in mainline atm, but used for CLONE_IO in -mm. While we must extend clone() (urgently) to solve this, it hopefully does not affect review of the rest of this patchset.

Changelog:
- Version 0: Based on earlier versions from Serge Hallyn and Matt Helsley.

Signed-off-by Sukadev Bhattiprolu <sukadev-r/[email protected]>

---
 fs/devpts/inode.c         |   84 +++++++++++++++++++++++++++++++++++++++-------
 include/linux/devpts_fs.h |   52 ++++++++++++++++++++++++++++
 include/linux/init_task.h |    1 
 include/linux/nsproxy.h   |    2 +
 include/linux/sched.h     |    2 +
 kernel/fork.c             |    2 -
 kernel/nsproxy.c          |   17 ++++++++-
 7 files changed, 146 insertions(+), 14 deletions(-)

Index: linux-2.6.24/fs/devpts/inode.c
===================================================================

fs/devpts/inode.c +11 -4

@@ -25,18 +25,25 @@
 #define DEVPTS_SUPER_MAGIC 0x1cd1
 
 extern int pty_limit;		/* Config limit on Unix98 ptys */
-static DEFINE_IDR(allocated_ptys);
 static DECLARE_MUTEX(allocated_ptys_lock);
+static struct file_system_type devpts_fs_type;
+
+struct pts_namespace init_pts_ns = {
+	.kref = {
+		.refcount = ATOMIC_INIT(2),
+	},
+	.allocated_ptys = IDR_INIT(init_pts_ns.allocated_ptys),
+	.mnt = NULL,
+};
 
 static inline struct idr *current_pts_ns_allocated_ptys(void)
 {
-	return &allocated_ptys;
+	return &current->nsproxy->pts_ns->allocated_ptys;
 }
 
-static struct vfsmount *devpts_mnt;
 static inline struct vfsmount *current_pts_ns_mnt(void)
 {
-	return devpts_mnt;
+	return current->nsproxy->pts_ns->mnt;
 }
 
 static struct {

@@ -59,6 +66,42 @@ static match_table_t tokens = {
 	{Opt_err, NULL}
 };
 

fs/devpts/inode.c static match_table_t tokens = { · line 77

Quoted 9 lines

fs/devpts/inode.c +9 -0

+struct pts_namespace *new_pts_ns(void)
+{
+	struct pts_namespace *ns;
+
+	ns = kmalloc(sizeof(*ns), GFP_KERNEL);
+	if (!ns)
+		return ERR_PTR(-ENOMEM);
+
+	ns->mnt = kern_mount_data(&devpts_fs_type, ns);

You create a circular references here - the namespace holds the vfsmnt, the vfsmnt holds a superblock, a superblock holds the namespace.

Quoted 65 lines · Pavel Emelyanov

Quoted 64 lines · sukadev-r/[email protected] wrote:

From: Sukadev Bhattiprolu <sukadev-r/[email protected]>
Subject: [RFC][PATCH 4/4]: Enable cloning PTY namespaces

Enable cloning PTY namespaces.

TODO:
This version temporarily uses the clone flag '0x80000000' which is unused in mainline atm, but used for CLONE_IO in -mm. While we must extend clone() (urgently) to solve this, it hopefully does not affect review of the rest of this patchset.

Changelog:
- Version 0: Based on earlier versions from Serge Hallyn and Matt Helsley.

Signed-off-by Sukadev Bhattiprolu <sukadev-r/[email protected]>

---
 fs/devpts/inode.c         |   84 +++++++++++++++++++++++++++++++++++++++-------
 include/linux/devpts_fs.h |   52 ++++++++++++++++++++++++++++
 include/linux/init_task.h |    1 
 include/linux/nsproxy.h   |    2 +
 include/linux/sched.h     |    2 +
 kernel/fork.c             |    2 -
 kernel/nsproxy.c          |   17 ++++++++-
 7 files changed, 146 insertions(+), 14 deletions(-)

Index: linux-2.6.24/fs/devpts/inode.c
===================================================================

fs/devpts/inode.c +11 -4

@@ -25,18 +25,25 @@
 #define DEVPTS_SUPER_MAGIC 0x1cd1
 
 extern int pty_limit;		/* Config limit on Unix98 ptys */
-static DEFINE_IDR(allocated_ptys);
 static DECLARE_MUTEX(allocated_ptys_lock);
+static struct file_system_type devpts_fs_type;
+
+struct pts_namespace init_pts_ns = {
+	.kref = {
+		.refcount = ATOMIC_INIT(2),
+	},
+	.allocated_ptys = IDR_INIT(init_pts_ns.allocated_ptys),
+	.mnt = NULL,
+};
 
 static inline struct idr *current_pts_ns_allocated_ptys(void)
 {
-	return &allocated_ptys;
+	return &current->nsproxy->pts_ns->allocated_ptys;
 }
 
-static struct vfsmount *devpts_mnt;
 static inline struct vfsmount *current_pts_ns_mnt(void)
 {
-	return devpts_mnt;
+	return current->nsproxy->pts_ns->mnt;
 }
 
 static struct {

@@ -59,6 +66,42 @@ static match_table_t tokens = {
 	{Opt_err, NULL}
 };
 

Quoted 12 lines

fs/devpts/inode.c static match_table_t tokens = { · line 77

Quoted 9 lines

fs/devpts/inode.c +9 -0

+struct pts_namespace *new_pts_ns(void)
+{
+	struct pts_namespace *ns;
+
+	ns = kmalloc(sizeof(*ns), GFP_KERNEL);
+	if (!ns)
+		return ERR_PTR(-ENOMEM);
+
+	ns->mnt = kern_mount_data(&devpts_fs_type, ns);

You create a circular references here - the namespace holds the vfsmnt, the vfsmnt holds a superblock, a superblock holds the namespace.

Hmm, yeah, good point. That was probably in my original version last year, so my fault not Suka's. Suka, would it work to have the sb->s_info point to the namespace but not grab a reference, than have

free_pts_ns() null out its sb->s_info, i.e. something like

void free_pts_ns(struct kref *ns_kref)

{
   struct pts_namespace *ns;
   struct super_block *sb;

   ns = container_of(ns_kref, struct pts_namespace, kref);
   BUG_ON(ns == &init_pts_ns);
   sb = ns->mnt->mnt_sb;

   mntput(ns->mnt);
   sb->s_info = NULL;

   /*
    * TODO:
    *      idr_remove_all(&ns->allocated_ptys); introduced in
.6.23
    */
   idr_destroy(&ns->allocated_ptys);
   kfree(ns);
}

Quoted 79 lines · Serge E. Hallyn

Quoted 77 lines · Pavel Emelyanov

Quoted 63 lines · sukadev-r/[email protected] wrote:

From: Sukadev Bhattiprolu <sukadev-r/[email protected]>
Subject: [RFC][PATCH 4/4]: Enable cloning PTY namespaces

Enable cloning PTY namespaces.

TODO:
This version temporarily uses the clone flag '0x80000000' which is unused in mainline atm, but used for CLONE_IO in -mm. While we must extend clone() (urgently) to solve this, it hopefully does not affect review of the rest of this patchset.

Changelog:
- Version 0: Based on earlier versions from Serge Hallyn and Matt Helsley.

Signed-off-by Sukadev Bhattiprolu <sukadev-r/[email protected]>

---
 fs/devpts/inode.c         |   84 +++++++++++++++++++++++++++++++++++++++-------
 include/linux/devpts_fs.h |   52 ++++++++++++++++++++++++++++
 include/linux/init_task.h |    1 
 include/linux/nsproxy.h   |    2 +
 include/linux/sched.h     |    2 +
 kernel/fork.c             |    2 -
 kernel/nsproxy.c          |   17 ++++++++-
 7 files changed, 146 insertions(+), 14 deletions(-)

Index: linux-2.6.24/fs/devpts/inode.c
===================================================================

fs/devpts/inode.c +11 -4

@@ -25,18 +25,25 @@
 #define DEVPTS_SUPER_MAGIC 0x1cd1
 
 extern int pty_limit;		/* Config limit on Unix98 ptys */
-static DEFINE_IDR(allocated_ptys);
 static DECLARE_MUTEX(allocated_ptys_lock);
+static struct file_system_type devpts_fs_type;
+
+struct pts_namespace init_pts_ns = {
+	.kref = {
+		.refcount = ATOMIC_INIT(2),
+	},
+	.allocated_ptys = IDR_INIT(init_pts_ns.allocated_ptys),
+	.mnt = NULL,
+};
 
 static inline struct idr *current_pts_ns_allocated_ptys(void)
 {
-	return &allocated_ptys;
+	return &current->nsproxy->pts_ns->allocated_ptys;
 }
 
-static struct vfsmount *devpts_mnt;
 static inline struct vfsmount *current_pts_ns_mnt(void)
 {
-	return devpts_mnt;
+	return current->nsproxy->pts_ns->mnt;
 }
 
 static struct {

@@ -59,6 +66,42 @@ static match_table_t tokens = {
 	{Opt_err, NULL}
 };

fs/devpts/inode.c static match_table_t tokens = { · line 77

Quoted 10 lines

fs/devpts/inode.c +9 -0

 
+struct pts_namespace *new_pts_ns(void)
+{
+	struct pts_namespace *ns;
+
+	ns = kmalloc(sizeof(*ns), GFP_KERNEL);
+	if (!ns)
+		return ERR_PTR(-ENOMEM);
+
+	ns->mnt = kern_mount_data(&devpts_fs_type, ns);

You create a circular references here - the namespace holds the vfsmnt, the vfsmnt holds a superblock, a superblock holds the namespace.

Quoted 3 lines

Hmm, yeah, good point. That was probably in my original version last year, so my fault not Suka's. Suka, would it work to have the sb->s_info point to the namespace but not grab a reference, than have

If you don't then you may be in situation, when this devpts is mounted from userspace and in case the namespace is dead superblock will point to garbage... Superblock MUST hold the namespace :)

Quoted 25 lines

free_pts_ns() null out its sb->s_info, i.e. something like

void free_pts_ns(struct kref *ns_kref)

{
   struct pts_namespace *ns;
   struct super_block *sb;

   ns = container_of(ns_kref, struct pts_namespace, kref);
   BUG_ON(ns == &init_pts_ns);
   sb = ns->mnt->mnt_sb;

   mntput(ns->mnt);
   sb->s_info = NULL;

   /*
    * TODO:
    *      idr_remove_all(&ns->allocated_ptys); introduced in
.6.23
    */
   idr_destroy(&ns->allocated_ptys);
   kfree(ns);
}

Quoted 82 lines · Pavel Emelyanov

Quoted 81 lines · Serge E. Hallyn

Quoted 65 lines · Pavel Emelyanov

Quoted 64 lines · sukadev-r/[email protected] wrote:

From: Sukadev Bhattiprolu <sukadev-r/[email protected]>
Subject: [RFC][PATCH 4/4]: Enable cloning PTY namespaces

Enable cloning PTY namespaces.

TODO:
This version temporarily uses the clone flag '0x80000000' which is unused in mainline atm, but used for CLONE_IO in -mm. While we must extend clone() (urgently) to solve this, it hopefully does not affect review of the rest of this patchset.

Changelog:
- Version 0: Based on earlier versions from Serge Hallyn and Matt Helsley.

Signed-off-by Sukadev Bhattiprolu <sukadev-r/[email protected]>

---
 fs/devpts/inode.c         |   84 +++++++++++++++++++++++++++++++++++++++-------
 include/linux/devpts_fs.h |   52 ++++++++++++++++++++++++++++
 include/linux/init_task.h |    1 
 include/linux/nsproxy.h   |    2 +
 include/linux/sched.h     |    2 +
 kernel/fork.c             |    2 -
 kernel/nsproxy.c          |   17 ++++++++-
 7 files changed, 146 insertions(+), 14 deletions(-)

Index: linux-2.6.24/fs/devpts/inode.c
===================================================================

fs/devpts/inode.c +11 -4

@@ -25,18 +25,25 @@
 #define DEVPTS_SUPER_MAGIC 0x1cd1
 
 extern int pty_limit;		/* Config limit on Unix98 ptys */
-static DEFINE_IDR(allocated_ptys);
 static DECLARE_MUTEX(allocated_ptys_lock);
+static struct file_system_type devpts_fs_type;
+
+struct pts_namespace init_pts_ns = {
+	.kref = {
+		.refcount = ATOMIC_INIT(2),
+	},
+	.allocated_ptys = IDR_INIT(init_pts_ns.allocated_ptys),
+	.mnt = NULL,
+};
 
 static inline struct idr *current_pts_ns_allocated_ptys(void)
 {
-	return &allocated_ptys;
+	return &current->nsproxy->pts_ns->allocated_ptys;
 }
 
-static struct vfsmount *devpts_mnt;
 static inline struct vfsmount *current_pts_ns_mnt(void)
 {
-	return devpts_mnt;
+	return current->nsproxy->pts_ns->mnt;
 }
 
 static struct {

@@ -59,6 +66,42 @@ static match_table_t tokens = {
 	{Opt_err, NULL}
 };
 

Quoted 12 lines

fs/devpts/inode.c static match_table_t tokens = { · line 77

Quoted 9 lines

fs/devpts/inode.c +9 -0

+struct pts_namespace *new_pts_ns(void)
+{
+	struct pts_namespace *ns;
+
+	ns = kmalloc(sizeof(*ns), GFP_KERNEL);
+	if (!ns)
+		return ERR_PTR(-ENOMEM);
+
+	ns->mnt = kern_mount_data(&devpts_fs_type, ns);

You create a circular references here - the namespace holds the vfsmnt, the vfsmnt holds a superblock, a superblock holds the namespace.

Hmm, yeah, good point. That was probably in my original version last year, so my fault not Suka's. Suka, would it work to have the sb->s_info point to the namespace but not grab a reference, than have

Quoted 4 lines

If you don't then you may be in situation, when this devpts is mounted from userspace and in case the namespace is dead superblock will point to garbage... Superblock MUST hold the namespace :)

But when the ns is freed sb->s_info would be NULL. Surely the helpers can be made to handle that safely?

Quoted 24 lines

Quoted 24 lines

free_pts_ns() null out its sb->s_info, i.e. something like

void free_pts_ns(struct kref *ns_kref)

{
   struct pts_namespace *ns;
   struct super_block *sb;

   ns = container_of(ns_kref, struct pts_namespace, kref);
   BUG_ON(ns == &init_pts_ns);
   sb = ns->mnt->mnt_sb;

   mntput(ns->mnt);
   sb->s_info = NULL;

   /*
    * TODO:
    *      idr_remove_all(&ns->allocated_ptys); introduced in
.6.23
    */
   idr_destroy(&ns->allocated_ptys);
   kfree(ns);
}

Quoted 83 lines · Serge E. Hallyn

Quoted 82 lines · Pavel Emelyanov

Quoted 81 lines · Serge E. Hallyn

Quoted 65 lines · Pavel Emelyanov

Quoted 64 lines · sukadev-r/[email protected] wrote:

From: Sukadev Bhattiprolu <sukadev-r/[email protected]>
Subject: [RFC][PATCH 4/4]: Enable cloning PTY namespaces

Enable cloning PTY namespaces.

TODO:
This version temporarily uses the clone flag '0x80000000' which is unused in mainline atm, but used for CLONE_IO in -mm. While we must extend clone() (urgently) to solve this, it hopefully does not affect review of the rest of this patchset.

Changelog:
- Version 0: Based on earlier versions from Serge Hallyn and Matt Helsley.

Signed-off-by Sukadev Bhattiprolu <sukadev-r/[email protected]>

---
 fs/devpts/inode.c         |   84 +++++++++++++++++++++++++++++++++++++++-------
 include/linux/devpts_fs.h |   52 ++++++++++++++++++++++++++++
 include/linux/init_task.h |    1 
 include/linux/nsproxy.h   |    2 +
 include/linux/sched.h     |    2 +
 kernel/fork.c             |    2 -
 kernel/nsproxy.c          |   17 ++++++++-
 7 files changed, 146 insertions(+), 14 deletions(-)

Index: linux-2.6.24/fs/devpts/inode.c
===================================================================

fs/devpts/inode.c +11 -4

@@ -25,18 +25,25 @@
 #define DEVPTS_SUPER_MAGIC 0x1cd1
 
 extern int pty_limit;		/* Config limit on Unix98 ptys */
-static DEFINE_IDR(allocated_ptys);
 static DECLARE_MUTEX(allocated_ptys_lock);
+static struct file_system_type devpts_fs_type;
+
+struct pts_namespace init_pts_ns = {
+	.kref = {
+		.refcount = ATOMIC_INIT(2),
+	},
+	.allocated_ptys = IDR_INIT(init_pts_ns.allocated_ptys),
+	.mnt = NULL,
+};
 
 static inline struct idr *current_pts_ns_allocated_ptys(void)
 {
-	return &allocated_ptys;
+	return &current->nsproxy->pts_ns->allocated_ptys;
 }
 
-static struct vfsmount *devpts_mnt;
 static inline struct vfsmount *current_pts_ns_mnt(void)
 {
-	return devpts_mnt;
+	return current->nsproxy->pts_ns->mnt;
 }
 
 static struct {

@@ -59,6 +66,42 @@ static match_table_t tokens = {
 	{Opt_err, NULL}
 };
 

Quoted 12 lines

fs/devpts/inode.c static match_table_t tokens = { · line 77

Quoted 9 lines

fs/devpts/inode.c +9 -0

+struct pts_namespace *new_pts_ns(void)
+{
+	struct pts_namespace *ns;
+
+	ns = kmalloc(sizeof(*ns), GFP_KERNEL);
+	if (!ns)
+		return ERR_PTR(-ENOMEM);
+
+	ns->mnt = kern_mount_data(&devpts_fs_type, ns);

You create a circular references here - the namespace holds the vfsmnt, the vfsmnt holds a superblock, a superblock holds the namespace.

Hmm, yeah, good point. That was probably in my original version last year, so my fault not Suka's. Suka, would it work to have the sb->s_info point to the namespace but not grab a reference, than have

Quoted 7 lines

Quoted 4 lines

If you don't then you may be in situation, when this devpts is mounted from userspace and in case the namespace is dead superblock will point to garbage... Superblock MUST hold the namespace :)

But when the ns is freed sb->s_info would be NULL. Surely the helpers can be made to handle that safely?

Hm... How do we find the proper superblock? Have a reference on it from the namespace? I'm afraid it will be easy to resolve the locking issues here.

I propose another scheme - we simply don't have ANY references from namespace to superblock/vfsmount, but get the current namespace in devpts_get_sb() and put in devpts_free_sb().

Quoted 25 lines

Quoted 24 lines

Quoted 24 lines

free_pts_ns() null out its sb->s_info, i.e. something like

void free_pts_ns(struct kref *ns_kref)

{
   struct pts_namespace *ns;
   struct super_block *sb;

   ns = container_of(ns_kref, struct pts_namespace, kref);
   BUG_ON(ns == &init_pts_ns);
   sb = ns->mnt->mnt_sb;

   mntput(ns->mnt);
   sb->s_info = NULL;

   /*
    * TODO:
    *      idr_remove_all(&ns->allocated_ptys); introduced in
.6.23
    */
   idr_destroy(&ns->allocated_ptys);
   kfree(ns);
}

Quoted 94 lines · Pavel Emelyanov

Quoted 83 lines · Serge E. Hallyn

Quoted 82 lines · Pavel Emelyanov

Quoted 81 lines · Serge E. Hallyn

Quoted 65 lines · Pavel Emelyanov

Quoted 64 lines · sukadev-r/[email protected] wrote:

From: Sukadev Bhattiprolu <sukadev-r/[email protected]>
Subject: [RFC][PATCH 4/4]: Enable cloning PTY namespaces

Enable cloning PTY namespaces.

TODO:
This version temporarily uses the clone flag '0x80000000' which is unused in mainline atm, but used for CLONE_IO in -mm. While we must extend clone() (urgently) to solve this, it hopefully does not affect review of the rest of this patchset.

Changelog:
- Version 0: Based on earlier versions from Serge Hallyn and Matt Helsley.

Signed-off-by Sukadev Bhattiprolu <sukadev-r/[email protected]>

---
 fs/devpts/inode.c         |   84 +++++++++++++++++++++++++++++++++++++++-------
 include/linux/devpts_fs.h |   52 ++++++++++++++++++++++++++++
 include/linux/init_task.h |    1 
 include/linux/nsproxy.h   |    2 +
 include/linux/sched.h     |    2 +
 kernel/fork.c             |    2 -
 kernel/nsproxy.c          |   17 ++++++++-
 7 files changed, 146 insertions(+), 14 deletions(-)

Index: linux-2.6.24/fs/devpts/inode.c
===================================================================

fs/devpts/inode.c +11 -4

@@ -25,18 +25,25 @@
 #define DEVPTS_SUPER_MAGIC 0x1cd1
 
 extern int pty_limit;		/* Config limit on Unix98 ptys */
-static DEFINE_IDR(allocated_ptys);
 static DECLARE_MUTEX(allocated_ptys_lock);
+static struct file_system_type devpts_fs_type;
+
+struct pts_namespace init_pts_ns = {
+	.kref = {
+		.refcount = ATOMIC_INIT(2),
+	},
+	.allocated_ptys = IDR_INIT(init_pts_ns.allocated_ptys),
+	.mnt = NULL,
+};
 
 static inline struct idr *current_pts_ns_allocated_ptys(void)
 {
-	return &allocated_ptys;
+	return &current->nsproxy->pts_ns->allocated_ptys;
 }
 
-static struct vfsmount *devpts_mnt;
 static inline struct vfsmount *current_pts_ns_mnt(void)
 {
-	return devpts_mnt;
+	return current->nsproxy->pts_ns->mnt;
 }
 
 static struct {

@@ -59,6 +66,42 @@ static match_table_t tokens = {
 	{Opt_err, NULL}
 };
 

Quoted 12 lines

fs/devpts/inode.c static match_table_t tokens = { · line 77

Quoted 9 lines

fs/devpts/inode.c +9 -0

+struct pts_namespace *new_pts_ns(void)
+{
+	struct pts_namespace *ns;
+
+	ns = kmalloc(sizeof(*ns), GFP_KERNEL);
+	if (!ns)
+		return ERR_PTR(-ENOMEM);
+
+	ns->mnt = kern_mount_data(&devpts_fs_type, ns);

You create a circular references here - the namespace holds the vfsmnt, the vfsmnt holds a superblock, a superblock holds the namespace.

Hmm, yeah, good point. That was probably in my original version last year, so my fault not Suka's. Suka, would it work to have the sb->s_info point to the namespace but not grab a reference, than have

Quoted 6 lines

Quoted 4 lines

If you don't then you may be in situation, when this devpts is mounted from userspace and in case the namespace is dead superblock will point to garbage... Superblock MUST hold the namespace :)

But when the ns is freed sb->s_info would be NULL. Surely the helpers can be made to handle that safely?

Hm... How do we find the proper superblock? Have a reference on it from the namespace? I'm afraid it will be easy to resolve the locking issues here.

Quoted 3 lines

I propose another scheme - we simply don't have ANY references from namespace to superblock/vfsmount, but get the current namespace in devpts_get_sb() and put in devpts_free_sb().

But then it really does become impossible to use a /dev/pts from another namespace, right?