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

[sukadev-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org]

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

Ok.

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

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