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

在 2026/7/11 18:17, Qu Wenruo 写道:
> 
> 
> 在 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.

Got it. Thanks for the clarification. I'll rework this and send a v2 later.