Message-ID: <87y4d0sjff.fsf@x220.int.ebiederm.org>
Date:	Fri, 11 Dec 2015 16:35:16 -0600
From:	ebiederm@...ssion.com (Eric W. Biederman)
To:	Andy Lutomirski <luto@...capital.net>
Cc:	"H. Peter Anvin" <hpa@...or.com>,
	Al Viro <viro@...iv.linux.org.uk>, Greg KH <greg@...ah.com>,
	Jiri Slaby <jslaby@...e.com>,
	Linus Torvalds <torvalds@...ux-foundation.org>,
	Aurelien Jarno <aurelien@...el32.net>,
	Florian Weimer <fw@...eb.enyo.de>,
	Serge Hallyn <serge.hallyn@...ntu.com>,
	Jann Horn <jann@...jh.net>,
	"security\@kernel.org" <security@...nel.org>,
	"security\@ubuntu.com \>\> security" <security@...ntu.com>,
	security@...ian.org, Willy Tarreau <w@....eu>,
	"linux-kernel\@vger.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] devpts: Sensible /dev/ptmx & force newinstance

Andy Lutomirski <luto@...capital.net> writes:

> On Fri, Dec 11, 2015 at 2:07 PM, H. Peter Anvin <hpa@...or.com> wrote:
>> On 12/11/15 13:48, Andy Lutomirski wrote:
>>> On Fri, Dec 11, 2015 at 1:11 PM, Eric W. Biederman
>>> <ebiederm@...ssion.com> wrote:
>>>> Al Viro <viro@...IV.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 *);

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists