[v2,1/2] btrfs: Check each block group has corresponding chunk at mount time
diff mbox

Message ID 20180703080830.8300-1-wqu@suse.com
State New
Headers show

Commit Message

Qu Wenruo July 3, 2018, 8:08 a.m. UTC
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 <wen.xu@gatech.edu>
Signed-off-by: Qu Wenruo <wqu@suse.com>
---
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(-)

Comments

Nikolay Borisov July 3, 2018, 8:33 a.m. UTC | #1
On  3.07.2018 11:08, Qu Wenruo wrote:
> 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 <wen.xu@gatech.edu>
> Signed-off-by: Qu Wenruo <wqu@suse.com>
> ---
> 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.
--
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
Nikolay Borisov July 3, 2018, 8:40 a.m. UTC | #2
On  3.07.2018 11:33, Nikolay Borisov wrote:
> 
> 
> On  3.07.2018 11:08, Qu Wenruo wrote:
>> 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 <wen.xu@gatech.edu>
>> Signed-off-by: Qu Wenruo <wqu@suse.com>
>> ---
>> 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.

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.

> --
> 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
> 
--
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
Qu Wenruo July 3, 2018, 8:47 a.m. UTC | #3
On 2018年07月03日 16:33, Nikolay Borisov wrote:
> 
> 
> On  3.07.2018 11:08, Qu Wenruo wrote:
>> 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 <wen.xu@gatech.edu>
>> Signed-off-by: Qu Wenruo <wqu@suse.com>
>> ---
>> 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.

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

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

> 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

> --
> 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
> 
--
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
Nikolay Borisov July 3, 2018, 9:08 a.m. UTC | #4
On  3.07.2018 11:47, Qu Wenruo wrote:
> 
> 
> On 2018年07月03日 16:33, Nikolay Borisov wrote:
>>
>>
>> On  3.07.2018 11:08, Qu Wenruo wrote:
>>> 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 <wen.xu@gatech.edu>
>>> Signed-off-by: Qu Wenruo <wqu@suse.com>
>>> ---
>>> 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.
> 
> Indeed, I'll send the branch and push it to github.
> 
>> 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 :)
> 
>> 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.

> 
> 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
>>
> --
> 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
> 
--
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
Martin Steigerwald July 3, 2018, 6:58 p.m. UTC | #5
Nikolay Borisov - 03.07.18, 11:08:
> On  3.07.2018 11:47, Qu Wenruo wrote:
> > On 2018年07月03日 16:33, Nikolay Borisov wrote:
> >> On  3.07.2018 11:08, Qu Wenruo wrote:
> >>> 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 <wen.xu@gatech.edu>
> >>> Signed-off-by: Qu Wenruo <wqu@suse.com>
> >>> ---
> >>> 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.
> > 
> > Indeed, I'll send the branch and push it to github.
> > 
> >> 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 :)> 
> >> 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.

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.

> 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.
[…]
David Sterba July 4, 2018, 3:37 p.m. UTC | #6
On Tue, Jul 03, 2018 at 08:58:05PM +0200, Martin Steigerwald wrote:
> Nikolay Borisov - 03.07.18, 11:08:
> > On  3.07.2018 11:47, Qu Wenruo wrote:
> > > On 2018年07月03日 16:33, Nikolay Borisov wrote:
> > >> On  3.07.2018 11:08, Qu Wenruo wrote:
> > >>> 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 <wen.xu@gatech.edu>
> > >>> Signed-off-by: Qu Wenruo <wqu@suse.com>
> > >>> ---
> > >>> 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.
> > > 
> > > Indeed, I'll send the branch and push it to github.
> > > 
> > >> 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 :)> 
> > >> 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.
> 
> 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.
--
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
David Sterba July 4, 2018, 3:45 p.m. UTC | #7
On Tue, Jul 03, 2018 at 12:08:02PM +0300, Nikolay Borisov wrote:
> >> 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.

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

Patch
diff mbox

diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
index 3d9fe58c0080..82b446f014b9 100644
--- a/fs/btrfs/extent-tree.c
+++ b/fs/btrfs/extent-tree.c
@@ -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 &&