BUG: failure at fs/btrfs/ctree.h:337/btrfs_chunk_item_size()!

BUG: failure at fs/btrfs/ctree.h:337/btrfs_chunk_item_size()!

[Vegard Nossum]

[-- Attachment #1: Type: text/plain, Size: 1151 bytes --]

Hi,

With the attached btrfs image, I get the following splat when mounting:

"""
# mount -o loop -t btrfs ./btrfs.0 /mnt/0/
BTRFS: device fsid 9006933e-2a9a-44f0-917f-514252aeec2c devid 1 transid 
7 /dev/loop0
BTRFS info (device loop0): disk space caching is enabled
BUG: failure at fs/btrfs/ctree.h:337/btrfs_chunk_item_size()!
Kernel panic - not syncing: BUG!
CPU: 0 PID: 313 Comm: mount Not tainted 4.2.5-00657-ge047887-dirty #25
Stack:
  637af890 60062489 602aeb2e 604192ba
  60387961 00000011 637af8a0 6038a835
  637af9c0 6038776b 634ef32b 00000000
Call Trace:
  [<6001c86d>] show_stack+0xfe/0x15b
  [<6038a835>] dump_stack+0x2a/0x2c
  [<6038776b>] panic+0x13e/0x2b3
  [<6020f099>] btrfs_read_sys_array+0x25d/0x2ff
  [<601cfbbe>] open_ctree+0x192d/0x27af
  [<6019c2c1>] btrfs_mount+0x8f5/0xb9a
  [<600bc9a7>] mount_fs+0x11/0xf3
  [<600d5167>] vfs_kern_mount+0x75/0x11a
  [<6019bcb0>] btrfs_mount+0x2e4/0xb9a
  [<600bc9a7>] mount_fs+0x11/0xf3
  [<600d5167>] vfs_kern_mount+0x75/0x11a
  [<600d710b>] do_mount+0xa35/0xbc9
  [<600d7557>] SyS_mount+0x95/0xc8
  [<6001e884>] handle_syscall+0x6b/0x8e

Aborted
"""

I can test patches. Thanks,


Vegard

[-- Attachment #2: btrfs.0.bz2 --]
[-- Type: application/x-bzip, Size: 6684 bytes --]

Re: BUG: failure at fs/btrfs/ctree.h:337/btrfs_chunk_item_size()!

[David Sterba]

On Sun, Nov 15, 2015 at 07:21:17PM +0100, Vegard Nossum wrote:
> With the attached btrfs image, I get the following splat when mounting:
> 
> """
> # mount -o loop -t btrfs ./btrfs.0 /mnt/0/
> BTRFS: device fsid 9006933e-2a9a-44f0-917f-514252aeec2c devid 1 transid 
> 7 /dev/loop0
> BTRFS info (device loop0): disk space caching is enabled
> BUG: failure at fs/btrfs/ctree.h:337/btrfs_chunk_item_size()!

Looks like a fuzzed image, it blows when num_stripes is 0

 335 static inline unsigned long btrfs_chunk_item_size(int num_stripes)
 336 {
 337         BUG_ON(num_stripes == 0);
 338         return sizeof(struct btrfs_chunk) +
 339                 sizeof(struct btrfs_stripe) * (num_stripes - 1);
 340 }

called from btrfs_read_sys_array. The check for num_stripes == 0 seems to be
missing from kernel code, 'btrfs check' detects that and does not crash.

The userspace code has some more validations so I'll port them to kernel
and this should fix the mount crash.

Re: BUG: failure at fs/btrfs/ctree.h:337/btrfs_chunk_item_size()!

[David Sterba]

On Mon, Nov 30, 2015 at 02:48:51PM +0100, David Sterba wrote:
> On Sun, Nov 15, 2015 at 07:21:17PM +0100, Vegard Nossum wrote:
> > With the attached btrfs image, I get the following splat when mounting:
> > 
> > """
> > # mount -o loop -t btrfs ./btrfs.0 /mnt/0/
> > BTRFS: device fsid 9006933e-2a9a-44f0-917f-514252aeec2c devid 1 transid 
> > 7 /dev/loop0
> > BTRFS info (device loop0): disk space caching is enabled
> > BUG: failure at fs/btrfs/ctree.h:337/btrfs_chunk_item_size()!

The fix, worked for me on the provided image:
https://patchwork.kernel.org/patch/7728051/

I'll add the attached image to btrfs-progs testsuite as it triggers
crashes in other tools.

Re: BUG: failure at fs/btrfs/ctree.h:337/btrfs_chunk_item_size()!

[Vegard Nossum]

[-- Attachment #1: Type: text/plain, Size: 2245 bytes --]

On 11/30/2015 05:34 PM, David Sterba wrote:
> On Mon, Nov 30, 2015 at 02:48:51PM +0100, David Sterba wrote:
>> On Sun, Nov 15, 2015 at 07:21:17PM +0100, Vegard Nossum wrote:
>>> With the attached btrfs image, I get the following splat when mounting:
>>>
>>> """
>>> # mount -o loop -t btrfs ./btrfs.0 /mnt/0/
>>> BTRFS: device fsid 9006933e-2a9a-44f0-917f-514252aeec2c devid 1 transid
>>> 7 /dev/loop0
>>> BTRFS info (device loop0): disk space caching is enabled
>>> BUG: failure at fs/btrfs/ctree.h:337/btrfs_chunk_item_size()!
>
> The fix, worked for me on the provided image:
> https://patchwork.kernel.org/patch/7728051/
>
> I'll add the attached image to btrfs-progs testsuite as it triggers
> crashes in other tools.
>

Thanks, that seems to fix the problem.

With your patch and a new image, I run into a second issue (which is 
probably unrelated):

BTRFS critical (device loop0): unable to find logical 4294963200 len 
40966cc7e != de8
BUG: failure at fs/btrfs/inode.c:1805/btrfs_merge_bio_hook()!
Kernel panic - not syncing: BUG! errors
CPU: 0 PID: 913 Comm: mount Not tainted 4.2.5 #1cc7e devid 1 transid 7 
/dev/loop0
Stack: dev_item UUID does not match fsid: 
de80ced1-18ac-490c-9afb-cf0a7d66cc7e != de8
   e0147430 60075412 600bd457 603f5080
   606dfc7a 605e2b14 e0147440 605e595fors
   e0147560 605e291e e00cb2e8 00001000
  Call Trace:
   [<60029f3b>] show_stack+0xdb/0x1a0
   [<605e595f>] dump_stack+0x2a/0x2c
   [<605e291e>] panic+0x137/0x2ac
   [<602b8d0c>] btrfs_merge_bio_hook+0xfc/0x100
   [<602e3923>] submit_extent_page+0x223/0x330
   [<602e51a6>] __do_readpage+0x476/0xb60
   [<602e59bf>] __extent_read_full_page+0x12f/0x140
   [<602e8264>] read_extent_buffer_pages+0x1e4/0x410
   [<602a9d46>] btree_read_extent_buffer_pages.constprop.24+0x106/0x1a0
   [<602aa67e>] read_tree_block+0x5e/0xa0
   [<602b0064>] open_ctree+0x1814/0x2db0
   [<60273a3b>] btrfs_mount+0xf3b/0x1010
   [<601054d3>] mount_fs+0x33/0x210
   [<60124b94>] vfs_kern_mount+0x74/0x170
   [<60272df3>] btrfs_mount+0x2f3/0x1010
   [<601054d3>] mount_fs+0x33/0x210
   [<60124b94>] vfs_kern_mount+0x74/0x170
   [<601264dc>] do_mount+0x26c/0xf30
   [<6012768b>] SyS_mount+0xab/0x120

Should I start a new thread? I've attached the new image. Thanks,


Vegard

[-- Attachment #2: btrfs.1.bz2 --]
[-- Type: application/x-bzip, Size: 6887 bytes --]

Re: BUG: failure at fs/btrfs/ctree.h:337/btrfs_chunk_item_size()!

[Qu Wenruo]

Vegard Nossum wrote on 2015/11/30 23:05 +0100:
> On 11/30/2015 05:34 PM, David Sterba wrote:
>> On Mon, Nov 30, 2015 at 02:48:51PM +0100, David Sterba wrote:
>>> On Sun, Nov 15, 2015 at 07:21:17PM +0100, Vegard Nossum wrote:
>>>> With the attached btrfs image, I get the following splat when mounting:
>>>>
>>>> """
>>>> # mount -o loop -t btrfs ./btrfs.0 /mnt/0/
>>>> BTRFS: device fsid 9006933e-2a9a-44f0-917f-514252aeec2c devid 1 transid
>>>> 7 /dev/loop0
>>>> BTRFS info (device loop0): disk space caching is enabled
>>>> BUG: failure at fs/btrfs/ctree.h:337/btrfs_chunk_item_size()!
>>
>> The fix, worked for me on the provided image:
>> https://patchwork.kernel.org/patch/7728051/
>>
>> I'll add the attached image to btrfs-progs testsuite as it triggers
>> crashes in other tools.
>>
>
> Thanks, that seems to fix the problem.
>
> With your patch and a new image, I run into a second issue (which is
> probably unrelated):
>
> BTRFS critical (device loop0): unable to find logical 4294963200 len
> 40966cc7e != de8
> BUG: failure at fs/btrfs/inode.c:1805/btrfs_merge_bio_hook()!
> Kernel panic - not syncing: BUG! errors
> CPU: 0 PID: 913 Comm: mount Not tainted 4.2.5 #1cc7e devid 1 transid 7
> /dev/loop0
> Stack: dev_item UUID does not match fsid:
> de80ced1-18ac-490c-9afb-cf0a7d66cc7e != de8
>    e0147430 60075412 600bd457 603f5080
>    606dfc7a 605e2b14 e0147440 605e595fors
>    e0147560 605e291e e00cb2e8 00001000
>   Call Trace:
>    [<60029f3b>] show_stack+0xdb/0x1a0
>    [<605e595f>] dump_stack+0x2a/0x2c
>    [<605e291e>] panic+0x137/0x2ac
>    [<602b8d0c>] btrfs_merge_bio_hook+0xfc/0x100
>    [<602e3923>] submit_extent_page+0x223/0x330
>    [<602e51a6>] __do_readpage+0x476/0xb60
>    [<602e59bf>] __extent_read_full_page+0x12f/0x140
>    [<602e8264>] read_extent_buffer_pages+0x1e4/0x410
>    [<602a9d46>] btree_read_extent_buffer_pages.constprop.24+0x106/0x1a0
>    [<602aa67e>] read_tree_block+0x5e/0xa0
>    [<602b0064>] open_ctree+0x1814/0x2db0
>    [<60273a3b>] btrfs_mount+0xf3b/0x1010
>    [<601054d3>] mount_fs+0x33/0x210
>    [<60124b94>] vfs_kern_mount+0x74/0x170
>    [<60272df3>] btrfs_mount+0x2f3/0x1010
>    [<601054d3>] mount_fs+0x33/0x210
>    [<60124b94>] vfs_kern_mount+0x74/0x170
>    [<601264dc>] do_mount+0x26c/0xf30
>    [<6012768b>] SyS_mount+0xab/0x120
>
> Should I start a new thread? I've attached the new image. Thanks,
>
>
> Vegard
>
>
Glad to see btrfsck is much robust than kernel now. :)

Btrfsck already gives quite good clue on the problem:

ERROR: chunk_root block unaligned: 4294967168
ERROR: chunk_root block unaligned: 4294967168

Chunk root in superblock is not aligned, and the superblock is not valid.

It would be better to copy all these btrfs-progs enhancement to kernel.

Thanks,
Qu

Re: BUG: failure at fs/btrfs/ctree.h:337/btrfs_chunk_item_size()!

[David Sterba]

On Tue, Dec 01, 2015 at 08:50:57AM +0800, Qu Wenruo wrote:
> Btrfsck already gives quite good clue on the problem:
> 
> ERROR: chunk_root block unaligned: 4294967168
> ERROR: chunk_root block unaligned: 4294967168
> 
> Chunk root in superblock is not aligned, and the superblock is not valid.
> 
> It would be better to copy all these btrfs-progs enhancement to kernel.

We do have the alignment check in kernel, but it's in the early phase
where we don't know if nodesize is reliable and print only a warning.

Re: BUG: failure at fs/btrfs/ctree.h:337/btrfs_chunk_item_size()!

[Qu Wenruo]

David Sterba wrote on 2015/12/03 18:47 +0100:
> On Tue, Dec 01, 2015 at 08:50:57AM +0800, Qu Wenruo wrote:
>> Btrfsck already gives quite good clue on the problem:
>>
>> ERROR: chunk_root block unaligned: 4294967168
>> ERROR: chunk_root block unaligned: 4294967168
>>
>> Chunk root in superblock is not aligned, and the superblock is not valid.
>>
>> It would be better to copy all these btrfs-progs enhancement to kernel.
>
> We do have the alignment check in kernel, but it's in the early phase
> where we don't know if nodesize is reliable and print only a warning.
>
>
This can be enhanced by the following method:

1) Check sectorsize first
    Only several sector size is valid for current btrfs:
    4K, 8K, 16K, 32K, 64K
    Just five numbers, quite easy to check.
    Or if anyone is going to extend supported sectorsize, we can change
    the check to if the number is power of 2 starting from 4K.

2) Check nodesize/leafsize then
    It should be aligned to sectorsize.
    And nodesize must match with leafsize.
    Currently, it's done out of check_super_valid(), we can integrate it.

