From: ebiederm@xmission.com (Eric W. Biederman)
To: Linus Torvalds <torvalds@linux-foundation.org>
Cc: 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>
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>
Date: Wed, 16 Aug 2017 17:46:46 -0500
In-Reply-To: <CA+55aFxXzwoDdN72m2oaV4XuxO0DGoT+Bp84i07VFpLQ3Ftcpw@mail.gmail.com>
        (Linus Torvalds's message of "Wed, 16 Aug 2017 14:03:14 -0700")
Message-ID: <87pobvruzt.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: 2323
Lines: 75

Linus Torvalds <torvalds@linux-foundation.org> writes:

> On Wed, Aug 16, 2017 at 1:30 PM, Linus Torvalds
> <torvalds@linux-foundation.org> wrote:
>>
>> I suspect the easiest fix is to just add a "mnt" argument to
>> devpts_acquire(),  It shouldn't be too painful. Let me try.
>
> Ok, here's a *very* lightly tested patch. It might have new bugs, but
> it makes your test program DTRT.
>
> Al, mind going over this and making sure I didn't miss anything?
>
> And Christian, if you can beat on this, that would be good.

Linus reading through this it looks like your error handling is wrong.

> diff --git a/drivers/tty/pty.c b/drivers/tty/pty.c
> index 284749fb0f6b..432f514e3f42 100644
> --- a/drivers/tty/pty.c
> +++ b/drivers/tty/pty.c
> @@ -793,6 +793,7 @@ static int ptmx_open(struct inode *inode, struct file *filp)
>  	struct tty_struct *tty;
>  	struct path *pts_path;
>  	struct dentry *dentry;
> +	struct vfsmount *mnt;
>  	int retval;
>  	int index;
>  
> @@ -805,7 +806,7 @@ static int ptmx_open(struct inode *inode, struct file *filp)
>  	if (retval)
>  		return retval;
>  
> -	fsi = devpts_acquire(filp);
> +	fsi = devpts_acquire(filp, &mnt);
>  	if (IS_ERR(fsi)) {
>  		retval = PTR_ERR(fsi);
>  		goto out_free_file;
> @@ -849,9 +850,14 @@ static int ptmx_open(struct inode *inode, struct file *filp)
>  	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);
> +
> +	/*
> +	 * The mnt already got a ref from devpts_acquire(),
> +	 * so we only dget() on the dentry.
> +	 */
> +	pts_path->mnt = mnt;
> +	pts_path->dentry = dget(dentry);
> +
>  	tty->link->driver_data = pts_path;
>  
>  	retval = ptm_driver->ops->open(tty, filp);
        ^^^^^^^

If this open fails the code jumps to err_put_path which falls
through into out_put_fsi.  But it also does path_put(pts_path).
Which will result in a double mntput of mnt.

So I think err_path_put needs to be updated to just put the dentry,
and let the later mntput put the mount.

> @@ -874,6 +880,7 @@ static int ptmx_open(struct inode *inode, struct file *filp)
>  	devpts_kill_index(fsi, index);
>  out_put_fsi:
>  	devpts_release(fsi);
> +	mntput(mnt);
>  out_free_file:
>  	tty_free_file(filp);
>  	return retval;

Eric