Re: [PATCH] btrfs: fixup error handling in fixup_inode_link_counts

[David Sterba <dsterba@suse.cz>]

On Wed, May 19, 2021 at 01:13:15PM -0400, Josef Bacik wrote:
> This function has the following pattern
> 
> 	while (1) {
> 		ret = whatever();
> 		if (ret)
> 			goto out;
> 	}
> 	ret = 0
> out:
> 	return ret;
> 
> However several places in this while loop we simply break; when there's
> a problem, thus clearing the return value, and in one case we do a
> return -EIO, and leak the memory for the path.
> 
> Fix this by re-arranging the loop to deal with ret == 1 coming from
> btrfs_search_slot, and then simply delete the
> 
> 	ret = 0;
> out:
> 
> bit so everybody can break if there is an error, which will allow for
> proper error handling to occur.
> 
> Signed-off-by: Josef Bacik <josef@toxicpanda.com>
> ---
>  fs/btrfs/tree-log.c | 15 ++++++++-------
>  1 file changed, 8 insertions(+), 7 deletions(-)
> 
> diff --git a/fs/btrfs/tree-log.c b/fs/btrfs/tree-log.c
> index 18009095908b..f8f708c02462 100644
> --- a/fs/btrfs/tree-log.c
> +++ b/fs/btrfs/tree-log.c
> @@ -1778,7 +1778,7 @@ static noinline int fixup_inode_link_counts(struct btrfs_trans_handle *trans,
>  					    struct btrfs_root *root,
>  					    struct btrfs_path *path)
>  {
> -	int ret;
> +	int ret = 0;
>  	struct btrfs_key key;
>  	struct inode *inode;
>  
> @@ -1791,6 +1791,7 @@ static noinline int fixup_inode_link_counts(struct btrfs_trans_handle *trans,
>  			break;
>  
>  		if (ret == 1) {
> +			ret = 0;
>  			if (path->slots[0] == 0)
>  				break;
>  			path->slots[0]--;
> @@ -1803,17 +1804,19 @@ static noinline int fixup_inode_link_counts(struct btrfs_trans_handle *trans,
>  
>  		ret = btrfs_del_item(trans, root, path);
>  		if (ret)
> -			goto out;
> +			break;
>  
>  		btrfs_release_path(path);
>  		inode = read_one_inode(root, key.offset);
> -		if (!inode)
> -			return -EIO;
> +		if (!inode) {
> +			ret = -EIO;

Not related to your patch, but forcing EIO does not seem to be
right. read_one_inode is a simple wrapper around read_one_inode that
returns ERR_PTR so we should perhaps return that

> +			break;
> +		}
>  
>  		ret = fixup_inode_link_count(trans, root, inode);
>  		iput(inode);
>  		if (ret)
> -			goto out;
> +			break;
>  
>  		/*
>  		 * fixup on a directory may create new entries,
> @@ -1822,8 +1825,6 @@ static noinline int fixup_inode_link_counts(struct btrfs_trans_handle *trans,
>  		 */
>  		key.offset = (u64)-1;
>  	}
> -	ret = 0;
> -out:
>  	btrfs_release_path(path);
>  	return ret;
>  }
> -- 
> 2.26.3