3) Check all super root bytenr against *sectorsize*
    Yeah, not nodesize.
    As some old bad convert will cause metadata extent unaligned to
    nodesize(just before my convert rework patch), but only aligned to
    sectorsize.
    So only check alignment of sectorsize.

If you are OK with this idea, I can make the patch soon.

Thanks,
Qu

Re: BUG: failure at fs/btrfs/ctree.h:337/btrfs_chunk_item_size()!

[David Sterba]

On Fri, Dec 04, 2015 at 09:21:59AM +0800, Qu Wenruo wrote:
> > We do have the alignment check in kernel, but it's in the early phase
> > where we don't know if nodesize is reliable and print only a warning.
> >
> This can be enhanced by the following method:

At minimum, we can promote the 4k alignment checks in
btrfs_check_super_valid from a warning to an error. The blocks must be
4k aligned, regardless of sectorsize or nodesize.

> 1) Check sectorsize first
>     Only several sector size is valid for current btrfs:
>     4K, 8K, 16K, 32K, 64K
>     Just five numbers, quite easy to check.

The sectorsize must be PAGE_SIZE at the moment. This will change with
Chandan's patchset though.

>     Or if anyone is going to extend supported sectorsize, we can change
>     the check to if the number is power of 2 starting from 4K.
> 
> 2) Check nodesize/leafsize then
>     It should be aligned to sectorsize.

