[PATCH 0/2] btrfs: cleanup and fix sample_block_group_extent_item()

[PATCH 0/2] btrfs: cleanup and fix sample_block_group_extent_item()

[fdmanana]

From: Filipe Manana <fdmanana@suse.com>

Trivial changes, details in the change logs.

Filipe Manana (2):
  btrfs: remove bogus root search condition in sample_block_group_extent_item()
  btrfs: deal with missing root in sample_block_group_extent_item()

 fs/btrfs/block-group.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

-- 
2.47.2

[PATCH 1/2] btrfs: remove bogus root search condition in sample_block_group_extent_item()

[fdmanana]

From: Filipe Manana <fdmanana@suse.com>

There's no need to pass the maximum between the block group's start offset
and BTRFS_SUPER_INFO_OFFSET (64K) since we can't have any block groups
allocated in the first megabyte, as that's reserved space. Furthermore,
even if we could, the correct thing to do was to pass the block group's
start offset anyway - and that's precisely what we do for block groups
that happen to contain superblock mirror (the range for the super block
is never marked as free and it's marked as dirty in the
fs_info->excluded_extents io tree).

So simplify this and get rid of that maximum expression.

Signed-off-by: Filipe Manana <fdmanana@suse.com>
---
 fs/btrfs/block-group.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/fs/btrfs/block-group.c b/fs/btrfs/block-group.c
index feec6f513ea0..6387e11f8f8e 100644
--- a/fs/btrfs/block-group.c
+++ b/fs/btrfs/block-group.c
@@ -606,8 +606,7 @@ static int sample_block_group_extent_item(struct btrfs_caching_control *caching_
 	lockdep_assert_held(&caching_ctl->mutex);
 	lockdep_assert_held_read(&fs_info->commit_root_sem);
 
-	extent_root = btrfs_extent_root(fs_info, max_t(u64, block_group->start,
-						       BTRFS_SUPER_INFO_OFFSET));
+	extent_root = btrfs_extent_root(fs_info, block_group->start);
 
 	search_offset = index * div_u64(block_group->length, max_index);
 	search_key.objectid = block_group->start + search_offset;
-- 
2.47.2

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

[fdmanana]

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.

Signed-off-by: Filipe Manana <fdmanana@suse.com>
---
 fs/btrfs/block-group.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/fs/btrfs/block-group.c b/fs/btrfs/block-group.c
index 6387e11f8f8e..b3345792f3a1 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,
+			  "Could not find extent root for block group at offset %llu\n",
+			  block_group->start);
+		return -EUCLEAN;
+	}
 
 	search_offset = index * div_u64(block_group->length, max_index);
 	search_key.objectid = block_group->start + search_offset;
-- 
2.47.2

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

[Filipe Manana]

On Wed, Jan 21, 2026 at 9:08 PM <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.
>
> Signed-off-by: Filipe Manana <fdmanana@suse.com>
> ---
>  fs/btrfs/block-group.c | 6 ++++++
>  1 file changed, 6 insertions(+)
>
> diff --git a/fs/btrfs/block-group.c b/fs/btrfs/block-group.c
> index 6387e11f8f8e..b3345792f3a1 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,
> +                         "Could not find extent root for block group at offset %llu\n",

Unnecessary newline, will remove before commit or on v2 if needed.

> +                         block_group->start);
> +               return -EUCLEAN;
> +       }
>
>         search_offset = index * div_u64(block_group->length, max_index);
>         search_key.objectid = block_group->start + search_offset;
> --
> 2.47.2
>
>

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

[Boris Burkov]

On Wed, Jan 21, 2026 at 08:52:38PM +0000, fdmanana@kernel.org wrote:
> From: Filipe Manana <fdmanana@suse.com>
> 
> In case the does not exists, which is unexpected, btrfs_extent_root()

In case the root does not exist

> 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.
> 
> Signed-off-by: Filipe Manana <fdmanana@suse.com>
> ---
>  fs/btrfs/block-group.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/fs/btrfs/block-group.c b/fs/btrfs/block-group.c
> index 6387e11f8f8e..b3345792f3a1 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,
> +			  "Could not find extent root for block group at offset %llu\n",
> +			  block_group->start);
> +		return -EUCLEAN;
> +	}
>  
>  	search_offset = index * div_u64(block_group->length, max_index);
>  	search_key.objectid = block_group->start + search_offset;
> -- 
> 2.47.2
> 

Re: [PATCH 0/2] btrfs: cleanup and fix sample_block_group_extent_item()

[Boris Burkov]

On Wed, Jan 21, 2026 at 08:52:36PM +0000, fdmanana@kernel.org wrote:
> From: Filipe Manana <fdmanana@suse.com>
> 
> Trivial changes, details in the change logs.

LGTM, thanks.

Reviewed-by: Boris Burkov <boris@bur.io>

> 
> Filipe Manana (2):
>   btrfs: remove bogus root search condition in sample_block_group_extent_item()
>   btrfs: deal with missing root in sample_block_group_extent_item()
> 
>  fs/btrfs/block-group.c | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
> 
> -- 
> 2.47.2
> 

Re: [PATCH 0/2] btrfs: cleanup and fix sample_block_group_extent_item()

[Qu Wenruo]

在 2026/1/22 07:22, fdmanana@kernel.org 写道:
> From: Filipe Manana <fdmanana@suse.com>
> 
> Trivial changes, details in the change logs.

Reviewed-by: Qu Wenruo <wqu@suse.com>

Thanks,
Qu

> 
> Filipe Manana (2):
>    btrfs: remove bogus root search condition in sample_block_group_extent_item()
>    btrfs: deal with missing root in sample_block_group_extent_item()
> 
>   fs/btrfs/block-group.c | 9 +++++++--
>   1 file changed, 7 insertions(+), 2 deletions(-)
> 

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

[David Sterba]

On Wed, Jan 21, 2026 at 08:52:38PM +0000, 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.
> 
> Signed-off-by: Filipe Manana <fdmanana@suse.com>
> ---
>  fs/btrfs/block-group.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/fs/btrfs/block-group.c b/fs/btrfs/block-group.c
> index 6387e11f8f8e..b3345792f3a1 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,
> +			  "Could not find extent root for block group at offset %llu\n",

This is probably a message copied from elsewhere, we do't capitalize the
first letter and the trailing "\n" is provided by btrfs_err()

> +			  block_group->start);
> +		return -EUCLEAN;
> +	}
>  
>  	search_offset = index * div_u64(block_group->length, max_index);
>  	search_key.objectid = block_group->start + search_offset;
> -- 
> 2.47.2
> 

Re: [PATCH 0/2] btrfs: cleanup and fix sample_block_group_extent_item()

[David Sterba]

On Wed, Jan 21, 2026 at 08:52:36PM +0000, fdmanana@kernel.org wrote:
> From: Filipe Manana <fdmanana@suse.com>
> 
> Trivial changes, details in the change logs.
> 
> Filipe Manana (2):
>   btrfs: remove bogus root search condition in sample_block_group_extent_item()
>   btrfs: deal with missing root in sample_block_group_extent_item()

Reviewed-by: David Sterba <dsterba@suse.com>

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

[Chris Mason]

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?