btrfs: Check each block group has corresponding chunk at mount time

PATCH Superseded Qu Wenruo 5 participants 9 messages Jul 3, 2018 – Jul 4, 2018 Btrfs file system

Share & tools

Qu Wenruo

author

2018-07-03 08:08 UTC #0 lore raw

PATCH v2 1/2 btrfs: Check each block group has corresponding chunk at mount time

Reported in https://bugzilla.kernel.org/show_bug.cgi?id=199837, if a crafted btrfs with incorrect chunk<->block group mapping, it could leads to a lot of unexpected behavior.

Although the crafted image can be catched by block group item checker added in "[PATCH] btrfs: tree-checker: Verify block_group_item", if one crafted a valid enough block group item which can pass above check but still mismatch with existing chunk, it could cause a lot of undefined behavior.

This patch will add extra block group -> chunk mapping check, to ensure we have a completely matching (start, len, flags) chunk for each block group at mount time.

Reported-by Xu Wen <[email protected]>

Signed-off-by Qu Wenruo <[email protected]>

---
changelog:
v2:
Add better error message for each mismatch case. Rename function name, to co-operate with later patch. Add flags mismatch check.

---
 fs/btrfs/extent-tree.c | 55 ++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 53 insertions(+), 2 deletions(-)

fs/btrfs/extent-tree.c +53 -2

index 3d9fe58c0080..82b446f014b9 100644
@@ -10003,6 +10003,41 @@ btrfs_create_block_group_cache(struct btrfs_fs_info *fs_info,
 	return cache;
 }
 
