* [PATCH] btrfs-progs: ctree: Add extra level check for read_node_slot()
@ 2018-02-07 9:18 Qu Wenruo
2018-02-07 14:35 ` David Sterba
0 siblings, 1 reply; 3+ messages in thread
From: Qu Wenruo @ 2018-02-07 9:18 UTC (permalink / raw)
To: linux-btrfs, dsterba; +Cc: ralphgauges
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 <ralphgauges@googlemail.com>
Signed-off-by: Qu Wenruo <wqu@suse.com>
---
ctree.c | 13 ++++++++++++-
1 file changed, 12 insertions(+), 1 deletion(-)
diff --git a/ctree.c b/ctree.c
index 4fc33b14000a..ddb1e9cc6d37 100644
--- a/ctree.c
+++ b/ctree.c
@@ -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,
--
2.16.1
^ permalink raw reply related [flat|nested] 3+ messages in thread* Re: [PATCH] btrfs-progs: ctree: Add extra level check for read_node_slot()
2018-02-07 9:18 [PATCH] btrfs-progs: ctree: Add extra level check for read_node_slot() Qu Wenruo
@ 2018-02-07 14:35 ` David Sterba
2018-02-07 14:46 ` Qu Wenruo
0 siblings, 1 reply; 3+ messages in thread
From: David Sterba @ 2018-02-07 14:35 UTC (permalink / raw)
To: Qu Wenruo; +Cc: linux-btrfs, dsterba, ralphgauges
On Wed, Feb 07, 2018 at 05:18:25PM +0800, Qu Wenruo wrote:
> 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 <ralphgauges@googlemail.com>
> Signed-off-by: Qu Wenruo <wqu@suse.com>
> ---
> ctree.c | 13 ++++++++++++-
> 1 file changed, 12 insertions(+), 1 deletion(-)
>
> diff --git a/ctree.c b/ctree.c
> index 4fc33b14000a..ddb1e9cc6d37 100644
> --- a/ctree.c
> +++ b/ctree.c
> @@ -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),
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).
> 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,
> --
> 2.16.1
>
> --
> 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
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [PATCH] btrfs-progs: ctree: Add extra level check for read_node_slot()
2018-02-07 14:35 ` David Sterba
@ 2018-02-07 14:46 ` Qu Wenruo
0 siblings, 0 replies; 3+ messages in thread
From: Qu Wenruo @ 2018-02-07 14:46 UTC (permalink / raw)
To: dsterba, Qu Wenruo, linux-btrfs, ralphgauges
[-- Attachment #1.1: Type: text/plain, Size: 3339 bytes --]
On 2018年02月07日 22:35, David Sterba wrote:
> On Wed, Feb 07, 2018 at 05:18:25PM +0800, Qu Wenruo wrote:
>> 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 <ralphgauges@googlemail.com>
>> Signed-off-by: Qu Wenruo <wqu@suse.com>
>> ---
>> ctree.c | 13 ++++++++++++-
>> 1 file changed, 12 insertions(+), 1 deletion(-)
>>
>> diff --git a/ctree.c b/ctree.c
>> index 4fc33b14000a..ddb1e9cc6d37 100644
>> --- a/ctree.c
>> +++ b/ctree.c
>> @@ -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),
>
> 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
>
>> 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,
>> --
>> 2.16.1
>>
>> --
>> 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
>
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 520 bytes --]
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2018-02-07 14:46 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2018-02-07 9:18 [PATCH] btrfs-progs: ctree: Add extra level check for read_node_slot() Qu Wenruo
2018-02-07 14:35 ` David Sterba
2018-02-07 14:46 ` Qu Wenruo
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox