Message-ID: <cae729f9-beea-ee04-1258-af393a858430@gmx.com>
Date:   Mon, 17 Oct 2022 17:35:39 +0800
From:   Qu Wenruo <quwenruo.btrfs@....com>
To:     Dmitry Vyukov <dvyukov@...gle.com>, Qu Wenruo <wqu@...e.com>
Cc:     Hrutvik Kanabar <hrkanabar@...il.com>,
        Hrutvik Kanabar <hrutvik@...gle.com>,
        Marco Elver <elver@...gle.com>,
        Aleksandr Nogikh <nogikh@...gle.com>,
        kasan-dev@...glegroups.com,
        Alexander Viro <viro@...iv.linux.org.uk>,
        linux-fsdevel@...r.kernel.org, linux-kernel@...r.kernel.org,
        Theodore Ts'o <tytso@....edu>,
        Andreas Dilger <adilger.kernel@...ger.ca>,
        linux-ext4@...r.kernel.org, Chris Mason <clm@...com>,
        Josef Bacik <josef@...icpanda.com>,
        David Sterba <dsterba@...e.com>, linux-btrfs@...r.kernel.org,
        Jaegeuk Kim <jaegeuk@...nel.org>, Chao Yu <chao@...nel.org>,
        linux-f2fs-devel@...ts.sourceforge.net,
        "Darrick J . Wong" <djwong@...nel.org>, linux-xfs@...r.kernel.org,
        Namjae Jeon <linkinjeon@...nel.org>,
        Sungjong Seo <sj1557.seo@...sung.com>,
        Anton Altaparmakov <anton@...era.com>,
        linux-ntfs-dev@...ts.sourceforge.net
Subject: Re: [PATCH RFC 3/7] fs/btrfs: support `DISABLE_FS_CSUM_VERIFICATION`
 config option



