Re: [PATCH] btrfs: add a sanity check for csum root before fill the data csum

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

在 2024/10/26 05:14, David Sterba 写道:
> On Wed, Oct 23, 2024 at 07:04:40PM +0800, Edward Adam Davis wrote:
>> Syzbot reported a null-ptr-deref in btrfs_lookup_csums_bitmap.
>> The btrfs info contains IGNOREDATACSUMS, which prevents the csum root from
>> being loaded.
>> Before filling in the csum data, check the flag BTRFS_FS_STATE_NO_DATA_CSUMS
>> to confirm that the csum root has been loaded.
>>
>> Reported-and-tested-by: syzbot+5d2b33d7835870519b5f@syzkaller.appspotmail.com
>> Closes: https://syzkaller.appspot.com/bug?extid=5d2b33d7835870519b5f
>> Signed-off-by: Edward Adam Davis <eadavis@qq.com>
>
> Added to for-next, thanks.

Wait for a second, I believe LiZhi Xu's solution is better.

And sorry I didn't notice that until his patch is submitted.

The problem for this fix is, although it fixes the crash, it also gives
a false feel of safety that scrub is finding nothing wrong.

But the truth is, there is no csum root, and everything can go wrong.

Thus I'd prefer LiZhi's solution which error out and terminate the scrub
immediately.

Thanks,
Qu
>
>> ---
>>   fs/btrfs/scrub.c | 3 ++-
>>   1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/fs/btrfs/scrub.c b/fs/btrfs/scrub.c
>> index 3a3427428074..1ba4d8ba902b 100644
>> --- a/fs/btrfs/scrub.c
>> +++ b/fs/btrfs/scrub.c
>> @@ -1602,7 +1602,8 @@ static int scrub_find_fill_first_stripe(struct btrfs_block_group *bg,
>>   	}
>>
>>   	/* Now fill the data csum. */
>> -	if (bg->flags & BTRFS_BLOCK_GROUP_DATA) {
>> +	if (!test_bit(BTRFS_FS_STATE_NO_DATA_CSUMS, &fs_info->fs_state) &&
>
> I've updatd the coment as this is double negation that could be
> confusing on a quick read.
>
>> +	    bg->flags & BTRFS_BLOCK_GROUP_DATA) {
>>   		int sector_nr;
>>   		unsigned long csum_bitmap = 0;
>>
>> --
>> 2.43.0
>>
>