Message-ID: <87wp5tfynr.fsf@xmission.com>
Date:   Wed, 23 Aug 2017 22:11:04 -0500
From:   ebiederm@...ssion.com (Eric W. Biederman)
To:     Linus Torvalds <torvalds@...ux-foundation.org>
Cc:     Stefan Lippers-Hollmann <s.l-h@....de>,
        Christian Brauner <christian.brauner@...onical.com>,
        Christian Brauner <christian.brauner@...ntu.com>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        "Serge E. Hallyn" <serge@...lyn.com>,
        Al Viro <viro@...iv.linux.org.uk>,
        Thorsten Leemhuis <regressions@...mhuis.info>
Subject: Re: [PATCH 0/1] devpts: use dynamic_dname() to generate proc name

Linus Torvalds <torvalds@...ux-foundation.org> writes:

> On Wed, Aug 23, 2017 at 6:49 PM, Linus Torvalds
> <torvalds@...ux-foundation.org> wrote:
>>
>> Argh. And it's *not* fairly straightforward, because the
>> tty_operations "ioctl()" function pointer only gets 'struct tty *'.
>>
>> So in the TIOCGPTPEER path, we don't actually have access to the file
>> pointer of the fd we're doing the ioctl on.
>>
>> And that's where the 'struct path' to the 'ptmx' node is - which we
>> need to then look up the 'pts' directory.
>>
>> How very annoying. I think that's why we did it all at ptmx_open()
>> time, because then we had all the information.
>
> Anyway, the revert is pushed out. So we're back to the old behavior
> that gives the wrong pathname in /proc.
>
> And I think I can handle the lack of a 'struct file *' to the ioctl
> operations by just special-casing TIOCGPTPEER directly in tty_ioctl()
> itself.
>
> That's where we handle "generic" tty ioctls, and doing pty stuff there
> is kind of wrong, but pty's are special.
>
> But I think I'll leave it for tomorrow. So Eric, if you feel like
> looking at this, I'd appreciate it.

This is so far untested (except for compiling) but I think this will
work.

I factor out devpts_ptmx_path out of devpts_acquire so the code
doesn't have to do unnecessary and confusing work, and add the
new function devpts_mnt.

I revert the code to keep anything except a dentry in
tty->link->driver_data.

And reduce the peer opening to a single function ptm_open_peer.

It takes lines of code but the result is very straightforward code.

Eric


 drivers/tty/pty.c         | 63 ++++++++++++++++++++---------------------------
 drivers/tty/tty_io.c      |  3 +++
 fs/devpts/inode.c         | 60 +++++++++++++++++++++++++++++++++-----------
 include/linux/devpts_fs.h | 10 ++++++++
 4 files changed, 85 insertions(+), 51 deletions(-)
diff --git a/drivers/tty/pty.c b/drivers/tty/pty.c
index 284749fb0f6b..269e6ea65a33 100644
--- a/drivers/tty/pty.c
+++ b/drivers/tty/pty.c
@@ -69,13 +69,8 @@ static void pty_close(struct tty_struct *tty, struct file *filp)
 #ifdef CONFIG_UNIX98_PTYS
 		if (tty->driver == ptm_driver) {
 			mutex_lock(&devpts_mutex);
-			if (tty->link->driver_data) {
-				struct path *path = tty->link->driver_data;
-
-				devpts_pty_kill(path->dentry);
-				path_put(path);
-				kfree(path);
-			}
+			if (tty->link->driver_data)
+				devpts_pty_kill(tty->link->driver_data);
 			mutex_unlock(&devpts_mutex);
 		}
 #endif
