linux-kernel - Re: [PATCH 09/20] ext4: get rid of ppath in get_ext

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <Zqs2YYSZ2fPBAUw1@li-bb2b2a4c-3307-11b2-a85c-8fa5c3a69313.ibm.com>
Date: Thu, 1 Aug 2024 12:46:49 +0530
From: Ojaswin Mujoo <ojaswin@...ux.ibm.com>
To: libaokun@...weicloud.com
Cc: linux-ext4@...r.kernel.org, tytso@....edu, adilger.kernel@...ger.ca,
        jack@...e.cz, ritesh.list@...il.com, linux-kernel@...r.kernel.org,
        yi.zhang@...wei.com, yangerkun@...wei.com,
        Baokun Li <libaokun1@...wei.com>
Subject: Re: [PATCH 09/20] ext4: get rid of ppath in get_ext_path()

On Wed, Jul 10, 2024 at 12:06:43PM +0800, libaokun@...weicloud.com wrote:
> From: Baokun Li <libaokun1@...wei.com>
> 
> 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.
> 
> After getting rid of ppath in get_ext_path(), its caller may pass an error
> pointer to ext4_free_ext_path(), so it needs to teach ext4_free_ext_path()
> and ext4_ext_drop_refs() to skip the error pointer. No functional changes.
> 
> Signed-off-by: Baokun Li <libaokun1@...wei.com>
> ---
>  fs/ext4/extents.c     |  5 +++--
>  fs/ext4/move_extent.c | 34 +++++++++++++++++-----------------
>  2 files changed, 20 insertions(+), 19 deletions(-)
> 
> diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
> index 5217e6f53467..6dfb5d03e197 100644
> --- a/fs/ext4/extents.c
> +++ b/fs/ext4/extents.c
> @@ -116,7 +116,7 @@ static void ext4_ext_drop_refs(struct ext4_ext_path *path)
>  {
>  	int depth, i;
>  
> -	if (!path)
> +	if (IS_ERR_OR_NULL(path))
>  		return;
>  	depth = path->p_depth;
>  	for (i = 0; i <= depth; i++, path++)
> @@ -125,6 +125,8 @@ static void ext4_ext_drop_refs(struct ext4_ext_path *path)
>  
>  void ext4_free_ext_path(struct ext4_ext_path *path)
>  {
> +	if (IS_ERR_OR_NULL(path))
> +		return;
>  	ext4_ext_drop_refs(path);
>  	kfree(path);
>  }
> @@ -4191,7 +4193,6 @@ int ext4_ext_map_blocks(handle_t *handle, struct inode *inode,
>  	path = ext4_find_extent(inode, map->m_lblk, NULL, 0);
>  	if (IS_ERR(path)) {
>  		err = PTR_ERR(path);
> -		path = NULL;
>  		goto out;
>  	}
>  
> diff --git a/fs/ext4/move_extent.c b/fs/ext4/move_extent.c
> index b0ab19a913bf..a7186d63725a 100644
> --- a/fs/ext4/move_extent.c
> +++ b/fs/ext4/move_extent.c
> @@ -17,27 +17,23 @@
>   * get_ext_path() - Find an extent path for designated logical block number.
>   * @inode:	inode to be searched
>   * @lblock:	logical block number to find an extent path
> - * @ppath:	pointer to an extent path pointer (for output)
> + * @path:	pointer to an extent path
>   *
> - * ext4_find_extent wrapper. Return 0 on success, or a negative error value
> - * on failure.
> + * ext4_find_extent wrapper. Return an extent path pointer on success,
> + * or an error pointer on failure.
>   */
> -static inline int
> +static inline struct ext4_ext_path *
>  get_ext_path(struct inode *inode, ext4_lblk_t lblock,
> -		struct ext4_ext_path **ppath)
> +	     struct ext4_ext_path *path)
>  {
> -	struct ext4_ext_path *path = *ppath;
> -
> -	*ppath = NULL;
>  	path = ext4_find_extent(inode, lblock, path, EXT4_EX_NOCACHE);
>  	if (IS_ERR(path))
> -		return PTR_ERR(path);
> +		return path;
>  	if (path[ext_depth(inode)].p_ext == NULL) {
>  		ext4_free_ext_path(path);
> -		return -ENODATA;
> +		return ERR_PTR(-ENODATA);
>  	}
> -	*ppath = path;
> -	return 0;
> +	return path;
>  }
>  
>  /**
> @@ -95,9 +91,11 @@ mext_check_coverage(struct inode *inode, ext4_lblk_t from, ext4_lblk_t count,
>  	int ret = 0;
>  	ext4_lblk_t last = from + count;
>  	while (from < last) {
> -		*err = get_ext_path(inode, from, &path);
> -		if (*err)
> -			goto out;
> +		path = get_ext_path(inode, from, path);
> +		if (IS_ERR(path)) {
> +			*err = PTR_ERR(path);
> +			return ret;
> +		}
>  		ext = path[ext_depth(inode)].p_ext;
>  		if (unwritten != ext4_ext_is_unwritten(ext))
>  			goto out;
> @@ -624,9 +622,11 @@ ext4_move_extents(struct file *o_filp, struct file *d_filp, __u64 orig_blk,
>  		int offset_in_page;
>  		int unwritten, cur_len;
>  
> -		ret = get_ext_path(orig_inode, o_start, &path);
> -		if (ret)
> +		path = get_ext_path(orig_inode, o_start, path);
> +		if (IS_ERR(path)) {
> +			ret = PTR_ERR(path);
>  			goto out;
> +		}
>  		ex = path[path->p_depth].p_ext;
>  		cur_blk = le32_to_cpu(ex->ee_block);
>  		cur_len = ext4_ext_get_actual_len(ex);
> -- 
> 2.39.2
Looks good Baokun, feel free to add:

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

Regards,
ojaswin
>