diff mbox series

[07/20] ext4: drop ppath from ext4_ext_replay_update_ex() to avoid double-free

Message ID 20240710040654.1714672-8-libaokun@huaweicloud.com
State Superseded
Headers show
Series ext4: some bugfixes and cleanups for ext4 extents path | expand

Commit Message

Baokun Li July 10, 2024, 4:06 a.m. UTC 
From: Baokun Li <libaokun1@huawei.com>

When calling ext4_force_split_extent_at() in ext4_ext_replay_update_ex(),
the 'ppath' is updated but it is the 'path' that is freed, thus potentially
triggering a double-free in the following process:

ext4_ext_replay_update_ex
  ppath = path
  ext4_force_split_extent_at(&ppath)
    ext4_split_extent_at
      ext4_ext_insert_extent
        ext4_ext_create_new_leaf
          ext4_ext_grow_indepth
            ext4_find_extent
              if (depth > path[0].p_maxdepth)
                kfree(path)                 ---> path First freed
                *orig_path = path = NULL    ---> null ppath
  kfree(path)                               ---> path double-free !!!

So drop the unnecessary ppath and use path directly to avoid this problem.
And use ext4_find_extent() directly to update path, avoiding unnecessary
memory allocation and freeing. Also, propagate the error returned by
ext4_find_extent() instead of using strange error codes.

Fixes: 8016e29f4362 ("ext4: fast commit recovery path")
Cc: stable@kernel.org
Signed-off-by: Baokun Li <libaokun1@huawei.com>
---
 fs/ext4/extents.c | 21 ++++++++++-----------
 1 file changed, 10 insertions(+), 11 deletions(-)

Comments

Jan Kara July 25, 2024, 10:31 a.m. UTC | #1 
On Wed 10-07-24 12:06:41, libaokun@huaweicloud.com wrote:
> From: Baokun Li <libaokun1@huawei.com>
> 
> When calling ext4_force_split_extent_at() in ext4_ext_replay_update_ex(),
> the 'ppath' is updated but it is the 'path' that is freed, thus potentially
> triggering a double-free in the following process:
> 
> ext4_ext_replay_update_ex
>   ppath = path
>   ext4_force_split_extent_at(&ppath)
>     ext4_split_extent_at
>       ext4_ext_insert_extent
>         ext4_ext_create_new_leaf
>           ext4_ext_grow_indepth
>             ext4_find_extent
>               if (depth > path[0].p_maxdepth)
>                 kfree(path)                 ---> path First freed
>                 *orig_path = path = NULL    ---> null ppath
>   kfree(path)                               ---> path double-free !!!
> 
> So drop the unnecessary ppath and use path directly to avoid this problem.
> And use ext4_find_extent() directly to update path, avoiding unnecessary
> memory allocation and freeing. Also, propagate the error returned by
> ext4_find_extent() instead of using strange error codes.
> 
> Fixes: 8016e29f4362 ("ext4: fast commit recovery path")
> Cc: stable@kernel.org
> Signed-off-by: Baokun Li <libaokun1@huawei.com>

Nice! Feel free to add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza

