Re: [PATCH 1/3] btrfs: avoid NULL pointer dereference when block group tree root is missing

[Qu Wenruo <quwenruo.btrfs@gmx.com>]

在 2026/7/11 19:22, Dongjiang Zhu 写道:
> 在 2026/7/11 14:20, Qu Wenruo 写道:
>>
>>
>> 在 2026/7/11 14:49, Dongjiang Zhu 写道:
>>> [BUG]
>>> For a filesystem using the block group tree feature, corrupting the
>>> block
>>> group tree root and mounting with rescue=ibadroots triggers the
>>> following
>>> NULL pointer dereference:
>>>
>>>     BTRFS warning (device loop0 state E): checksum verify failed on
>>> logical 30654464 mirror 1 wanted 0x0f55b851 found 0xd7efdde7 level 0
>>>     BTRFS warning (device loop0 state E): checksum verify failed on
>>> logical 30654464 mirror 2 wanted 0x0f55b851 found 0xd7efdde7 level 0
>>>     BUG: kernel NULL pointer dereference, address: 0000000000000100
>>>     #PF: supervisor read access in kernel mode
>>>     #PF: error_code(0x0000) - not-present page
>>>     Oops: Oops: 0000 [#1] SMP NOPTI
>>>     RIP: 0010:btrfs_update_global_block_rsv+0x9d/0x1c0 [btrfs]
>>
>> The problem is why we need to use global block rsv in the first place?
>>
>> The whole fs is never going to be mounted RW as long as "rescue="
>> mount option is utilized.
>>
>> In the case of a rescue mount, we should just fill in a dummy block
>> rsv, with block_rsv->full set.
>>
>> This will also fix the next bug you're fixing.
> 
> That makes sense, thanks.
> 
> I had considered simply skipping btrfs_update_global_block_rsv() from
> btrfs_init_global_block_rsv() when rescue=ibadroots is set, but did not
> choose that approach in the end.
> 
> Your suggestion is semantically more precise. If I understand it
> correctly, would the implementation look like this?
> 
> void btrfs_init_global_block_rsv(struct btrfs_fs_info *fs_info)
> {
>           struct btrfs_space_info *space_info;
> 
>           /* Existing space_info initialization. */
> 
>           if (unlikely(BTRFS_FS_ERROR(fs_info))) {

I'd prefer to put this inside btrfs_update_global_block_rsv().

Just in case there is some other call sites that can still hit 
btrfs_update_global_block_rsv() for a fully RO fs.

>                   fs_info->global_block_rsv.full = true;
>                   return;
>           }
> 
>           btrfs_update_global_block_rsv(fs_info);
> }
> 
> Would you agree with using BTRFS_FS_ERROR() here, or would you prefer the
> narrower fs_is_full_ro() or IGNOREBADROOTS condition?

I'd prefer fs_is_full_ro().

BTRFS_FS_ERROR() can be met for critical runtime errors, and we do not 
want to mess global_block_rsv at all, as it may contain critical info 
for debug.

For fs_is_full_ro() is very specific for rescue options, thus nothing to 
save for debug in the first place.

> 
> Thanks,
> Dongjiang