Skip to contentSkip to conversation

btrfs-progs: ctree: Add extra level check for read_node_slot()

PATCH Superseded Qu Wenruo 3 participants 3 messages Feb 7, 2018 Btrfs file system

v1 of 2 Superseded Latest is v2 →

lore
Share & tools
lore

Qu Wenruo

author
#0 lore raw

PATCH 1/1 btrfs-progs: ctree: Add extra level check for read_node_slot()

Strangely, we have level check in btrfs_print_tree() while we don't have the same check in read_node_slot().

That's to say, for the following corruption, btrfs_search_slot() or btrfs_next_leaf() can return invalid leaf:

Parent eb:
node XXXXXX level 1
^^^^^^^
Child should be leaf (level 0)
...
key (XXX XXX XXX) block YYYYYY

Child eb:
leaf YYYYYY level 1
^^^^^^^
Something went wrong now

And for the corrupted leaf returned, later caller can be screwed up easily.

Although the root cause (powerloss, but still something wrong breaking metadata CoW of btrfs) is still unknown, at least enhance btrfs-progs to avoid SEGV.

Reported-by Ralph Gauges <[email protected]>
Signed-off-by Qu Wenruo <[email protected]>
---
 ctree.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)
ctree.c +12 -1
index 4fc33b14000a..ddb1e9cc6d37 100644
@@ -22,6 +22,7 @@
 #include "repair.h"
 #include "internal.h"
 #include "sizes.h"
+#include "messages.h"
 
 static int split_node(struct btrfs_trans_handle *trans, struct btrfs_root
 		      *root, struct btrfs_path *path, int level);

