[PATCH 0/6] btrfs-progs: Fixes for github issues

[PATCH 0/6] btrfs-progs: Fixes for github issues

[Qu Wenruo]

There are a new batch of fuzzed images for btrfs-progs. They are all
reported by Ruud van Asseldonk from github.

Patch 1 will make QA life easier by remove the extra 300s wait time.
Patch 2~5 are all the meat for the fuzzed images.

Just a kind reminder, mkfs/020 test will fail due to tons of problems:
- Undefined $csum variable
- Undefined $dev1 variable
- Bad kernel probe for support csum
  E.g. if Blake2 not compiled, it still shows up in supported csum algo,
  but will fail to mount.

All other tests pass.

Qu Wenruo (6):
  btrfs-progs: tests: Add --force for repair command
  btrfs-progs: check/original: Do extra verification on file extent item
  btrfs-progs: disk-io: Verify the bytenr passed in is mapped for
    read_tree_block()
  btrfs-progs: Add extra chunk item size check
  btrfs-progs: extent-tree: Kill the BUG_ON() in btrfs_chunk_readonly()
  btrfs-progs: extent-tree: Fix a by-one error in
    exclude_super_stripes()

 check/main.c  | 34 ++++++++++++++++++++++++++++++++--
 extent-tree.c | 11 ++++++++---
 extent_io.c   | 26 ++++++++++++++++++++++++++
 extent_io.h   |  2 ++
 tests/common  |  2 +-
 volumes.c     | 41 ++++++++++++++++++++++++++++++++++++-----
 6 files changed, 105 insertions(+), 11 deletions(-)

-- 
2.24.1

[PATCH 1/6] btrfs-progs: tests: Add --force for repair command

[Qu Wenruo]

Since commit e388bf38 ("btrfs-progs: check: warn users about the
possible dangers of --repair") `btrfs check --repair` will wait 10
seconds before really repair the fs.

This hugely slow down the fsck tests. Add --force for check_image()

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 tests/common | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/common b/tests/common
index ca098444..605cf72c 100644
--- a/tests/common
+++ b/tests/common
@@ -331,7 +331,7 @@ check_image()
 	"$TOP/btrfs" check "$image" >> "$RESULTS" 2>&1
 	[ $? -eq 0 ] && _fail "btrfs check should have detected corruption"
 
-	run_check "$TOP/btrfs" check --repair "$image"
+	run_check "$TOP/btrfs" check --repair --force "$image"
 	run_check "$TOP/btrfs" check "$image"
 }
 
-- 
2.24.1

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

[Qu Wenruo]

[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>
---
 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*/
+			if (btrfs_item_size_nr(buf, i) < inline_offset) {
+				ret = -EUCLEAN;
+				error(
+		"invalid file extent item size, have %u expect (%lu, %lu]",
+					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);
-- 
2.24.1

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

[Su Yue]

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)"?

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);
>

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

[Qu Wenruo]

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



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].

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);
>>
> 


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

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

[Su Yue]

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);
>>>
>>
> 

[PATCH 3/6] btrfs-progs: disk-io: Verify the bytenr passed in is mapped for read_tree_block()

[Qu Wenruo]

[BUG]
For a fuzzed image, `btrfs check` will segfault at open_ctree() stage:
  $ btrfs check --mode=lowmem issue_207.raw
  Opening filesystem to check...
  extent_io.c:665: free_extent_buffer_internal: BUG_ON `eb->refs < 0` triggered, value 1
  btrfs(+0x6bf67)[0x56431d278f67]
  btrfs(+0x6c16e)[0x56431d27916e]
  btrfs(alloc_extent_buffer+0x45)[0x56431d279db5]
  btrfs(read_tree_block+0x59)[0x56431d2848f9]
  btrfs(btrfs_setup_all_roots+0x29c)[0x56431d28535c]
  btrfs(+0x78903)[0x56431d285903]
  btrfs(open_ctree_fs_info+0x90)[0x56431d285b60]
  btrfs(+0x45a01)[0x56431d252a01]
  btrfs(main+0x94)[0x56431d2220c4]
  /usr/lib/libc.so.6(__libc_start_main+0xf3)[0x7f6e28519153]
  btrfs(_start+0x2e)[0x56431d22235e]

[CAUSE]
The fuzzed image has a strange log root bytenr:
  log_root                61440
  log_root_transid        0

In fact, the log_root seems to be fuzzed, as its transid is 0, which is
invalid.

Note that range [61440, 77824) covers the physical offset of the primary
super block.

The bug is caused by the following sequence:
1. cache for tree block [64K, 68K) is created by open_ctree()
   __open_ctree_fd()
   |- btrfs_setup_chunk_tree_and_device_map()
      |- btrfs_read_sys_array()
         |- sb = btrfs_find_create_tree_block()
         |- free_extent_buffer(sb)

   This created an extent buffer [64K, 68K) in fs_info->extent_cache, then
   reduce the refcount of that eb back to 0, but not freed yet.