+static int check_exist_chunk(struct btrfs_fs_info *fs_info, u64 start, u64 len,
+			     u64 flags)
+{
+	struct btrfs_mapping_tree *map_tree = &fs_info->mapping_tree;
+	struct extent_map *em;
+	int ret;
+
+	read_lock(&map_tree->map_tree.lock);
+	em = lookup_extent_mapping(&map_tree->map_tree, start, len);
+	read_unlock(&map_tree->map_tree.lock);
+
+	if (!em) {
+		btrfs_err_rl(fs_info,
+	"block group start=%llu len=%llu doesn't have corresponding chunk",
+			     start, len);
+		ret = -ENOENT;
+		goto out;
+	}
+	if (em->start != start || em->len != len ||
+	    (em->map_lookup->type & BTRFS_BLOCK_GROUP_TYPE_MASK) !=
+	    (flags & BTRFS_BLOCK_GROUP_TYPE_MASK)) {
+		btrfs_err_rl(fs_info,
+"block group start=%llu len=%llu flags=0x%llx doesn't match with chunk start=%llu len=%llu flags=0x%llx",
+			     start, len , flags & BTRFS_BLOCK_GROUP_TYPE_MASK,
+			     em->start, em->len, em->map_lookup->type &
+			     BTRFS_BLOCK_GROUP_TYPE_MASK);
+		ret = -EUCLEAN;
+		goto out;
+	}
+	ret = 0;
+out:
+	free_extent_map(em);
+	return ret;
+}
+
 int btrfs_read_block_groups(struct btrfs_fs_info *info)
 {
 	struct btrfs_path *path;

@@ -10036,6 +10071,9 @@ int btrfs_read_block_groups(struct btrfs_fs_info *info)
 		need_clear = 1;
 
 	while (1) {
+		struct btrfs_block_group_item bg;
+		int slot;
+
 		ret = find_first_block_group(info, path, &key);
 		if (ret > 0)
 			break;

@@ -10043,7 +10081,20 @@ int btrfs_read_block_groups(struct btrfs_fs_info *info)
 			goto error;
 
 		leaf = path->nodes[0];
-		btrfs_item_key_to_cpu(leaf, &found_key, path->slots[0]);
+		slot = path->slots[0];
+		btrfs_item_key_to_cpu(leaf, &found_key, slot);
+
+		read_extent_buffer(leaf, &bg, btrfs_item_ptr_offset(leaf, slot),
+				   sizeof(bg));
+		/*
+		 * Chunk and block group must have 1:1 mapping.
+		 * So there must be a chunk for this block group.
+		 */
+		ret = check_exist_chunk(info, found_key.objectid,
+					found_key.offset,
+					btrfs_block_group_flags(&bg));
+		if (ret < 0)
+			goto error;
 
 		cache = btrfs_create_block_group_cache(info, found_key.objectid,
 						       found_key.offset);

@@ -10068,7 +10119,7 @@ int btrfs_read_block_groups(struct btrfs_fs_info *info)
 		}
 
 		read_extent_buffer(leaf, &cache->item,
-				   btrfs_item_ptr_offset(leaf, path->slots[0]),
+				   btrfs_item_ptr_offset(leaf, slot),
 				   sizeof(cache->item));
 		cache->flags = btrfs_block_group_flags(&cache->item);
 		if (!mixed &&

Signature · 1 line

2.18.0

Qu Wenruo

author

2018-07-03 08:08 UTC #1 lore raw

PATCH v2 2/2 btrfs: Verify every chunk has corresponding block group at mount time

Reported in https://bugzilla.kernel.org/show_bug.cgi?id=199847, the image has empty extent tree.

Although that image also has empty root tree, thus can be catched by tree checker on empty leaves, but if a crafted btrfs has missing block group items, it could still cause problem.

This patch will do extra check to ensure each chunk has its corresponding block group.

Reported-by Xu Wen <[email protected]>

Signed-off-by Qu Wenruo <[email protected]>

---
changelog:
v2:
Newly added

---
 fs/btrfs/extent-tree.c | 52 +++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 51 insertions(+), 1 deletion(-)

fs/btrfs/extent-tree.c +51 -1

index 82b446f014b9..746095034ca2 100644
@@ -10038,6 +10038,56 @@ static int check_exist_chunk(struct btrfs_fs_info *fs_info, u64 start, u64 len,
 	return ret;
 }
 
+/*
+ * Iterate all chunks and verify each of them has corresponding block group
+ */
+static int check_chunk_block_group_mappings(struct btrfs_fs_info *fs_info)
+{
+	struct btrfs_mapping_tree *map_tree = &fs_info->mapping_tree;
+	struct extent_map *em;
+	struct btrfs_block_group_cache *bg;
+	u64 start = 0;
+	int ret = 0;
+
+	while (1) {
+		read_lock(&map_tree->map_tree.lock);
+		em = lookup_extent_mapping(&map_tree->map_tree, start,
+					   (u64)-1 - start);
+		read_unlock(&map_tree->map_tree.lock);
+		if (!em)
+			break;
+
+		bg = btrfs_lookup_block_group(fs_info, em->start);
+		if (!bg) {
+			btrfs_err_rl(fs_info,
+	"chunk start=%llu len=%llu doesn't have corresponding block group",
+				     em->start, em->len);
+			ret = -ENOENT;
+			free_extent_map(em);
+			break;
+		}
+		if (bg->key.objectid != em->start ||
+		    bg->key.offset != em->len ||
+		    (bg->flags & BTRFS_BLOCK_GROUP_TYPE_MASK) !=
+		    (em->map_lookup->type & BTRFS_BLOCK_GROUP_TYPE_MASK)) {
+			btrfs_err_rl(fs_info,
+"chunk start=%llu len=%llu flags=0x%llx doesn't match with block group start=%llu len=%llu flags=0x%llx",
+				em->start, em->len,
+				em->map_lookup->type & BTRFS_BLOCK_GROUP_TYPE_MASK,
+				bg->key.objectid, bg->key.offset,
+				bg->flags & BTRFS_BLOCK_GROUP_TYPE_MASK);
+			ret = -EUCLEAN;
+			free_extent_map(em);
+			btrfs_put_block_group(bg);
+			break;
+		}
+		start = em->start + em->len;
+		free_extent_map(em);
+		btrfs_put_block_group(bg);
+	}
+	return ret;
+}
+
 int btrfs_read_block_groups(struct btrfs_fs_info *info)
 {
 	struct btrfs_path *path;

@@ -10227,7 +10277,7 @@ int btrfs_read_block_groups(struct btrfs_fs_info *info)
 
 	btrfs_add_raid_kobjects(info);
 	init_global_block_rsv(info);
-	ret = 0;
+	ret = check_chunk_block_group_mappings(info);
 error:
 	btrfs_free_path(path);
 	return ret;

Signature · 1 line

2.18.0

Nikolay Borisov

2018-07-03 08:33 UTC #2 lore raw

In reply to Qu Wenruo

Quoted 16 lines · Qu Wenruo

Reported in https://bugzilla.kernel.org/show_bug.cgi?id=199837, if a crafted btrfs with incorrect chunk<->block group mapping, it could leads to a lot of unexpected behavior.
Although the crafted image can be catched by block group item checker added in "[PATCH] btrfs: tree-checker: Verify block_group_item", if one crafted a valid enough block group item which can pass above check but still mismatch with existing chunk, it could cause a lot of undefined behavior.
This patch will add extra block group -> chunk mapping check, to ensure we have a completely matching (start, len, flags) chunk for each block group at mount time.
Reported-by Xu Wen <[email protected]>
Signed-off-by Qu Wenruo <[email protected]>

Quoted 7 lines

---
changelog:
v2:
Add better error message for each mismatch case. Rename function name, to co-operate with later patch. Add flags mismatch check.
---

It's getting really hard to keep track of the various validation patches you sent with multiple versions + new checks. Please batch everything in a topic series i.e "Making checks stricter" or some such and send everything again nicely packed, otherwise the risk of mis-merging is increased. I now see that Gu Jinxiang from fujitsu also started sending validation fixes.

Nikolay Borisov

2018-07-03 08:40 UTC #3 lore raw

Quoted 27 lines · Nikolay Borisov

Quoted 16 lines · Qu Wenruo
Reported in https://bugzilla.kernel.org/show_bug.cgi?id=199837, if a crafted btrfs with incorrect chunk<->block group mapping, it could leads to a lot of unexpected behavior.
Although the crafted image can be catched by block group item checker added in "[PATCH] btrfs: tree-checker: Verify block_group_item", if one crafted a valid enough block group item which can pass above check but still mismatch with existing chunk, it could cause a lot of undefined behavior.
This patch will add extra block group -> chunk mapping check, to ensure we have a completely matching (start, len, flags) chunk for each block group at mount time.
Reported-by Xu Wen <[email protected]>
Signed-off-by Qu Wenruo <[email protected]>
Quoted 7 lines
---
changelog:
v2:
Add better error message for each mismatch case. Rename function name, to co-operate with later patch. Add flags mismatch check.
---

Quoted 6 lines

It's getting really hard to keep track of the various validation patches you sent with multiple versions + new checks. Please batch everything in a topic series i.e "Making checks stricter" or some such and send everything again nicely packed, otherwise the risk of mis-merging is increased. I now see that Gu Jinxiang from fujitsu also started sending validation fixes.

Also for evry patch which fixes a specific issue from one of the reported on bugzilla.kernel.org just use the Link: tag to point to the original report on bugzilla that will make it easier to relate the fixes to the original report.

Quoted 5 lines

--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Qu Wenruo

2018-07-03 08:47 UTC #4 lore raw

In reply to Nikolay Borisov

Quoted 27 lines · Nikolay Borisov

Quoted 16 lines · Qu Wenruo
Reported in https://bugzilla.kernel.org/show_bug.cgi?id=199837, if a crafted btrfs with incorrect chunk<->block group mapping, it could leads to a lot of unexpected behavior.
Although the crafted image can be catched by block group item checker added in "[PATCH] btrfs: tree-checker: Verify block_group_item", if one crafted a valid enough block group item which can pass above check but still mismatch with existing chunk, it could cause a lot of undefined behavior.
This patch will add extra block group -> chunk mapping check, to ensure we have a completely matching (start, len, flags) chunk for each block group at mount time.
Reported-by Xu Wen <[email protected]>
Signed-off-by Qu Wenruo <[email protected]>
Quoted 7 lines
---
changelog:
v2:
Add better error message for each mismatch case. Rename function name, to co-operate with later patch. Add flags mismatch check.
---

Quoted 5 lines

It's getting really hard to keep track of the various validation patches you sent with multiple versions + new checks. Please batch everything in a topic series i.e "Making checks stricter" or some such and send everything again nicely packed, otherwise the risk of mis-merging is increased.

Indeed, I'll send the branch and push it to github.

Quoted 2 lines

I now see that Gu Jinxiang from fujitsu also started sending validation fixes.

No need to worry, that will be the only patch related to that thread of bugzilla from Fujitsu.
As all the other cases can be addressed by my patches, sorry Fujitsu guys :)

Quoted 4 lines

Also for evry patch which fixes a specific issue from one of the reported on bugzilla.kernel.org just use the Link: tag to point to the original report on bugzilla that will make it easier to relate the fixes to the original report.

Never heard of "Link:" tag.
Maybe it's a good idea to added it to "submitting-patches.rst"?

Thanks,
Qu

Quoted 5 lines

--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Nikolay Borisov

2018-07-03 09:08 UTC #5 lore raw

Quoted 45 lines · Qu Wenruo

Quoted 27 lines · Nikolay Borisov
Quoted 16 lines · Qu Wenruo
Reported in https://bugzilla.kernel.org/show_bug.cgi?id=199837, if a crafted btrfs with incorrect chunk<->block group mapping, it could leads to a lot of unexpected behavior.
Although the crafted image can be catched by block group item checker added in "[PATCH] btrfs: tree-checker: Verify block_group_item", if one crafted a valid enough block group item which can pass above check but still mismatch with existing chunk, it could cause a lot of undefined behavior.
This patch will add extra block group -> chunk mapping check, to ensure we have a completely matching (start, len, flags) chunk for each block group at mount time.
Reported-by Xu Wen <[email protected]>
Signed-off-by Qu Wenruo <[email protected]>
Quoted 7 lines
---
changelog:
v2:
Add better error message for each mismatch case. Rename function name, to co-operate with later patch. Add flags mismatch check.
---
Quoted 5 lines
It's getting really hard to keep track of the various validation patches you sent with multiple versions + new checks. Please batch everything in a topic series i.e "Making checks stricter" or some such and send everything again nicely packed, otherwise the risk of mis-merging is increased.
Indeed, I'll send the branch and push it to github.
Quoted 2 lines
I now see that Gu Jinxiang from fujitsu also started sending validation fixes.
No need to worry, that will be the only patch related to that thread of bugzilla from Fujitsu.
As all the other cases can be addressed by my patches, sorry Fujitsu guys :)

Quoted 7 lines

Quoted 4 lines
Also for evry patch which fixes a specific issue from one of the reported on bugzilla.kernel.org just use the Link: tag to point to the original report on bugzilla that will make it easier to relate the fixes to the original report.
Never heard of "Link:" tag.
Maybe it's a good idea to added it to "submitting-patches.rst"?

I guess it's not officially documented but if you do git log --grep "Link:" you'd see quite a lot of patches actually have a Link pointing to the original thread if it has sparked some pertinent discussion. In this case those patches are a direct result of a bugzilla bugreport so having a Link: tag makes sense.

In the example of the qgroup patch I sent yesterday resulting from Misono's report there was also an involved discussion hence I added a link to the original thread.

Quoted 14 lines

Thanks,
Qu
Quoted 5 lines
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Martin Steigerwald

2018-07-03 18:58 UTC #6 lore raw

Nikolay Borisov - 03.07.18, 11:08:

Quoted 51 lines · Qu Wenruo

Quoted 50 lines · Nikolay Borisov
Quoted 27 lines · Qu Wenruo
Quoted 26 lines
Reported in https://bugzilla.kernel.org/show_bug.cgi?id=199837, if a
crafted btrfs with incorrect chunk<->block group mapping, it could leads to a lot of unexpected behavior.
Although the crafted image can be catched by block group item checker
added in "[PATCH] btrfs: tree-checker: Verify block_group_item", if one crafted a valid enough block group item which can pass above check but still mismatch with existing chunk, it could cause a lot of undefined behavior.
This patch will add extra block group -> chunk mapping check, to ensure we have a completely matching (start, len, flags) chunk for each block group at mount time.
Reported-by Xu Wen <[email protected]>
Signed-off-by Qu Wenruo <[email protected]>
---
changelog:
v2:
Add better error message for each mismatch case. Rename function name, to co-operate with later patch. Add flags mismatch check.
Quoted 7 lines
Quoted 1 line
---
It's getting really hard to keep track of the various validation patches you sent with multiple versions + new checks. Please batch everything in a topic series i.e "Making checks stricter" or some such and send everything again nicely packed, otherwise the risk of mis-merging is increased.
Indeed, I'll send the branch and push it to github.
Quoted 2 lines
I now see that Gu Jinxiang from fujitsu also started sending validation fixes.
No need to worry, that will be the only patch related to that thread of bugzilla from Fujitsu.
As all the other cases can be addressed by my patches, sorry Fujitsu guys :)>
Quoted 4 lines
Also for evry patch which fixes a specific issue from one of the reported on bugzilla.kernel.org just use the Link: tag to point to the original report on bugzilla that will make it easier to relate the fixes to the original report.

