Re: [PATCH 2/6] btrfs-progs: check/original: Do extra verification on file extent item

Su Yue
2019-12-18T02:20:10Z

On 2019/12/18 10:17 AM, Qu Wenruo wrote:
>
>
> On 2019/12/18 上午10:09, Su Yue wrote:
>> On 2019/12/18 9:19 AM, Qu Wenruo wrote:
>>> [BUG]
>>> For certain fuzzed image, `btrfs check` will fail with the following
>>> call trace:
>>>    Checking filesystem on issue_213.raw
>>>    UUID: 99e50868-0bda-4d89-b0e4-7e8560312ef9
>>>    [1/7] checking root items
>>>    [2/7] checking extents
>>>    Program received signal SIGABRT, Aborted.
>>>    0x00007ffff7c88f25 in raise () from /usr/lib/libc.so.6
>>>    (gdb) bt
>>>    #0  0x00007ffff7c88f25 in raise () from /usr/lib/libc.so.6
>>>    #1  0x00007ffff7c72897 in abort () from /usr/lib/libc.so.6
>>>    #2  0x00005555555abc3e in run_next_block (...) at check/main.c:6398
>>>    #3  0x00005555555b0f36 in deal_root_from_list (...) at
>>> check/main.c:8408
>>>    #4  0x00005555555b1a3d in check_chunks_and_extents
>>> (fs_info=0x5555556a1e30) at check/main.c:8690
>>>    #5  0x00005555555b1e3e in do_check_chunks_and_extents
>>> (fs_info=0x5555556a1e30) a
>>>    #6  0x00005555555b5710 in cmd_check (cmd=0x555555696920
>>> <cmd_struct_check>, argc
>>>    #7  0x0000555555568dc7 in cmd_execute (cmd=0x555555696920
>>> <cmd_struct_check>, ar
>>>    #8  0x0000555555569713 in main (argc=2, argv=0x7fffffffde70) at
>>> btrfs.c:386
>>>
>>> [CAUSE]
>>> This fuzzed images has a corrupted EXTENT_DATA item in data reloc tree:
>>>          item 1 key (256 EXTENT_DATA 256) itemoff 16111 itemsize 12
>>>                  generation 0 type 2 (prealloc)
>>>                  prealloc data disk byte 16777216 nr 0
>>>                  prealloc data offset 0 nr 0
>>>
>>> There are several problems with the item:
>>> - Bad item size
>>>    12 is too small.
>>> - Bad key offset
>>>    offset of EXTENT_DATA type key represents file offset, which should
>>>    always be aligned to sector size (4K in this particular case).
>>>
>>> [FIX]
>>> Do extra item size and key offset check for original mode, and remove
>>> the abort() call in run_next_block().
>>>
>>> And to show off how robust lowmem mode is, lowmem can handle it without
>>> any hiccup.
>>>
>>> With this fix, original mode can detect the problem properly:
>>>    Checking filesystem on issue_213.raw
>>>    UUID: 99e50868-0bda-4d89-b0e4-7e8560312ef9
>>>    [1/7] checking root items
>>>    [2/7] checking extents
>>>    ERROR: invalid file extent item size, have 12 expect (21, 16283]
>>>    ERROR: errors found in extent allocation tree or chunk allocation
>>>    [3/7] checking free space cache
>>>    [4/7] checking fs roots
>>>    root 18446744073709551607 root dir 256 error
>>>    root 18446744073709551607 inode 256 errors 62, no orphan item, odd
>>> file extent, bad file extent
>>>    ERROR: errors found in fs roots
>>>    found 131072 bytes used, error(s) found
>>>    total csum bytes: 0
>>>    total tree bytes: 131072
>>>    total fs tree bytes: 32768
>>>    total extent tree bytes: 16384
>>>    btree space waste bytes: 124774
>>>    file data blocks allocated: 0
>>>     referenced 0
>>>
>>> Issue: #213
>>> Signed-off-by: Qu Wenruo <wqu@suse.com>
>>
>> Almost fine. Two nitpicks below.
>> I guess that they could be fixed when merging.
>>
>>> ---
>>>   check/main.c | 34 ++++++++++++++++++++++++++++++++--
>>>   1 file changed, 32 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/check/main.c b/check/main.c
>>> index 08dc9e66..91752dce 100644
>>> --- a/check/main.c
>>> +++ b/check/main.c
>>> @@ -6268,7 +6268,10 @@ static int run_next_block(struct btrfs_root *root,
>>>           btree_space_waste += btrfs_leaf_free_space(buf);
>>>           for (i = 0; i < nritems; i++) {
>>>               struct btrfs_file_extent_item *fi;
>>> +            unsigned long inline_offset;
>>>
>>> +            inline_offset = offsetof(struct btrfs_file_extent_item,
>>> +                         disk_bytenr);
>>>               btrfs_item_key_to_cpu(buf, &key, i);
>>>               /*
>>>                * Check key type against the leaf owner.
>>> @@ -6384,18 +6387,45 @@ static int run_next_block(struct btrfs_root
>>> *root,
>>>               }
>>>               if (key.type != BTRFS_EXTENT_DATA_KEY)
>>>                   continue;
>>> +            /* Check itemsize before we continue*/
>>
>> One more space at the tail.
>>> +            if (btrfs_item_size_nr(buf, i) < inline_offset) {
>>> +                ret = -EUCLEAN;
>>> +                error(
>>> +        "invalid file extent item size, have %u expect (%lu, %lu]",
>>
>> should it be "[%llu, %lu)"?
>
> If the file extent size matches inline_offset, then it's an empty inline
> file extent, which is not valid.
> So left side must be '('.
>
> For the right side, it can take the whole leaf, e.g. for 4K nodesize.
>
> So it's (%llu, %lu].
>

Reviewed-by: Su Yue <Damenly_Su@gmx.com>

> Thanks,
> Qu
>>
>> Thanks.
>>> +                    btrfs_item_size_nr(buf, i),
>>> +                    inline_offset,
>>> +                    BTRFS_LEAF_DATA_SIZE(fs_info));
>>> +                continue;
>>> +            }
>>>               fi = btrfs_item_ptr(buf, i,
>>>                           struct btrfs_file_extent_item);
>>>               if (btrfs_file_extent_type(buf, fi) ==
>>>                   BTRFS_FILE_EXTENT_INLINE)
>>>                   continue;
>>> +
>>> +            /* Prealloc/regular extent must have fixed item size */
>>> +            if (btrfs_item_size_nr(buf, i) !=
>>> +                sizeof(struct btrfs_file_extent_item)) {
>>> +                ret = -EUCLEAN;
>>> +                error(
>>> +            "invalid file extent item size, have %u expect %zu",
>>> +                    btrfs_item_size_nr(buf, i),
>>> +                    sizeof(struct btrfs_file_extent_item));
>>> +                continue;
>>> +            }
>>> +            /* key.offset (file offset) must be aligned */
>>> +            if (!IS_ALIGNED(key.offset, fs_info->sectorsize)) {
>>> +                ret = -EUCLEAN;
>>> +                error(
>>> +            "invalid file offset, have %llu expect aligned to %u",
>>> +                    key.offset, fs_info->sectorsize);
>>> +                continue;
>>> +            }
>>>               if (btrfs_file_extent_disk_bytenr(buf, fi) == 0)
>>>                   continue;
>>>
>>>               data_bytes_allocated +=
>>>                   btrfs_file_extent_disk_num_bytes(buf, fi);
>>> -            if (data_bytes_allocated < root->fs_info->sectorsize)
>>> -                abort();
>>>
>>>               data_bytes_referenced +=
>>>                   btrfs_file_extent_num_bytes(buf, fi);
>>>
>>
>