diff mbox

[4/4] btrfs-progs: Add extra chunk item check to avoid btrfs-progs crash.

Message ID 1431508536-7275-5-git-send-email-quwenruo@cn.fujitsu.com (mailing list archive)
State Accepted
Headers show

Commit Message

Qu Wenruo May 13, 2015, 9:15 a.m. UTC 
Adds extra check when reading a chunk item:
1) Check chunk type.
Don't allow any unsupported type/profile bit.

2) Check num_stripes
Any chunk item should contain at least one stripe.
For system chunk, the chunk item size(calculated by btrfs_stripe size *
(num_stripes - 1) + btrfs_chunk size) should not exceed
BTRFS_SYSTEM_CHUNK_SIZE(2048).
For normal chunk, the chunk item size(calculated) should match the chunk
item size.

3) Check num_stripes/sub_stripes against chunk profile.
Num_stripes/sub_stripes must meet its lower limit for its chunk profile.

Reported-by: Lukas Lueg <lukas.lueg@gmail.com>
Signed-off-by: Qu Wenruo <quwenruo@cn.fujitsu.com>
---
 volumes.c | 60 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 57 insertions(+), 3 deletions(-)

Comments

David Sterba May 13, 2015, 4:18 p.m. UTC | #1 
On Wed, May 13, 2015 at 05:15:36PM +0800, Qu Wenruo wrote:
> Adds extra check when reading a chunk item:
> 1) Check chunk type.
> Don't allow any unsupported type/profile bit.
> 
> 2) Check num_stripes
> Any chunk item should contain at least one stripe.
> For system chunk, the chunk item size(calculated by btrfs_stripe size *
> (num_stripes - 1) + btrfs_chunk size) should not exceed
> BTRFS_SYSTEM_CHUNK_SIZE(2048).
> For normal chunk, the chunk item size(calculated) should match the chunk
> item size.
> 
> 3) Check num_stripes/sub_stripes against chunk profile.
> Num_stripes/sub_stripes must meet its lower limit for its chunk profile.
> 
> Reported-by: Lukas Lueg <lukas.lueg@gmail.com>
> Signed-off-by: Qu Wenruo <quwenruo@cn.fujitsu.com>

Applied, thanks.
--
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
WorMzy Tykashi May 19, 2015, 4:33 p.m. UTC | #2 
(resending to list because of gmail's daft HTML headers getting the
original message rejected)

Hi guys,

Following a bisect, it appears that this patch breaks fsck test 006:

$ git checkout f146c40c65e0142b52418a0a1cbaf2
808e658d76
HEAD is now at f146c40... btrfs-progs: Add extra chunk item check to
avoid btrfs-progs crash.
...autogen, configure, make..
$ make test-fsck
    [TEST]   fsck-tests.sh
    [TEST]   001-bad-file-extent-bytenr
    [TEST]   002-bad-transid
parent transid verify failed on 29360128 wanted 9 found 755944791
parent transid verify failed on 29360128 wanted 9 found 755944791
Ignoring transid failure
    [TEST]   003-shift-offsets
    [TEST]   004-no-dir-index
    [TEST]   005-bad-item-offset
    [TEST]   006-bad-root-items
failed: /home/wormzy/btrfs-progs-unstable/btrfs check --repair test.img
test failed for case 006-bad-root-items
Makefile:169: recipe for target 'test-fsck' failed
make: *** [test-fsck] Error 1

Does this test just need updating?

Cheers,


WorMzy

On 13 May 2015 at 17:18, David Sterba <dsterba@suse.cz> wrote:
> On Wed, May 13, 2015 at 05:15:36PM +0800, Qu Wenruo wrote:
>> Adds extra check when reading a chunk item:
>> 1) Check chunk type.
>> Don't allow any unsupported type/profile bit.
>>
>> 2) Check num_stripes
>> Any chunk item should contain at least one stripe.
>> For system chunk, the chunk item size(calculated by btrfs_stripe size *
>> (num_stripes - 1) + btrfs_chunk size) should not exceed
>> BTRFS_SYSTEM_CHUNK_SIZE(2048).
>> For normal chunk, the chunk item size(calculated) should match the chunk
>> item size.
>>
>> 3) Check num_stripes/sub_stripes against chunk profile.
>> Num_stripes/sub_stripes must meet its lower limit for its chunk profile.
>>
>> Reported-by: Lukas Lueg <lukas.lueg@gmail.com>
>> Signed-off-by: Qu Wenruo <quwenruo@cn.fujitsu.com>
>
> Applied, thanks.
> --
> 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 May 20, 2015, 12:25 a.m. UTC | #3 
Yes, I also find the problem and already sent the fixing patch:
https://patchwork.kernel.org/patch/6411191/

