[PATCH v2] btrfs-progs: ctree: Add extra level check for read_node_slot()

[PATCH v2] btrfs-progs: ctree: Add extra level check for read_node_slot()

[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 <ralphgauges@googlemail.com>
Signed-off-by: Qu Wenruo <wqu@suse.com>
---
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(-)

diff --git a/ctree.c b/ctree.c
index 4fc33b14000a..430805e3043f 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,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,
-- 
2.16.1

Re: [PATCH v2] btrfs-progs: ctree: Add extra level check for read_node_slot()

[Nikolay Borisov]

On  8.02.2018 02:59, 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>

Reviewed-by: Nikolay Borisov <nborisov@suse.com>

> ---
> 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(-)
> 
> diff --git a/ctree.c b/ctree.c
> index 4fc33b14000a..430805e3043f 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,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,
> 

Re: [PATCH v2] btrfs-progs: ctree: Add extra level check for read_node_slot()

[David Sterba]

On Thu, Feb 08, 2018 at 08:59:40AM +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>

Applied, thanks.

> ---
> 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(-)
> 
> diff --git a/ctree.c b/ctree.c
> index 4fc33b14000a..430805e3043f 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,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",

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.

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

Re: [PATCH v2] btrfs-progs: ctree: Add extra level check for read_node_slot()

[Qu Wenruo]

[-- Attachment #1.1: Type: text/plain, Size: 4000 bytes --]



On 2018年03月19日 20:36, David Sterba wrote:
> On Thu, Feb 08, 2018 at 08:59:40AM +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>
> 
> Applied, thanks.
> 
>> ---
>> 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(-)
>>
>> diff --git a/ctree.c b/ctree.c
>> index 4fc33b14000a..430805e3043f 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,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",
> 
> 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

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

Re: [PATCH v2] btrfs-progs: ctree: Add extra level check for read_node_slot()

[Qu Wenruo]

[-- Attachment #1.1: Type: text/plain, Size: 4311 bytes --]



On 2018年03月20日 13:16, Qu Wenruo wrote:
> 
> 
> On 2018年03月19日 20:36, David Sterba wrote:
>> On Thu, Feb 08, 2018 at 08:59:40AM +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>
>>
>> Applied, thanks.
>>
>>> ---
>>> 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(-)
>>>
>>> diff --git a/ctree.c b/ctree.c
>>> index 4fc33b14000a..430805e3043f 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,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",
>>
>> 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?

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

Thanks,
Qu

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

[PATCH v2 00/10] Chunk allocator unification

[Qu Wenruo]

This patchset can be fetched from github:
https://github.com/adam900710/btrfs-progs/tree/libbtrfs_prepare

This patchset unified a large part of chunk allocator (free device
extent search) between kernel and btrfs-progs.
And reuses kernel function structures like btrfs_finish_chunk_alloc()
and btrfs_alloc_dev_extent().

Before the unification:
               Kernel                |              Btrfs-progs
 btrfs_alloc_chunk()                 | btrfs_alloc_chunk()
 |- __btrfs_alloc_chunk()            | |- Do all the work
                                     |
 btrfs_create_pending_block_groups() |
 |- btrfs_insert_item()              |
 |- btrfs_finish_chunk_alloc()       |
    |- btrfs_alloc_dev_extent()      |

After the unification:
               Kernel                |              Btrfs-progs
 btrfs_alloc_chunk()                 | btrfs_alloc_chunk()
 |- __btrfs_alloc_chunk()            | |- __btrfs_alloc_chunk()
                                     | |- btrfs_finish_chunk_alloc()
 btrfs_create_pending_block_groups() |    |- btrfs_alloc_dev_extent()
 |- btrfs_insert_item()              |
 |- btrfs_finish_chunk_alloc()       |

And the similiar functions are share the same code base, with minor
member/functions change.

This update only modifies patches 7 and after.

Changelog:
v2:
  Make error handler in patch 7 better.
  New patches to unify more functions used in btrfs_alloc_chunk()

Qu Wenruo (10):
  btrfs-progs: Refactor parameter of BTRFS_MAX_DEVS() from root to
    fs_info
  btrfs-progs: Merge btrfs_alloc_data_chunk into btrfs_alloc_chunk
  btrfs-progs: Make btrfs_alloc_chunk to handle block group creation
  btrfs-progs: Introduce btrfs_raid_array and related infrastructures
  btrfs-progs: volumes: Allow find_free_dev_extent() to return maximum
    hole size
  btrfs-progs: kernel-lib: Port kernel sort() to btrfs-progs
  btrfs-progs: volumes: Unify free dev extent search behavior between
    kernel and btrfs-progs.
  btrfs-progs: Move chunk stripe size calcution function to volumes.h
  btrfs-progs: Use btrfs_device->fs_info to replace
    btrfs_device->dev_root
  btrfs-progs: Refactor btrfs_alloc_chunk to mimic kernel structure and
    behavior

 Makefile          |   3 +-
 check/main.c      |  22 --
 convert/main.c    |  10 +-
 ctree.h           |  12 +-
 extent-tree.c     |  12 +-
 kerncompat.h      |   5 +
 kernel-lib/sort.c | 104 ++++++
 kernel-lib/sort.h |  16 +
 mkfs/main.c       |  27 +-
 utils.c           |   2 +-
 volumes.c         | 927 ++++++++++++++++++++++++++++--------------------------
 volumes.h         |  66 +++-
 12 files changed, 695 insertions(+), 511 deletions(-)
 create mode 100644 kernel-lib/sort.c
 create mode 100644 kernel-lib/sort.h

-- 
2.16.1

[PATCH v2] btrfs-progs: ctree: Add extra level check for read_node_slot()

[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 <ralphgauges@googlemail.com>
Signed-off-by: Qu Wenruo <wqu@suse.com>
---
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(-)

diff --git a/ctree.c b/ctree.c
index 4fc33b14000a..430805e3043f 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,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,
-- 
2.16.1

Re: [PATCH v2] btrfs-progs: ctree: Add extra level check for read_node_slot()

[Nikolay Borisov]

On  9.02.2018 09:44, 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>
> ---
> 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(-)

That was sent separately so I'd assume it was in the wrong dir ;)

> 
> diff --git a/ctree.c b/ctree.c
> index 4fc33b14000a..430805e3043f 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,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,
> 

Re: [PATCH v2] btrfs-progs: ctree: Add extra level check for read_node_slot()

[Qu Wenruo]

[-- Attachment #1.1: Type: text/plain, Size: 3233 bytes --]



On 2018年02月09日 16:09, Nikolay Borisov wrote:
> 
> 
> On  9.02.2018 09:44, 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>
>> ---
>> 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(-)
> 
> That was sent separately so I'd assume it was in the wrong dir ;)

Yep, my bad habit of putting all patches in project dir.

Thanks,
Qu

> 
>>
>> diff --git a/ctree.c b/ctree.c
>> index 4fc33b14000a..430805e3043f 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,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,
>>
> --
> 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 --]