Linus Torvalds <torva...@linux-foundation.org> writes: > On Wed, Aug 23, 2017 at 6:49 PM, Linus Torvalds > <torva...@linux-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