[PATCH 0/3] btrfs: Part 2 of enhanced defence against fuzzed images

[PATCH 0/3] btrfs: Part 2 of enhanced defence against fuzzed images

[Qu Wenruo]

This wave has the following features:
- Hunt down BUG_ON() in btrfs_delete_delayed_dir_index()
  EEXIST can cause BUG_ON(). And all callers of this function has
  already handled error by aborting transacation.

- Only allocate extents from the same block group type
  This is a very tricky bug, needs MIXED_GROUP super flag with regular
  block groups (separate META and DATA) and corrupted extent tree.

- ROOT_ITEM check for tree checker
  This kills the unaligned bytenr, invalid level and incorrect reloc
  tree.

Reported-by: Jungyeon Yoon <jungyeon.yoon@gmail.com>

Qu Wenruo (3):
  btrfs: delayed-inode: Kill the BUG_ON() in
    btrfs_delete_delayed_dir_index()
  btrfs: extent-tree: Make sure we only allocate extents from block
    groups with the same type
  btrfs: tree-checker: Add ROOT_ITEM check

 fs/btrfs/delayed-inode.c | 14 +++++-
 fs/btrfs/extent-tree.c   |  9 ++++
 fs/btrfs/tree-checker.c  | 92 ++++++++++++++++++++++++++++++++++++++++
 3 files changed, 113 insertions(+), 2 deletions(-)

-- 
2.22.0

[PATCH 1/3] btrfs: delayed-inode: Kill the BUG_ON() in btrfs_delete_delayed_dir_index()

[Qu Wenruo]

There is one report of fuzzed image which leads to BUG_ON() in
btrfs_delete_delayed_dir_index().

Although that fuzzed image can already be addressed by enhanced
extent-tree error handler, it's still better to hunt down more BUG_ON().

This patch will hunt down two BUG_ON()s in
btrfs_delete_delayed_dir_index():
- One for error from btrfs_delayed_item_reserve_metadata()
  Instead of BUG_ON(), we output an error message and free the item.
  And return the error.
  All callers of this function handles the error by aborting current
  trasaction.

- One for possible EEXIST from __btrfs_add_delayed_deletion_item()
  That function can return -EEXIST.
  We already have a good enough error message for that, only need to
  clean up the reserved metadata space and allocated item.

To help above cleanup, also modifiy __btrfs_remove_delayed_item() called
in btrfs_release_delayed_item(), to skip unassociated item.

Link: https://bugzilla.kernel.org/show_bug.cgi?id=203253
Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 fs/btrfs/delayed-inode.c | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/fs/btrfs/delayed-inode.c b/fs/btrfs/delayed-inode.c
index 43fdb2992956..c4946d58f49b 100644
--- a/fs/btrfs/delayed-inode.c
+++ b/fs/btrfs/delayed-inode.c
@@ -474,6 +474,9 @@ static void __btrfs_remove_delayed_item(struct btrfs_delayed_item *delayed_item)
 	struct rb_root_cached *root;
 	struct btrfs_delayed_root *delayed_root;
 
+	/* Not associated with any delayed_node */
+	if (!delayed_item->delayed_node)
+		return;
 	delayed_root = delayed_item->delayed_node->root->fs_info->delayed_root;
 
 	BUG_ON(!delayed_root);
@@ -1525,7 +1528,13 @@ int btrfs_delete_delayed_dir_index(struct btrfs_trans_handle *trans,
 	 * we have reserved enough space when we start a new transaction,
 	 * so reserving metadata failure is impossible.
 	 */
-	BUG_ON(ret);
+	if (ret < 0) {
+		btrfs_err(trans->fs_info,
+"metadata reserve fail at %s, should have already reserved space, ret=%d",
+			  __func__, ret);
+		btrfs_release_delayed_item(item);
+		goto end;
+	}
 
 	mutex_lock(&node->mutex);
 	ret = __btrfs_add_delayed_deletion_item(node, item);
@@ -1534,7 +1543,8 @@ int btrfs_delete_delayed_dir_index(struct btrfs_trans_handle *trans,
 			  "err add delayed dir index item(index: %llu) into the deletion tree of the delayed node(root id: %llu, inode id: %llu, errno: %d)",
 			  index, node->root->root_key.objectid,
 			  node->inode_id, ret);
-		BUG();
+		btrfs_delayed_item_release_metadata(dir->root, item);
+		btrfs_release_delayed_item(item);
 	}
 	mutex_unlock(&node->mutex);
 end:
