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

[Linus Torvalds]

On Wed, Aug 16, 2017 at 4:51 PM, Eric W. Biederman
<ebiederm@xxxxxxxxxxxx> 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?

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 *);