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

[Linus Torvalds]

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.

> -       /* 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