Re: [PATCH] btrfs: tree-checker: add btrfs dev extent checks

[Qu Wenruo <wqu@suse.com>]

在 2024/8/14 08:41, David Sterba 写道:
> On Sun, Aug 11, 2024 at 03:20:08PM +0930, Qu Wenruo wrote:
>> [REPORT]
>> There is a corruption report that btrfs refuse to mount a fs that has
>> overlapping dev extents:
>>
>>   BTRFS error (device sdc): dev extent devid 4 physical offset
>> 14263979671552 overlap with previous dev extent end 14263980982272
>>   BTRFS error (device sdc): failed to verify dev extents against chunks: -117
>>   BTRFS error (device sdc): open_ctree failed
>>
>> [CAUSE]
>> The cause is very obvious, there is a bad dev extent item with incorrect
>> length.
>> Although we are not 100% sure of the cause before getting the dev tree
>> dump, I'm already surprised that we do not have any checks on dev tree.
>>
>> Currently we only do the dev-extent verification at mount time, but if the
>> corruption is caused by memory bitflip, we really want to catch it before
>> writing the corruption to the storage.
>>
>> Furthermore the dev extent items has the following key definition:
>>
>> 	(<device id> DEV_EXTENT <physical offset>)
>>
>> Thus we can not just rely on the generic key order check to make sure
>> there is no overlapping.
>>
>> [ENHANCEMENT]
>> Introduce dedicated dev extent checks, including:
>>
>> - Fixed member checks
>>    * chunk_tree should always be BTRFS_CHUNK_TREE_OBJECTID (3)
>>    * chunk_objectid should always be
>>      BTRFS_FIRST_CHUNK_CHUNK_TREE_OBJECTID (256)
>>
>> - Alignment checks
>>    * chunk_offset should be aligned to sectorsize
>>    * length should be aligned to sectorsize
>>    * key.offset should be aligned to sectorsize
>>
>> - Overlap checks
>>    If the previous key is also a dev-extent item, with the same
>>    device id, make sure we do not overlap with the previous dev extent.
>>
>> Reported: Stefan N <stefannnau@gmail.com>
>> Link: https://lore.kernel.org/linux-btrfs/CA+W5K0rSO3koYTo=nzxxTm1-Pdu1HYgVxEpgJ=aGc7d=E8mGEg@mail.gmail.com/
>> Signed-off-by: Qu Wenruo <wqu@suse.com>
> 
> Looks like we missed some simple tree item checks indeed.

The original idea is, we have btrfs_verify_dev_extents() at mount time, 
thus it's enough to reject bad dev extents, and no need for tree-checker 
for dev-extents.

But this method doesn't prevent bitflip from sneaking in during runtime.

So in the long run, our sanity checks should:

- Do cross-checks at mount time for critical infrastructure
   To prevent corruption sneaking in undetected.

- Do in-leaf checks at tree-checker
   To prevent corruption reach storage.

- Do extra read-time cross-checks
   Just like the dir item checks we did.

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