The problem is that my previous check patch is too restrict, making DUP 
chunk with only 1 stripe invalid.

Fixing patch will allow degraded chunk to exist and fix the bug.

Thanks,
Qu

-------- Original Message  --------
Subject: Re: [PATCH 4/4] btrfs-progs: Add extra chunk item check to 
avoid btrfs-progs crash.
From: WorMzy Tykashi <wormzy.tykashi@gmail.com>
To: David Sterba <dsterba@suse.cz>, Qu Wenruo <quwenruo@cn.fujitsu.com>, 
linux-btrfs@vger.kernel.org <linux-btrfs@vger.kernel.org>, 
<lukas.lueg@gmail.com>
Date: 2015?05?20? 00:30

> Hi guys,
>
> Following a bisect, it appears that this patch breaks fsck test 006:
>
> $ git checkout f146c40c65e0142b52418a0a1cbaf2808e658d76
> HEAD is now at f146c40... btrfs-progs: Add extra chunk item check to
> avoid btrfs-progs crash.
> ...autogen, configure, make..
> $ make test-fsck
>      [TEST]   fsck-tests.sh
>      [TEST]   001-bad-file-extent-bytenr
>      [TEST]   002-bad-transid
> parent transid verify failed on 29360128 wanted 9 found 755944791
> parent transid verify failed on 29360128 wanted 9 found 755944791
> Ignoring transid failure
>      [TEST]   003-shift-offsets
>      [TEST]   004-no-dir-index
>      [TEST]   005-bad-item-offset
>      [TEST]   006-bad-root-items
> failed: /home/wormzy/btrfs-progs-unstable/btrfs check --repair test.img
> test failed for case 006-bad-root-items
> Makefile:169: recipe for target 'test-fsck' failed
> make: *** [test-fsck] Error 1
>
> Does this test just need updating?
>
> Cheers,
>
>
> WorMzy
>
> On 13 May 2015 at 17:18, David Sterba <dsterba@suse.cz
> <mailto:dsterba@suse.cz>> wrote:
>
>     On Wed, May 13, 2015 at 05:15:36PM +0800, Qu Wenruo wrote:
>     > Adds extra check when reading a chunk item:
>     > 1) Check chunk type.
>     > Don't allow any unsupported type/profile bit.
>     >
>     > 2) Check num_stripes
>     > Any chunk item should contain at least one stripe.
>     > For system chunk, the chunk item size(calculated by btrfs_stripe size *
>     > (num_stripes - 1) + btrfs_chunk size) should not exceed
>     > BTRFS_SYSTEM_CHUNK_SIZE(2048).
>     > For normal chunk, the chunk item size(calculated) should match the chunk
>     > item size.
>     >
>     > 3) Check num_stripes/sub_stripes against chunk profile.
>     > Num_stripes/sub_stripes must meet its lower limit for its chunk profile.
>     >
>     > Reported-by: Lukas Lueg <lukas.lueg@gmail.com <mailto:lukas.lueg@gmail.com>>
>     > Signed-off-by: Qu Wenruo <quwenruo@cn.fujitsu.com <mailto:quwenruo@cn.fujitsu.com>>
>
>     Applied, thanks.
>     --
>     To unsubscribe from this list: send the line "unsubscribe
>     linux-btrfs" in
>     the body of a message to majordomo@vger.kernel.org
>     <mailto: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
WorMzy Tykashi May 20, 2015, 8:03 a.m. UTC | #4 
On 20 May 2015 01:25:03 BST, Qu Wenruo <quwenruo@cn.fujitsu.com> wrote:
>Yes, I also find the problem and already sent the fixing patch:
>https://patchwork.kernel.org/patch/6411191/
>
>The problem is that my previous check patch is too restrict, making DUP
>
>chunk with only 1 stripe invalid.
>
>Fixing patch will allow degraded chunk to exist and fix the bug.
>
>Thanks,
>Qu
>
>-------- Original Message  --------
>Subject: Re: [PATCH 4/4] btrfs-progs: Add extra chunk item check to 
>avoid btrfs-progs crash.
>From: WorMzy Tykashi <wormzy.tykashi@gmail.com>
>To: David Sterba <dsterba@suse.cz>, Qu Wenruo
><quwenruo@cn.fujitsu.com>, 
>linux-btrfs@vger.kernel.org <linux-btrfs@vger.kernel.org>, 
><lukas.lueg@gmail.com>
>Date: 2015?05?20? 00:30
>
>> Hi guys,
>>
>> Following a bisect, it appears that this patch breaks fsck test 006:
>>
>> $ git checkout f146c40c65e0142b52418a0a1cbaf2808e658d76
>> HEAD is now at f146c40... btrfs-progs: Add extra chunk item check to
>> avoid btrfs-progs crash.
>> ...autogen, configure, make..
>> $ make test-fsck
>>      [TEST]   fsck-tests.sh
>>      [TEST]   001-bad-file-extent-bytenr
>>      [TEST]   002-bad-transid
>> parent transid verify failed on 29360128 wanted 9 found 755944791
>> parent transid verify failed on 29360128 wanted 9 found 755944791
>> Ignoring transid failure
>>      [TEST]   003-shift-offsets
>>      [TEST]   004-no-dir-index
>>      [TEST]   005-bad-item-offset
>>      [TEST]   006-bad-root-items
>> failed: /home/wormzy/btrfs-progs-unstable/btrfs check --repair
>test.img
>> test failed for case 006-bad-root-items
>> Makefile:169: recipe for target 'test-fsck' failed
>> make: *** [test-fsck] Error 1
>>
>> Does this test just need updating?
>>
>> Cheers,
>>
>>
>> WorMzy
>>
>> On 13 May 2015 at 17:18, David Sterba <dsterba@suse.cz
>> <mailto:dsterba@suse.cz>> wrote:
>>
>>     On Wed, May 13, 2015 at 05:15:36PM +0800, Qu Wenruo wrote:
>>     > Adds extra check when reading a chunk item:
>>     > 1) Check chunk type.
>>     > Don't allow any unsupported type/profile bit.
>>     >
>>     > 2) Check num_stripes
>>     > Any chunk item should contain at least one stripe.
>>     > For system chunk, the chunk item size(calculated by
>btrfs_stripe size *
>>     > (num_stripes - 1) + btrfs_chunk size) should not exceed
>>     > BTRFS_SYSTEM_CHUNK_SIZE(2048).
>>     > For normal chunk, the chunk item size(calculated) should match
>the chunk
>>     > item size.
>>     >
>>     > 3) Check num_stripes/sub_stripes against chunk profile.
>>     > Num_stripes/sub_stripes must meet its lower limit for its chunk
>profile.
>>     >
>>     > Reported-by: Lukas Lueg <lukas.lueg@gmail.com
><mailto:lukas.lueg@gmail.com>>
>>     > Signed-off-by: Qu Wenruo <quwenruo@cn.fujitsu.com
><mailto:quwenruo@cn.fujitsu.com>>
>>
>>     Applied, thanks.
>>     --
>>     To unsubscribe from this list: send the line "unsubscribe
>>     linux-btrfs" in
>>     the body of a message to majordomo@vger.kernel.org
>>     <mailto:majordomo@vger.kernel.org>
>>     More majordomo info at http://vger.kernel.org/majordomo-info.html
>>
>>

