Messages in this thread | | | From | (Eric W. Biederman) | Date | Thu, 24 Aug 2017 10:51:05 -0500 | Subject | Re: [PATCH 0/1] devpts: use dynamic_dname() to generate proc name |
| |
Linus Torvalds <torvalds@linux-foundation.org> writes:
> On Wed, Aug 23, 2017 at 8:11 PM, Eric W. Biederman > <ebiederm@xmission.com> 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.
No. That seems correct. Change made. If nothing else it is cheaper and clearer so even if the other version wasn't wrong it is a good idea.
>> - /* 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.
That change is a revert to the old v4.12 code. So it is definitely not regression inducing.
Further devpts_pty_new allocates a dentry keeps it in the devpts filesystem. The dentry is good until devpts_pty_kill where the dentry is unlinked and killed.
I figure not differences from v4.12 if the logic hasn't changed seems a good good way to cut down on the search for bugs/regressions.
>> +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?
Correct. That is buggy. Will fix before I resend.
> 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.
Eric
| |