Messages in this thread | | | Subject | Re: [PATCH] devpts: Sensible /dev/ptmx & force newinstance | From | "H. Peter Anvin" <> | Date | Fri, 11 Dec 2015 14:57:59 -0800 |
| |
On December 11, 2015 2:35:16 PM PST, ebiederm@xmission.com wrote: >Andy Lutomirski <luto@amacapital.net> writes: > >> On Fri, Dec 11, 2015 at 2:07 PM, H. Peter Anvin <hpa@zytor.com> >wrote: >>> On 12/11/15 13:48, Andy Lutomirski wrote: >>>> On Fri, Dec 11, 2015 at 1:11 PM, Eric W. Biederman >>>> <ebiederm@xmission.com> wrote: >>>>> Al Viro <viro@ZenIV.linux.org.uk> 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.
| |