On Wed, Jul 10, 2024 at 12:06:41PM +0800, libaokun@xxxxxxxxxxxxxxx wrote:
> From: Baokun Li <libaokun1@xxxxxxxxxx>
>
> 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@xxxxxxxxxx
> Signed-off-by: Baokun Li <libaokun1@xxxxxxxxxx>
Hey Baokun,
These ppath bugs are really subtle indeed :)
Anyways, feel free to add:
Reviewed-by: Ojaswin Mujoo <ojaswin@xxxxxxxxxxxxx>
> ---
> 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
>
