[PATCH] btrfs: tree-checker: Check if the file extent end overflow

[PATCH] btrfs: tree-checker: Check if the file extent end overflow

[Qu Wenruo]

Under certain condition, we could have strange file extent item in log
tree like:

  item 18 key (69599 108 397312) itemoff 15208 itemsize 53
	extent data disk bytenr 0 nr 0
	extent data offset 0 nr 18446744073709547520 ram 18446744073709547520

The num_bytes and ram_bytes part overflow.

For num_bytes part, we can detect such overflow along with file offset
(key->offset), as file_offset + num_bytes should never go beyond u64
max.

For ram_bytes part, it's about the decompressed size of the extent, not
directly related to the size.
In theory is OK to have super large value, and put extra
limitation on ram bytes may cause unexpected false alert.

So in tree-checker, we only check if the file offset and num bytes
overflow.

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

diff --git a/fs/btrfs/tree-checker.c b/fs/btrfs/tree-checker.c
index 4f4f5af6345a..795eb6c18456 100644
--- a/fs/btrfs/tree-checker.c
+++ b/fs/btrfs/tree-checker.c
@@ -112,6 +112,7 @@ static int check_extent_data_item(struct extent_buffer *leaf,
 	struct btrfs_file_extent_item *fi;
 	u32 sectorsize = fs_info->sectorsize;
 	u32 item_size = btrfs_item_size_nr(leaf, slot);
+	u64 extent_end;
 
 	if (!IS_ALIGNED(key->offset, sectorsize)) {
 		file_extent_err(leaf, slot,
@@ -186,6 +187,14 @@ static int check_extent_data_item(struct extent_buffer *leaf,
 	    CHECK_FE_ALIGNED(leaf, slot, fi, offset, sectorsize) ||
 	    CHECK_FE_ALIGNED(leaf, slot, fi, num_bytes, sectorsize))
 		return -EUCLEAN;
+	/* Catch extent end overflow case */
+	if (check_add_overflow(btrfs_file_extent_num_bytes(leaf, fi),
+			       key->offset, &extent_end)) {
+		file_extent_err(leaf, slot,
+	"extent end overflow, have file offset %llu extent num bytes %llu",
+				key->offset,
+				btrfs_file_extent_num_bytes(leaf, fi));
+	}
 	return 0;
 }
 
-- 
2.21.0

Re: [PATCH] btrfs: tree-checker: Check if the file extent end overflow

[David Sterba]

On Fri, May 03, 2019 at 08:30:54AM +0800, Qu Wenruo wrote:
> Under certain condition, we could have strange file extent item in log
> tree like:
> 
>   item 18 key (69599 108 397312) itemoff 15208 itemsize 53
> 	extent data disk bytenr 0 nr 0
> 	extent data offset 0 nr 18446744073709547520 ram 18446744073709547520
> 
> The num_bytes and ram_bytes part overflow.
> 
> For num_bytes part, we can detect such overflow along with file offset
> (key->offset), as file_offset + num_bytes should never go beyond u64
> max.
> 
> For ram_bytes part, it's about the decompressed size of the extent, not
> directly related to the size.
> In theory is OK to have super large value, and put extra
> limitation on ram bytes may cause unexpected false alert.
> 
> So in tree-checker, we only check if the file offset and num bytes
> overflow.
> 
> Signed-off-by: Qu Wenruo <wqu@suse.com>

So this patch can be dropped because of "Btrfs: tree-checker: detect
file extent items with overlapping ranges", right?

Re: [PATCH] btrfs: tree-checker: Check if the file extent end overflow

[Qu Wenruo]

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



On 2019/5/16 下午8:57, David Sterba wrote:
> On Fri, May 03, 2019 at 08:30:54AM +0800, Qu Wenruo wrote:
>> Under certain condition, we could have strange file extent item in log
>> tree like:
>>
>>   item 18 key (69599 108 397312) itemoff 15208 itemsize 53
>> 	extent data disk bytenr 0 nr 0
>> 	extent data offset 0 nr 18446744073709547520 ram 18446744073709547520
>>
>> The num_bytes and ram_bytes part overflow.
>>
>> For num_bytes part, we can detect such overflow along with file offset
>> (key->offset), as file_offset + num_bytes should never go beyond u64
>> max.
>>
>> For ram_bytes part, it's about the decompressed size of the extent, not
>> directly related to the size.
>> In theory is OK to have super large value, and put extra
>> limitation on ram bytes may cause unexpected false alert.
>>
>> So in tree-checker, we only check if the file offset and num bytes
>> overflow.
>>
>> Signed-off-by: Qu Wenruo <wqu@suse.com>
> 
> So this patch can be dropped because of "Btrfs: tree-checker: detect
> file extent items with overlapping ranges", right?
> 
Nope, there is still a case can be detected by this patch only.

If the last file extent overflow, that patch can not detect it.

Although that patch has a much better coverage than this one.

Thanks,
Qu


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] btrfs: tree-checker: Check if the file extent end overflow

[David Sterba]

On Fri, May 03, 2019 at 08:30:54AM +0800, Qu Wenruo wrote:
> Under certain condition, we could have strange file extent item in log
> tree like:
> 
>   item 18 key (69599 108 397312) itemoff 15208 itemsize 53
> 	extent data disk bytenr 0 nr 0
> 	extent data offset 0 nr 18446744073709547520 ram 18446744073709547520
> 
> The num_bytes and ram_bytes part overflow.
> 
> For num_bytes part, we can detect such overflow along with file offset
> (key->offset), as file_offset + num_bytes should never go beyond u64
> max.
> 
> For ram_bytes part, it's about the decompressed size of the extent, not
> directly related to the size.
> In theory is OK to have super large value, and put extra
> limitation on ram bytes may cause unexpected false alert.
> 
> So in tree-checker, we only check if the file offset and num bytes
> overflow.
> 
> Signed-off-by: Qu Wenruo <wqu@suse.com>
> ---
>  fs/btrfs/tree-checker.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/fs/btrfs/tree-checker.c b/fs/btrfs/tree-checker.c
> index 4f4f5af6345a..795eb6c18456 100644
> --- a/fs/btrfs/tree-checker.c
> +++ b/fs/btrfs/tree-checker.c
> @@ -112,6 +112,7 @@ static int check_extent_data_item(struct extent_buffer *leaf,
>  	struct btrfs_file_extent_item *fi;
>  	u32 sectorsize = fs_info->sectorsize;
>  	u32 item_size = btrfs_item_size_nr(leaf, slot);
> +	u64 extent_end;
>  
>  	if (!IS_ALIGNED(key->offset, sectorsize)) {
>  		file_extent_err(leaf, slot,
> @@ -186,6 +187,14 @@ static int check_extent_data_item(struct extent_buffer *leaf,
>  	    CHECK_FE_ALIGNED(leaf, slot, fi, offset, sectorsize) ||
>  	    CHECK_FE_ALIGNED(leaf, slot, fi, num_bytes, sectorsize))
>  		return -EUCLEAN;
> +	/* Catch extent end overflow case */
> +	if (check_add_overflow(btrfs_file_extent_num_bytes(leaf, fi),
> +			       key->offset, &extent_end)) {
> +		file_extent_err(leaf, slot,
> +	"extent end overflow, have file offset %llu extent num bytes %llu",
> +				key->offset,
> +				btrfs_file_extent_num_bytes(leaf, fi));

Isn't this missing

		return -EUCLEAN;

?

> +	}
>  	return 0;
>  }
>  
> -- 
> 2.21.0

Re: [PATCH] btrfs: tree-checker: Check if the file extent end overflow

[Qu Wenruo]

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



On 2019/5/30 下午8:02, David Sterba wrote:
> On Fri, May 03, 2019 at 08:30:54AM +0800, Qu Wenruo wrote:
>> Under certain condition, we could have strange file extent item in log
>> tree like:
>>
>>   item 18 key (69599 108 397312) itemoff 15208 itemsize 53
>> 	extent data disk bytenr 0 nr 0
>> 	extent data offset 0 nr 18446744073709547520 ram 18446744073709547520
>>
>> The num_bytes and ram_bytes part overflow.
>>
>> For num_bytes part, we can detect such overflow along with file offset
>> (key->offset), as file_offset + num_bytes should never go beyond u64
>> max.
>>
>> For ram_bytes part, it's about the decompressed size of the extent, not
>> directly related to the size.
>> In theory is OK to have super large value, and put extra
>> limitation on ram bytes may cause unexpected false alert.
>>
>> So in tree-checker, we only check if the file offset and num bytes
>> overflow.
>>
>> Signed-off-by: Qu Wenruo <wqu@suse.com>
>> ---
>>  fs/btrfs/tree-checker.c | 9 +++++++++
>>  1 file changed, 9 insertions(+)
>>
>> diff --git a/fs/btrfs/tree-checker.c b/fs/btrfs/tree-checker.c
>> index 4f4f5af6345a..795eb6c18456 100644
>> --- a/fs/btrfs/tree-checker.c
>> +++ b/fs/btrfs/tree-checker.c
>> @@ -112,6 +112,7 @@ static int check_extent_data_item(struct extent_buffer *leaf,
>>  	struct btrfs_file_extent_item *fi;
>>  	u32 sectorsize = fs_info->sectorsize;
>>  	u32 item_size = btrfs_item_size_nr(leaf, slot);
>> +	u64 extent_end;
>>  
>>  	if (!IS_ALIGNED(key->offset, sectorsize)) {
>>  		file_extent_err(leaf, slot,
>> @@ -186,6 +187,14 @@ static int check_extent_data_item(struct extent_buffer *leaf,
>>  	    CHECK_FE_ALIGNED(leaf, slot, fi, offset, sectorsize) ||
>>  	    CHECK_FE_ALIGNED(leaf, slot, fi, num_bytes, sectorsize))
>>  		return -EUCLEAN;
>> +	/* Catch extent end overflow case */
>> +	if (check_add_overflow(btrfs_file_extent_num_bytes(leaf, fi),
>> +			       key->offset, &extent_end)) {
>> +		file_extent_err(leaf, slot,
>> +	"extent end overflow, have file offset %llu extent num bytes %llu",
>> +				key->offset,
>> +				btrfs_file_extent_num_bytes(leaf, fi));
> 
> Isn't this missing
> 
> 		return -EUCLEAN;

Oh, my bad.

Would you mind to fold that return line?

Thanks,
Qu

> 
> ?
> 
>> +	}
>>  	return 0;
>>  }
>>  
>> -- 
>> 2.21.0


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] btrfs: tree-checker: Check if the file extent end overflow

[David Sterba]

On Thu, May 30, 2019 at 08:03:48PM +0800, Qu Wenruo wrote:
> >> @@ -112,6 +112,7 @@ static int check_extent_data_item(struct extent_buffer *leaf,
> >>  	struct btrfs_file_extent_item *fi;
> >>  	u32 sectorsize = fs_info->sectorsize;
> >>  	u32 item_size = btrfs_item_size_nr(leaf, slot);
> >> +	u64 extent_end;
> >>  
> >>  	if (!IS_ALIGNED(key->offset, sectorsize)) {
> >>  		file_extent_err(leaf, slot,
> >> @@ -186,6 +187,14 @@ static int check_extent_data_item(struct extent_buffer *leaf,
> >>  	    CHECK_FE_ALIGNED(leaf, slot, fi, offset, sectorsize) ||
> >>  	    CHECK_FE_ALIGNED(leaf, slot, fi, num_bytes, sectorsize))
> >>  		return -EUCLEAN;
> >> +	/* Catch extent end overflow case */
> >> +	if (check_add_overflow(btrfs_file_extent_num_bytes(leaf, fi),
> >> +			       key->offset, &extent_end)) {
> >> +		file_extent_err(leaf, slot,
> >> +	"extent end overflow, have file offset %llu extent num bytes %llu",
> >> +				key->offset,
> >> +				btrfs_file_extent_num_bytes(leaf, fi));
> > 
> > Isn't this missing
> > 
> > 		return -EUCLEAN;
> 
> Oh, my bad.
> 
> Would you mind to fold that return line?

Udated and added to misc-next.