@@ -640,7 +641,9 @@ static int bin_search(struct extent_buffer *eb, struct btrfs_key *key,
 struct extent_buffer *read_node_slot(struct btrfs_fs_info *fs_info,
 				   struct extent_buffer *parent, int slot)
 {
+	struct extent_buffer *ret;
 	int level = btrfs_header_level(parent);
+
 	if (slot < 0)
 		return NULL;
 	if (slot >= btrfs_header_nritems(parent))

@@ -649,8 +652,16 @@ struct extent_buffer *read_node_slot(struct btrfs_fs_info *fs_info,
 	if (level == 0)
 		return NULL;
 
-	return read_tree_block(fs_info, btrfs_node_blockptr(parent, slot),
+	ret = read_tree_block(fs_info, btrfs_node_blockptr(parent, slot),
 		       btrfs_node_ptr_generation(parent, slot));
+	if (btrfs_header_level(ret) != level - 1) {
+		error("child eb corrupted: parent bytenr=%llu item=%d parent level=%d child level=%d",
+		      btrfs_header_bytenr(parent), slot,
+		      btrfs_header_level(parent), btrfs_header_level(ret));
+		free_extent_buffer(ret);
+		return ERR_PTR(-EIO);
+	}
+	return ret;
 }
 
 static int balance_level(struct btrfs_trans_handle *trans,
Signature · 1 line
2.16.1
#1 lore raw
Quoted 54 lines · Qu Wenruo

Strangely, we have level check in btrfs_print_tree() while we don't have the same check in read_node_slot().

That's to say, for the following corruption, btrfs_search_slot() or btrfs_next_leaf() can return invalid leaf:

Parent eb:
node XXXXXX level 1
^^^^^^^
Child should be leaf (level 0)
...
key (XXX XXX XXX) block YYYYYY

Child eb:
leaf YYYYYY level 1
^^^^^^^
Something went wrong now

And for the corrupted leaf returned, later caller can be screwed up easily.

Although the root cause (powerloss, but still something wrong breaking metadata CoW of btrfs) is still unknown, at least enhance btrfs-progs to avoid SEGV.

Reported-by Ralph Gauges <[email protected]>
Signed-off-by Qu Wenruo <[email protected]>
---
 ctree.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)
ctree.c +3 -0
index 4fc33b14000a..ddb1e9cc6d37 100644
@@ -22,6 +22,7 @@
 #include "repair.h"
 #include "internal.h"
 #include "sizes.h"
+#include "messages.h"
 
 static int split_node(struct btrfs_trans_handle *trans, struct btrfs_root
 		      *root, struct btrfs_path *path, int level);

@@ -640,7 +641,9 @@ static int bin_search(struct extent_buffer *eb, struct btrfs_key *key,
 struct extent_buffer *read_node_slot(struct btrfs_fs_info *fs_info,
 				   struct extent_buffer *parent, int slot)
 {
+	struct extent_buffer *ret;
 	int level = btrfs_header_level(parent);
+
 	if (slot < 0)
 		return NULL;
 	if (slot >= btrfs_header_nritems(parent))

ctree.c struct extent_buffer *read_node_slot(struct btrfs_fs_info *fs_info, · line 655

Quoted 5 lines
ctree.c +1 -1
@@ -649,8 +652,16 @@ struct extent_buffer *read_node_slot(struct btrfs_fs_info *fs_info,
 	if (level == 0)
 		return NULL;
 
-	return read_tree_block(fs_info, btrfs_node_blockptr(parent, slot),
+	ret = read_tree_block(fs_info, btrfs_node_blockptr(parent, slot),

The result of read_tree_block should be checked before use, by extent_buffer_uptodate, null check or IS_ERR at least (depending on the context of use).

Quoted 19 lines
ctree.c +8 -0
 		       btrfs_node_ptr_generation(parent, slot));
+	if (btrfs_header_level(ret) != level - 1) {
+		error("child eb corrupted: parent bytenr=%llu item=%d parent level=%d child level=%d",
+		      btrfs_header_bytenr(parent), slot,
+		      btrfs_header_level(parent), btrfs_header_level(ret));
+		free_extent_buffer(ret);
+		return ERR_PTR(-EIO);
+	}
+	return ret;
 }
 
 static int balance_level(struct btrfs_trans_handle *trans,
Signature · 6 lines
2.16.1

--
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
#2 lore raw
Quoted 58 lines · David Sterba
Quoted 57 lines · Qu Wenruo

Strangely, we have level check in btrfs_print_tree() while we don't have the same check in read_node_slot().

That's to say, for the following corruption, btrfs_search_slot() or btrfs_next_leaf() can return invalid leaf:

Parent eb:
node XXXXXX level 1
^^^^^^^
Child should be leaf (level 0)
...
key (XXX XXX XXX) block YYYYYY

Child eb:
leaf YYYYYY level 1
^^^^^^^
Something went wrong now

And for the corrupted leaf returned, later caller can be screwed up easily.

Although the root cause (powerloss, but still something wrong breaking metadata CoW of btrfs) is still unknown, at least enhance btrfs-progs to avoid SEGV.

Reported-by Ralph Gauges <[email protected]>
Signed-off-by Qu Wenruo <[email protected]>
---
 ctree.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)
ctree.c +3 -0
index 4fc33b14000a..ddb1e9cc6d37 100644
@@ -22,6 +22,7 @@
 #include "repair.h"
 #include "internal.h"
 #include "sizes.h"
+#include "messages.h"
 
 static int split_node(struct btrfs_trans_handle *trans, struct btrfs_root
 		      *root, struct btrfs_path *path, int level);

@@ -640,7 +641,9 @@ static int bin_search(struct extent_buffer *eb, struct btrfs_key *key,
 struct extent_buffer *read_node_slot(struct btrfs_fs_info *fs_info,
 				   struct extent_buffer *parent, int slot)
 {
+	struct extent_buffer *ret;
 	int level = btrfs_header_level(parent);
+
 	if (slot < 0)
 		return NULL;
 	if (slot >= btrfs_header_nritems(parent))

@@ -649,8 +652,16 @@ struct extent_buffer *read_node_slot(struct btrfs_fs_info *fs_info,
 	if (level == 0)
 		return NULL;
Quoted 6 lines

ctree.c struct extent_buffer *read_node_slot(struct btrfs_fs_info *fs_info, · line 655

Quoted 2 lines
ctree.c +1 -1
-	return read_tree_block(fs_info, btrfs_node_blockptr(parent, slot),
+	ret = read_tree_block(fs_info, btrfs_node_blockptr(parent, slot),

The result of read_tree_block should be checked before use, by extent_buffer_uptodate, null check or IS_ERR at least (depending on the context of use).

Right, just forgot that.

Will fix it in next version.

Thanks,
Qu

Quoted 25 lines
Quoted 20 lines
ctree.c +8 -0
 		       btrfs_node_ptr_generation(parent, slot));
+	if (btrfs_header_level(ret) != level - 1) {
+		error("child eb corrupted: parent bytenr=%llu item=%d parent level=%d child level=%d",
+		      btrfs_header_bytenr(parent), slot,
+		      btrfs_header_level(parent), btrfs_header_level(ret));
+		free_extent_buffer(ret);
+		return ERR_PTR(-EIO);
+	}
+	return ret;
 }
 
 static int balance_level(struct btrfs_trans_handle *trans,
Signature · 6 lines
2.16.1

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