Re: [PATCH v12 05/41] btrfs: release path before calling into btrfs_load_block_group_zone_info

From: Josef Bacik <josef@toxicpanda.com>
To: Naohiro Aota <naohiro.aota@wdc.com>,
	linux-btrfs@vger.kernel.org, dsterba@suse.com
Cc: hare@suse.com, linux-fsdevel@vger.kernel.org,
	Jens Axboe <axboe@kernel.dk>,
	Christoph Hellwig <hch@infradead.org>,
	"Darrick J. Wong" <darrick.wong@oracle.com>,
	Johannes Thumshirn <johannes.thumshirn@wdc.com>
Subject: Re: [PATCH v12 05/41] btrfs: release path before calling into btrfs_load_block_group_zone_info
Date: Fri, 15 Jan 2021 17:22:31 -0500	[thread overview]
Message-ID: <8f7434ae-fdb8-32be-f781-a47f32ace949@toxicpanda.com> (raw)
In-Reply-To: <0786a9782ec6306cddb0a2808116c3f95a88849b.1610693037.git.naohiro.aota@wdc.com>

On 1/15/21 1:53 AM, Naohiro Aota wrote:
> From: Johannes Thumshirn <johannes.thumshirn@wdc.com>
> 
> Since we have no write pointer in conventional zones, we cannot determine
> the allocation offset from it. Instead, we set the allocation offset after
> the highest addressed extent. This is done by reading the extent tree in
> btrfs_load_block_group_zone_info().
> 
> However, this function is called from btrfs_read_block_groups(), so the
> read lock for the tree node can recursively taken.
> 
> To avoid this unsafe locking scenario, release the path before reading the
> extent tree to get the allocation offset.
> 
> Signed-off-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>
> ---
>   fs/btrfs/block-group.c | 39 ++++++++++++++++++---------------------
>   1 file changed, 18 insertions(+), 21 deletions(-)
> 
> diff --git a/fs/btrfs/block-group.c b/fs/btrfs/block-group.c
> index b8bbdd95743e..ff13f7554ee5 100644
> --- a/fs/btrfs/block-group.c
> +++ b/fs/btrfs/block-group.c
> @@ -1806,24 +1806,8 @@ static int check_chunk_block_group_mappings(struct btrfs_fs_info *fs_info)
>   	return ret;
>   }
>   
> -static void read_block_group_item(struct btrfs_block_group *cache,
> -				 struct btrfs_path *path,
> -				 const struct btrfs_key *key)
> -{
> -	struct extent_buffer *leaf = path->nodes[0];
> -	struct btrfs_block_group_item bgi;
> -	int slot = path->slots[0];
> -
> -	cache->length = key->offset;
> -
> -	read_extent_buffer(leaf, &bgi, btrfs_item_ptr_offset(leaf, slot),
> -			   sizeof(bgi));
> -	cache->used = btrfs_stack_block_group_used(&bgi);
> -	cache->flags = btrfs_stack_block_group_flags(&bgi);
> -}
> -
>   static int read_one_block_group(struct btrfs_fs_info *info,
> -				struct btrfs_path *path,
> +				struct btrfs_block_group_item *bgi,
>   				const struct btrfs_key *key,
>   				int need_clear)
>   {
> @@ -1838,7 +1822,9 @@ static int read_one_block_group(struct btrfs_fs_info *info,
>   	if (!cache)
>   		return -ENOMEM;
>   
> -	read_block_group_item(cache, path, key);
> +	cache->length = key->offset;
> +	cache->used = btrfs_stack_block_group_used(bgi);
> +	cache->flags = btrfs_stack_block_group_flags(bgi);
>   
>   	set_free_space_tree_thresholds(cache);
>   
> @@ -1997,19 +1983,30 @@ int btrfs_read_block_groups(struct btrfs_fs_info *info)
>   		need_clear = 1;
>   
>   	while (1) {
> +		struct btrfs_block_group_item bgi;
> +		struct extent_buffer *leaf;
> +		int slot;
> +
>   		ret = find_first_block_group(info, path, &key);
>   		if (ret > 0)
>   			break;
>   		if (ret != 0)
>   			goto error;
>   
> -		btrfs_item_key_to_cpu(path->nodes[0], &key, path->slots[0]);
> -		ret = read_one_block_group(info, path, &key, need_clear);
> +		leaf = path->nodes[0];
> +		slot = path->slots[0];
> +		btrfs_release_path(path);

You're releasing the path and then reading from it, a potential UAF.  Thanks,

Josef