Linus Torvalds <torva...@linux-foundation.org> writes:
> On Wed, Aug 16, 2017 at 4:51 PM, Eric W. Biederman
> <ebied...@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" <ebied...@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 *);