2. Try to read that corrupted log root
   __open_ctree_fd()
   |- btrfs_setup_chunk_tree_and_device_map()
   |- btrfs_setup_all_roots()
      |- find_and_setup_log_root()
         |- read_tree_block()
            |- btrfs_find_create_tree_block()
               |- alloc_extent_buffer()

   The final alloc_extent_buffer() will try to free that cached eb
   [64K, 68K), since it doesn't match with current search.
   And since that cached eb is already released (refcount == 0), the
   extra free_extent_buffer() will cause above BUG_ON().

[FIX]
Here we fix it through a more comprehensive method, instead of simply
verifying log_root_transid, here we just don't pollute eb cache when
reading sys chunk array.

So that we won't have an eb cache [64K, 68K), and will error out at
logical mapping phase.

Issue: #207
Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 extent_io.c | 26 ++++++++++++++++++++++++++
 extent_io.h |  2 ++
 volumes.c   |  3 ++-
 3 files changed, 30 insertions(+), 1 deletion(-)

diff --git a/extent_io.c b/extent_io.c
index d4a68279..7f05a729 100644
--- a/extent_io.c
+++ b/extent_io.c
@@ -765,6 +765,32 @@ struct extent_buffer *alloc_extent_buffer(struct btrfs_fs_info *fs_info,
 	return eb;
 }
 