-- 
2.22.0

[PATCH 2/3] btrfs: extent-tree: Make sure we only allocate extents from block groups with the same type

[Qu Wenruo]

[BUG]
With fuzzed image and MIXED_GROUPS super flag, we can hitthe following
BUG_ON():
  ------------[ cut here ]------------
  kernel BUG at fs/btrfs/delayed-ref.c:491!
  invalid opcode: 0000 [#1] PREEMPT SMP NOPTI
  CPU: 0 PID: 1849 Comm: sync Tainted: G           O      5.2.0-custom #27
  RIP: 0010:update_existing_head_ref.cold+0x44/0x46 [btrfs]
  Call Trace:
   add_delayed_ref_head+0x20c/0x2d0 [btrfs]
   btrfs_add_delayed_tree_ref+0x1fc/0x490 [btrfs]
   btrfs_free_tree_block+0x123/0x380 [btrfs]
   __btrfs_cow_block+0x435/0x500 [btrfs]
   btrfs_cow_block+0x110/0x240 [btrfs]
   btrfs_search_slot+0x230/0xa00 [btrfs]
   ? __lock_acquire+0x105e/0x1e20
   btrfs_insert_empty_items+0x67/0xc0 [btrfs]
   alloc_reserved_file_extent+0x9e/0x340 [btrfs]
   __btrfs_run_delayed_refs+0x78e/0x1240 [btrfs]
   ? kvm_clock_read+0x18/0x30
   ? __sched_clock_gtod_offset+0x21/0x50
   btrfs_run_delayed_refs.part.0+0x4e/0x180 [btrfs]
   btrfs_run_delayed_refs+0x23/0x30 [btrfs]
   btrfs_commit_transaction+0x53/0x9f0 [btrfs]
   btrfs_sync_fs+0x7c/0x1c0 [btrfs]
   ? __ia32_sys_fdatasync+0x20/0x20
   sync_fs_one_sb+0x23/0x30
   iterate_supers+0x95/0x100
   ksys_sync+0x62/0xb0
   __ia32_sys_sync+0xe/0x20
   do_syscall_64+0x65/0x240
   entry_SYSCALL_64_after_hwframe+0x49/0xbe
  ---[ end trace 4372e814d777d9b9 ]---

[CAUSE]
This situation is caused by several factors:
- Fuzzed image
  The extent tree of this fs missed one backref for extent tree root.
  So we can allocated space from that slot.

- MIXED_BG feature
  Super block has MIXED_BG flag.

- No mixed block groups exists
  All block groups are just regular ones.

This makes data space_info->block_groups[] contains metadata block
groups.
And when we reserve space for data, we can use space in metadata block
group.

Then we hit the following file operations:
- falloc
  We need to allocate data extents.
  find_free_extent() choose to use the metadata block to allocate space
  from, and choose the space of extent tree root, since its backref is
  missing.

  This generate one delayed ref head with is_data = 1.

- extent tree update
  We need to update extent tree at run_delayed_ref time.

  This generate one delayed ref head with is_data = 0, for the same
  bytenr of old extent tree root.

Then we trigger the BUG_ON().

[FIX]
The quick fix here is to check block_group->flags before using it.

The problem can only happen for MIXED_GROUPS fs. Regular filesystems
won't have space_info with DATA|METADATA flag, and no way to hit the
bug.

Link: https://bugzilla.kernel.org/show_bug.cgi?id=203255
Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 fs/btrfs/extent-tree.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
index 72868d9ac58e..69d6b2e308fc 100644
--- a/fs/btrfs/extent-tree.c
+++ b/fs/btrfs/extent-tree.c
@@ -8013,6 +8013,15 @@ static noinline int find_free_extent(struct btrfs_fs_info *fs_info,
 			 */
 			if ((flags & extra) && !(block_group->flags & extra))
 				goto loop;
+
+			/*
+			 * This block group has different flags than we what.
+			 * It's possible that we have MIXED_GROUP flag but no
+			 * block groups is mixed.
+			 * Just skip such block group.
+			 */
+			btrfs_release_block_group(block_group, delalloc);
+			continue;
 		}
 
 have_block_group:
-- 
2.22.0

[PATCH 3/3] btrfs: tree-checker: Add ROOT_ITEM check

[Qu Wenruo]

This patch will introduce ROOT_ITEM check, which includes:
- Key->objectid and key->offset check
  Currently only some easy check, e.g. 0 as rootid is invalid.

- Item size check
  Root item size is fixed.

- Generation checks
  Generation, v2_generaetion and last_snapshot should not pass super
  generation + 1

- Level and alignment check
  Level should be in [0, 7], and bytenr must be aligned to sector size.

- Flags check

Link: https://bugzilla.kernel.org/show_bug.cgi?id=203261
Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 fs/btrfs/tree-checker.c | 92 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 92 insertions(+)

diff --git a/fs/btrfs/tree-checker.c b/fs/btrfs/tree-checker.c
index a4c7f7ed8490..71bbbce3076d 100644
--- a/fs/btrfs/tree-checker.c
+++ b/fs/btrfs/tree-checker.c
@@ -810,6 +810,95 @@ static int check_inode_item(struct extent_buffer *leaf,
 	return 0;
 }
 
+static int check_root_item(struct extent_buffer *leaf, struct btrfs_key *key,
+			   int slot)
+{
+	struct btrfs_fs_info *fs_info = leaf->fs_info;
+	struct btrfs_root_item ri;
+	u64 valid_root_flags = BTRFS_ROOT_SUBVOL_RDONLY |
+			       BTRFS_ROOT_SUBVOL_DEAD;
+
+	/* No Such Tree id*/
+	if (key->objectid == 0) {
+		generic_err(leaf, slot, "invalid root id 0");
+		return -EUCLEAN;
+	}
+
+	/*
+	 * Some older kernel may create ROOT_ITEM with non-zero offset,
+	 * so here we only check offset for reloc tree whose key->offset
+	 * must be a valid tree.
+	 */
+	if (key->objectid == BTRFS_TREE_RELOC_OBJECTID && key->offset == 0) {
+		generic_err(leaf, slot, "invalid root id 0 for reloc tree");
+		return -EUCLEAN;
+	}
+
+	if (btrfs_item_size_nr(leaf, slot) != sizeof(ri)) {
+		generic_err(leaf, slot,
+			"invalid root item size, have %u expect %lu",
+			    btrfs_item_size_nr(leaf, slot), sizeof(ri));
+	}
+
+	read_extent_buffer(leaf, &ri, btrfs_item_ptr_offset(leaf, slot),
+			   sizeof(ri));
+
+	/* Generateion related */
+	if (btrfs_root_generation(&ri) >
+	    btrfs_super_generation(fs_info->super_copy) + 1) {
+		generic_err(leaf, slot,
+			"invalid root generaetion, have %llu expect (0, %llu]",
+			    btrfs_root_generation(&ri),
+			    btrfs_super_generation(fs_info->super_copy) + 1);
+		return -EUCLEAN;
+	}
+	if (btrfs_root_generation_v2(&ri) >
+	    btrfs_super_generation(fs_info->super_copy) + 1) {
+		generic_err(leaf, slot,
+		"invalid root v2 generaetion, have %llu expect (0, %llu]",
+			    btrfs_root_generation_v2(&ri),
+			    btrfs_super_generation(fs_info->super_copy) + 1);
+		return -EUCLEAN;
+	}
+	if (btrfs_root_last_snapshot(&ri) >
+	    btrfs_super_generation(fs_info->super_copy) + 1) {
+		generic_err(leaf, slot,
+		"invalid root last_snapshot, have %llu expect (0, %llu]",
+			    btrfs_root_last_snapshot(&ri),
+			    btrfs_super_generation(fs_info->super_copy) + 1);
+		return -EUCLEAN;
+	}
+
+	/* Alignment and level check */
+	if (!IS_ALIGNED(btrfs_root_bytenr(&ri), fs_info->sectorsize)) {
+		generic_err(leaf, slot,
+			"invalid root bytenr, have %llu expect to be aligned to %u",
+			    btrfs_root_bytenr(&ri), fs_info->sectorsize);
+		return -EUCLEAN;
+	}
+	if (btrfs_root_level(&ri) >= BTRFS_MAX_LEVEL) {
+		generic_err(leaf, slot,
+			"invalid root level, have %u expect [0, %u]",
+			    btrfs_root_level(&ri), BTRFS_MAX_LEVEL - 1);
+		return -EUCLEAN;
+	}
+	if (ri.drop_level >= BTRFS_MAX_LEVEL) {
+		generic_err(leaf, slot,
+			"invalid root level, have %u expect [0, %u]",
+			    ri.drop_level, BTRFS_MAX_LEVEL - 1);
+		return -EUCLEAN;
+	}
+
+	/* Flags check */
+	if (btrfs_root_flags(&ri) & ~valid_root_flags) {
+		generic_err(leaf, slot,
+			"invalid root flags, have 0x%llx expect mask 0x%llu",
+			    btrfs_root_flags(&ri), valid_root_flags);
+		return -EUCLEAN;
+	}
+	return 0;
+}
+
 /*
  * Common point to switch the item-specific validation.
  */
@@ -845,6 +934,9 @@ static int check_leaf_item(struct extent_buffer *leaf,
 	case BTRFS_INODE_ITEM_KEY:
 		ret = check_inode_item(leaf, key, slot);
 		break;
+	case BTRFS_ROOT_ITEM_KEY:
+		ret = check_root_item(leaf, key, slot);
+		break;
 	}
 	return ret;
 }
