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

[Qu Wenruo <quwenruo.btrfs@gmx.com>]

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