On 2022/10/17 16:43, Dmitry Vyukov wrote:
> On Fri, 14 Oct 2022 at 12:24, 'Qu Wenruo' via kasan-dev
> <kasan-dev@...glegroups.com> wrote:
>>
>> On 2022/10/14 16:48, Hrutvik Kanabar wrote:
>>> From: Hrutvik Kanabar <hrutvik@...gle.com>
>>>
>>> When `DISABLE_FS_CSUM_VERIFICATION` is enabled, bypass checksum
>>> verification.
>>>
>>> Signed-off-by: Hrutvik Kanabar <hrutvik@...gle.com>
>>
>> I always want more fuzz for btrfs, so overall this is pretty good.
>>
>> But there are some comments related to free space cache part.
>>
>> Despite the details, I'm wondering would it be possible for your fuzzing
>> tool to do a better job at user space? Other than relying on loosen
>> checks from kernel?
>>
>> For example, implement a (mostly) read-only tool to do the following
>> workload:
>>
>> - Open the fs
>>     Including understand the checksum algo, how to re-generate the csum.
>>
>> - Read out the used space bitmap
>>     In btrfs case, it's going to read the extent tree, process the
>>     backrefs items.
>>
>> - Choose the victim sectors and corrupt them
>>     Obviously, vitims should be choosen from above used space bitmap.
>>
>> - Re-calculate the checksum for above corrupted sectors
>>     For btrfs, if it's a corrupted metadata, re-calculate the checksum.
>>
>> By this, we can avoid such change to kernel, and still get a much better
>> coverage.
>>
>> If you need some help on such user space tool, I'm pretty happy to
>> provide help.
>>
>>> ---
>>>    fs/btrfs/check-integrity.c  | 3 ++-
>>>    fs/btrfs/disk-io.c          | 6 ++++--
>>>    fs/btrfs/free-space-cache.c | 3 ++-
>>>    fs/btrfs/inode.c            | 3 ++-
>>>    fs/btrfs/scrub.c            | 9 ++++++---
>>>    5 files changed, 16 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/fs/btrfs/check-integrity.c b/fs/btrfs/check-integrity.c
>>> index 98c6e5feab19..eab82593a325 100644
>>> --- a/fs/btrfs/check-integrity.c
>>> +++ b/fs/btrfs/check-integrity.c
>>> @@ -1671,7 +1671,8 @@ static noinline_for_stack int btrfsic_test_for_metadata(
>>>                crypto_shash_update(shash, data, sublen);
>>>        }
>>>        crypto_shash_final(shash, csum);
>>> -     if (memcmp(csum, h->csum, fs_info->csum_size))
>>> +     if (!IS_ENABLED(CONFIG_DISABLE_FS_CSUM_VERIFICATION) &&
>>> +         memcmp(csum, h->csum, fs_info->csum_size))
>>>                return 1;
>>>
>>>        return 0; /* is metadata */
>>> diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
>>> index a2da9313c694..7cd909d44b24 100644
>>> --- a/fs/btrfs/disk-io.c
>>> +++ b/fs/btrfs/disk-io.c
>>> @@ -184,7 +184,8 @@ static int btrfs_check_super_csum(struct btrfs_fs_info *fs_info,
>>>        crypto_shash_digest(shash, raw_disk_sb + BTRFS_CSUM_SIZE,
>>>                            BTRFS_SUPER_INFO_SIZE - BTRFS_CSUM_SIZE, result);
>>>
>>> -     if (memcmp(disk_sb->csum, result, fs_info->csum_size))
>>> +     if (!IS_ENABLED(CONFIG_DISABLE_FS_CSUM_VERIFICATION) &&
>>> +         memcmp(disk_sb->csum, result, fs_info->csum_size))
>>>                return 1;
>>>
>>>        return 0;
>>> @@ -494,7 +495,8 @@ static int validate_extent_buffer(struct extent_buffer *eb)
>>>        header_csum = page_address(eb->pages[0]) +
>>>                get_eb_offset_in_page(eb, offsetof(struct btrfs_header, csum));
>>>
>>> -     if (memcmp(result, header_csum, csum_size) != 0) {
>>> +     if (!IS_ENABLED(CONFIG_DISABLE_FS_CSUM_VERIFICATION) &&
>>> +         memcmp(result, header_csum, csum_size) != 0) {
>>
>> I believe this is the main thing fuzzing would take advantage of.
>>
>> It would be much better if this is the only override...
>>
>>>                btrfs_warn_rl(fs_info,
>>>    "checksum verify failed on logical %llu mirror %u wanted " CSUM_FMT " found " CSUM_FMT " level %d",
>>>                              eb->start, eb->read_mirror,
>>> diff --git a/fs/btrfs/free-space-cache.c b/fs/btrfs/free-space-cache.c
>>> index f4023651dd68..203c8a9076a6 100644
>>> --- a/fs/btrfs/free-space-cache.c
>>> +++ b/fs/btrfs/free-space-cache.c
>>> @@ -574,7 +574,8 @@ static int io_ctl_check_crc(struct btrfs_io_ctl *io_ctl, int index)
>>>        io_ctl_map_page(io_ctl, 0);
>>>        crc = btrfs_crc32c(crc, io_ctl->orig + offset, PAGE_SIZE - offset);
>>>        btrfs_crc32c_final(crc, (u8 *)&crc);
>>> -     if (val != crc) {
>>> +     if (!IS_ENABLED(CONFIG_DISABLE_FS_CSUM_VERIFICATION) &&
>>> +         val != crc) {
>>
>> I'm already seeing this to cause problems, especially for btrfs.
>>
>> Btrfs has a very strong dependency on free space tracing, as all of our
>> metadata (and data by default) relies on COW to keep the fs consistent.
>>
>> I tried a lot of different methods in the past to make sure we won't
>> write into previously used space, but it's causing a lot of performance
>> impact.
>>
>> Unlike tree-checker, we can not easily got a centerlized space to handle
>> all the free space cross-check thing (thus it's only verified by things
>> like btrfs-check).
>>
>> Furthermore, even if you skip this override, with latest default
>> free-space-tree feature, free space info is stored in regular btrfs
>> metadata (tree blocks), with regular metadata checksum protection.
>>
>> Thus I'm pretty sure we will have tons of reports on this, and
>> unfortunately we can only go whac-a-mole way for it.
>
> Hi Qu,
>
> I don't fully understand what you mean. Could you please elaborate?
>
> Do you mean that btrfs uses this checksum check to detect blocks that
> were written to w/o updating the checksum?

I mean, btrfs uses this particular checksum for its (free) space cache,
and currently btrfs just trust the space cache completely to do COW.

This means, if we ignore the checksum for free space cache, we can
easily screw up the COW, e.g. allocate a range for the new metadata to
be written into.

But the truth is, that range is still being utilized by some other
metadata. Thus would completely break COW.


This is indeed a problem for btrfs, but it is not that easiy to fix,
since this involves cross-check 3 different data (free space cache for
free space, extent tree for used space, and the metadata itself).

Thus my concern is, disabling free space cache csum can easily lead to
various crashes, all related to broken COW, and we don't have a good
enough way to validate the result.

>
>
>
>
>>>                btrfs_err_rl(io_ctl->fs_info,
>>>                        "csum mismatch on free space cache");
>>>                io_ctl_unmap_page(io_ctl);
>>> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
>>> index b0807c59e321..1a49d897b5c1 100644
>>> --- a/fs/btrfs/inode.c
>>> +++ b/fs/btrfs/inode.c
>>> @@ -3434,7 +3434,8 @@ int btrfs_check_sector_csum(struct btrfs_fs_info *fs_info, struct page *page,
>>>        crypto_shash_digest(shash, kaddr, fs_info->sectorsize, csum);
>>>        kunmap_local(kaddr);
>>>
>>> -     if (memcmp(csum, csum_expected, fs_info->csum_size))
>>> +     if (!IS_ENABLED(CONFIG_DISABLE_FS_CSUM_VERIFICATION) &&
>>> +         memcmp(csum, csum_expected, fs_info->csum_size))
>>
>> This skips data csum check, I don't know how valueable it is, but this
>> should be harmless mostly.
>>
>> If we got reports related to this, it would be a nice surprise.
>>
>>>                return -EIO;
>>>        return 0;
>>>    }
>>> diff --git a/fs/btrfs/scrub.c b/fs/btrfs/scrub.c
>>> index f260c53829e5..a7607b492f47 100644
>>> --- a/fs/btrfs/scrub.c
>>> +++ b/fs/btrfs/scrub.c
>>> @@ -1997,7 +1997,8 @@ static int scrub_checksum_data(struct scrub_block *sblock)
>>>
>>>        crypto_shash_digest(shash, kaddr, fs_info->sectorsize, csum);
>>>
>>> -     if (memcmp(csum, sector->csum, fs_info->csum_size))
>>> +     if (!IS_ENABLED(CONFIG_DISABLE_FS_CSUM_VERIFICATION) &&
>>> +         memcmp(csum, sector->csum, fs_info->csum_size))
>>
>> Same as data csum verification overide.
>> Not necessary/useful but good to have.
>>
>>>                sblock->checksum_error = 1;
>>>        return sblock->checksum_error;
>>>    }
>>> @@ -2062,7 +2063,8 @@ static int scrub_checksum_tree_block(struct scrub_block *sblock)
>>>        }
>>>
>>>        crypto_shash_final(shash, calculated_csum);
>>> -     if (memcmp(calculated_csum, on_disk_csum, sctx->fs_info->csum_size))
>>> +     if (!IS_ENABLED(CONFIG_DISABLE_FS_CSUM_VERIFICATION) &&
>>> +         memcmp(calculated_csum, on_disk_csum, sctx->fs_info->csum_size))
>>
>> This is much less valueable, since it's only affecting scrub, and scrub
>> itself is already very little checking the content of metadata.
>
> Could you please elaborate here as well?

These checksum verification is only done in the scrub path (just as the
file name indicates).

> This is less valuable from what perspective?

It's just much harder to trigger, regular filesystem operations won't go
into scrub path.

Unless there is also a full ioctl fuzzing tests, after corrupting the image.

> The data loaded from disk can have any combination of
> (correct/incorrect metadata) x (correct/incorrect checksum).
> Correctness of metadata and checksum are effectively orthogonal,

Oh, I almost forgot another problem with the compile time csum
verification skip.

If we skip csum check completely, just like the patch, it may cause less
path coverage (this is very btrfs specific)

The problem is, btrfs has some repair path (scrub, and read-time), which
requires to have a checksum mismatch (and a good copy with good checksum).

Thus if we ignore csum completely, the repair path will never be covered
(as we treat them all as csum match).

Thanks,
Qu

> right?
>
>
>
>> Thanks,
>> Qu
>>
>>>                sblock->checksum_error = 1;
>>>
>>>        return sblock->header_error || sblock->checksum_error;
>>> @@ -2099,7 +2101,8 @@ static int scrub_checksum_super(struct scrub_block *sblock)
>>>        crypto_shash_digest(shash, kaddr + BTRFS_CSUM_SIZE,
>>>                        BTRFS_SUPER_INFO_SIZE - BTRFS_CSUM_SIZE, calculated_csum);
>>>
>>> -     if (memcmp(calculated_csum, s->csum, sctx->fs_info->csum_size))
>>> +     if (!IS_ENABLED(CONFIG_DISABLE_FS_CSUM_VERIFICATION) &&
>>> +         memcmp(calculated_csum, s->csum, sctx->fs_info->csum_size))
>>>                ++fail_cor;
>>>
>>>        return fail_cor + fail_gen;

Powered by blists - more mailing lists