-- 
2.22.0

Re: [PATCH 3/3] btrfs: tree-checker: Add ROOT_ITEM check

[David Sterba]

On Tue, Jul 16, 2019 at 05:00:34PM +0800, Qu Wenruo wrote:
> This patch will introduce ROOT_ITEM check, which includes:
> - Key->objectid and key->offset check
>   Currently only some easy check, e.g. 0 as rootid is invalid.
> 
> - Item size check
>   Root item size is fixed.
> 
> - Generation checks
>   Generation, v2_generaetion and last_snapshot should not pass super
>   generation + 1
> 
> - Level and alignment check
>   Level should be in [0, 7], and bytenr must be aligned to sector size.
> 
> - Flags check

Nice. I found some small things that I can fix, no need to resend.

> Link: https://bugzilla.kernel.org/show_bug.cgi?id=203261
> Signed-off-by: Qu Wenruo <wqu@suse.com>
> ---
>  fs/btrfs/tree-checker.c | 92 +++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 92 insertions(+)
> 
> diff --git a/fs/btrfs/tree-checker.c b/fs/btrfs/tree-checker.c
> index a4c7f7ed8490..71bbbce3076d 100644
> --- a/fs/btrfs/tree-checker.c
> +++ b/fs/btrfs/tree-checker.c
> @@ -810,6 +810,95 @@ static int check_inode_item(struct extent_buffer *leaf,
>  	return 0;
>  }
>  
> +static int check_root_item(struct extent_buffer *leaf, struct btrfs_key *key,
> +			   int slot)
> +{
> +	struct btrfs_fs_info *fs_info = leaf->fs_info;
> +	struct btrfs_root_item ri;
> +	u64 valid_root_flags = BTRFS_ROOT_SUBVOL_RDONLY |
> +			       BTRFS_ROOT_SUBVOL_DEAD;

You can use 'const' here.

> +
> +	/* No Such Tree id*/
> +	if (key->objectid == 0) {
> +		generic_err(leaf, slot, "invalid root id 0");
> +		return -EUCLEAN;
> +	}
> +
> +	/*
> +	 * Some older kernel may create ROOT_ITEM with non-zero offset,
> +	 * so here we only check offset for reloc tree whose key->offset
> +	 * must be a valid tree.
> +	 */
> +	if (key->objectid == BTRFS_TREE_RELOC_OBJECTID && key->offset == 0) {
> +		generic_err(leaf, slot, "invalid root id 0 for reloc tree");
> +		return -EUCLEAN;
> +	}
> +
> +	if (btrfs_item_size_nr(leaf, slot) != sizeof(ri)) {
> +		generic_err(leaf, slot,
> +			"invalid root item size, have %u expect %lu",
> +			    btrfs_item_size_nr(leaf, slot), sizeof(ri));

sizeof needs %zu

> +	}
> +
> +	read_extent_buffer(leaf, &ri, btrfs_item_ptr_offset(leaf, slot),
> +			   sizeof(ri));
> +
> +	/* Generateion related */

typo here and a few more times below

> +	if (btrfs_root_generation(&ri) >
> +	    btrfs_super_generation(fs_info->super_copy) + 1) {
> +		generic_err(leaf, slot,
> +			"invalid root generaetion, have %llu expect (0, %llu]",
> +			    btrfs_root_generation(&ri),
> +			    btrfs_super_generation(fs_info->super_copy) + 1);
> +		return -EUCLEAN;
> +	}
> +	if (btrfs_root_generation_v2(&ri) >
> +	    btrfs_super_generation(fs_info->super_copy) + 1) {
> +		generic_err(leaf, slot,
> +		"invalid root v2 generaetion, have %llu expect (0, %llu]",

So (0, %llu] here means that it must be greater than zero, right? I'm
not sure that everyone uses the same notation for open/closed notation.

> +			    btrfs_root_generation_v2(&ri),
> +			    btrfs_super_generation(fs_info->super_copy) + 1);
> +		return -EUCLEAN;
> +	}
> +	if (btrfs_root_last_snapshot(&ri) >
> +	    btrfs_super_generation(fs_info->super_copy) + 1) {
> +		generic_err(leaf, slot,
> +		"invalid root last_snapshot, have %llu expect (0, %llu]",
> +			    btrfs_root_last_snapshot(&ri),
> +			    btrfs_super_generation(fs_info->super_copy) + 1);
> +		return -EUCLEAN;
> +	}
> +
> +	/* Alignment and level check */
> +	if (!IS_ALIGNED(btrfs_root_bytenr(&ri), fs_info->sectorsize)) {
> +		generic_err(leaf, slot,
> +			"invalid root bytenr, have %llu expect to be aligned to %u",
> +			    btrfs_root_bytenr(&ri), fs_info->sectorsize);
> +		return -EUCLEAN;
> +	}
> +	if (btrfs_root_level(&ri) >= BTRFS_MAX_LEVEL) {
> +		generic_err(leaf, slot,
> +			"invalid root level, have %u expect [0, %u]",
> +			    btrfs_root_level(&ri), BTRFS_MAX_LEVEL - 1);
> +		return -EUCLEAN;
> +	}
> +	if (ri.drop_level >= BTRFS_MAX_LEVEL) {
> +		generic_err(leaf, slot,
> +			"invalid root level, have %u expect [0, %u]",
> +			    ri.drop_level, BTRFS_MAX_LEVEL - 1);
> +		return -EUCLEAN;
> +	}
> +
> +	/* Flags check */
> +	if (btrfs_root_flags(&ri) & ~valid_root_flags) {
> +		generic_err(leaf, slot,
> +			"invalid root flags, have 0x%llx expect mask 0x%llu",

                                                                     0x%llx

> +			    btrfs_root_flags(&ri), valid_root_flags);
> +		return -EUCLEAN;
> +	}
> +	return 0;
> +}
> +
>  /*
>   * Common point to switch the item-specific validation.
>   */
> @@ -845,6 +934,9 @@ static int check_leaf_item(struct extent_buffer *leaf,
>  	case BTRFS_INODE_ITEM_KEY:
>  		ret = check_inode_item(leaf, key, slot);
>  		break;
> +	case BTRFS_ROOT_ITEM_KEY:
> +		ret = check_root_item(leaf, key, slot);
> +		break;
>  	}
>  	return ret;
>  }
> -- 
> 2.22.0

