Re: [PATCH] btrfs: Do super block verification before writing it to disk

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

[-- Attachment #1.1: Type: text/plain, Size: 4885 bytes --]



On 2018年04月17日 22:32, Anand Jain wrote:
> 
> 
> On 04/17/2018 05:58 PM, Qu Wenruo wrote:
>>
>>
>> On 2018年04月17日 17:05, Anand Jain wrote:
>>>
>>>> v3:
>>>>     Update commit message to show the corruption in details.
>>>>     Modify the kernel error message to show corruption is detected
>>>> before
>>>>     transaction commitment.
>>>   Nice. Thanks. more below.
>>>
>>>> @@ -3310,6 +3311,27 @@ static int write_dev_supers(struct btrfs_device
>>>> *device,
>>>>              btrfs_set_super_bytenr(sb, bytenr);
>>>>    +        /* check the validation of the primary sb before writing */
>>>> +        if (i == 0) {
>>>> +            ret = btrfs_check_super_valid(device->fs_info, sb);
>>>> +            if (ret) {
>>>> +                btrfs_err(device->fs_info,
>>>> +"superblock corruption detected before transaction commitment for
>>>> device %llu",
>>>> +                      device->devid);
>>>> +                return -EUCLEAN;
>>>> +            }
>>>
>>>   Why not move this entire check further below, after we have the ready
>>>   crc and use btrfs_check_super_csum(), instead of
>>>   btrfs_check_super_valid()? so that we verify only what is known to be
>>>   corrupted that is ..
>>
>> The problem is, we don't know the cause yet, so we must check the whole
>> superblock.
>>
>> For example, if the corruption is caused by some wild pointer of other
>> kernel module, and we're just unlucky that one day it corrupts nodesize,
>> then we can't detect it if we only check certain members.
> 
> Right I notice that.
> 
> But without btrfs_check_super_csum(), it leaves out checking one of the
> member (csum_type) which is know to be corrupted at the two instances,
> so it can also include btrfs_check_super_csum().
> 
> There were two cases, both of them corrupted the same offset, its not
> just a coincidence that both of these reported corrupted the same
> offset.

Yep, but since we're here to do extra verification, checking everything
is never a bad idea. By this we don't need to bother checking other
members when new corruption pops out.

> 
> Also the incompatible features flags (169) are still valid in both the
> cases. It looks as if we wrote u32 to a u64. I notice that we provide
> the options to write the incompatible flags through mount option, sysfs
> and ioctl.

While I don't think that's the cause of these reported corruption.
I'd prefer some under/over flow of memory which corrupted
fs_info->super_copy somehow. It may be btrfs or it may not.

It's pretty hard to determine with just 2 reports.
Especially for ben's report, he is using latest vega graphics IIRC, who
knows what could went wrong with latest amd drm codes.

> 
> 
>>> btrfs_super_block {
>>> ::
>>>          __le64 incompat_flags;
>>>          __le16 csum_type;
>>> ::
>>> }
>>>
>>>   And also can you dump contents of incompat_flags and csum_type at both
>>>     fs_info->super_copy
>>>   and
>>>     fs_info->super_for_commit
>>
>> Not really needed, as when corruption happens, it's super_copy
>> corrupted, not something went wrong after we called memcpy()
> 
> As shown below, we aren't memcpy()-ing in the btrfs_sync_log() thread,
> did you check if btrfs_sync_log() can not be the last person to write
> at umount?

I checked the dump super output, where log_tree output is all 0, means
no log tree, hence not btrfs_sync_log() caused the problem.

From Ben's:
------
chunk_root		5518540881920
chunk_root_level	1
log_root		0
log_root_transid	0
log_root_level		0
------

And from Ken's
------
chunk_root        21004288
chunk_root_level    1
log_root        0
log_root_transid    0
log_root_level        0
------

So at least for current only reports, it's not btrfs_sync_log() causing
the problem.

Thanks,
Qu

> 
> Thanks, Anand
> 
>> Thanks,
>> Qu
>>
>>>
>>>   Because at each commit transaction we
>>>
>>> btrfs_commit_transaction()
>>> {
>>>    ::
>>>          memcpy(fs_info->super_for_commit, fs_info->super_copy,
>>>                 sizeof(*fs_info->super_copy));
>>>    ::
>>>          ret = write_all_supers(fs_info, 0);
>>>
>>> }
>>>
>>>   And also the sync log can write the
>>>
>>> btrfs_sync_log()
>>> {
>>> ::
>>>          ret = write_all_supers(fs_info, 1);
>>>
>>>
>>>   Finally locks between these two threads needs a review as well.
>>>
>>> Thanks, Anand
>>> -- 
>>> To unsubscribe from this list: send the line "unsubscribe
>>> linux-btrfs" in
>>> the body of a message to majordomo@vger.kernel.org
>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]