Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754242AbbLKWom (ORCPT ); Fri, 11 Dec 2015 17:44:42 -0500 Received: from out03.mta.xmission.com ([166.70.13.233]:52215 "EHLO out03.mta.xmission.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750724AbbLKWok (ORCPT ); Fri, 11 Dec 2015 17:44:40 -0500 From: ebiederm@xmission.com (Eric W. Biederman) To: Andy Lutomirski Cc: "H. Peter Anvin" , 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" 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> Date: Fri, 11 Dec 2015 16:35:16 -0600 In-Reply-To: (Andy Lutomirski's message of "Fri, 11 Dec 2015 14:12:27 -0800") Message-ID: <87y4d0sjff.fsf@x220.int.ebiederm.org> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/24.5 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain X-XM-AID: U2FsdGVkX1+kv7afv+G0MJ7U/fko81LAkhGlqJAlQdY= X-SA-Exim-Connect-IP: 70.59.167.217 X-SA-Exim-Mail-From: ebiederm@xmission.com X-Spam-Report: * -1.0 ALL_TRUSTED Passed through trusted hosts only via SMTP * 0.0 TVD_RCVD_IP Message was received from an IP address * 0.0 T_TM2_M_HEADER_IN_MSG BODY: No description available. * 0.8 BAYES_50 BODY: Bayes spam probability is 40 to 60% * [score: 0.5000] * -0.0 DCC_CHECK_NEGATIVE Not listed in DCC * [sa06 1397; Body=1 Fuz1=1 Fuz2=1] X-Spam-DCC: XMission; sa06 1397; Body=1 Fuz1=1 Fuz2=1 X-Spam-Combo: ;Andy Lutomirski X-Spam-Relay-Country: X-Spam-Timing: total 3880 ms - load_scoreonly_sql: 1.86 (0.0%), signal_user_changed: 5 (0.1%), b_tie_ro: 3.3 (0.1%), parse: 1.40 (0.0%), extract_message_metadata: 24 (0.6%), get_uri_detail_list: 3.5 (0.1%), tests_pri_-1000: 11 (0.3%), tests_pri_-950: 1.35 (0.0%), tests_pri_-900: 1.14 (0.0%), tests_pri_-400: 37 (1.0%), check_bayes: 36 (0.9%), b_tokenize: 12 (0.3%), b_tok_get_all: 10 (0.3%), b_comp_prob: 3.1 (0.1%), b_tok_touch_all: 6 (0.2%), b_finish: 2.1 (0.1%), tests_pri_0: 405 (10.4%), check_dkim_signature: 0.68 (0.0%), check_dkim_adsp: 3.1 (0.1%), tests_pri_500: 3388 (87.3%), poll_dns_idle: 3375 (87.0%), rewrite_mail: 0.00 (0.0%) Subject: Re: [PATCH] devpts: Sensible /dev/ptmx & force newinstance X-Spam-Flag: No X-SA-Exim-Version: 4.2.1 (built Wed, 24 Sep 2014 11:00:52 -0600) X-SA-Exim-Scanned: Yes (on in01.mta.xmission.com) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 5072 Lines: 140 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 *); -- 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/