devpts multiple instances feedback

devpts multiple instances feedback

[Christoph Hellwig]

I just took a look at the changes going into Linus current tree and
here's some feedback about the devpts multiple instances code:

 - the ptmx node is quite useful, I think it should always be around,
   even for normal devpts mounts.  That way distros can slowly migrate
   over to just using it by default and making the containers
   interaction easier.  It's also in many ways much nicer to have
   all the pty handling in one filesystems instead of sometimes
   using the character device.
 - the 000 mode is very weird, given how the /dev/ptmx operates
   it doesn't really make much sense to have it different than 0666
   unless you want to disable ptys.
 - why does pts_sb_from_inode have to check s_magic, I can't see
   it ever used on an inode not from the devpts filesystem
 - parsing the options twice is rather odd, I'd rather parse it into
   a once allocated structure then passed on through the private
   data void pointer into get_sb_nodev
 - creating the ptmx node should happen inside devfs_fill_super
 - once the ptmx mknod is gone I think new_pts_mount,
   is_new_instance_mount, init_pts_mount and maybe even get_init_pts_sb
   should be merged into devpts_get_sb to make the whole mounting
   scenario easier to follow instead of having to jump through half
   a dozen functions
 - I think CONFIG_DEVPTS_MULTIPLE_INSTANCES is not a good idea,
   it's not much code and could either be enabled unconditionally or
   based on the presence of a generic namespaces config option.
   (btw, this also applies to the other namespaces options, there's
   not much of a reason to have millions of options for them,
   one single option would be a lot easier for the user..)

Re: devpts multiple instances feedback

[Christoph Hellwig]

This is a little untested patch to massage the mount code into about
how it should look like:


Index: linux-2.6/fs/devpts/inode.c
===================================================================
--- linux-2.6.orig/fs/devpts/inode.c	2009-01-03 16:52:22.823672077 +0100
+++ linux-2.6/fs/devpts/inode.c	2009-01-03 17:14:24.153711598 +0100
@@ -329,106 +329,6 @@ static int compare_init_pts_sb(struct su
 }
 
 /*
- * Safely parse the mount options in @data and update @opts.
- *
- * devpts ends up parsing options two times during mount, due to the
- * two modes of operation it supports. The first parse occurs in
- * devpts_get_sb() when determining the mode (single-instance or
- * multi-instance mode). The second parse happens in devpts_remount()
- * or new_pts_mount() depending on the mode.
- *
- * Parsing of options modifies the @data making subsequent parsing
- * incorrect. So make a local copy of @data and parse it.
- *
- * Return: 0 On success, -errno on error
- */
-static int safe_parse_mount_options(void *data, struct pts_mount_opts *opts)
-{
-	int rc;
-	void *datacp;
-
-	if (!data)
-		return 0;
-
-	/* Use kstrdup() ?  */
-	datacp = kmalloc(PAGE_SIZE, GFP_KERNEL);
-	if (!datacp)
-		return -ENOMEM;
-
-	memcpy(datacp, data, PAGE_SIZE);
-	rc = parse_mount_options((char *)datacp, PARSE_MOUNT, opts);
-	kfree(datacp);
-
-	return rc;
-}
-
-/*
- * Mount a new (private) instance of devpts.  PTYs created in this
- * instance are independent of the PTYs in other devpts instances.
- */
-static int new_pts_mount(struct file_system_type *fs_type, int flags,
-		void *data, struct vfsmount *mnt)
-{
-	int err;
-	struct pts_fs_info *fsi;
-	struct pts_mount_opts *opts;
-
-	printk(KERN_NOTICE "devpts: newinstance mount\n");
-
-	err = get_sb_nodev(fs_type, flags, data, devpts_fill_super, mnt);
-	if (err)
-		return err;
-
-	fsi = DEVPTS_SB(mnt->mnt_sb);
-	opts = &fsi->mount_opts;
-
-	err = parse_mount_options(data, PARSE_MOUNT, opts);
-	if (err)
-		goto fail;
-
-	err = mknod_ptmx(mnt->mnt_sb);
-	if (err)
-		goto fail;
-
-	return 0;
-
-fail:
-	dput(mnt->mnt_sb->s_root);
-	deactivate_super(mnt->mnt_sb);
-	return err;
-}
-
-/*
- * Check if 'newinstance' mount option was specified in @data.
- *
- * Return: -errno  	on error (eg: invalid mount options specified)
- * 	 : 1 		if 'newinstance' mount option was specified
- * 	 : 0 		if 'newinstance' mount option was NOT specified
- */
-static int is_new_instance_mount(void *data)
-{
-	int rc;
-	struct pts_mount_opts opts;
-
-	if (!data)
-		return 0;
-
-	rc = safe_parse_mount_options(data, &opts);
-	if (!rc)
-		rc = opts.newinstance;
-
-	return rc;
-}
-
-/*
- * get_init_pts_sb()
- *
- *     This interface is needed to support multiple namespace semantics in
- *     devpts while preserving backward compatibility of the current 'single-
- *     namespace' semantics. i.e all mounts of devpts without the 'newinstance'
- *     mount option should bind to the initial kernel mount, like
- *     get_sb_single().
- *
  *     Mounts with 'newinstance' option create a new private namespace.
  *
  *     But for single-mount semantics, devpts cannot use get_sb_single(),
@@ -436,20 +336,43 @@ static int is_new_instance_mount(void *d
  *     the most recent mount of devpts. But that recent mount may be a
  *     'newinstance' mount and get_sb_single() would pick the newinstance
  *     super-block instead of the initial super-block.
- *
- *     This interface is identical to get_sb_single() except that it
- *     consistently selects the 'single-namespace' superblock even in the
- *     presence of the private namespace (i.e 'newinstance') super-blocks.
  */
