Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751189AbdHXDL2 (ORCPT ); Wed, 23 Aug 2017 23:11:28 -0400 Received: from out01.mta.xmission.com ([166.70.13.231]:57882 "EHLO out01.mta.xmission.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751124AbdHXDL0 (ORCPT ); Wed, 23 Aug 2017 23:11:26 -0400 From: ebiederm@xmission.com (Eric W. Biederman) To: Linus Torvalds Cc: Stefan Lippers-Hollmann , Christian Brauner , Christian Brauner , Linux Kernel Mailing List , "Serge E. Hallyn" , Al Viro , Thorsten Leemhuis References: <20170816171211.4021-1-christian.brauner@ubuntu.com> <20170816194805.hnof3aqiqykwki7p@gmail.com> <87pobvruzt.fsf@xmission.com> <87ziazqdfr.fsf@xmission.com> <20170824022436.44adb497@mir> <87378hhi3y.fsf@xmission.com> Date: Wed, 23 Aug 2017 22:11:04 -0500 In-Reply-To: (Linus Torvalds's message of "Wed, 23 Aug 2017 19:01:10 -0700") Message-ID: <87wp5tfynr.fsf@xmission.com> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/25.1 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain X-XM-SPF: eid=1dkiY9-0003TB-Vb;;;mid=<87wp5tfynr.fsf@xmission.com>;;;hst=in01.mta.xmission.com;;;ip=67.3.200.44;;;frm=ebiederm@xmission.com;;;spf=neutral X-XM-AID: U2FsdGVkX19FCdmJvtsVdoJ0Irjf4PDrSfA7tA/sO20= X-SA-Exim-Connect-IP: 67.3.200.44 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.7 XMSubLong Long Subject * 1.5 XMNoVowels Alpha-numberic number with no vowels * 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 * [sa01 1397; Body=1 Fuz1=1 Fuz2=1] * 0.0 T_TooManySym_02 5+ unique symbols in subject * 0.0 T_TooManySym_03 6+ unique symbols in subject * 0.0 T_TooManySym_01 4+ unique symbols in subject X-Spam-DCC: XMission; sa01 1397; Body=1 Fuz1=1 Fuz2=1 X-Spam-Combo: **;Linus Torvalds X-Spam-Relay-Country: X-Spam-Timing: total 5331 ms - load_scoreonly_sql: 0.06 (0.0%), signal_user_changed: 3.4 (0.1%), b_tie_ro: 2.3 (0.0%), parse: 2.2 (0.0%), extract_message_metadata: 34 (0.6%), get_uri_detail_list: 10 (0.2%), tests_pri_-1000: 11 (0.2%), tests_pri_-950: 2.1 (0.0%), tests_pri_-900: 1.69 (0.0%), tests_pri_-400: 63 (1.2%), check_bayes: 60 (1.1%), b_tokenize: 32 (0.6%), b_tok_get_all: 14 (0.3%), b_comp_prob: 7 (0.1%), b_tok_touch_all: 4.0 (0.1%), b_finish: 0.81 (0.0%), tests_pri_0: 2328 (43.7%), check_dkim_signature: 1.18 (0.0%), check_dkim_adsp: 4.3 (0.1%), tests_pri_500: 2879 (54.0%), poll_dns_idle: 2871 (53.8%), rewrite_mail: 0.00 (0.0%) Subject: Re: [PATCH 0/1] devpts: use dynamic_dname() to generate proc name X-Spam-Flag: No X-SA-Exim-Version: 4.2.1 (built Thu, 05 May 2016 13:38:54 -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: 8795 Lines: 304 Linus Torvalds writes: > On Wed, Aug 23, 2017 at 6:49 PM, Linus Torvalds > 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