On Wed, Jul 10, 2024 at 12:06:44PM +0800, libaokun@xxxxxxxxxxxxxxx wrote:
> From: Baokun Li <libaokun1@xxxxxxxxxx>
>
> The use of path and ppath is now very confusing, so to make the code more
> readable, pass path between functions uniformly, and get rid of ppath.
>
> To get rid of the ppath in ext4_ext_create_new_leaf(), the following is
> done here:
>
> * Free the extents path when an error is encountered.
> * Its caller needs to update ppath if it uses ppath.
>
> No functional changes.
>
> Signed-off-by: Baokun Li <libaokun1@xxxxxxxxxx>
Hi Baokun,
The changes look good to me, feel free to add:
Reviewed-by: Ojaswin Mujoo <ojaswin@xxxxxxxxxxxxx>
That being said, IIUC i think this patchset also fixes a potential UAF
bug. Below is a sample trace with dummy values:
ext4_ext_insert_extent
path = *ppath = 2000
ext4_ext_create_new_leaf(ppath)
path = *ppath = 2000
ext4_find_extent(path = 2000)
if (depth > path[0].p_maxdepth)
kfree(path = 2000);
path = NULL;
path = kcalloc() = 3000
...
return path;
path = 3000
*ppath = 3000;
return;
/* here path is still 2000 *, UAF! */
eh = path[depth].p_hdr
I'm not completely sure if we can hit (depth > path[0].p_maxdepth) in the
above codepath but I think the flow is still a bit fragile. Maybe this
should be fixed in a separate patch first. What do you think?
Regards,
ojaswin
---
> fs/ext4/extents.c | 41 +++++++++++++++++++++--------------------
> 1 file changed, 21 insertions(+), 20 deletions(-)
>
> diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
> index 6dfb5d03e197..0d6ce9e74b01 100644
> --- a/fs/ext4/extents.c
> +++ b/fs/ext4/extents.c
> @@ -1397,13 +1397,12 @@ static int ext4_ext_grow_indepth(handle_t *handle, struct inode *inode,
> * finds empty index and adds new leaf.
> * if no free index is found, then it requests in-depth growing.
> */
> -static int ext4_ext_create_new_leaf(handle_t *handle, struct inode *inode,
> - unsigned int mb_flags,
> - unsigned int gb_flags,
> - struct ext4_ext_path **ppath,
> - struct ext4_extent *newext)
> +static struct ext4_ext_path *
> +ext4_ext_create_new_leaf(handle_t *handle, struct inode *inode,
> + unsigned int mb_flags, unsigned int gb_flags,
> + struct ext4_ext_path *path,
> + struct ext4_extent *newext)
> {
> - struct ext4_ext_path *path = *ppath;
> struct ext4_ext_path *curp;
> int depth, i, err = 0;
>
> @@ -1424,28 +1423,24 @@ static int ext4_ext_create_new_leaf(handle_t *handle, struct inode *inode,
> * entry: create all needed subtree and add new leaf */
> err = ext4_ext_split(handle, inode, mb_flags, path, newext, i);
> if (err)
> - goto out;
> + goto errout;
>
> /* refill path */
> path = ext4_find_extent(inode,
> (ext4_lblk_t)le32_to_cpu(newext->ee_block),
> path, gb_flags);
> - if (IS_ERR(path))
> - err = PTR_ERR(path);
> } else {
> /* tree is full, time to grow in depth */
> err = ext4_ext_grow_indepth(handle, inode, mb_flags);
> if (err)
> - goto out;
> + goto errout;
>
> /* refill path */
> path = ext4_find_extent(inode,
> (ext4_lblk_t)le32_to_cpu(newext->ee_block),
> path, gb_flags);
> - if (IS_ERR(path)) {
> - err = PTR_ERR(path);
> - goto out;
> - }
> + if (IS_ERR(path))
> + return path;
>
> /*
> * only first (depth 0 -> 1) produces free space;
> @@ -1457,9 +1452,11 @@ static int ext4_ext_create_new_leaf(handle_t *handle, struct inode *inode,
> goto repeat;
> }
> }
> -out:
> - *ppath = IS_ERR(path) ? NULL : path;
> - return err;
> + return path;
> +
> +errout:
> + ext4_free_ext_path(path);
> + return ERR_PTR(err);
> }
>
> /*
> @@ -2112,10 +2109,14 @@ int ext4_ext_insert_extent(handle_t *handle, struct inode *inode,
> */
> if (gb_flags & EXT4_GET_BLOCKS_METADATA_NOFAIL)
> mb_flags |= EXT4_MB_USE_RESERVED;
> - err = ext4_ext_create_new_leaf(handle, inode, mb_flags, gb_flags,
> - ppath, newext);
> - if (err)
> + path = ext4_ext_create_new_leaf(handle, inode, mb_flags, gb_flags,
> + path, newext);
> + if (IS_ERR(path)) {
> + *ppath = NULL;
> + err = PTR_ERR(path);
> goto cleanup;
> + }
> + *ppath = path;
> depth = ext_depth(inode);
> eh = path[depth].p_hdr;
>
> --
> 2.39.2
>
