Re: [PATCH 10/20] ext4: get rid of ppath in ext4_ext_create_new_leaf()

[Ojaswin Mujoo]

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
>