This particular check is missing but is implicit because of the
sectorsize == PAGE_SIZE restriction.

>     And nodesize must match with leafsize.
>     Currently, it's done out of check_super_valid(), we can integrate it.

Yeah it's done, then I don't see why we should add it agian.

> 3) Check all super root bytenr against *sectorsize*
>     Yeah, not nodesize.
>     As some old bad convert will cause metadata extent unaligned to
>     nodesize(just before my convert rework patch), but only aligned to
>     sectorsize.
>     So only check alignment of sectorsize.

While the real check should be against the sectorsize, at the moment I
think it's covered by the 4k checks anyway. I understand why we can't
use the nodesize.

So, if we do the warning -> error, we're fine for now. Some of the
checks you suggest would be good to merge when the subpage blocksize
patchset is merged.

Re: BUG: failure at fs/btrfs/ctree.h:337/btrfs_chunk_item_size()!

[Qu Wenruo]

On 12/04/2015 09:12 PM, David Sterba wrote:
> On Fri, Dec 04, 2015 at 09:21:59AM +0800, Qu Wenruo wrote:
>>> We do have the alignment check in kernel, but it's in the early phase
>>> where we don't know if nodesize is reliable and print only a warning.
>>>
>> This can be enhanced by the following method:
>
> At minimum, we can promote the 4k alignment checks in
> btrfs_check_super_valid from a warning to an error. The blocks must be
> 4k aligned, regardless of sectorsize or nodesize.
>
>> 1) Check sectorsize first
>>      Only several sector size is valid for current btrfs:
>>      4K, 8K, 16K, 32K, 64K
>>      Just five numbers, quite easy to check.
>
> The sectorsize must be PAGE_SIZE at the moment. This will change with
> Chandan's patchset though.

