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>
        <87pobvruzt.fsf@xmission.com>
        <CA+55aFyJsxsxeW-8RX83AM2zs+_i8h85i99X+YkuFEqpqjtp4w@mail.gmail.com>
        <87ziazqdfr.fsf@xmission.com>
        <CA+55aFwssaZpapQdc2iVVmMt0KxTHYdeo71Xbp4Ex-pYP_MpMw@mail.gmail.com>
Date: Wed, 16 Aug 2017 20:24:23 -0500
In-Reply-To: <CA+55aFwssaZpapQdc2iVVmMt0KxTHYdeo71Xbp4Ex-pYP_MpMw@mail.gmail.com>
        (Linus Torvalds's message of "Wed, 16 Aug 2017 17:08:07 -0700")
Message-ID: <87o9rfq94o.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: 3948
Lines: 125

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

> On Wed, Aug 16, 2017 at 4:51 PM, Eric W. Biederman
> <ebiederm@xmission.com> wrote:
>>
>> *Blink* You are right I missed that.
>>
>> In which case I am concerned about failures that make it to err_release.
>> Unless I am missing something (again) failures that jump to err_release
>> won't call mntput and will result in a mnt leak.
>
> Yes, I think you're right.
>
> Maybe this attached patch is better anyway. It's smaller, because it
> keeps more closely to the old code, and just adds a mntput() in all
> the exit cases, and depends on the "path_get()" to have incremented
> the mnt refcount one extra time.
>
> Can you find something in this one?

My eyeballs don't see any problems with the patch below.

Acked-by: "Eric W. Biederman" <ebiederm@xmission.com>


Eric


> ENTIRELY UNTESTED!
>
>                Linus
>
>  drivers/tty/pty.c         | 7 +++++--
>  fs/devpts/inode.c         | 4 +++-
>  include/linux/devpts_fs.h | 2 +-
>  3 files changed, 9 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/tty/pty.c b/drivers/tty/pty.c
> index 284749fb0f6b..1fc80ea87c13 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,7 +850,7 @@ 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->mnt = mnt;
>  	pts_path->dentry = dentry;
>  	path_get(pts_path);
>  	tty->link->driver_data = pts_path;
> @@ -866,6 +867,7 @@ static int ptmx_open(struct inode *inode, struct file *filp)
>  	path_put(pts_path);
>  	kfree(pts_path);
>  err_release:
> +	mntput(mnt);
>  	tty_unlock(tty);
>  	// This will also put-ref the fsi
>  	tty_release(inode, filp);
> @@ -874,6 +876,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;
> diff --git a/fs/devpts/inode.c b/fs/devpts/inode.c
> index 108df2e3602c..44dfbca9306f 100644
> --- a/fs/devpts/inode.c
> +++ b/fs/devpts/inode.c
> @@ -133,7 +133,7 @@ static inline struct pts_fs_info *DEVPTS_SB(struct super_block *sb)
>  	return sb->s_fs_info;
>  }
>  
> -struct pts_fs_info *devpts_acquire(struct file *filp)
> +struct pts_fs_info *devpts_acquire(struct file *filp, struct vfsmount **ptsmnt)
>  {
>  	struct pts_fs_info *result;
>  	struct path path;
> @@ -142,6 +142,7 @@ struct pts_fs_info *devpts_acquire(struct file *filp)
>  
>  	path = filp->f_path;
>  	path_get(&path);
> +	*ptsmnt = NULL;
>  
>  	/* Has the devpts filesystem already been found? */
>  	sb = path.mnt->mnt_sb;
> @@ -165,6 +166,7 @@ struct pts_fs_info *devpts_acquire(struct file *filp)
>  	 * pty code needs to hold extra references in case of last /dev/tty close
>  	 */
>  	atomic_inc(&sb->s_active);
> +	*ptsmnt = mntget(path.mnt);
>  	result = DEVPTS_SB(sb);
>  
>  out:
> diff --git a/include/linux/devpts_fs.h b/include/linux/devpts_fs.h
> index 277ab9af9ac2..7883e901f65c 100644
> --- a/include/linux/devpts_fs.h
> +++ b/include/linux/devpts_fs.h
> @@ -19,7 +19,7 @@
>  
>  struct pts_fs_info;
>  
> -struct pts_fs_info *devpts_acquire(struct file *);
> +struct pts_fs_info *devpts_acquire(struct file *, struct vfsmount **ptsmnt);
>  void devpts_release(struct pts_fs_info *);
>  
>  int devpts_new_index(struct pts_fs_info *);