[Devel,rh7] fs: hold reference on original path

Submitted by Maxim Patlasov on Sept. 22, 2016, 12:15 a.m.

Details

Message ID 147450331278.16411.8409891487033844937.stgit@maxim-thinkpad
State New
Series "fs: hold reference on original path"
Headers show

Commit Message

Maxim Patlasov Sept. 22, 2016, 12:15 a.m.
struct file holds references on its f_path.mnt and f_path.dentry by calling
path_get(&f->f_path) from do_dentry_open(). Let's use the same technique
for f->f_original_path. Otherwise, f_original_path.dentry can be deleted while
file still references it leading to NULL-ptr-deref on f->f_original_path.dentry->d_inode.

https://jira.sw.ru/browse/PSBM-52373

Signed-off-by: Maxim Patlasov <mpatlasov@virtuozzo.com>
---
 fs/file_table.c |    6 ++++++
 fs/open.c       |   18 +++++++++++++++---
 2 files changed, 21 insertions(+), 3 deletions(-)

Patch hide | download patch | download mbox

diff --git a/fs/file_table.c b/fs/file_table.c
index 957c476..b8982d8 100644
--- a/fs/file_table.c
+++ b/fs/file_table.c
@@ -242,6 +242,8 @@  static void __fput(struct file *file)
 	struct dentry *dentry = file->f_path.dentry;
 	struct vfsmount *mnt = file->f_path.mnt;
 	struct inode *inode = dentry->d_inode;
+	struct dentry *original_dentry = file->f_original_path.dentry;
+	struct vfsmount *original_mnt = file->f_original_path.mnt;
 
 	might_sleep();
 
@@ -273,10 +275,14 @@  static void __fput(struct file *file)
 		drop_file_write_access(file);
 	file->f_path.dentry = NULL;
 	file->f_path.mnt = NULL;
+	file->f_original_path.dentry = NULL;
+	file->f_original_path.mnt = NULL;
 	file->f_inode = NULL;
 	file_free(file);
 	dput(dentry);
 	mntput(mnt);
+	dput(original_dentry);
+	mntput(original_mnt);
 }
 
 static DEFINE_SPINLOCK(delayed_fput_lock);