@@ -607,25 +602,25 @@ static inline void legacy_pty_init(void) { }
 static struct cdev ptmx_cdev;
 
 /**
- *	pty_open_peer - open the peer of a pty
- *	@tty: the peer of the pty being opened
+ *	ptm_open_peer - open the peer of a pty
+ *	@master: the open struct file of the ptmx device node
+ *	@tty: the master of the pty being opened
+ *	@flags: the flags for open
  *
- *	Open the cached dentry in tty->link, providing a safe way for userspace
- *	to get the slave end of a pty (where they have the master fd and cannot
- *	access or trust the mount namespace /dev/pts was mounted inside).
+ *	Provide a race free way for userspace to open the slave end of a pty
+ *	(where they have the master fd and cannot access or trust the mount
+ *	namespace /dev/pts was mounted inside).
  */
-static struct file *pty_open_peer(struct tty_struct *tty, int flags)
-{
-	if (tty->driver->subtype != PTY_TYPE_MASTER)
-		return ERR_PTR(-EIO);
-	return dentry_open(tty->link->driver_data, flags, current_cred());
-}
-
-static int pty_get_peer(struct tty_struct *tty, int flags)
+int ptm_open_peer(struct file *master, struct tty_struct *tty, int flags)
 {
 	int fd = -1;
 	struct file *filp = NULL;
 	int retval = -EINVAL;
+	struct path path;
+
+	if ((tty->driver->type != TTY_DRIVER_TYPE_PTY) ||
+	    (tty->driver->subtype != PTY_TYPE_MASTER))
+		return -EIO;
 
 	fd = get_unused_fd_flags(0);
 	if (fd < 0) {
@@ -633,7 +628,16 @@ static int pty_get_peer(struct tty_struct *tty, int flags)
 		goto err;
 	}
 
-	filp = pty_open_peer(tty, flags);
+	/* Compute the slave's path */
+	path.mnt = devpts_mnt(filp);
+	if (IS_ERR(path.mnt)) {
+		retval = PTR_ERR(path.mnt);
+		goto err_put;
+	}
+	path.dentry = tty->link->driver_data;
+
+	filp = dentry_open(&path, flags, current_cred());
+	mntput(path.mnt);
 	if (IS_ERR(filp)) {
 		retval = PTR_ERR(filp);
 		goto err_put;
@@ -662,8 +666,6 @@ static int pty_unix98_ioctl(struct tty_struct *tty,
 		return pty_get_pktmode(tty, (int __user *)arg);
 	case TIOCGPTN: /* Get PT Number */
 		return put_user(tty->index, (unsigned int __user *)arg);
-	case TIOCGPTPEER: /* Open the other end */
-		return pty_get_peer(tty, (int) arg);
 	case TIOCSIG:    /* Send signal to other side of pty */
 		return pty_signal(tty, (int) arg);
 	}
@@ -791,7 +793,6 @@ static int ptmx_open(struct inode *inode, struct file *filp)
 {
 	struct pts_fs_info *fsi;
 	struct tty_struct *tty;
-	struct path *pts_path;
 	struct dentry *dentry;
 	int retval;
 	int index;
@@ -845,26 +846,16 @@ static int ptmx_open(struct inode *inode, struct file *filp)
 		retval = PTR_ERR(dentry);
 		goto err_release;
 	}
-	/* We need to cache a fake path for TIOCGPTPEER. */
-	pts_path = kmalloc(sizeof(struct path), GFP_KERNEL);
-	if (!pts_path)
-		goto err_release;
-	pts_path->mnt = filp->f_path.mnt;
-	pts_path->dentry = dentry;
-	path_get(pts_path);
-	tty->link->driver_data = pts_path;
+	tty->link->driver_data = dentry;
 
 	retval = ptm_driver->ops->open(tty, filp);
 	if (retval)
-		goto err_path_put;
+		goto err_release;
 
 	tty_debug_hangup(tty, "opening (count=%d)\n", tty->count);
 
 	tty_unlock(tty);
 	return 0;
-err_path_put:
-	path_put(pts_path);
-	kfree(pts_path);
 err_release:
 	tty_unlock(tty);
 	// This will also put-ref the fsi
diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
index 974b13d24401..ba3371449a5c 100644
--- a/drivers/tty/tty_io.c
+++ b/drivers/tty/tty_io.c
@@ -2518,6 +2518,9 @@ long tty_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 	case TIOCSSERIAL:
 		tty_warn_deprecated_flags(p);
 		break;
+	case TIOCGPTPEER:
+		retval = ptm_open_peer(file, tty, (int)arg);
+		break;
 	default:
 		retval = tty_jobctrl_ioctl(tty, real_tty, file, cmd, arg);
 		if (retval != -ENOIOCTLCMD)
diff --git a/fs/devpts/inode.c b/fs/devpts/inode.c
index 108df2e3602c..6e8816cf7d54 100644
--- a/fs/devpts/inode.c
+++ b/fs/devpts/inode.c
@@ -133,37 +133,67 @@ static inline struct pts_fs_info *DEVPTS_SB(struct super_block *sb)
 	return sb->s_fs_info;
 }
 
-struct pts_fs_info *devpts_acquire(struct file *filp)
+static int devpts_ptmx_path(struct path *path)
 {
-	struct pts_fs_info *result;
-	struct path path;
 	struct super_block *sb;
-	int err;
-
-	path = filp->f_path;
-	path_get(&path);
+	int err = 0;
 
 	/* Has the devpts filesystem already been found? */
-	sb = path.mnt->mnt_sb;
+	sb = path->mnt->mnt_sb;
 	if (sb->s_magic != DEVPTS_SUPER_MAGIC) {
 		/* Is a devpts filesystem at "pts" in the same directory? */
-		err = path_pts(&path);
-		if (err) {
-			result = ERR_PTR(err);
+		err = path_pts(path);
+		if (err)
 			goto out;
-		}
 
 		/* Is the path the root of a devpts filesystem? */
-		result = ERR_PTR(-ENODEV);
-		sb = path.mnt->mnt_sb;
+		err = -ENODEV;
+		sb = path->mnt->mnt_sb;
 		if ((sb->s_magic != DEVPTS_SUPER_MAGIC) ||
-		    (path.mnt->mnt_root != sb->s_root))
+		    (path->mnt->mnt_root != sb->s_root))
 			goto out;
 	}
 
+out:
+	return err;
+}
+
+struct vfsmount *devpts_mnt(struct file *filp)
+{
+	struct path path;
+	int err;
+
+	path = filp->f_path;
+	path_get(&path);
+
+	err = devpts_ptmx_path(&path);
+	if (err) {
+		path_put(&path);
+		path.mnt = ERR_PTR(err);
+	}
+	return path.mnt;
+}
+
+struct pts_fs_info *devpts_acquire(struct file *filp)
+{
+	struct pts_fs_info *result;
+	struct path path;
+	struct super_block *sb;
+	int err;
+
+	path = filp->f_path;
+	path_get(&path);
+
+	err = devpts_ptmx_path(&path);
+	if (err) {
+		result = ERR_PTR(err);
+		goto out;
+	}
+
 	/*
 	 * pty code needs to hold extra references in case of last /dev/tty close
 	 */
+	sb = path.mnt->mnt_sb;
 	atomic_inc(&sb->s_active);
 	result = DEVPTS_SB(sb);
 
diff --git a/include/linux/devpts_fs.h b/include/linux/devpts_fs.h
index 277ab9af9ac2..e27c548acfb0 100644
--- a/include/linux/devpts_fs.h
+++ b/include/linux/devpts_fs.h
@@ -19,6 +19,7 @@
 
 struct pts_fs_info;
 
+struct vfsmount *devpts_mnt(struct file *);
 struct pts_fs_info *devpts_acquire(struct file *);
 void devpts_release(struct pts_fs_info *);
 
@@ -32,6 +33,15 @@ void *devpts_get_priv(struct dentry *);
 /* unlink */
 void devpts_pty_kill(struct dentry *);
 
+/* in pty.c */
+int ptm_open_peer(struct file *master, struct tty_struct *tty, int flags);
+
+#else
+static inline int
+ptm_open_peer(struct file *master, struct tty_struct *tty, int flags)
+{
+	return -EIO;
+}
 #endif
 
 

Powered by blists - more mailing lists