+/*
+ * Allocate a dummy extent buffer which won't be inserted into extent
+ * buffer cache.
+ *
+ * This mostly allows super block read write using existing eb infrastructure
+ * without pulluting the eb cache.
+ *
+ * This is especially important to avoid injecting eb->start == SZ_64K, as
+ * fuzzed image could have invalid tree bytenr covers super block range,
+ * and cause ref count underflow.
+ */
+struct extent_buffer *alloc_dummy_extent_buffer(struct btrfs_fs_info *fs_info,
+						u64 bytenr, u32 blocksize)
+{
+	struct extent_buffer *ret;
+
+	ret = __alloc_extent_buffer(fs_info, bytenr, blocksize);
+	if (!ret)
+		return NULL;
+
+	ret->tree = NULL;
+	ret->flags |= EXTENT_BUFFER_DUMMY;
+
+	return ret;
+}
+
 int read_extent_from_disk(struct extent_buffer *eb,
 			  unsigned long offset, unsigned long len)
 {
diff --git a/extent_io.h b/extent_io.h
index 1715acc6..c6b8acf9 100644
--- a/extent_io.h
+++ b/extent_io.h
@@ -149,6 +149,8 @@ struct extent_buffer *find_first_extent_buffer(struct extent_io_tree *tree,
 struct extent_buffer *alloc_extent_buffer(struct btrfs_fs_info *fs_info,
 					  u64 bytenr, u32 blocksize);
 struct extent_buffer *btrfs_clone_extent_buffer(struct extent_buffer *src);
+struct extent_buffer *alloc_dummy_extent_buffer(struct btrfs_fs_info *fs_info,
+						u64 bytenr, u32 blocksize);
 void free_extent_buffer(struct extent_buffer *eb);
 void free_extent_buffer_nocache(struct extent_buffer *eb);
 int read_extent_from_disk(struct extent_buffer *eb,
diff --git a/volumes.c b/volumes.c
index 143164f0..44789f20 100644
--- a/volumes.c
+++ b/volumes.c
@@ -2144,7 +2144,8 @@ int btrfs_read_sys_array(struct btrfs_fs_info *fs_info)
 				fs_info->nodesize);
 		return -EINVAL;
 	}
-	sb = btrfs_find_create_tree_block(fs_info, BTRFS_SUPER_INFO_OFFSET);
+	sb = alloc_dummy_extent_buffer(fs_info, BTRFS_SUPER_INFO_OFFSET,
+				       BTRFS_SUPER_INFO_SIZE);
 	if (!sb)
 		return -ENOMEM;
 	btrfs_set_buffer_uptodate(sb);
-- 
2.24.1

[PATCH 4/6] btrfs-progs: Add extra chunk item size check

[Qu Wenruo]

[BUG]
For one fuzzed image, `btrfs check` both modes will trigger a BUG_ON():
  Opening filesystem to check...
  Checking filesystem on issue_208.raw
  UUID: 99e50868-0bda-4d89-b0e4-7e8560312ef9
  [1/7] checking root items
  [2/7] checking extents
  ctree.h:320: btrfs_chunk_item_size: BUG_ON `num_stripes == 0` triggered, value 1
  btrfs(+0x2f712)[0x55829afa6712]
  btrfs(+0x322e5)[0x55829afa92e5]
  btrfs(+0x6892a)[0x55829afdf92a]
  btrfs(+0x69099)[0x55829afe0099]
  btrfs(+0x69c68)[0x55829afe0c68]
  btrfs(+0x6dc27)[0x55829afe4c27]
  btrfs(main+0x94)[0x55829af8b0c4]
  /usr/lib/libc.so.6(__libc_start_main+0xf3)[0x7f3edc715ee3]
  btrfs(_start+0x2e)[0x55829af8b35e]

[CAUSE]
The fuzzed image has an invalid chunk item in chunk tree:
  item 1 key (FIRST_CHUNK_TREE CHUNK_ITEM 13631488) itemoff 16105 itemsize 80
  invalid num_stripes: 0

Which triggers that BUG_ON().

[FIX]
Here we enhance the verification of btrfs_check_chunk_valid(), to check
the num_stripes and item size.

Issue: #208
Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 volumes.c | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/volumes.c b/volumes.c
index 44789f20..0f618978 100644
--- a/volumes.c
+++ b/volumes.c
@@ -1858,12 +1858,36 @@ int btrfs_check_chunk_valid(struct btrfs_fs_info *fs_info,
 	u32 chunk_ondisk_size;
 	u32 sectorsize = fs_info->sectorsize;
 
+	/*
+	 * Basic chunk item size check.
+	 * Note btrfs_chunk has already contained one stripe, so no == check.
+	 */
+	if (slot >= 0 &&
+	    btrfs_item_size_nr(leaf, slot) < sizeof(struct btrfs_chunk)) {
+		error("invalid chunk item size, have %u expect [%zu, %lu)",
+			btrfs_item_size_nr(leaf, slot),
+			sizeof(struct btrfs_chunk),
+			BTRFS_LEAF_DATA_SIZE(fs_info));
+		return -EUCLEAN;
+	}
 	length = btrfs_chunk_length(leaf, chunk);
 	stripe_len = btrfs_chunk_stripe_len(leaf, chunk);
 	num_stripes = btrfs_chunk_num_stripes(leaf, chunk);
 	sub_stripes = btrfs_chunk_sub_stripes(leaf, chunk);
 	type = btrfs_chunk_type(leaf, chunk);
 
+	if (num_stripes == 0) {
+		error("invalid num_stripes, have %u expect non-zero",
+			num_stripes);
+		return -EUCLEAN;
+	}
+	if (slot >= 0 && btrfs_chunk_item_size(num_stripes) !=
+	    btrfs_item_size_nr(leaf, slot)) {
+		error("invalid chunk item size, have %u expect %lu",
+			btrfs_item_size_nr(leaf, slot),
+			btrfs_chunk_item_size(num_stripes));
+		return -EUCLEAN;
+	}
 	/*
 	 * These valid checks may be insufficient to cover every corner cases.
 	 */
-- 
2.24.1

[PATCH 5/6] btrfs-progs: extent-tree: Kill the BUG_ON() in btrfs_chunk_readonly()

[Qu Wenruo]

[BUG]
For a fuzzed image, `btrfs check` both modes trigger BUG_ON():

  Opening filesystem to check...
  volumes.c:1795: btrfs_chunk_readonly: BUG_ON `!ce` triggered, value 1
  btrfs(+0x2f712)[0x557beff3b712]
  btrfs(+0x32059)[0x557beff3e059]
  btrfs(btrfs_read_block_groups+0x282)[0x557beff30972]
  btrfs(btrfs_setup_all_roots+0x3f3)[0x557beff2ab23]
  btrfs(+0x1ef53)[0x557beff2af53]
  btrfs(open_ctree_fs_info+0x90)[0x557beff2b1a0]
  btrfs(+0x6d3f9)[0x557beff793f9]
  btrfs(main+0x94)[0x557beff200c4]
  /usr/lib/libc.so.6(__libc_start_main+0xf3)[0x7f623ac97ee3]
  btrfs(_start+0x2e)[0x557beff2035e]

[CAUSE]
The fuzzed image has a bad extent tree:

        item 0 key (288230376165343232 BLOCK_GROUP_ITEM 8388608) itemoff 16259 itemsize 24
                block group used 0 chunk_objectid 256 flags DATA

There is no corresponding chunk for the block group.

In then we hit the BUG_ON(), which expects chunk mapping for
btrfs_chunk_readonly().

[FIX]
Remove that BUG_ON() with proper error handling, and make
btrfs_read_block_groups() handle the -ENOENT error from
read_one_block_group() to continue.

So one corrupted block group item won't screw up the remaining block
group items.

Issue: #209
Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 extent-tree.c |  9 +++++++--
 volumes.c     | 14 ++++++++++----
 2 files changed, 17 insertions(+), 6 deletions(-)

diff --git a/extent-tree.c b/extent-tree.c
index 53be4f4c..6288c8a3 100644
--- a/extent-tree.c
+++ b/extent-tree.c
@@ -2708,7 +2708,12 @@ static int read_one_block_group(struct btrfs_fs_info *fs_info,
 		bit = BLOCK_GROUP_METADATA;
 	}
 	set_avail_alloc_bits(fs_info, cache->flags);
-	if (btrfs_chunk_readonly(fs_info, cache->key.objectid))
+	ret = btrfs_chunk_readonly(fs_info, cache->key.objectid);
+	if (ret < 0) {
+		free(cache);
+		return ret;
+	}
+	if (ret)
 		cache->ro = 1;
 	exclude_super_stripes(fs_info, cache);
 
@@ -2753,7 +2758,7 @@ int btrfs_read_block_groups(struct btrfs_fs_info *fs_info)
 		btrfs_item_key_to_cpu(path.nodes[0], &key, path.slots[0]);
 
 		ret = read_one_block_group(fs_info, &path);
-		if (ret < 0)
+		if (ret < 0 && ret != -ENOENT)
 			goto error;
 
 		if (key.offset == 0)
diff --git a/volumes.c b/volumes.c
index 0f618978..6c22c742 100644
--- a/volumes.c
+++ b/volumes.c
@@ -1802,6 +1802,11 @@ btrfs_find_device_by_devid(struct btrfs_fs_devices *fs_devices,
 	return NULL;
 }
 
+/*
+ * Return 0 if the chunk at @chunk_offset exists and is not read-only.
+ * Return 1 if the chunk at @chunk_offset exists and is read-only.
+ * Return <0 if we can't find chunk at @chunk_offset.
+ */
 int btrfs_chunk_readonly(struct btrfs_fs_info *fs_info, u64 chunk_offset)
 {
 	struct cache_extent *ce;
@@ -1814,12 +1819,13 @@ int btrfs_chunk_readonly(struct btrfs_fs_info *fs_info, u64 chunk_offset)
 	 * During chunk recovering, we may fail to find block group's
 	 * corresponding chunk, we will rebuild it later
 	 */
-	ce = search_cache_extent(&map_tree->cache_tree, chunk_offset);
-	if (!fs_info->is_chunk_recover)
-		BUG_ON(!ce);
-	else
+	if (fs_info->is_chunk_recover)
 		return 0;
 
+	ce = search_cache_extent(&map_tree->cache_tree, chunk_offset);
+	if (!ce)
+		return -ENOENT;
+
 	map = container_of(ce, struct map_lookup, ce);
 	for (i = 0; i < map->num_stripes; i++) {
 		if (!map->stripes[i].dev->writeable) {
-- 
2.24.1

[PATCH 6/6] btrfs-progs: extent-tree: Fix a by-one error in exclude_super_stripes()

[Qu Wenruo]

[BUG]
For certain btrfs images, a BUG_ON() can be triggered at open_ctree()
time:
  Opening filesystem to check...
  extent_io.c:158: insert_state: BUG_ON `end < start` triggered, value 1
  btrfs(+0x2de57)[0x560c4d7cfe57]
  btrfs(+0x2e210)[0x560c4d7d0210]
  btrfs(set_extent_bits+0x254)[0x560c4d7d0854]
  btrfs(exclude_super_stripes+0xbf)[0x560c4d7c65ff]
  btrfs(btrfs_read_block_groups+0x29d)[0x560c4d7c698d]
  btrfs(btrfs_setup_all_roots+0x3f3)[0x560c4d7c0b23]
  btrfs(+0x1ef53)[0x560c4d7c0f53]
  btrfs(open_ctree_fs_info+0x90)[0x560c4d7c11a0]
  btrfs(+0x6d3f9)[0x560c4d80f3f9]
  btrfs(main+0x94)[0x560c4d7b60c4]
  /usr/lib/libc.so.6(__libc_start_main+0xf3)[0x7fd189773ee3]
  btrfs(_start+0x2e)[0x560c4d7b635e]

[CAUSE]
This is caused by passing @len == 0 to add_excluded_extent(), which
means one revsere mapped range is just out of the block group range,
normally means a by-one error.

[FIX]
Fix the boundary check on the reserve mapped range against block group
range.
If a reverse mapped super block starts at the end of the block group, it
doesn't cover so we don't need to bother the case.

Issue: #210
Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 extent-tree.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/extent-tree.c b/extent-tree.c
index 6288c8a3..7ba80375 100644
--- a/extent-tree.c
+++ b/extent-tree.c
@@ -3640,7 +3640,7 @@ int exclude_super_stripes(struct btrfs_fs_info *fs_info,
 		while (nr--) {
 			u64 start, len;
 
-			if (logical[nr] > cache->key.objectid +
+			if (logical[nr] >= cache->key.objectid +
 			    cache->key.offset)
 				continue;
 
-- 
2.24.1

Re: [PATCH 6/6] btrfs-progs: extent-tree: Fix a by-one error in exclude_super_stripes()

[David Sterba]

On Wed, Dec 18, 2019 at 09:19:42AM +0800, Qu Wenruo wrote:
> [BUG]
> For certain btrfs images, a BUG_ON() can be triggered at open_ctree()
> time:
>   Opening filesystem to check...
>   extent_io.c:158: insert_state: BUG_ON `end < start` triggered, value 1
>   btrfs(+0x2de57)[0x560c4d7cfe57]
>   btrfs(+0x2e210)[0x560c4d7d0210]
>   btrfs(set_extent_bits+0x254)[0x560c4d7d0854]
>   btrfs(exclude_super_stripes+0xbf)[0x560c4d7c65ff]
>   btrfs(btrfs_read_block_groups+0x29d)[0x560c4d7c698d]
>   btrfs(btrfs_setup_all_roots+0x3f3)[0x560c4d7c0b23]
>   btrfs(+0x1ef53)[0x560c4d7c0f53]
>   btrfs(open_ctree_fs_info+0x90)[0x560c4d7c11a0]
>   btrfs(+0x6d3f9)[0x560c4d80f3f9]
>   btrfs(main+0x94)[0x560c4d7b60c4]
>   /usr/lib/libc.so.6(__libc_start_main+0xf3)[0x7fd189773ee3]
>   btrfs(_start+0x2e)[0x560c4d7b635e]
> 
> [CAUSE]
> This is caused by passing @len == 0 to add_excluded_extent(), which
> means one revsere mapped range is just out of the block group range,
> normally means a by-one error.
> 
> [FIX]
> Fix the boundary check on the reserve mapped range against block group
> range.
> If a reverse mapped super block starts at the end of the block group, it
> doesn't cover so we don't need to bother the case.
> 
> Issue: #210
> Signed-off-by: Qu Wenruo <wqu@suse.com>
> ---
>  extent-tree.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/extent-tree.c b/extent-tree.c
> index 6288c8a3..7ba80375 100644
> --- a/extent-tree.c
> +++ b/extent-tree.c
> @@ -3640,7 +3640,7 @@ int exclude_super_stripes(struct btrfs_fs_info *fs_info,
>  		while (nr--) {
>  			u64 start, len;
>  
> -			if (logical[nr] > cache->key.objectid +
> +			if (logical[nr] >= cache->key.objectid +
>  			    cache->key.offset)

Do we have the same problem in kernel? The code does ">".

Re: [PATCH 6/6] btrfs-progs: extent-tree: Fix a by-one error in exclude_super_stripes()

[Qu Wenruo]

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



On 2020/1/3 上午12:56, David Sterba wrote:
> On Wed, Dec 18, 2019 at 09:19:42AM +0800, Qu Wenruo wrote:
>> [BUG]
>> For certain btrfs images, a BUG_ON() can be triggered at open_ctree()
>> time:
>>   Opening filesystem to check...
>>   extent_io.c:158: insert_state: BUG_ON `end < start` triggered, value 1
>>   btrfs(+0x2de57)[0x560c4d7cfe57]
>>   btrfs(+0x2e210)[0x560c4d7d0210]
>>   btrfs(set_extent_bits+0x254)[0x560c4d7d0854]
>>   btrfs(exclude_super_stripes+0xbf)[0x560c4d7c65ff]
>>   btrfs(btrfs_read_block_groups+0x29d)[0x560c4d7c698d]
>>   btrfs(btrfs_setup_all_roots+0x3f3)[0x560c4d7c0b23]
>>   btrfs(+0x1ef53)[0x560c4d7c0f53]
>>   btrfs(open_ctree_fs_info+0x90)[0x560c4d7c11a0]
>>   btrfs(+0x6d3f9)[0x560c4d80f3f9]
>>   btrfs(main+0x94)[0x560c4d7b60c4]
>>   /usr/lib/libc.so.6(__libc_start_main+0xf3)[0x7fd189773ee3]
>>   btrfs(_start+0x2e)[0x560c4d7b635e]
>>
>> [CAUSE]
>> This is caused by passing @len == 0 to add_excluded_extent(), which
>> means one revsere mapped range is just out of the block group range,
>> normally means a by-one error.
>>
>> [FIX]
>> Fix the boundary check on the reserve mapped range against block group
>> range.
>> If a reverse mapped super block starts at the end of the block group, it
>> doesn't cover so we don't need to bother the case.
>>
>> Issue: #210
>> Signed-off-by: Qu Wenruo <wqu@suse.com>
>> ---
>>  extent-tree.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/extent-tree.c b/extent-tree.c
>> index 6288c8a3..7ba80375 100644
>> --- a/extent-tree.c
>> +++ b/extent-tree.c
>> @@ -3640,7 +3640,7 @@ int exclude_super_stripes(struct btrfs_fs_info *fs_info,
>>  		while (nr--) {
>>  			u64 start, len;
>>  
>> -			if (logical[nr] > cache->key.objectid +
>> +			if (logical[nr] >= cache->key.objectid +
>>  			    cache->key.offset)
> 
> Do we have the same problem in kernel? The code does ">".
> 
Oh, kernel looks to have the same problem.

Time to fix it.

Thanks,
Qu


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

Re: [PATCH 6/6] btrfs-progs: extent-tree: Fix a by-one error in exclude_super_stripes()

[Su Yue]

On 2020/1/3 8:42 AM, Qu Wenruo wrote:
>
>
> On 2020/1/3 上午12:56, David Sterba wrote:
>> On Wed, Dec 18, 2019 at 09:19:42AM +0800, Qu Wenruo wrote:
>>> [BUG]
>>> For certain btrfs images, a BUG_ON() can be triggered at open_ctree()
>>> time:
>>>    Opening filesystem to check...
>>>    extent_io.c:158: insert_state: BUG_ON `end < start` triggered, value 1
>>>    btrfs(+0x2de57)[0x560c4d7cfe57]
>>>    btrfs(+0x2e210)[0x560c4d7d0210]
>>>    btrfs(set_extent_bits+0x254)[0x560c4d7d0854]
>>>    btrfs(exclude_super_stripes+0xbf)[0x560c4d7c65ff]
>>>    btrfs(btrfs_read_block_groups+0x29d)[0x560c4d7c698d]
>>>    btrfs(btrfs_setup_all_roots+0x3f3)[0x560c4d7c0b23]
>>>    btrfs(+0x1ef53)[0x560c4d7c0f53]
>>>    btrfs(open_ctree_fs_info+0x90)[0x560c4d7c11a0]
>>>    btrfs(+0x6d3f9)[0x560c4d80f3f9]
>>>    btrfs(main+0x94)[0x560c4d7b60c4]
>>>    /usr/lib/libc.so.6(__libc_start_main+0xf3)[0x7fd189773ee3]
>>>    btrfs(_start+0x2e)[0x560c4d7b635e]
>>>
>>> [CAUSE]
>>> This is caused by passing @len == 0 to add_excluded_extent(), which
>>> means one revsere mapped range is just out of the block group range,
>>> normally means a by-one error.
>>>
>>> [FIX]
>>> Fix the boundary check on the reserve mapped range against block group
>>> range.
>>> If a reverse mapped super block starts at the end of the block group, it
>>> doesn't cover so we don't need to bother the case.
>>>
>>> Issue: #210
>>> Signed-off-by: Qu Wenruo <wqu@suse.com>
>>> ---
>>>   extent-tree.c | 2 +-
>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/extent-tree.c b/extent-tree.c
>>> index 6288c8a3..7ba80375 100644
>>> --- a/extent-tree.c
>>> +++ b/extent-tree.c
>>> @@ -3640,7 +3640,7 @@ int exclude_super_stripes(struct btrfs_fs_info *fs_info,
>>>   		while (nr--) {
>>>   			u64 start, len;
>>>
>>> -			if (logical[nr] > cache->key.objectid +
>>> +			if (logical[nr] >= cache->key.objectid +
>>>   			    cache->key.offset)
>>
>> Do we have the same problem in kernel? The code does ">".
>>
> Oh, kernel looks to have the same problem.
>

Gentle reminder to save your time.

I saw the '>' as in the kernel code and send a patch for it.
Then Nikolay pointed out that the patch is unnecessary[1].
The fuzz image is rejected to be mounted by tree-checker earlier.

Besides, a cleanup series[2] was sent by him.

[1]:
https://lore.kernel.org/linux-btrfs/24721dc8-8bda-a086-ff1a-31a0b21a02b4@suse.com/T/#m4cc5153d1645177e0f81f00c86e10dfb36015def
[2]: https://patchwork.kernel.org/project/linux-btrfs/list/?series=205103

> Time to fix it.
>
> Thanks,
> Qu
>

Re: [PATCH 0/6] btrfs-progs: Fixes for github issues

[David Sterba]

On Wed, Dec 18, 2019 at 09:19:36AM +0800, Qu Wenruo wrote:
> There are a new batch of fuzzed images for btrfs-progs. They are all
> reported by Ruud van Asseldonk from github.
> 
> Patch 1 will make QA life easier by remove the extra 300s wait time.
> Patch 2~5 are all the meat for the fuzzed images.
> 
> Just a kind reminder, mkfs/020 test will fail due to tons of problems:
> - Undefined $csum variable
> - Undefined $dev1 variable

These are fixed in devel now.

> - Bad kernel probe for support csum
>   E.g. if Blake2 not compiled, it still shows up in supported csum algo,
>   but will fail to mount.

The list of supported is from the point of view of the filesystem.
Providing the module is up to the user.

> All other tests pass.
> 
> Qu Wenruo (6):
>   btrfs-progs: tests: Add --force for repair command
>   btrfs-progs: check/original: Do extra verification on file extent item
>   btrfs-progs: disk-io: Verify the bytenr passed in is mapped for
>     read_tree_block()
>   btrfs-progs: Add extra chunk item size check
>   btrfs-progs: extent-tree: Kill the BUG_ON() in btrfs_chunk_readonly()
>   btrfs-progs: extent-tree: Fix a by-one error in
>     exclude_super_stripes()

All added to devel, thanks.

Re: [PATCH 0/6] btrfs-progs: Fixes for github issues

[Qu Wenruo]

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



On 2020/1/3 上午1:10, David Sterba wrote:
> On Wed, Dec 18, 2019 at 09:19:36AM +0800, Qu Wenruo wrote:
>> There are a new batch of fuzzed images for btrfs-progs. They are all
>> reported by Ruud van Asseldonk from github.
>>
>> Patch 1 will make QA life easier by remove the extra 300s wait time.
>> Patch 2~5 are all the meat for the fuzzed images.
>>
>> Just a kind reminder, mkfs/020 test will fail due to tons of problems:
>> - Undefined $csum variable
>> - Undefined $dev1 variable
> 
> These are fixed in devel now.
> 
>> - Bad kernel probe for support csum
>>   E.g. if Blake2 not compiled, it still shows up in supported csum algo,
>>   but will fail to mount.
> 
> The list of supported is from the point of view of the filesystem.
> Providing the module is up to the user.

IIRC, doing such probe at btrfs module load time would be more user
friendly though.

Thanks,
Qu

> 
>> All other tests pass.
>>
>> Qu Wenruo (6):
>>   btrfs-progs: tests: Add --force for repair command
>>   btrfs-progs: check/original: Do extra verification on file extent item
>>   btrfs-progs: disk-io: Verify the bytenr passed in is mapped for
>>     read_tree_block()
>>   btrfs-progs: Add extra chunk item size check
>>   btrfs-progs: extent-tree: Kill the BUG_ON() in btrfs_chunk_readonly()
>>   btrfs-progs: extent-tree: Fix a by-one error in
>>     exclude_super_stripes()
> 
> All added to devel, thanks.
> 


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

Re: [PATCH 0/6] btrfs-progs: Fixes for github issues

[David Sterba]

On Fri, Jan 03, 2020 at 08:43:01AM +0800, Qu Wenruo wrote:
> 
> 
> On 2020/1/3 上午1:10, David Sterba wrote:
> > On Wed, Dec 18, 2019 at 09:19:36AM +0800, Qu Wenruo wrote:
> >> There are a new batch of fuzzed images for btrfs-progs. They are all
> >> reported by Ruud van Asseldonk from github.
> >>
> >> Patch 1 will make QA life easier by remove the extra 300s wait time.
> >> Patch 2~5 are all the meat for the fuzzed images.
> >>
> >> Just a kind reminder, mkfs/020 test will fail due to tons of problems:
> >> - Undefined $csum variable
> >> - Undefined $dev1 variable
> > 
> > These are fixed in devel now.
> > 
> >> - Bad kernel probe for support csum
> >>   E.g. if Blake2 not compiled, it still shows up in supported csum algo,
> >>   but will fail to mount.
> > 
> > The list of supported is from the point of view of the filesystem.
> > Providing the module is up to the user.
> 
> IIRC, doing such probe at btrfs module load time would be more user
> friendly though.

I don't understand how you think this could be improved. The list of
algorithms is provided by the filesystem, the implementations are
provided by the crypto subsystem (either compiled in or as modules). Two
different things.

So you mean that at btrfs module load time, all hash algorithms are
probed? What if some of them gets unloaded, or loaded later (so modprobe
won't see it at btrfs load time). This would require keeping the state
up to date and this is out of scope what filesystem should do.

Re: [PATCH 0/6] btrfs-progs: Fixes for github issues

[Qu Wenruo]

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



On 2020/1/3 下午11:27, David Sterba wrote:
> On Fri, Jan 03, 2020 at 08:43:01AM +0800, Qu Wenruo wrote:
>>
>>
>> On 2020/1/3 上午1:10, David Sterba wrote:
>>> On Wed, Dec 18, 2019 at 09:19:36AM +0800, Qu Wenruo wrote:
>>>> There are a new batch of fuzzed images for btrfs-progs. They are all
>>>> reported by Ruud van Asseldonk from github.
>>>>
>>>> Patch 1 will make QA life easier by remove the extra 300s wait time.
>>>> Patch 2~5 are all the meat for the fuzzed images.
>>>>
>>>> Just a kind reminder, mkfs/020 test will fail due to tons of problems:
>>>> - Undefined $csum variable
>>>> - Undefined $dev1 variable
>>>
>>> These are fixed in devel now.
>>>
>>>> - Bad kernel probe for support csum
>>>>   E.g. if Blake2 not compiled, it still shows up in supported csum algo,
>>>>   but will fail to mount.
>>>
>>> The list of supported is from the point of view of the filesystem.
>>> Providing the module is up to the user.
>>
>> IIRC, doing such probe at btrfs module load time would be more user
>> friendly though.
> 
> I don't understand how you think this could be improved. The list of
> algorithms is provided by the filesystem, the implementations are
> provided by the crypto subsystem (either compiled in or as modules). Two
> different things.
> 
> So you mean that at btrfs module load time, all hash algorithms are
> probed?

Yes, that's why I mean.

> What if some of them gets unloaded, or loaded later (so modprobe
> won't see it at btrfs load time). This would require keeping the state
> up to date and this is out of scope what filesystem should do.
> 
Isn't there any mechanism to load the module when necessary?

Like loopback, it's only loaded when we first setup loopback device.

Thanks,
Qu


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

Re: [PATCH 0/6] btrfs-progs: Fixes for github issues

[David Sterba]

On Sat, Jan 04, 2020 at 09:26:25AM +0800, Qu Wenruo wrote:
> 
> 
> On 2020/1/3 下午11:27, David Sterba wrote:
> > On Fri, Jan 03, 2020 at 08:43:01AM +0800, Qu Wenruo wrote:
> >>
> >>
> >> On 2020/1/3 上午1:10, David Sterba wrote:
> >>> On Wed, Dec 18, 2019 at 09:19:36AM +0800, Qu Wenruo wrote:
> >>>> There are a new batch of fuzzed images for btrfs-progs. They are all
> >>>> reported by Ruud van Asseldonk from github.
> >>>>
> >>>> Patch 1 will make QA life easier by remove the extra 300s wait time.
> >>>> Patch 2~5 are all the meat for the fuzzed images.
> >>>>
> >>>> Just a kind reminder, mkfs/020 test will fail due to tons of problems:
> >>>> - Undefined $csum variable
> >>>> - Undefined $dev1 variable
> >>>
> >>> These are fixed in devel now.
> >>>
> >>>> - Bad kernel probe for support csum
> >>>>   E.g. if Blake2 not compiled, it still shows up in supported csum algo,
> >>>>   but will fail to mount.
> >>>
> >>> The list of supported is from the point of view of the filesystem.
> >>> Providing the module is up to the user.
> >>
> >> IIRC, doing such probe at btrfs module load time would be more user
> >> friendly though.
> > 
> > I don't understand how you think this could be improved. The list of
> > algorithms is provided by the filesystem, the implementations are
> > provided by the crypto subsystem (either compiled in or as modules). Two
> > different things.
> > 
> > So you mean that at btrfs module load time, all hash algorithms are
> > probed?
> 
> Yes, that's why I mean.
> 
> > What if some of them gets unloaded, or loaded later (so modprobe
> > won't see it at btrfs load time). This would require keeping the state
> > up to date and this is out of scope what filesystem should do.
> > 
> Isn't there any mechanism to load the module when necessary?

Yes, that's what we rely on, once a filesystem is mounted it will
instantiate the crypto shash, that in turn will trigger module load if
necessary.

Probing all algorithms would load more modules than needed, for the only
reason to store/print list of what succeeded at that time. Whatever
happens to the system later will not be reflected here. And that's why I
don't want to do that, so we list checksums understood by btrfs module.

Re: [PATCH 0/6] btrfs-progs: Fixes for github issues

[Qu Wenruo]

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



On 2020/1/6 下午11:45, David Sterba wrote:
> On Sat, Jan 04, 2020 at 09:26:25AM +0800, Qu Wenruo wrote:
>>
>>
>> On 2020/1/3 下午11:27, David Sterba wrote:
>>> On Fri, Jan 03, 2020 at 08:43:01AM +0800, Qu Wenruo wrote:
>>>>
>>>>
>>>> On 2020/1/3 上午1:10, David Sterba wrote:
>>>>> On Wed, Dec 18, 2019 at 09:19:36AM +0800, Qu Wenruo wrote:
>>>>>> There are a new batch of fuzzed images for btrfs-progs. They are all
>>>>>> reported by Ruud van Asseldonk from github.
>>>>>>
>>>>>> Patch 1 will make QA life easier by remove the extra 300s wait time.
>>>>>> Patch 2~5 are all the meat for the fuzzed images.
>>>>>>
>>>>>> Just a kind reminder, mkfs/020 test will fail due to tons of problems:
>>>>>> - Undefined $csum variable
>>>>>> - Undefined $dev1 variable
>>>>>
>>>>> These are fixed in devel now.
>>>>>
>>>>>> - Bad kernel probe for support csum
>>>>>>   E.g. if Blake2 not compiled, it still shows up in supported csum algo,
>>>>>>   but will fail to mount.
>>>>>
>>>>> The list of supported is from the point of view of the filesystem.
>>>>> Providing the module is up to the user.
>>>>
>>>> IIRC, doing such probe at btrfs module load time would be more user
>>>> friendly though.
>>>
>>> I don't understand how you think this could be improved. The list of
>>> algorithms is provided by the filesystem, the implementations are
>>> provided by the crypto subsystem (either compiled in or as modules). Two
>>> different things.
>>>
>>> So you mean that at btrfs module load time, all hash algorithms are
>>> probed?
>>
>> Yes, that's why I mean.
>>
>>> What if some of them gets unloaded, or loaded later (so modprobe
>>> won't see it at btrfs load time). This would require keeping the state
>>> up to date and this is out of scope what filesystem should do.
>>>
>> Isn't there any mechanism to load the module when necessary?
> 
> Yes, that's what we rely on, once a filesystem is mounted it will
> instantiate the crypto shash, that in turn will trigger module load if
> necessary.
> 
> Probing all algorithms would load more modules than needed, for the only
> reason to store/print list of what succeeded at that time. Whatever
> happens to the system later will not be reflected here. And that's why I
> don't want to do that, so we list checksums understood by btrfs module.
> 
But we don't have requirement for all supported csum algo.

So it's completely possible to have a case where we don't have csum
support for BLAKE2.
Furthermore, there is no way to determine whether we support it at runtime.

Now we depend on distro to include all needed algorithms, although most
distro would include all these algos, but this still look a little
strange to me.

Any better solution for this?

Thanks,
Qu


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