Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751241AbdHXDYi (ORCPT ); Wed, 23 Aug 2017 23:24:38 -0400 Received: from mail-oi0-f68.google.com ([209.85.218.68]:33101 "EHLO mail-oi0-f68.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751207AbdHXDYh (ORCPT ); Wed, 23 Aug 2017 23:24:37 -0400 MIME-Version: 1.0 In-Reply-To: <87wp5tfynr.fsf@xmission.com> 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> <87wp5tfynr.fsf@xmission.com> From: Linus Torvalds Date: Wed, 23 Aug 2017 20:24:35 -0700 X-Google-Sender-Auth: ZA84LlVqPv3qzmjd0Of10dmnbyE Message-ID: Subject: Re: [PATCH 0/1] devpts: use dynamic_dname() to generate proc name To: "Eric W. Biederman" Cc: Stefan Lippers-Hollmann , Christian Brauner , Christian Brauner , Linux Kernel Mailing List , "Serge E. Hallyn" , Al Viro , Thorsten Leemhuis Content-Type: text/plain; charset="UTF-8" Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2370 Lines: 74 On Wed, Aug 23, 2017 at 8:11 PM, Eric W. Biederman wrote: > -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; No. Afaik, that could be a legact PTY, which wouldn't be ok. I think you need to do if (tty->driver != ptm_driver) return -EIO; which should check both that it's the unix98 pty, and that it's the master. Maybe I'm missing something. That check used to be implicit, in that only the unix98 pty's could reach that pty_unix98_ioctl() function, so then testing just that it was a master was sufficient. > - /* 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; We used to do "path_get()". Shouldn't we now use "dget()"? But maybe the slave dentry is guaranteed to be around and we don't need to do that. So your approach may be fine. You did remove all the path_put() calls too, so I guess it all matches up. So this looks like it could be fine, but I'd like to make sure. > +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; > +} That can't be right. You're leaking the dentry that you're not returning, no? But yes, apart from those comments, this looks like what I envisioned. Needs testing, and needs more looking at those reference counts, but otherwise looks good. And while the patch is a bit bigger, I do like getting rid of that 'struct path' thing, and keeping just the dentry. Linus