Re: [PATCH 3/3] btrfs: tree-checker: Add ROOT_ITEM check

[Qu Wenruo]

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

[snip]
> 
>> +	}
>> +
>> +	read_extent_buffer(leaf, &ri, btrfs_item_ptr_offset(leaf, slot),
>> +			   sizeof(ri));
>> +
>> +	/* Generateion related */
> 
> typo here and a few more times below
> 
>> +	if (btrfs_root_generation(&ri) >
>> +	    btrfs_super_generation(fs_info->super_copy) + 1) {
>> +		generic_err(leaf, slot,
>> +			"invalid root generaetion, have %llu expect (0, %llu]",
>> +			    btrfs_root_generation(&ri),
>> +			    btrfs_super_generation(fs_info->super_copy) + 1);
>> +		return -EUCLEAN;
>> +	}
>> +	if (btrfs_root_generation_v2(&ri) >
>> +	    btrfs_super_generation(fs_info->super_copy) + 1) {
>> +		generic_err(leaf, slot,
>> +		"invalid root v2 generaetion, have %llu expect (0, %llu]",
> 
> So (0, %llu] here means that it must be greater than zero, right? I'm
> not sure that everyone uses the same notation for open/closed notation.

AFAIK in tree checker it's all the same notation.

Or any better solution for that?

Thanks,
Qu

> 
>> +			    btrfs_root_generation_v2(&ri),
>> +			    btrfs_super_generation(fs_info->super_copy) + 1);
>> +		return -EUCLEAN;
>> +	}
>> +	if (btrfs_root_last_snapshot(&ri) >
>> +	    btrfs_super_generation(fs_info->super_copy) + 1) {
>> +		generic_err(leaf, slot,
>> +		"invalid root last_snapshot, have %llu expect (0, %llu]",
>> +			    btrfs_root_last_snapshot(&ri),
>> +			    btrfs_super_generation(fs_info->super_copy) + 1);
>> +		return -EUCLEAN;
>> +	}
>> +
>> +	/* Alignment and level check */
>> +	if (!IS_ALIGNED(btrfs_root_bytenr(&ri), fs_info->sectorsize)) {
>> +		generic_err(leaf, slot,
>> +			"invalid root bytenr, have %llu expect to be aligned to %u",
>> +			    btrfs_root_bytenr(&ri), fs_info->sectorsize);
>> +		return -EUCLEAN;
>> +	}
>> +	if (btrfs_root_level(&ri) >= BTRFS_MAX_LEVEL) {
>> +		generic_err(leaf, slot,
>> +			"invalid root level, have %u expect [0, %u]",
>> +			    btrfs_root_level(&ri), BTRFS_MAX_LEVEL - 1);
>> +		return -EUCLEAN;
>> +	}
>> +	if (ri.drop_level >= BTRFS_MAX_LEVEL) {
>> +		generic_err(leaf, slot,
>> +			"invalid root level, have %u expect [0, %u]",
>> +			    ri.drop_level, BTRFS_MAX_LEVEL - 1);
>> +		return -EUCLEAN;
>> +	}
>> +
>> +	/* Flags check */
>> +	if (btrfs_root_flags(&ri) & ~valid_root_flags) {
>> +		generic_err(leaf, slot,
>> +			"invalid root flags, have 0x%llx expect mask 0x%llu",
> 
>                                                                      0x%llx
> 
>> +			    btrfs_root_flags(&ri), valid_root_flags);
>> +		return -EUCLEAN;
>> +	}
>> +	return 0;
>> +}
>> +
>>  /*
>>   * Common point to switch the item-specific validation.
>>   */
>> @@ -845,6 +934,9 @@ static int check_leaf_item(struct extent_buffer *leaf,
>>  	case BTRFS_INODE_ITEM_KEY:
>>  		ret = check_inode_item(leaf, key, slot);
>>  		break;
>> +	case BTRFS_ROOT_ITEM_KEY:
>> +		ret = check_root_item(leaf, key, slot);
>> +		break;
>>  	}
>>  	return ret;
>>  }
>> -- 
>> 2.22.0


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

