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

[ebiederm@xmission.com (Eric W. Biederman)]

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