Quoted 8 lines

Quoted 2 lines
Never heard of "Link:" tag.
Maybe it's a good idea to added it to "submitting-patches.rst"?
I guess it's not officially documented but if you do git log --grep "Link:" you'd see quite a lot of patches actually have a Link pointing to the original thread if it has sparked some pertinent discussion. In this case those patches are a direct result of a bugzilla bugreport so having a Link: tag makes sense.

For Bugzilla reports I saw something like

Fixes https://bugzilla.kernel.org/show_bug.cgi?id=43511

in a patch I was Cc´d to.

Of course that does only apply if the patch in question fixes the reported bug.

Quoted 3 lines

In the example of the qgroup patch I sent yesterday resulting from Misono's report there was also an involved discussion hence I added a link to the original thread.

[…]

Signature · 1 line

Martin

David Sterba

2018-07-04 15:37 UTC #7 lore raw

Quoted 61 lines · Martin Steigerwald

Nikolay Borisov - 03.07.18, 11:08:
Quoted 51 lines · Qu Wenruo
Quoted 50 lines · Nikolay Borisov
Quoted 27 lines · Qu Wenruo
Quoted 26 lines
Reported in https://bugzilla.kernel.org/show_bug.cgi?id=199837, if a
crafted btrfs with incorrect chunk<->block group mapping, it could leads to a lot of unexpected behavior.
Although the crafted image can be catched by block group item checker
added in "[PATCH] btrfs: tree-checker: Verify block_group_item", if one crafted a valid enough block group item which can pass above check but still mismatch with existing chunk, it could cause a lot of undefined behavior.
This patch will add extra block group -> chunk mapping check, to ensure we have a completely matching (start, len, flags) chunk for each block group at mount time.
Reported-by Xu Wen <[email protected]>
Signed-off-by Qu Wenruo <[email protected]>
---
changelog:
v2:
Add better error message for each mismatch case. Rename function name, to co-operate with later patch. Add flags mismatch check.
Quoted 7 lines
Quoted 1 line
---
It's getting really hard to keep track of the various validation patches you sent with multiple versions + new checks. Please batch everything in a topic series i.e "Making checks stricter" or some such and send everything again nicely packed, otherwise the risk of mis-merging is increased.
Indeed, I'll send the branch and push it to github.
Quoted 2 lines
I now see that Gu Jinxiang from fujitsu also started sending validation fixes.
No need to worry, that will be the only patch related to that thread of bugzilla from Fujitsu.
As all the other cases can be addressed by my patches, sorry Fujitsu guys :)>
Quoted 4 lines
Also for evry patch which fixes a specific issue from one of the reported on bugzilla.kernel.org just use the Link: tag to point to the original report on bugzilla that will make it easier to relate the fixes to the original report.
Quoted 8 lines
Quoted 2 lines
Never heard of "Link:" tag.
Maybe it's a good idea to added it to "submitting-patches.rst"?
I guess it's not officially documented but if you do git log --grep "Link:" you'd see quite a lot of patches actually have a Link pointing to the original thread if it has sparked some pertinent discussion. In this case those patches are a direct result of a bugzilla bugreport so having a Link: tag makes sense.