Thanks Qu, I missed that patch. 

Cheers, 


WorMzy 
--
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 May 20, 2015, 12:20 p.m. UTC | #5 
On Wed, May 20, 2015 at 08:25:03AM +0800, Qu Wenruo wrote:
> Yes, I also find the problem and already sent the fixing patch:
> https://patchwork.kernel.org/patch/6411191/
> 
> The problem is that my previous check patch is too restrict, making DUP 
> chunk with only 1 stripe invalid.
> 
> Fixing patch will allow degraded chunk to exist and fix the bug.

Next time please mention that in the changelog or at least as a side
note that it fixes the broken test or that fixes some previous commit.
--
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 May 21, 2015, 12:21 a.m. UTC | #6 
-------- Original Message  --------
Subject: Re: [PATCH 4/4] btrfs-progs: Add extra chunk item check to 
avoid btrfs-progs crash.
From: David Sterba <dsterba@suse.cz>
To: Qu Wenruo <quwenruo@cn.fujitsu.com>
Date: 2015?05?20? 20:20

> On Wed, May 20, 2015 at 08:25:03AM +0800, Qu Wenruo wrote:
>> Yes, I also find the problem and already sent the fixing patch:
>> https://patchwork.kernel.org/patch/6411191/
>>
>> The problem is that my previous check patch is too restrict, making DUP
>> chunk with only 1 stripe invalid.
>>
>> Fixing patch will allow degraded chunk to exist and fix the bug.
>
> Next time please mention that in the changelog or at least as a side
> note that it fixes the broken test or that fixes some previous commit.
>
Sorry, my fault.

I'll pay more attention on such patch.

Thanks for pointing it out.
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
diff mbox

Patch

diff --git a/volumes.c b/volumes.c
index 16dbf64..77cc305 100644
--- a/volumes.c
+++ b/volumes.c
@@ -1575,9 +1575,14 @@  static struct btrfs_device *fill_missing_device(u64 devid)
 	return device;
 }
 