-static int get_init_pts_sb(struct file_system_type *fs_type, int flags,
-		void *data, struct vfsmount *mnt)
+static int devpts_get_sb(struct file_system_type *fs_type, int flags,
+		const char *dev_name, void *data, struct vfsmount *mnt)
 {
+	struct pts_mount_opts opts = { 0, };
 	struct super_block *s;
 	int error;
 
-	s = sget(fs_type, compare_init_pts_sb, set_anon_super, NULL);
-	if (IS_ERR(s))
-		return PTR_ERR(s);
+	if (data) {
+		error = parse_mount_options(data, PARSE_MOUNT, &opts);
+		if (error)
+			return error;
+	}
+
+	if (opts.newinstance) {
+		/*
+		 * Mount a new (private) instance of devpts.  PTYs created
+		 * in this instance are independent of the PTYs in other devpts
+		 * instances.
+		 */
+
+		printk(KERN_NOTICE "devpts: creating new instance\n");
+
+		s = sget(fs_type, NULL, set_anon_super, NULL);
+		if (IS_ERR(s))
+			return PTR_ERR(s);
+	} else {
+		/*
+		 * Mount or remount the initial kernel mount of devpts.
+		 *
+		 * This type of mount maintains the legacy, single-instance
+		 * semantics, while the kernel still allows multiple-instances.
+		 */
+		s = sget(fs_type, compare_init_pts_sb, set_anon_super, NULL);
+		if (IS_ERR(s))
+			return PTR_ERR(s);
+	}
 
 	if (!s->s_root) {
 		s->s_flags = flags;
@@ -461,46 +384,26 @@ static int get_init_pts_sb(struct file_s
 		}
 		s->s_flags |= MS_ACTIVE;
 	}
-	do_remount_sb(s, flags, data, 0);
-	return simple_set_mnt(mnt, s);
-}
-
-/*
- * Mount or remount the initial kernel mount of devpts. This type of
- * mount maintains the legacy, single-instance semantics, while the
- * kernel still allows multiple-instances.
- */
-static int init_pts_mount(struct file_system_type *fs_type, int flags,
-		void *data, struct vfsmount *mnt)
-{
-	int err;
-
-	err = get_init_pts_sb(fs_type, flags, data, mnt);
-	if (err)
-		return err;
-
-	err = mknod_ptmx(mnt->mnt_sb);
-	if (err) {
-		dput(mnt->mnt_sb->s_root);
-		deactivate_super(mnt->mnt_sb);
-	}
-
-	return err;
-}
-
-static int devpts_get_sb(struct file_system_type *fs_type,
-	int flags, const char *dev_name, void *data, struct vfsmount *mnt)
-{
-	int new;
 
-	new = is_new_instance_mount(data);
-	if (new < 0)
-		return new;
+	/*
+	 * Copy over mount options structure into the superblock.
+	 */
+	memcpy(&DEVPTS_SB(mnt->mnt_sb)->mount_opts, &opts, sizeof(opts));
+
+	error = simple_set_mnt(mnt, s);
+	if (error)
+		return error;
+
+	error = mknod_ptmx(mnt->mnt_sb);
+	if (error)
+		goto out_dput;
 
-	if (new)
-		return new_pts_mount(fs_type, flags, data, mnt);
+	return 0;
 
-	return init_pts_mount(fs_type, flags, data, mnt);
+ out_dput:
+	dput(mnt->mnt_sb->s_root);
+	deactivate_super(mnt->mnt_sb);
+	return error;
 }
 #else
 /*

Re: devpts multiple instances feedback

[Christoph Hellwig]

On Sat, Jan 03, 2009 at 05:15:44PM +0100, Christoph Hellwig wrote:
> This is a little untested patch to massage the mount code into about
> how it should look like:

ping?

> 
> Index: linux-2.6/fs/devpts/inode.c
> ===================================================================
> --- linux-2.6.orig/fs/devpts/inode.c	2009-01-03 16:52:22.823672077 +0100
> +++ linux-2.6/fs/devpts/inode.c	2009-01-03 17:14:24.153711598 +0100
> @@ -329,106 +329,6 @@ static int compare_init_pts_sb(struct su
>  }
>  
>  /*
> - * Safely parse the mount options in @data and update @opts.
> - *
> - * devpts ends up parsing options two times during mount, due to the
> - * two modes of operation it supports. The first parse occurs in
> - * devpts_get_sb() when determining the mode (single-instance or
> - * multi-instance mode). The second parse happens in devpts_remount()
> - * or new_pts_mount() depending on the mode.
> - *
> - * Parsing of options modifies the @data making subsequent parsing
> - * incorrect. So make a local copy of @data and parse it.
> - *
> - * Return: 0 On success, -errno on error
> - */
> -static int safe_parse_mount_options(void *data, struct pts_mount_opts *opts)
> -{
> -	int rc;
> -	void *datacp;
> -
> -	if (!data)
> -		return 0;
> -
> -	/* Use kstrdup() ?  */
> -	datacp = kmalloc(PAGE_SIZE, GFP_KERNEL);
> -	if (!datacp)
> -		return -ENOMEM;
> -
> -	memcpy(datacp, data, PAGE_SIZE);
> -	rc = parse_mount_options((char *)datacp, PARSE_MOUNT, opts);
> -	kfree(datacp);
> -
> -	return rc;
> -}
> -
> -/*
> - * Mount a new (private) instance of devpts.  PTYs created in this
> - * instance are independent of the PTYs in other devpts instances.
> - */
> -static int new_pts_mount(struct file_system_type *fs_type, int flags,
> -		void *data, struct vfsmount *mnt)
> -{
> -	int err;
> -	struct pts_fs_info *fsi;
> -	struct pts_mount_opts *opts;
> -
> -	printk(KERN_NOTICE "devpts: newinstance mount\n");
> -
> -	err = get_sb_nodev(fs_type, flags, data, devpts_fill_super, mnt);
> -	if (err)
> -		return err;
> -
> -	fsi = DEVPTS_SB(mnt->mnt_sb);
> -	opts = &fsi->mount_opts;
> -
> -	err = parse_mount_options(data, PARSE_MOUNT, opts);
> -	if (err)
> -		goto fail;
> -
> -	err = mknod_ptmx(mnt->mnt_sb);
> -	if (err)
> -		goto fail;
> -
> -	return 0;
> -
> -fail:
> -	dput(mnt->mnt_sb->s_root);
> -	deactivate_super(mnt->mnt_sb);
> -	return err;
> -}
> -
> -/*
> - * Check if 'newinstance' mount option was specified in @data.
> - *
> - * Return: -errno  	on error (eg: invalid mount options specified)
> - * 	 : 1 		if 'newinstance' mount option was specified
> - * 	 : 0 		if 'newinstance' mount option was NOT specified
> - */
> -static int is_new_instance_mount(void *data)
> -{
> -	int rc;
> -	struct pts_mount_opts opts;
> -
> -	if (!data)
> -		return 0;
> -
> -	rc = safe_parse_mount_options(data, &opts);
> -	if (!rc)
> -		rc = opts.newinstance;
> -
> -	return rc;
> -}
> -
> -/*
> - * get_init_pts_sb()
> - *
> - *     This interface is needed to support multiple namespace semantics in
> - *     devpts while preserving backward compatibility of the current 'single-
> - *     namespace' semantics. i.e all mounts of devpts without the 'newinstance'
> - *     mount option should bind to the initial kernel mount, like
> - *     get_sb_single().
> - *
>   *     Mounts with 'newinstance' option create a new private namespace.
>   *
>   *     But for single-mount semantics, devpts cannot use get_sb_single(),
> @@ -436,20 +336,43 @@ static int is_new_instance_mount(void *d
>   *     the most recent mount of devpts. But that recent mount may be a
>   *     'newinstance' mount and get_sb_single() would pick the newinstance
>   *     super-block instead of the initial super-block.
> - *
> - *     This interface is identical to get_sb_single() except that it
> - *     consistently selects the 'single-namespace' superblock even in the
> - *     presence of the private namespace (i.e 'newinstance') super-blocks.
>   */
> -static int get_init_pts_sb(struct file_system_type *fs_type, int flags,
> -		void *data, struct vfsmount *mnt)
> +static int devpts_get_sb(struct file_system_type *fs_type, int flags,
> +		const char *dev_name, void *data, struct vfsmount *mnt)
>  {
> +	struct pts_mount_opts opts = { 0, };
>  	struct super_block *s;
>  	int error;
>  
> -	s = sget(fs_type, compare_init_pts_sb, set_anon_super, NULL);
> -	if (IS_ERR(s))
> -		return PTR_ERR(s);
> +	if (data) {
> +		error = parse_mount_options(data, PARSE_MOUNT, &opts);
> +		if (error)
> +			return error;
> +	}
> +
> +	if (opts.newinstance) {
> +		/*
> +		 * Mount a new (private) instance of devpts.  PTYs created
> +		 * in this instance are independent of the PTYs in other devpts
> +		 * instances.
> +		 */
> +
> +		printk(KERN_NOTICE "devpts: creating new instance\n");
> +
> +		s = sget(fs_type, NULL, set_anon_super, NULL);
> +		if (IS_ERR(s))
> +			return PTR_ERR(s);
> +	} else {
> +		/*
> +		 * Mount or remount the initial kernel mount of devpts.
> +		 *
> +		 * This type of mount maintains the legacy, single-instance
> +		 * semantics, while the kernel still allows multiple-instances.
> +		 */
> +		s = sget(fs_type, compare_init_pts_sb, set_anon_super, NULL);
> +		if (IS_ERR(s))
> +			return PTR_ERR(s);
> +	}
>  
>  	if (!s->s_root) {
>  		s->s_flags = flags;
> @@ -461,46 +384,26 @@ static int get_init_pts_sb(struct file_s
>  		}
>  		s->s_flags |= MS_ACTIVE;
>  	}
> -	do_remount_sb(s, flags, data, 0);
> -	return simple_set_mnt(mnt, s);
> -}
> -
> -/*
> - * Mount or remount the initial kernel mount of devpts. This type of
> - * mount maintains the legacy, single-instance semantics, while the
> - * kernel still allows multiple-instances.
> - */
> -static int init_pts_mount(struct file_system_type *fs_type, int flags,
> -		void *data, struct vfsmount *mnt)
> -{
> -	int err;
> -
> -	err = get_init_pts_sb(fs_type, flags, data, mnt);
> -	if (err)
> -		return err;
> -
> -	err = mknod_ptmx(mnt->mnt_sb);
> -	if (err) {
> -		dput(mnt->mnt_sb->s_root);
> -		deactivate_super(mnt->mnt_sb);
> -	}
> -
> -	return err;
> -}
> -
> -static int devpts_get_sb(struct file_system_type *fs_type,
> -	int flags, const char *dev_name, void *data, struct vfsmount *mnt)
> -{
> -	int new;
>  
> -	new = is_new_instance_mount(data);
> -	if (new < 0)
> -		return new;
> +	/*
> +	 * Copy over mount options structure into the superblock.
> +	 */
> +	memcpy(&DEVPTS_SB(mnt->mnt_sb)->mount_opts, &opts, sizeof(opts));
> +
> +	error = simple_set_mnt(mnt, s);
> +	if (error)
> +		return error;
> +
> +	error = mknod_ptmx(mnt->mnt_sb);
> +	if (error)
> +		goto out_dput;
>  
> -	if (new)
> -		return new_pts_mount(fs_type, flags, data, mnt);
> +	return 0;
>  
> -	return init_pts_mount(fs_type, flags, data, mnt);
> + out_dput:
> +	dput(mnt->mnt_sb->s_root);
> +	deactivate_super(mnt->mnt_sb);
> +	return error;
>  }
>  #else
>  /*
---end quoted text---

Re: devpts multiple instances feedback

[Sukadev Bhattiprolu]

Christoph Hellwig [hch@lst.de] wrote:
| On Sat, Jan 03, 2009 at 05:15:44PM +0100, Christoph Hellwig wrote:
| > This is a little untested patch to massage the mount code into about
| > how it should look like:
| 
| ping?

I was waiting for discussion on the other thread. But yes, this does
simplify the code by copying in the common code in get_sb_single() and
get_sb_nodev() into devpts, and eliminating the do_remount() stuff in
'single-mount' mode.

We may also be able to define an "empty" mknod_ptmx() when
CONFIG_DEVPTS_MULTIPLE_INSTANCES=n and share a bit more code, but will
defer that to a separate patch.

Will test this out some more and resend the modified patch.

Sukadev

Re: devpts multiple instances feedback

[Sukadev Bhattiprolu]

Christoph Hellwig [hch@lst.de] wrote:
| I just took a look at the changes going into Linus current tree and
| here's some feedback about the devpts multiple instances code:

Thanks for the review. Here are some quick responses and will go over
comments/patch more closely.

Ccing Alan Cox.

| 
|  - the ptmx node is quite useful, I think it should always be around,
|    even for normal devpts mounts.  That way distros can slowly migrate
|    over to just using it by default and making the containers
|    interaction easier.  It's also in many ways much nicer to have
|    all the pty handling in one filesystems instead of sometimes
|    using the character device.

Making the pts/ptmx node would certianly simplify the code. But we
ended up with some of the complexity to preserve the legacy behavior.
I believe there was some concern that the presence of a "shadow"
ptmx node on older distros might affect rights management (eg: if
the older distro which does not know about /dev/pts/ptmx, applied
a security label to /dev/ptmx that label could be subverted by using
/dev/pts/ptmx ?

That was also one of the reasons for the default 000 mode on the pts/ptmx
device node


|  - the 000 mode is very weird, given how the /dev/ptmx operates
|    it doesn't really make much sense to have it different than 0666
|    unless you want to disable ptys.
|  - why does pts_sb_from_inode have to check s_magic, I can't see
|    it ever used on an inode not from the devpts filesystem

If /dev/ptmx is not a symlink to pts/ptmx, we would need the s_magic
check ? (eg: when called from devpts_new_index()). The check would
not be needed if /dev/ptmx is always a symlink.

|  - parsing the options twice is rather odd, I'd rather parse it into
|    a once allocated structure then passed on through the private
|    data void pointer into get_sb_nodev

Agree :-)

|  - creating the ptmx node should happen inside devfs_fill_super
|  - once the ptmx mknod is gone I think new_pts_mount,
|    is_new_instance_mount, init_pts_mount and maybe even get_init_pts_sb
|    should be merged into devpts_get_sb to make the whole mounting
|    scenario easier to follow instead of having to jump through half
|    a dozen functions
|  - I think CONFIG_DEVPTS_MULTIPLE_INSTANCES is not a good idea,
|    it's not much code and could either be enabled unconditionally or
|    based on the presence of a generic namespaces config option.
|    (btw, this also applies to the other namespaces options, there's

The config token was not needed for the namespaces itself but more
to preserve the legacy behavior. If we don't need o preseve the
legacy mode, we could remove the token.
|    not much of a reason to have millions of options for them,
|    one single option would be a lot easier for the user..)
| --
| To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
| the body of a message to majordomo@vger.kernel.org
| More majordomo info at  http://vger.kernel.org/majordomo-info.html
| Please read the FAQ at  http://www.tux.org/lkml/

Re: devpts multiple instances feedback

[Christoph Hellwig]

On Mon, Jan 05, 2009 at 01:09:40PM -0800, Sukadev Bhattiprolu wrote:
> Making the pts/ptmx node would certianly simplify the code. But we
> ended up with some of the complexity to preserve the legacy behavior.
> I believe there was some concern that the presence of a "shadow"
> ptmx node on older distros might affect rights management (eg: if
> the older distro which does not know about /dev/pts/ptmx, applied
> a security label to /dev/ptmx that label could be subverted by using
> /dev/pts/ptmx ?
> 
> That was also one of the reasons for the default 000 mode on the pts/ptmx
> device node

So just make it 000 but always created it.

> 
> |  - the 000 mode is very weird, given how the /dev/ptmx operates
> |    it doesn't really make much sense to have it different than 0666
> |    unless you want to disable ptys.
> |  - why does pts_sb_from_inode have to check s_magic, I can't see
> |    it ever used on an inode not from the devpts filesystem
> 
> If /dev/ptmx is not a symlink to pts/ptmx, we would need the s_magic
> check ? (eg: when called from devpts_new_index()). The check would
> not be needed if /dev/ptmx is always a symlink.

Ok, so it's for the /dev/ptmx node.  Just make that explicit by
passing down a paramter then.

Re: devpts multiple instances feedback

[Alan Cox]

> > That was also one of the reasons for the default 000 mode on the pts/ptmx
> > device node
> 
> So just make it 000 but always created it.

That still allows it to be subverted with some security rulesets -
remember root can open a 000 file by default.

Re: devpts multiple instances feedback

[Christoph Hellwig]

On Mon, Jan 26, 2009 at 09:58:53PM +0000, Alan Cox wrote:
> > > That was also one of the reasons for the default 000 mode on the pts/ptmx
> > > device node
> > 
> > So just make it 000 but always created it.
> 
> That still allows it to be subverted with some security rulesets -
> remember root can open a 000 file by default.

root can also mknod device nodes by default.

Re: devpts multiple instances feedback

[Alan Cox]

On Sun, 1 Feb 2009 17:29:58 +0100
Christoph Hellwig <hch@lst.de> wrote:

> On Mon, Jan 26, 2009 at 09:58:53PM +0000, Alan Cox wrote:
> > > > That was also one of the reasons for the default 000 mode on the pts/ptmx
> > > > device node
> > > 
> > > So just make it 000 but always created it.
> > 
> > That still allows it to be subverted with some security rulesets -
> > remember root can open a 000 file by default.
> 
> root can also mknod device nodes by default.

That depends on your SELinux policy rules

Re: devpts multiple instances feedback

[H. Peter Anvin]

Christoph Hellwig wrote:
>>
>> That was also one of the reasons for the default 000 mode on the pts/ptmx
>> device node
> 
> So just make it 000 but always created it.
> 

That's what we do...

	-hpa

Re: devpts multiple instances feedback

[Christoph Hellwig]

On Mon, Jan 26, 2009 at 01:58:58PM -0800, H. Peter Anvin wrote:
> Christoph Hellwig wrote:
> >>
> >>That was also one of the reasons for the default 000 mode on the pts/ptmx
> >>device node
> >
> >So just make it 000 but always created it.
> >
> 
> That's what we do...

Only if the kernel is compiled with CONFIG_DEVPTS_MULTIPLE_INSTANCES
(which will be clearly superflous after the cleanup patches because
the amount of addition code is minimal) _and_ you mount with
-o newinstance, and then you have another -o ptmxmode=foo option
to make it useable.

Really, even if we're really cautious there should be no compiletime
and just a single mount time option, but I'm not even sure that is
needed with the default zero mode.