Re: [PATCH 3/3] btrfs: tree-checker: Add ROOT_ITEM check

[David Sterba]

On Sat, Jul 27, 2019 at 07:26:28AM +0800, Qu Wenruo wrote:
> [snip]
> > 
> >> +	}
> >> +
> >> +	read_extent_buffer(leaf, &ri, btrfs_item_ptr_offset(leaf, slot),
> >> +			   sizeof(ri));
> >> +
> >> +	/* Generateion related */
> > 
> > typo here and a few more times below
> > 
> >> +	if (btrfs_root_generation(&ri) >
> >> +	    btrfs_super_generation(fs_info->super_copy) + 1) {
> >> +		generic_err(leaf, slot,
> >> +			"invalid root generaetion, have %llu expect (0, %llu]",
> >> +			    btrfs_root_generation(&ri),
> >> +			    btrfs_super_generation(fs_info->super_copy) + 1);
> >> +		return -EUCLEAN;
> >> +	}
> >> +	if (btrfs_root_generation_v2(&ri) >
> >> +	    btrfs_super_generation(fs_info->super_copy) + 1) {
> >> +		generic_err(leaf, slot,
> >> +		"invalid root v2 generaetion, have %llu expect (0, %llu]",
> > 
> > So (0, %llu] here means that it must be greater than zero, right? I'm
> > not sure that everyone uses the same notation for open/closed notation.
> 
> AFAIK in tree checker it's all the same notation.
> 
> Or any better solution for that?

No this one is fine, let's use it and eventualy update comments or
messages where this notation is not used.

Re: [PATCH 1/3] btrfs: delayed-inode: Kill the BUG_ON() in btrfs_delete_delayed_dir_index()

[David Sterba]

On Tue, Jul 16, 2019 at 05:00:32PM +0800, Qu Wenruo wrote:
> There is one report of fuzzed image which leads to BUG_ON() in
> btrfs_delete_delayed_dir_index().
> 
> Although that fuzzed image can already be addressed by enhanced
> extent-tree error handler, it's still better to hunt down more BUG_ON().
> 
> This patch will hunt down two BUG_ON()s in
> btrfs_delete_delayed_dir_index():
> - One for error from btrfs_delayed_item_reserve_metadata()
>   Instead of BUG_ON(), we output an error message and free the item.
>   And return the error.
>   All callers of this function handles the error by aborting current
>   trasaction.
> 
> - One for possible EEXIST from __btrfs_add_delayed_deletion_item()
>   That function can return -EEXIST.
>   We already have a good enough error message for that, only need to
>   clean up the reserved metadata space and allocated item.
> 
> To help above cleanup, also modifiy __btrfs_remove_delayed_item() called
> in btrfs_release_delayed_item(), to skip unassociated item.
> 
> Link: https://bugzilla.kernel.org/show_bug.cgi?id=203253
> Signed-off-by: Qu Wenruo <wqu@suse.com>
> ---
>  fs/btrfs/delayed-inode.c | 14 ++++++++++++--
>  1 file changed, 12 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/btrfs/delayed-inode.c b/fs/btrfs/delayed-inode.c
> index 43fdb2992956..c4946d58f49b 100644
> --- a/fs/btrfs/delayed-inode.c
> +++ b/fs/btrfs/delayed-inode.c
> @@ -474,6 +474,9 @@ static void __btrfs_remove_delayed_item(struct btrfs_delayed_item *delayed_item)
>  	struct rb_root_cached *root;
>  	struct btrfs_delayed_root *delayed_root;
>  
> +	/* Not associated with any delayed_node */
> +	if (!delayed_item->delayed_node)
> +		return;
>  	delayed_root = delayed_item->delayed_node->root->fs_info->delayed_root;
>  
>  	BUG_ON(!delayed_root);
> @@ -1525,7 +1528,13 @@ int btrfs_delete_delayed_dir_index(struct btrfs_trans_handle *trans,
>  	 * we have reserved enough space when we start a new transaction,
>  	 * so reserving metadata failure is impossible.
>  	 */
> -	BUG_ON(ret);
> +	if (ret < 0) {
> +		btrfs_err(trans->fs_info,
> +"metadata reserve fail at %s, should have already reserved space, ret=%d",

I'd rather avoid using the function name in non-debugging messages,
let's use eg "metadata reservation failed for delayed dir item deltion",
and printing ret here is not necessary as it will be printed in the
transaction abort message.

Re: [PATCH 0/3] btrfs: Part 2 of enhanced defence against fuzzed images

[David Sterba]

On Tue, Jul 16, 2019 at 05:00:31PM +0800, Qu Wenruo wrote:
> This wave has the following features:
> - Hunt down BUG_ON() in btrfs_delete_delayed_dir_index()
>   EEXIST can cause BUG_ON(). And all callers of this function has
>   already handled error by aborting transacation.
> 
> - Only allocate extents from the same block group type
>   This is a very tricky bug, needs MIXED_GROUP super flag with regular
>   block groups (separate META and DATA) and corrupted extent tree.
> 
> - ROOT_ITEM check for tree checker
>   This kills the unaligned bytenr, invalid level and incorrect reloc
>   tree.
> 
> Reported-by: Jungyeon Yoon <jungyeon.yoon@gmail.com>
> 
> Qu Wenruo (3):
>   btrfs: delayed-inode: Kill the BUG_ON() in
>     btrfs_delete_delayed_dir_index()
>   btrfs: extent-tree: Make sure we only allocate extents from block
>     groups with the same type
>   btrfs: tree-checker: Add ROOT_ITEM check

Added to misc-next, with some minor updates, thanks.