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

PATCH Applied Qu Wenruo 4 participants 5 messages Feb 8, 2018 – Mar 20, 2018 Btrfs file system

lore

Share & tools

lore

Qu Wenruo

author

2018-02-08 00:59 UTC #0 lore raw

PATCH v2 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]>

---
changlog:
v2:
Check if the extent buffer is up-to-date before checking its level to avoid possible NULL pointer access.

---
 ctree.c | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

ctree.c +15 -1

index 4fc33b14000a..430805e3043f 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,19 @@ 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 (!extent_buffer_uptodate(ret))
+		return ERR_PTR(-EIO);
+
+	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

Nikolay Borisov

Reviewed-by on v2 1/1

2018-02-08 07:38 UTC #1 lore raw

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

Quoted 6 lines

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

Reviewed-by Nikolay Borisov <[email protected]>

Quoted 53 lines

---
changlog:
v2:
Check if the extent buffer is up-to-date before checking its level to avoid possible NULL pointer access.

---
 ctree.c | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

ctree.c +15 -1

index 4fc33b14000a..430805e3043f 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,19 @@ 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 (!extent_buffer_uptodate(ret))
+		return ERR_PTR(-EIO);
+
+	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,

David Sterba

Applied

2018-03-19 12:39 UTC #2 lore raw

In reply to Qu Wenruo

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

Quoted 6 lines

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

Applied, thanks.

Quoted 33 lines

---
changlog:
v2:
Check if the extent buffer is up-to-date before checking its level to avoid possible NULL pointer access.

---
 ctree.c | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

ctree.c +3 -0

index 4fc33b14000a..430805e3043f 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,19 @@ struct extent_buffer *read_node_slot(struct btrfs_fs_info *fs_info,
 	if (level == 0)

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

Quoted 10 lines

ctree.c +6 -1

 		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 (!extent_buffer_uptodate(ret))
+		return ERR_PTR(-EIO);
+
+	if (btrfs_header_level(ret) != level - 1) {
+		error("child eb corrupted: parent bytenr=%llu item=%d parent level=%d child level=%d",

Please unindent the strings that are do not fit 80 chars on the line. I've fixed that now, but I do that too often despite this has been known to be the preferred style.

Quoted 16 lines

ctree.c +6 -0

+		      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

Qu Wenruo

2018-03-20 05:16 UTC #3 lore raw

Quoted 66 lines · David Sterba

Quoted 21 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.
Quoted 6 lines
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]>
Applied, thanks.
Quoted 35 lines
---
changlog:
v2:
Check if the extent buffer is up-to-date before checking its level to avoid possible NULL pointer access.
---
 ctree.c | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)
ctree.c +3 -0
index 4fc33b14000a..430805e3043f 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,19 @@ struct extent_buffer *read_node_slot(struct btrfs_fs_info *fs_info,
 	if (level == 0)
 		return NULL;
 

Quoted 12 lines

ctree.c struct extent_buffer *read_node_slot(struct btrfs_fs_info *fs_info, · line 661
Quoted 8 lines
ctree.c +6 -1
-	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 (!extent_buffer_uptodate(ret))
+		return ERR_PTR(-EIO);
+
+	if (btrfs_header_level(ret) != level - 1) {
+		error("child eb corrupted: parent bytenr=%llu item=%d parent level=%d child level=%d",
Please unindent the strings that are do not fit 80 chars on the line. I've fixed that now, but I do that too often despite this has been known to be the preferred style.

Sorry for the extra trouble.

But I'm still a little uncertain about the correct way to handle such long string.
It would be pretty nice to have some recommendation for the following cases:

1) Super long, already over 80 chars even without indent No indent at all or just normal indent?

2) Short enough to have some indent, but can't be aligned to the bracket Should we use as much indent as possible or leave no indent at all?

Thanks,
Qu

Quoted 22 lines

Quoted 17 lines
ctree.c +6 -0
+		      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

Qu Wenruo

2018-03-20 05:24 UTC #4 lore raw

Quoted 82 lines · Qu Wenruo

Quoted 66 lines · David Sterba
Quoted 21 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.
Quoted 6 lines
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]>
Applied, thanks.
Quoted 35 lines
---
changlog:
v2:
Check if the extent buffer is up-to-date before checking its level to avoid possible NULL pointer access.
---
 ctree.c | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)
ctree.c +3 -0
index 4fc33b14000a..430805e3043f 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,19 @@ struct extent_buffer *read_node_slot(struct btrfs_fs_info *fs_info,
 	if (level == 0)
 		return NULL;
 
Quoted 12 lines
ctree.c struct extent_buffer *read_node_slot(struct btrfs_fs_info *fs_info, · line 661
Quoted 8 lines
ctree.c +6 -1
-	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 (!extent_buffer_uptodate(ret))
+		return ERR_PTR(-EIO);
+
+	if (btrfs_header_level(ret) != level - 1) {
+		error("child eb corrupted: parent bytenr=%llu item=%d parent level=%d child level=%d",
Please unindent the strings that are do not fit 80 chars on the line. I've fixed that now, but I do that too often despite this has been known to be the preferred style.

Quoted 8 lines

Sorry for the extra trouble.
But I'm still a little uncertain about the correct way to handle such long string.
It would be pretty nice to have some recommendation for the following cases:
1) Super long, already over 80 chars even without indent No indent at all or just normal indent?

And in that case, should we move the string to a new line? As neither way, it's already over 80 chars.

Thanks,
Qu

Quoted 30 lines

2) Short enough to have some indent, but can't be aligned to the bracket Should we use as much indent as possible or leave no indent at all?
Thanks,
Qu
Quoted 22 lines
Quoted 17 lines
ctree.c +6 -0
+		      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