Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755426AbbLKW7A (ORCPT ); Fri, 11 Dec 2015 17:59:00 -0500 Received: from terminus.zytor.com ([198.137.202.10]:51937 "EHLO mail.zytor.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755360AbbLKW66 (ORCPT ); Fri, 11 Dec 2015 17:58:58 -0500 User-Agent: K-9 Mail for Android In-Reply-To: <87y4d0sjff.fsf@x220.int.ebiederm.org> References: <1CB621EF-1647-463B-A144-D611DB150E15@zytor.com> <20151208223135.GA8352@kroah.com> <87oae0h2bo.fsf@x220.int.ebiederm.org> <56677DE3.5040705@zytor.com> <20151209012311.GA11794@kroah.com> <84B136DF-55E4-476A-9CB2-062B15677EE5@zytor.com> <20151209013859.GA12442@kroah.com> <20151209083225.GA30452@1wt.eu> <87wpskyds7.fsf_-_@x220.int.ebiederm.org> <20151211210400.GL20997@ZenIV.linux.org.uk> <87h9jovgf7.fsf@x220.int.ebiederm.org> <566B4931.5080806@zytor.com> <87y4d0sjff.fsf@x220.int.ebiederm.org> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit Content-Type: text/plain; charset=UTF-8 Subject: Re: [PATCH] devpts: Sensible /dev/ptmx & force newinstance From: "H. Peter Anvin" Date: Fri, 11 Dec 2015 14:57:59 -0800 To: ebiederm@xmission.com, Andy Lutomirski CC: Al Viro , Greg KH , Jiri Slaby , Linus Torvalds , Aurelien Jarno , Florian Weimer , Serge Hallyn , Jann Horn , "security@kernel.org" , "security@ubuntu.com >> security" , security@debian.org, Willy Tarreau , "linux-kernel@vger.kernel.org" Message-ID: Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 6021 Lines: 162 On December 11, 2015 2:35:16 PM PST, ebiederm@xmission.com wrote: >Andy Lutomirski writes: > >> On Fri, Dec 11, 2015 at 2:07 PM, H. Peter Anvin >wrote: >>> On 12/11/15 13:48, Andy Lutomirski wrote: >>>> On Fri, Dec 11, 2015 at 1:11 PM, Eric W. Biederman >>>> wrote: >>>>> Al Viro 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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/