+/*
+ * Slot is used to verfy the chunk item is valid
+ *
+ * For sys chunk in superblock, pass -1 to indicate sys chunk.
+ */
 static int read_one_chunk(struct btrfs_root *root, struct btrfs_key *key,
 			  struct extent_buffer *leaf,
-			  struct btrfs_chunk *chunk)
+			  struct btrfs_chunk *chunk, int slot)
 {
 	struct btrfs_mapping_tree *map_tree = &root->fs_info->mapping_tree;
 	struct map_lookup *map;
@@ -1615,6 +1620,51 @@  static int read_one_chunk(struct btrfs_root *root, struct btrfs_key *key,
 	map->type = btrfs_chunk_type(leaf, chunk);
 	map->sub_stripes = btrfs_chunk_sub_stripes(leaf, chunk);
 
+	/* Check on chunk item type */
+	if (map->type & ~(BTRFS_BLOCK_GROUP_TYPE_MASK |
+			  BTRFS_BLOCK_GROUP_PROFILE_MASK)) {
+		fprintf(stderr, "Unknown chunk type bits: %llu\n",
+			map->type & ~(BTRFS_BLOCK_GROUP_TYPE_MASK |
+				      BTRFS_BLOCK_GROUP_PROFILE_MASK));
+		ret = -EIO;
+		goto out;
+	}
+
+	/*
+	 * Btrfs_chunk contains at least one stripe, and for sys_chunk
+	 * it can't exceed the system chunk array size
+	 * For normal chunk, it should match its chunk item size.
+	 */
+	if (num_stripes < 1 ||
+	    (slot == -1 && sizeof(struct btrfs_stripe) * num_stripes >
+	     BTRFS_SYSTEM_CHUNK_ARRAY_SIZE) ||
+	    (slot >= 0 && sizeof(struct btrfs_stripe) * (num_stripes - 1) >
+	     btrfs_item_size_nr(leaf, slot))) {
+		fprintf(stderr, "Invalid num_stripes: %u\n",
+		        num_stripes);
+		ret = -EIO;
+		goto out;
+	}
+
+	/*
+	 * Device number check against profile
+	 */
+	if ((map->type & BTRFS_BLOCK_GROUP_RAID10 && num_stripes < 4 &&
+	     map->sub_stripes < 2) ||
+	    (map->type & BTRFS_BLOCK_GROUP_RAID1 && num_stripes < 2) ||
+	    (map->type & BTRFS_BLOCK_GROUP_RAID5 && num_stripes < 3) ||
+	    (map->type & BTRFS_BLOCK_GROUP_RAID6 && num_stripes < 4) ||
+	    (map->type & BTRFS_BLOCK_GROUP_DUP && num_stripes != 1) ||
+	    ((map->type & BTRFS_BLOCK_GROUP_PROFILE_MASK) == 0 &&
+	     num_stripes != 1)) {
+		fprintf(stderr,
+			"Invalid num_stripes:sub_stripes %u:%u for profile %llu\n",
+		        num_stripes, map->sub_stripes,
+			map->type & BTRFS_BLOCK_GROUP_PROFILE_MASK);
+		ret = -EIO;
+		goto out;
+	}
+
 	for (i = 0; i < num_stripes; i++) {
 		map->stripes[i].physical =
 			btrfs_stripe_offset_nr(leaf, chunk, i);
@@ -1635,6 +1685,9 @@  static int read_one_chunk(struct btrfs_root *root, struct btrfs_key *key,
 	BUG_ON(ret);
 
 	return 0;
+out:
+	free(map);
+	return ret;
 }
 
 static int fill_device_from_item(struct extent_buffer *leaf,
@@ -1773,7 +1826,7 @@  int btrfs_read_sys_array(struct btrfs_root *root)
 
 		if (key.type == BTRFS_CHUNK_ITEM_KEY) {
 			chunk = (struct btrfs_chunk *)(ptr - (u8 *)super_copy);
-			ret = read_one_chunk(root, &key, sb, chunk);
+			ret = read_one_chunk(root, &key, sb, chunk, -1);
 			if (ret)
 				break;
 			num_stripes = btrfs_chunk_num_stripes(sb, chunk);
@@ -1835,7 +1888,8 @@  int btrfs_read_chunk_tree(struct btrfs_root *root)
 		} else if (found_key.type == BTRFS_CHUNK_ITEM_KEY) {
 			struct btrfs_chunk *chunk;
 			chunk = btrfs_item_ptr(leaf, slot, struct btrfs_chunk);
-			ret = read_one_chunk(root, &found_key, leaf, chunk);
+			ret = read_one_chunk(root, &found_key, leaf, chunk,
+					     slot);
 			BUG_ON(ret);
 		}
 		path->slots[0]++;