Quoted 8 lines

For Bugzilla reports I saw something like
Fixes https://bugzilla.kernel.org/show_bug.cgi?id=43511
in a patch I was Cc´d to.
Of course that does only apply if the patch in question fixes the reported bug.

The tag 'Fixes:' already has some meaning and should point to the commit id and subject of a patch that it fixes. The stable team and its bots/filters recognize this flag and this helps maintainers to forward patches to the stable trees.

David Sterba

2018-07-04 15:45 UTC #8 lore raw

In reply to Nikolay Borisov

Quoted 5 lines · Nikolay Borisov

Quoted 5 lines
Quoted 4 lines
Also for evry patch which fixes a specific issue from one of the reported on bugzilla.kernel.org just use the Link: tag to point to the original report on bugzilla that will make it easier to relate the fixes to the original report.

Quoted 8 lines

Quoted 2 lines
Never heard of "Link:" tag.
Maybe it's a good idea to added it to "submitting-patches.rst"?
I guess it's not officially documented but if you do git log --grep "Link:" you'd see quite a lot of patches actually have a Link pointing to the original thread if it has sparked some pertinent discussion. In this case those patches are a direct result of a bugzilla bugreport so having a Link: tag makes sense.

The tag section of the commit has some predefined tags that could be somehow "legally binding" like the Signed-off, other can help to gather statistics about the devlopment process (reviewed, reported, tested) and the rest is basically free-form that should make sense to a human reader. So Link: or Bugzilla: would work. There were suggestions to formalize the links to email discussions but there was also opposition to formalize too much. Also, the important bits should be in the changelog as the mail archives are volatile as we've seen already.