Re: [PATCH 2/2] btrfs: deal with missing root in sample_block_group_extent_item()

[Chris Mason <clm@meta.com>]

fdmanana@kernel.org wrote:
> From: Filipe Manana <fdmanana@suse.com>
> 
> In case the does not exists, which is unexpected, btrfs_extent_root()
> returns NULL, but we ignore that and so if it happens we can trigger
> a NULL pointer dereference later. So verify if we found the root and
> log an error message in case it's missing.

Hi everyone,

I'm running the btrfs for-next branch, along with the encryption patch set
through my AI review prompts:

https://github.com/masoncl/review-prompts

I'm working on some btrfs specific instructions, but hopefully these reviews
are already useful.  I've kept the headers to make it easier to double check
that I'm actually replying to the right email, but please let me know if any
parts of this are incorrect:

commit a8bec25e014eab671ec8a25b03d391cad3e55230
Author: Filipe Manana <fdmanana@suse.com>

btrfs: deal with missing root in sample_block_group_extent_item()

This commit adds a NULL check for extent_root after calling
btrfs_extent_root() in sample_block_group_extent_item(), addressing
a potential NULL pointer dereference.

> diff --git a/fs/btrfs/block-group.c b/fs/btrfs/block-group.c
> index 66c512bc022c5..28aa87872f0d9 100644
> --- a/fs/btrfs/block-group.c
> +++ b/fs/btrfs/block-group.c
> @@ -607,6 +607,12 @@ static int sample_block_group_extent_item(struct btrfs_caching_control *caching_
>  	lockdep_assert_held_read(&fs_info->commit_root_sem);
>
>  	extent_root = btrfs_extent_root(fs_info, block_group->start);
> +	if (unlikely(!extent_root)) {
> +		btrfs_err(fs_info,
> +			  "missing extent root for block group at offset %llu",
> +			  block_group->start);
> +		return -EUCLEAN;
> +	}
>
>  	search_offset = index * div_u64(block_group->length, max_index);
>  	search_key.objectid = block_group->start + search_offset;

Does load_extent_tree_free() in the same file have the same issue?

Looking at load_extent_tree_free() around line 741:

	last = max_t(u64, block_group->start, BTRFS_SUPER_INFO_OFFSET);
	extent_root = btrfs_extent_root(fs_info, last);

and then at line 767:

	ret = btrfs_search_slot(NULL, extent_root, &key, path, 0, 0);

There's no NULL check between the btrfs_extent_root() call and the
btrfs_search_slot() call. If btrfs_extent_root() returns NULL (via
btrfs_global_root() when rb_find() fails), extent_root would be
dereferenced in btrfs_search_slot(), causing a NULL pointer
dereference.

Both sample_block_group_extent_item() and load_extent_tree_free() are
called from caching_thread() during block group caching operations, so
they have the same risk profile. Should load_extent_tree_free() have
the same NULL check added?