Re: [PATCH] devpts: Sensible /dev/ptmx & force newinstance

[H. Peter Anvin]

On December 11, 2015 2:35:16 PM PST, ebiederm@xxxxxxxxxxxx wrote:
>Andy Lutomirski <luto@xxxxxxxxxxxxxx> writes:
>
>> On Fri, Dec 11, 2015 at 2:07 PM, H. Peter Anvin <hpa@xxxxxxxxx>
>wrote:
>>> On 12/11/15 13:48, Andy Lutomirski wrote:
>>>> On Fri, Dec 11, 2015 at 1:11 PM, Eric W. Biederman
>>>> <ebiederm@xxxxxxxxxxxx> wrote:
>>>>> Al Viro <viro@xxxxxxxxxxxxxxxxxx> writes:
>>>>>
>>>>>> On Fri, Dec 11, 2015 at 01:40:40PM -0600, Eric W. Biederman
>wrote:
>>>>>>
>>>>>>> +    inode = path.dentry->d_inode;
>>>>>>> +    filp->f_path = path;
>>>>>>> +    filp->f_inode = inode;
>>>>>>> +    filp->f_mapping = inode->i_mapping;
>>>>>>> +    path_put(&old);
>>>>>>
>>>>>> Don't.  You are creating a fairly subtle constraint on what the
>code in
>>>>>> fs/open.c and fs/namei.c can do, for no good reason.  You can
>bloody
>>>>>> well maintain the information you need without that.
>>>>>
>>>>> There is a good reason.  We can not write a race free version of
>ptsname
>>>>> without it.
>>>>
>>>> As long as this is for new userspace code, would it make sense to
>just
>>>> add a new ioctl to ask "does this ptmx fd match this /dev/pts fd?"
>>>>
>>>
>>> For the newinstance case st_dev should match between the master and
>the
>>> slave.  Unfortunately this is not the case for a legacy ptmx, as a
>>> stat() on the master descriptor still returns the st_dev, st_rdev,
>and
>>> st_ino for the ptmx device node.
>>
>> Sure, but I'm not talking about stat.  I'm saying that we could add a
>> new ioctl that works on any ptmx fd (/dev/ptmx or /dev/pts/ptmx) that
>> answers the question "does this ptmx logically belong to the given
>> devpts filesystem".
>>
>> Since it's not stat, we can make it do whatever we want, including
>> following a link to the devpts instance that isn't f_path or f_inode.
>
>The useful ioctl to add in my opinion would be one that actually opens
>the slave, at which point ptsname could become ttyname, and that closes
>races in grantpt.
>
>I even posted an implementation earlier in the discussion and no one
>was
>interested.
>
>Honestly the more weird special cases we add to devpts the less likely
>userspace will be to get things right.  We have been trying since 1998
>and devpts is still a poor enough design we have not been able to get
>rid of /usr/lib/pt_chown.  Adding another case where we have to sand on
>one foot and touch our nose does not seem to likely to achieve
>widespread adoption.  How many version of libc are there now?
>
>So I think the following incremental patch makes sense to improve the
>maintainability of what I have written, but I haven't seen any
>arguments
>that it is actually a bad idea.
>
>Especially given that ptys are a core part of unix and they are used by
>everyone all of the time.
>
>Eric
>
>diff --git a/fs/devpts/inode.c b/fs/devpts/inode.c
>index 79e8d60ba0fe..588e0a049daf 100644
>--- a/fs/devpts/inode.c
>+++ b/fs/devpts/inode.c
>@@ -139,15 +139,14 @@ static inline struct pts_fs_info
>*DEVPTS_SB(struct super_block *sb)
> struct inode *devpts_ptmx(struct inode *inode, struct file *filp)
> {
> #ifdef CONFIG_DEVPTS_MULTIPLE_INSTANCES
>-	struct path path, old;
>+	struct path path;
> 	struct super_block *sb;
> 	struct dentry *root;
> 
> 	if (inode->i_sb->s_magic == DEVPTS_SUPER_MAGIC)
> 		return inode;
> 
>-	old = filp->f_path;
>-	path = old;
>+	path = filp->f_path;
> 	path_get(&path);
> 	if (kern_path_pts(&path)) {
> 		path_put(&path);
>@@ -172,10 +171,8 @@ struct inode *devpts_ptmx(struct inode *inode,
>struct file *filp)
> 	 * path to the devpts filesystem for reporting slave inodes.
> 	 */
> 	inode = path.dentry->d_inode;
>-	filp->f_path = path;
>-	filp->f_inode = inode;
>-	filp->f_mapping = inode->i_mapping;
>-	path_put(&old);
>+	filp_set_path(filp, &path);
>+	path_put(&path);
> #endif
> 	return inode;
> }
>diff --git a/fs/open.c b/fs/open.c
>index b6f1e96a7c0b..5234a791d9ae 100644
>--- a/fs/open.c
>+++ b/fs/open.c
>@@ -679,6 +679,19 @@ int open_check_o_direct(struct file *f)
> 	return 0;
> }
> 
>+void filp_set_path(struct file *filp, struct path *path)
>+{
>+	/* Only safe during open */
>+	struct path old = filp->f_path;
>+	struct inode *inode = path->dentry->d_inode;
>+
>+	path_get(path);
>+	filp->f_path = *path;
>+	filp->f_inode = inode;
>+	filp->f_mapping = inode->i_mapping;
>+	path_put(&old);
>+}
>+
> static int do_dentry_open(struct file *f,
> 			  struct inode *inode,
> 			  int (*open)(struct inode *, struct file *),
>diff --git a/include/linux/fs.h b/include/linux/fs.h
>index 3aa514254161..f3659a8a2eec 100644
>--- a/include/linux/fs.h
>+++ b/include/linux/fs.h
>@@ -2220,6 +2220,7 @@ extern struct file *file_open_root(struct dentry
>*, struct vfsmount *,
> 				   const char *, int);
>extern struct file * dentry_open(const struct path *, int, const struct
>cred *);
> extern int filp_close(struct file *, fl_owner_t id);
>+extern void filp_set_path(struct file *filp, struct path *path);
> 
>extern struct filename *getname_flags(const char __user *, int, int *);
> extern struct filename *getname(const char __user *);

I'm calling bullshit on that.  pt_chown is not and has not been needed on anything but severely misconfigured userspace since devpts was constructed.  The problem, rather, is that by not disabling pt_chown at the same time they switched to devpts distros allowed these severe misconfigurations to go on unnoticed.

So pt_chown *created* the problem.

The other problem we are trying to deal with is that the layout is suboptimal for the multi instance case, a legacy from an older SysV  layout with /dev/ptm/# and /dev/pts/#, the former replaced with the multiplex device /dev/ptmx, but that just shows the sheer amount of inertia we are dealing with.
-- 
Sent from my Android device with K-9 Mail. Please excuse brevity and formatting.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/