PAGE_SIZE would be good enough.

>
>>      Or if anyone is going to extend supported sectorsize, we can change
>>      the check to if the number is power of 2 starting from 4K.
>>
>> 2) Check nodesize/leafsize then
>>      It should be aligned to sectorsize.
>
> This particular check is missing but is implicit because of the
> sectorsize == PAGE_SIZE restriction.

But still need to check nodesize/leafsize validation against sectorsize.
Current btrfs is already using large nodesize by default.

For example, 20K nodesize can pass 4K page size check but still wrong.

(And I'm also wrong in previous mail, it's not only aligned to 
sectorisze, but also need to be power of 2)

>
>>      And nodesize must match with leafsize.
>>      Currently, it's done out of check_super_valid(), we can integrate it.
>
> Yeah it's done, then I don't see why we should add it agian.

Just want to move it to check_super_valid(), as it's better to put 
validation check codes together, and that's why we have check_super_valid().

>
>> 3) Check all super root bytenr against *sectorsize*
>>      Yeah, not nodesize.
>>      As some old bad convert will cause metadata extent unaligned to
>>      nodesize(just before my convert rework patch), but only aligned to
>>      sectorsize.
>>      So only check alignment of sectorsize.
>
> While the real check should be against the sectorsize, at the moment I
> think it's covered by the 4k checks anyway. I understand why we can't
> use the nodesize.
>

4K is good enough for x86 family but can't find all problem for 64K page 
size like PPC64 or AArch64.

So it's still better to change the check at least to page size even we 
don't have subpage size support yet.

> So, if we do the warning -> error, we're fine for now. Some of the
> checks you suggest would be good to merge when the subpage blocksize
> patchset is merged.

Right, more accurate check is only needed after subpage patchset.

Thanks,
Qu

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

Re: BUG: failure at fs/btrfs/ctree.h:337/btrfs_chunk_item_size()!

[David Sterba]

On Mon, Nov 30, 2015 at 11:05:20PM +0100, Vegard Nossum wrote:
> With your patch and a new image, I run into a second issue (which is 
> probably unrelated):

Yes, it's unrelated. As pointed out by Qu, the alignment is wrong and
more strict checks should catch it.

> Should I start a new thread? I've attached the new image. Thanks,

Yes please, the bugs from fuzzers are usually hitting random bugs.