Re: [PATCH 0/1] devpts: use dynamic_dname() to generate proc name

From: Eric W. Biederman
Date: Thu Aug 24 2017 - 11:51:34 EST


Linus Torvalds <torvalds@xxxxxxxxxxxxxxxxxxxx> writes:

> On Wed, Aug 23, 2017 at 8:11 PM, Eric W. Biederman
> <ebiederm@xxxxxxxxxxxx> 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