From: ebiederm@xmission.com (Eric W. Biederman)
To: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Stefan Lippers-Hollmann <s.l-h@gmx.de>,
        Christian Brauner <christian.brauner@canonical.com>,
        Christian Brauner <christian.brauner@ubuntu.com>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        "Serge E. Hallyn" <serge@hallyn.com>,
        Al Viro <viro@zeniv.linux.org.uk>,
        Thorsten Leemhuis <regressions@leemhuis.info>
References: <20170816171211.4021-1-christian.brauner@ubuntu.com>
        <CA+55aFzqedKjjq+FDF3AEgyQfqQRHyR-6PTR_BTvyyQLqnPwPw@mail.gmail.com>
        <CA+55aFyy1RU2emO6giU9gNVb_doYP+K_y61ij2cumgs1_VMMmA@mail.gmail.com>
        <20170816194805.hnof3aqiqykwki7p@gmail.com>
        <CA+55aFzmcydKsuwXpocd0hYMrACEXt+UUuaqa7j3L5LG7SjajQ@mail.gmail.com>
        <CA+55aFxMnEQVq8zRG1JEsW+6x23qkBV=KKXksgVHmBegPuepMg@mail.gmail.com>
        <CA+55aFz+qGxM9MidcvQkP4er-qn0_oEqCac=ob64f+twhptSgQ@mail.gmail.com>
        <CA+55aFxXzwoDdN72m2oaV4XuxO0DGoT+Bp84i07VFpLQ3Ftcpw@mail.gmail.com>
        <87pobvruzt.fsf@xmission.com>
        <CA+55aFyJsxsxeW-8RX83AM2zs+_i8h85i99X+YkuFEqpqjtp4w@mail.gmail.com>
        <87ziazqdfr.fsf@xmission.com>
        <CA+55aFwssaZpapQdc2iVVmMt0KxTHYdeo71Xbp4Ex-pYP_MpMw@mail.gmail.com>
        <20170824022436.44adb497@mir>
        <CA+55aFzHa0vL2Yui05r6aSQM0-oHuUAjqi1kxheG_tFQRkUGYw@mail.gmail.com>
        <87378hhi3y.fsf@xmission.com>
        <CA+55aFxhg8=KMZDtF-TrVjb3fPNvSWUveuRy1HHGuWXQ3+YzwQ@mail.gmail.com>
        <CA+55aFw737C9OVC2nQgGcazhi7Ca2K-frzgN2MiD=XqqFAp8YA@mail.gmail.com>
        <CA+55aFwvwtFCH2THn1LirOpMsZ57pnNi61nXQfQ6NBgakdXpTQ@mail.gmail.com>
        <87wp5tfynr.fsf@xmission.com>
        <CA+55aFxnDoH4RLHTvkGwg59LCDRh+r780b2MXCo3=Hb2pSfLww@mail.gmail.com>
Date: Thu, 24 Aug 2017 10:51:05 -0500
In-Reply-To: <CA+55aFxnDoH4RLHTvkGwg59LCDRh+r780b2MXCo3=Hb2pSfLww@mail.gmail.com>
        (Linus Torvalds's message of "Wed, 23 Aug 2017 20:24:35 -0700")
Message-ID: <87efs1ezh2.fsf@xmission.com>
User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/25.1 (gnu/linux)
MIME-Version: 1.0
Content-Type: text/plain
Subject: Re: [PATCH 0/1] devpts: use dynamic_dname() to generate proc name
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 3080
Lines: 91

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