diff --git a/fs/open.c b/fs/open.c
index 8c066b1..25dbc85 100644
--- a/fs/open.c
+++ b/fs/open.c
@@ -893,16 +893,28 @@  int vfs_open(const struct path *path, struct file *filp,
 {
 	struct inode *inode = path->dentry->d_inode;
 	iop_dentry_open_t dentry_open = get_dentry_open_iop(inode);
+	int do_cleanup = 0;
+	int ret;
 
-	if (!filp->f_original_path.mnt)
+	if (!filp->f_original_path.mnt) {
 		filp->f_original_path = *path;
+		path_get(&filp->f_original_path);
+		do_cleanup = 1;
+	}
 
 	if (dentry_open)
-		return dentry_open(path->dentry, filp, cred);
+		ret = dentry_open(path->dentry, filp, cred);
 	else {
 		filp->f_path = *path;
-		return do_dentry_open(filp, NULL, cred);
+		ret = do_dentry_open(filp, NULL, cred);
 	}
+
+	if (ret && do_cleanup) {
+		path_put(&filp->f_original_path);
+		filp->f_original_path.mnt = NULL;
+		filp->f_original_path.dentry = NULL;
+	}
+	return ret;
 }
 EXPORT_SYMBOL(vfs_open);
 

Comments

Konstantin Khorenko Sept. 22, 2016, 8:07 a.m.
Kirill, please review.

--
Best regards,

Konstantin Khorenko,
Virtuozzo Linux Kernel Team

On 09/22/2016 03:15 AM, Maxim Patlasov wrote:
> struct file holds references on its f_path.mnt and f_path.dentry by calling
> path_get(&f->f_path) from do_dentry_open(). Let's use the same technique
> for f->f_original_path. Otherwise, f_original_path.dentry can be deleted while
> file still references it leading to NULL-ptr-deref on f->f_original_path.dentry->d_inode.
>
> https://jira.sw.ru/browse/PSBM-52373
>
> Signed-off-by: Maxim Patlasov <mpatlasov@virtuozzo.com>
> ---
>  fs/file_table.c |    6 ++++++
>  fs/open.c       |   18 +++++++++++++++---
>  2 files changed, 21 insertions(+), 3 deletions(-)
>
> diff --git a/fs/file_table.c b/fs/file_table.c
> index 957c476..b8982d8 100644
> --- a/fs/file_table.c
> +++ b/fs/file_table.c
> @@ -242,6 +242,8 @@ static void __fput(struct file *file)
>  	struct dentry *dentry = file->f_path.dentry;
>  	struct vfsmount *mnt = file->f_path.mnt;
>  	struct inode *inode = dentry->d_inode;
> +	struct dentry *original_dentry = file->f_original_path.dentry;
> +	struct vfsmount *original_mnt = file->f_original_path.mnt;
>
>  	might_sleep();
>
> @@ -273,10 +275,14 @@ static void __fput(struct file *file)
>  		drop_file_write_access(file);
>  	file->f_path.dentry = NULL;
>  	file->f_path.mnt = NULL;
> +	file->f_original_path.dentry = NULL;
> +	file->f_original_path.mnt = NULL;
>  	file->f_inode = NULL;
>  	file_free(file);
>  	dput(dentry);
>  	mntput(mnt);
> +	dput(original_dentry);
> +	mntput(original_mnt);
>  }
>
>  static DEFINE_SPINLOCK(delayed_fput_lock);
> diff --git a/fs/open.c b/fs/open.c
> index 8c066b1..25dbc85 100644
> --- a/fs/open.c
> +++ b/fs/open.c
> @@ -893,16 +893,28 @@ int vfs_open(const struct path *path, struct file *filp,
>  {
>  	struct inode *inode = path->dentry->d_inode;
>  	iop_dentry_open_t dentry_open = get_dentry_open_iop(inode);
> +	int do_cleanup = 0;
> +	int ret;
>
> -	if (!filp->f_original_path.mnt)
> +	if (!filp->f_original_path.mnt) {
>  		filp->f_original_path = *path;
> +		path_get(&filp->f_original_path);
> +		do_cleanup = 1;
> +	}
>
>  	if (dentry_open)
> -		return dentry_open(path->dentry, filp, cred);
> +		ret = dentry_open(path->dentry, filp, cred);
>  	else {
>  		filp->f_path = *path;
> -		return do_dentry_open(filp, NULL, cred);
> +		ret = do_dentry_open(filp, NULL, cred);
>  	}
> +
> +	if (ret && do_cleanup) {
> +		path_put(&filp->f_original_path);
> +		filp->f_original_path.mnt = NULL;
> +		filp->f_original_path.dentry = NULL;
> +	}
> +	return ret;
>  }
>  EXPORT_SYMBOL(vfs_open);
>
>
> .
>
Kirill Tkhai Sept. 22, 2016, 12:41 p.m.
On 22.09.2016 03:15, Maxim Patlasov wrote:
> struct file holds references on its f_path.mnt and f_path.dentry by calling
> path_get(&f->f_path) from do_dentry_open(). Let's use the same technique
> for f->f_original_path. Otherwise, f_original_path.dentry can be deleted while
> file still references it leading to NULL-ptr-deref on f->f_original_path.dentry->d_inode.
> 
> https://jira.sw.ru/browse/PSBM-52373
> 
> Signed-off-by: Maxim Patlasov <mpatlasov@virtuozzo.com>

Reviewed-by: Kirill Tkhai <ktkhai@virtuozzo.com>

> ---
>  fs/file_table.c |    6 ++++++
>  fs/open.c       |   18 +++++++++++++++---
>  2 files changed, 21 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/file_table.c b/fs/file_table.c
> index 957c476..b8982d8 100644
> --- a/fs/file_table.c
> +++ b/fs/file_table.c
> @@ -242,6 +242,8 @@ static void __fput(struct file *file)
>  	struct dentry *dentry = file->f_path.dentry;
>  	struct vfsmount *mnt = file->f_path.mnt;
>  	struct inode *inode = dentry->d_inode;
> +	struct dentry *original_dentry = file->f_original_path.dentry;
> +	struct vfsmount *original_mnt = file->f_original_path.mnt;
>  
>  	might_sleep();
>  
> @@ -273,10 +275,14 @@ static void __fput(struct file *file)
>  		drop_file_write_access(file);
>  	file->f_path.dentry = NULL;
>  	file->f_path.mnt = NULL;
> +	file->f_original_path.dentry = NULL;
> +	file->f_original_path.mnt = NULL;
>  	file->f_inode = NULL;
>  	file_free(file);
>  	dput(dentry);
>  	mntput(mnt);
> +	dput(original_dentry);
> +	mntput(original_mnt);
>  }
>  
>  static DEFINE_SPINLOCK(delayed_fput_lock);
> diff --git a/fs/open.c b/fs/open.c
> index 8c066b1..25dbc85 100644
> --- a/fs/open.c
> +++ b/fs/open.c
> @@ -893,16 +893,28 @@ int vfs_open(const struct path *path, struct file *filp,
>  {
>  	struct inode *inode = path->dentry->d_inode;
>  	iop_dentry_open_t dentry_open = get_dentry_open_iop(inode);
> +	int do_cleanup = 0;
> +	int ret;
>  
> -	if (!filp->f_original_path.mnt)
> +	if (!filp->f_original_path.mnt) {
>  		filp->f_original_path = *path;
> +		path_get(&filp->f_original_path);
> +		do_cleanup = 1;
> +	}
>  
>  	if (dentry_open)
> -		return dentry_open(path->dentry, filp, cred);
> +		ret = dentry_open(path->dentry, filp, cred);
>  	else {
>  		filp->f_path = *path;
> -		return do_dentry_open(filp, NULL, cred);
> +		ret = do_dentry_open(filp, NULL, cred);
>  	}
> +
> +	if (ret && do_cleanup) {
> +		path_put(&filp->f_original_path);
> +		filp->f_original_path.mnt = NULL;
> +		filp->f_original_path.dentry = NULL;
> +	}
> +	return ret;
>  }
>  EXPORT_SYMBOL(vfs_open);
>  
>