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

From: H. Peter Anvin
Date: Fri Dec 11 2015 - 17:59:05 EST

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>
>>> 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
>>>>>>> + 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
>>>>>> well maintain the information you need without that.
>>>>> There is a good reason. We can not write a race free version of
>>>>> without it.
>>>> As long as this is for new userspace code, would it make sense to
>>>> 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
>>> 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,
>>> 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
>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
>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.
>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)
> {
>- 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
Please read the FAQ at