> ---
>  fs/ext4/extents.c | 21 ++++++++++-----------
>  1 file changed, 10 insertions(+), 11 deletions(-)
> 
> diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
> index 1660434fbfc7..b1cfce5b57d2 100644
> --- a/fs/ext4/extents.c
> +++ b/fs/ext4/extents.c
> @@ -5920,7 +5920,7 @@ int ext4_clu_mapped(struct inode *inode, ext4_lblk_t lclu)
>  int ext4_ext_replay_update_ex(struct inode *inode, ext4_lblk_t start,
>  			      int len, int unwritten, ext4_fsblk_t pblk)
>  {
> -	struct ext4_ext_path *path = NULL, *ppath;
> +	struct ext4_ext_path *path;
>  	struct ext4_extent *ex;
>  	int ret;
>  
> @@ -5936,30 +5936,29 @@ int ext4_ext_replay_update_ex(struct inode *inode, ext4_lblk_t start,
>  	if (le32_to_cpu(ex->ee_block) != start ||
>  		ext4_ext_get_actual_len(ex) != len) {
>  		/* We need to split this extent to match our extent first */
> -		ppath = path;
>  		down_write(&EXT4_I(inode)->i_data_sem);
> -		ret = ext4_force_split_extent_at(NULL, inode, &ppath, start, 1);
> +		ret = ext4_force_split_extent_at(NULL, inode, &path, start, 1);
>  		up_write(&EXT4_I(inode)->i_data_sem);
>  		if (ret)
>  			goto out;
> -		kfree(path);
> -		path = ext4_find_extent(inode, start, NULL, 0);
> +
> +		path = ext4_find_extent(inode, start, &path, 0);
>  		if (IS_ERR(path))
> -			return -1;
> -		ppath = path;
> +			return PTR_ERR(path);
>  		ex = path[path->p_depth].p_ext;
>  		WARN_ON(le32_to_cpu(ex->ee_block) != start);
> +
>  		if (ext4_ext_get_actual_len(ex) != len) {
>  			down_write(&EXT4_I(inode)->i_data_sem);
> -			ret = ext4_force_split_extent_at(NULL, inode, &ppath,
> +			ret = ext4_force_split_extent_at(NULL, inode, &path,
>  							 start + len, 1);
>  			up_write(&EXT4_I(inode)->i_data_sem);
>  			if (ret)
>  				goto out;
> -			kfree(path);
> -			path = ext4_find_extent(inode, start, NULL, 0);
> +
> +			path = ext4_find_extent(inode, start, &path, 0);
>  			if (IS_ERR(path))
> -				return -EINVAL;
> +				return PTR_ERR(path);
>  			ex = path[path->p_depth].p_ext;
>  		}
>  	}
> -- 
> 2.39.2
>
Ojaswin Mujoo July 27, 2024, 11:18 a.m. UTC | #2 
On Wed, Jul 10, 2024 at 12:06:41PM +0800, libaokun@huaweicloud.com wrote:
> From: Baokun Li <libaokun1@huawei.com>
> 
> When calling ext4_force_split_extent_at() in ext4_ext_replay_update_ex(),
> the 'ppath' is updated but it is the 'path' that is freed, thus potentially
> triggering a double-free in the following process:
> 
> ext4_ext_replay_update_ex
>   ppath = path
>   ext4_force_split_extent_at(&ppath)
>     ext4_split_extent_at
>       ext4_ext_insert_extent
>         ext4_ext_create_new_leaf
>           ext4_ext_grow_indepth
>             ext4_find_extent
>               if (depth > path[0].p_maxdepth)
>                 kfree(path)                 ---> path First freed
>                 *orig_path = path = NULL    ---> null ppath
>   kfree(path)                               ---> path double-free !!!
> 
> So drop the unnecessary ppath and use path directly to avoid this problem.
> And use ext4_find_extent() directly to update path, avoiding unnecessary
> memory allocation and freeing. Also, propagate the error returned by
> ext4_find_extent() instead of using strange error codes.
> 
> Fixes: 8016e29f4362 ("ext4: fast commit recovery path")
> Cc: stable@kernel.org
> Signed-off-by: Baokun Li <libaokun1@huawei.com>

Hey Baokun, 
These ppath bugs are really subtle indeed :)

Anyways, feel free to add:

Reviewed-by: Ojaswin Mujoo <ojaswin@linux.ibm.com>


> ---
>  fs/ext4/extents.c | 21 ++++++++++-----------
>  1 file changed, 10 insertions(+), 11 deletions(-)
> 
> diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
> index 1660434fbfc7..b1cfce5b57d2 100644
> --- a/fs/ext4/extents.c
> +++ b/fs/ext4/extents.c
> @@ -5920,7 +5920,7 @@ int ext4_clu_mapped(struct inode *inode, ext4_lblk_t lclu)
>  int ext4_ext_replay_update_ex(struct inode *inode, ext4_lblk_t start,
>  			      int len, int unwritten, ext4_fsblk_t pblk)
>  {
> -	struct ext4_ext_path *path = NULL, *ppath;
> +	struct ext4_ext_path *path;
>  	struct ext4_extent *ex;
>  	int ret;
>  
> @@ -5936,30 +5936,29 @@ int ext4_ext_replay_update_ex(struct inode *inode, ext4_lblk_t start,
>  	if (le32_to_cpu(ex->ee_block) != start ||
>  		ext4_ext_get_actual_len(ex) != len) {
>  		/* We need to split this extent to match our extent first */
> -		ppath = path;
>  		down_write(&EXT4_I(inode)->i_data_sem);
> -		ret = ext4_force_split_extent_at(NULL, inode, &ppath, start, 1);
> +		ret = ext4_force_split_extent_at(NULL, inode, &path, start, 1);
>  		up_write(&EXT4_I(inode)->i_data_sem);
>  		if (ret)
>  			goto out;
> -		kfree(path);
> -		path = ext4_find_extent(inode, start, NULL, 0);
> +
> +		path = ext4_find_extent(inode, start, &path, 0);
>  		if (IS_ERR(path))
> -			return -1;
> -		ppath = path;
> +			return PTR_ERR(path);
>  		ex = path[path->p_depth].p_ext;
>  		WARN_ON(le32_to_cpu(ex->ee_block) != start);
> +
>  		if (ext4_ext_get_actual_len(ex) != len) {
>  			down_write(&EXT4_I(inode)->i_data_sem);
> -			ret = ext4_force_split_extent_at(NULL, inode, &ppath,
> +			ret = ext4_force_split_extent_at(NULL, inode, &path,
>  							 start + len, 1);
>  			up_write(&EXT4_I(inode)->i_data_sem);
>  			if (ret)
>  				goto out;
> -			kfree(path);
> -			path = ext4_find_extent(inode, start, NULL, 0);
> +
> +			path = ext4_find_extent(inode, start, &path, 0);
>  			if (IS_ERR(path))
> -				return -EINVAL;
> +				return PTR_ERR(path);
>  			ex = path[path->p_depth].p_ext;
>  		}
>  	}
> -- 
> 2.39.2
>
diff mbox series

Patch

diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
index 1660434fbfc7..b1cfce5b57d2 100644
--- a/fs/ext4/extents.c
+++ b/fs/ext4/extents.c
@@ -5920,7 +5920,7 @@  int ext4_clu_mapped(struct inode *inode, ext4_lblk_t lclu)
 int ext4_ext_replay_update_ex(struct inode *inode, ext4_lblk_t start,
 			      int len, int unwritten, ext4_fsblk_t pblk)
 {
-	struct ext4_ext_path *path = NULL, *ppath;
+	struct ext4_ext_path *path;
 	struct ext4_extent *ex;
 	int ret;
 
@@ -5936,30 +5936,29 @@  int ext4_ext_replay_update_ex(struct inode *inode, ext4_lblk_t start,
 	if (le32_to_cpu(ex->ee_block) != start ||
 		ext4_ext_get_actual_len(ex) != len) {
 		/* We need to split this extent to match our extent first */
-		ppath = path;
 		down_write(&EXT4_I(inode)->i_data_sem);
-		ret = ext4_force_split_extent_at(NULL, inode, &ppath, start, 1);
+		ret = ext4_force_split_extent_at(NULL, inode, &path, start, 1);
 		up_write(&EXT4_I(inode)->i_data_sem);
 		if (ret)
 			goto out;
-		kfree(path);
-		path = ext4_find_extent(inode, start, NULL, 0);
+
+		path = ext4_find_extent(inode, start, &path, 0);
 		if (IS_ERR(path))
-			return -1;
-		ppath = path;
+			return PTR_ERR(path);
 		ex = path[path->p_depth].p_ext;
 		WARN_ON(le32_to_cpu(ex->ee_block) != start);
+
 		if (ext4_ext_get_actual_len(ex) != len) {
 			down_write(&EXT4_I(inode)->i_data_sem);
-			ret = ext4_force_split_extent_at(NULL, inode, &ppath,
+			ret = ext4_force_split_extent_at(NULL, inode, &path,
 							 start + len, 1);
 			up_write(&EXT4_I(inode)->i_data_sem);
 			if (ret)
 				goto out;
-			kfree(path);
-			path = ext4_find_extent(inode, start, NULL, 0);
+
+			path = ext4_find_extent(inode, start, &path, 0);
 			if (IS_ERR(path))
-				return -EINVAL;
+				return PTR_ERR(path);
 			ex = path[path->p_depth].p_ext;
 		}
 	}