[PATCH] btrfs: tree-checker: Check level for leaves and nodes

[PATCH] btrfs: tree-checker: Check level for leaves and nodes

[Qu Wenruo]

Although we have tree level check at tree read runtime, it's completely
based on its parent level.
We still need to do accurate level check to avoid invalid tree blocks
sneak into kernel space.

The check itself is simple, for leaf its level should always be 0.
For nodes its level should be in range [1, BTRFS_MAX_LEVEL - 1].

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 fs/btrfs/tree-checker.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/fs/btrfs/tree-checker.c b/fs/btrfs/tree-checker.c
index db835635372f..cab0b1f1f741 100644
--- a/fs/btrfs/tree-checker.c
+++ b/fs/btrfs/tree-checker.c
@@ -487,6 +487,13 @@ static int check_leaf(struct btrfs_fs_info *fs_info, struct extent_buffer *leaf,
 	u32 nritems = btrfs_header_nritems(leaf);
 	int slot;
 
+	if (btrfs_header_level(leaf) != 0) {
+		generic_err(fs_info, leaf, 0,
+			"invalid level for leaf, have %d expect 0",
+			btrfs_header_level(leaf));
+		return -EUCLEAN;
+	}
+
 	/*
 	 * Extent buffers from a relocation tree have a owner field that
 	 * corresponds to the subvolume tree they are based on. So just from an
@@ -645,9 +652,16 @@ int btrfs_check_node(struct btrfs_fs_info *fs_info, struct extent_buffer *node)
 	unsigned long nr = btrfs_header_nritems(node);
 	struct btrfs_key key, next_key;
 	int slot;
+	int level = btrfs_header_level(node);
 	u64 bytenr;
 	int ret = 0;
 
+	if (level <= 0 || level >= BTRFS_MAX_LEVEL) {
+		generic_err(fs_info, node, 0,
+			"invalid level for node, have %d expect [1, %d]",
+			level, BTRFS_MAX_LEVEL - 1);
+		return -EUCLEAN;
+	}
 	if (nr == 0 || nr > BTRFS_NODEPTRS_PER_BLOCK(fs_info)) {
 		btrfs_crit(fs_info,
 "corrupt node: root=%llu block=%llu, nritems too %s, have %lu expect range [1,%u]",
-- 
2.19.0

Re: [PATCH] btrfs: tree-checker: Check level for leaves and nodes

[Su Yue]

> Sent: Friday, September 28, 2018 at 7:59 AM
> From: "Qu Wenruo" <wqu@suse.com>
> To: linux-btrfs@vger.kernel.org
> Subject: [PATCH] btrfs: tree-checker: Check level for leaves and nodes
>
> Although we have tree level check at tree read runtime, it's completely
> based on its parent level.
> We still need to do accurate level check to avoid invalid tree blocks
> sneak into kernel space.
> 
> The check itself is simple, for leaf its level should always be 0.
> For nodes its level should be in range [1, BTRFS_MAX_LEVEL - 1].
> 
> Signed-off-by: Qu Wenruo <wqu@suse.com>

Reviewed-by: Su Yue <suy.fnst@cn.fujitsu.com>
> ---
>  fs/btrfs/tree-checker.c | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
> 
> diff --git a/fs/btrfs/tree-checker.c b/fs/btrfs/tree-checker.c
> index db835635372f..cab0b1f1f741 100644
> --- a/fs/btrfs/tree-checker.c
> +++ b/fs/btrfs/tree-checker.c
> @@ -487,6 +487,13 @@ static int check_leaf(struct btrfs_fs_info *fs_info, struct extent_buffer *leaf,
>  	u32 nritems = btrfs_header_nritems(leaf);
>  	int slot;
>  
> +	if (btrfs_header_level(leaf) != 0) {
> +		generic_err(fs_info, leaf, 0,
> +			"invalid level for leaf, have %d expect 0",
> +			btrfs_header_level(leaf));
> +		return -EUCLEAN;
> +	}
> +
>  	/*
>  	 * Extent buffers from a relocation tree have a owner field that
>  	 * corresponds to the subvolume tree they are based on. So just from an
> @@ -645,9 +652,16 @@ int btrfs_check_node(struct btrfs_fs_info *fs_info, struct extent_buffer *node)
>  	unsigned long nr = btrfs_header_nritems(node);
>  	struct btrfs_key key, next_key;
>  	int slot;
> +	int level = btrfs_header_level(node);
>  	u64 bytenr;
>  	int ret = 0;
>  
> +	if (level <= 0 || level >= BTRFS_MAX_LEVEL) {
> +		generic_err(fs_info, node, 0,
> +			"invalid level for node, have %d expect [1, %d]",
> +			level, BTRFS_MAX_LEVEL - 1);
> +		return -EUCLEAN;
> +	}
>  	if (nr == 0 || nr > BTRFS_NODEPTRS_PER_BLOCK(fs_info)) {
>  		btrfs_crit(fs_info,
>  "corrupt node: root=%llu block=%llu, nritems too %s, have %lu expect range [1,%u]",
> -- 
> 2.19.0
> 
> 

Re: [PATCH] btrfs: tree-checker: Check level for leaves and nodes

[Qu Wenruo]

On 2018/9/28 上午9:08, Su Yue wrote:
> 
> 
> On 9/28/18 7:59 AM, Qu Wenruo wrote:
>> Although we have tree level check at tree read runtime, it's completely
>> based on its parent level.
>> We still need to do accurate level check to avoid invalid tree blocks
>> sneak into kernel space.
>>
>> The check itself is simple, for leaf its level should always be 0.
>> For nodes its level should be in range [1, BTRFS_MAX_LEVEL - 1].
>>
>> Signed-off-by: Qu Wenruo <wqu@suse.com>
>> ---
>>   fs/btrfs/tree-checker.c | 14 ++++++++++++++
>>   1 file changed, 14 insertions(+)
>>
>> diff --git a/fs/btrfs/tree-checker.c b/fs/btrfs/tree-checker.c
>> index db835635372f..cab0b1f1f741 100644
>> --- a/fs/btrfs/tree-checker.c
>> +++ b/fs/btrfs/tree-checker.c
>> @@ -487,6 +487,13 @@ static int check_leaf(struct btrfs_fs_info
>> *fs_info, struct extent_buffer *leaf,
>>       u32 nritems = btrfs_header_nritems(leaf);
>>       int slot;
>>   +    if (btrfs_header_level(leaf) != 0) {
>> +        generic_err(fs_info, leaf, 0,
>> +            "invalid level for leaf, have %d expect 0",
>> +            btrfs_header_level(leaf));
>> +        return -EUCLEAN;
>> +    }
>> +
> BTW, output more info(bytenr, root) of the corrupted node/leaf may be
> better?

That's what will be outputted by generic_err() function.

Thanks,
Qu

> 
>>       /*
>>        * Extent buffers from a relocation tree have a owner field that
>>        * corresponds to the subvolume tree they are based on. So just
>> from an
>> @@ -645,9 +652,16 @@ int btrfs_check_node(struct btrfs_fs_info
>> *fs_info, struct extent_buffer *node)
>>       unsigned long nr = btrfs_header_nritems(node);
>>       struct btrfs_key key, next_key;
>>       int slot;
>> +    int level = btrfs_header_level(node);
>>       u64 bytenr;
>>       int ret = 0;
>>   +    if (level <= 0 || level >= BTRFS_MAX_LEVEL) {
>> +        generic_err(fs_info, node, 0,
>> +            "invalid level for node, have %d expect [1, %d]",
>> +            level, BTRFS_MAX_LEVEL - 1);
>> +        return -EUCLEAN;
>> +    }
>>       if (nr == 0 || nr > BTRFS_NODEPTRS_PER_BLOCK(fs_info)) {
>>           btrfs_crit(fs_info,
>>   "corrupt node: root=%llu block=%llu, nritems too %s, have %lu expect
>> range [1,%u]",
>>
> 
> 
> 

Re: [PATCH] btrfs: tree-checker: Check level for leaves and nodes

[Su Yue]

On 9/28/18 7:59 AM, Qu Wenruo wrote:
> Although we have tree level check at tree read runtime, it's completely
> based on its parent level.
> We still need to do accurate level check to avoid invalid tree blocks
> sneak into kernel space.
> 
> The check itself is simple, for leaf its level should always be 0.
> For nodes its level should be in range [1, BTRFS_MAX_LEVEL - 1].
> 
> Signed-off-by: Qu Wenruo <wqu@suse.com>
> ---
>   fs/btrfs/tree-checker.c | 14 ++++++++++++++
>   1 file changed, 14 insertions(+)
> 
> diff --git a/fs/btrfs/tree-checker.c b/fs/btrfs/tree-checker.c
> index db835635372f..cab0b1f1f741 100644
> --- a/fs/btrfs/tree-checker.c
> +++ b/fs/btrfs/tree-checker.c
> @@ -487,6 +487,13 @@ static int check_leaf(struct btrfs_fs_info *fs_info, struct extent_buffer *leaf,
>   	u32 nritems = btrfs_header_nritems(leaf);
>   	int slot;
>   
> +	if (btrfs_header_level(leaf) != 0) {
> +		generic_err(fs_info, leaf, 0,
> +			"invalid level for leaf, have %d expect 0",
> +			btrfs_header_level(leaf));
> +		return -EUCLEAN;
> +	}
> +
BTW, output more info(bytenr, root) of the corrupted node/leaf may be 
better?

>   	/*
>   	 * Extent buffers from a relocation tree have a owner field that
>   	 * corresponds to the subvolume tree they are based on. So just from an
> @@ -645,9 +652,16 @@ int btrfs_check_node(struct btrfs_fs_info *fs_info, struct extent_buffer *node)
>   	unsigned long nr = btrfs_header_nritems(node);
>   	struct btrfs_key key, next_key;
>   	int slot;
> +	int level = btrfs_header_level(node);
>   	u64 bytenr;
>   	int ret = 0;
>   
> +	if (level <= 0 || level >= BTRFS_MAX_LEVEL) {
> +		generic_err(fs_info, node, 0,
> +			"invalid level for node, have %d expect [1, %d]",
> +			level, BTRFS_MAX_LEVEL - 1);
> +		return -EUCLEAN;
> +	}
>   	if (nr == 0 || nr > BTRFS_NODEPTRS_PER_BLOCK(fs_info)) {
>   		btrfs_crit(fs_info,
>   "corrupt node: root=%llu block=%llu, nritems too %s, have %lu expect range [1,%u]",
> 

Re: [PATCH] btrfs: tree-checker: Check level for leaves and nodes

[David Sterba]

On Fri, Sep 28, 2018 at 07:59:34AM +0800, Qu Wenruo wrote:
> Although we have tree level check at tree read runtime, it's completely
> based on its parent level.
> We still need to do accurate level check to avoid invalid tree blocks
> sneak into kernel space.
> 
> The check itself is simple, for leaf its level should always be 0.
> For nodes its level should be in range [1, BTRFS_MAX_LEVEL - 1].
> 
> Signed-off-by: Qu Wenruo <wqu@suse.com>

Reviewed-by: David Sterba <dsterba@suse.com>

Added to misc-next.