[PATCH 0/2] Enhance btrfs_verify_dev_extents() to do more checks on dev extents

[PATCH 0/2] Enhance btrfs_verify_dev_extents() to do more checks on dev extents

[Qu Wenruo]

Inspired by Hans' possible flawed DUP chunk allocator, add the following
dev extents checker:

1) Dev extent overlap check
   Dev extents don't use extent_cache so it can't report dev extents
   overlap.
   So manually check dev extents overlap.
   This check is pretty simple since we're already iterating dev extents
   by its physical offset, we only need to remember previous checked dev
   extents to do such check.

2) Dev extent end check
   No dev extent should go beyond device boundary.

These two checks are pretty cheap so it shouldn't bring any performance
overhead.

Qu Wenruo (2):
  btrfs: volumes: Make sure there is no overlap dev extents at mount
    time
  btrfs: volumes: Make sure no dev extent is beyond device boundary

 fs/btrfs/volumes.c | 31 +++++++++++++++++++++++++++++++
 1 file changed, 31 insertions(+)

-- 
2.19.0

[PATCH 1/2] btrfs: volumes: Make sure there is no overlap dev extents at mount time

[Qu Wenruo]

Enhance btrfs_verify_dev_extents() to remember previous checked dev
extents, so it can check if one dev extent overlaps with previous dev extent.

Reported-by: Hans van Kranenburg <hans.van.kranenburg@mendix.com>
Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 fs/btrfs/volumes.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index da86706123ff..bf0b2c16847a 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -7462,6 +7462,8 @@ int btrfs_verify_dev_extents(struct btrfs_fs_info *fs_info)
 	struct btrfs_path *path;
 	struct btrfs_root *root = fs_info->dev_root;
 	struct btrfs_key key;
+	u64 prev_devid = 0;
+	u64 prev_dev_ext_end = 0;
 	int ret = 0;
 
 	key.objectid = 1;
@@ -7506,10 +7508,22 @@ int btrfs_verify_dev_extents(struct btrfs_fs_info *fs_info)
 		chunk_offset = btrfs_dev_extent_chunk_offset(leaf, dext);
 		physical_len = btrfs_dev_extent_length(leaf, dext);
 
+		/* Check if this dev extent over lap with previous one */
+		if (devid == prev_devid && physical_offset < prev_dev_ext_end) {
+			btrfs_err(fs_info,
+"dev extent devid %llu physical offset %llu overlap with previous dev extent end %llu",
+				  devid, physical_offset, prev_dev_ext_end);
+			ret = -EUCLEAN;
+			goto out;
+		}
+
 		ret = verify_one_dev_extent(fs_info, chunk_offset, devid,
 					    physical_offset, physical_len);
 		if (ret < 0)
 			goto out;
+		prev_devid = devid;
+		prev_dev_ext_end = physical_offset + physical_len;
+
 		ret = btrfs_next_item(root, path);
 		if (ret < 0)
 			goto out;
-- 
2.19.0

[PATCH 2/2] btrfs: volumes: Make sure no dev extent is beyond device boundary

[Qu Wenruo]

Add extra dev extent end check against device boundary.

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

diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index bf0b2c16847a..9fb40e30be6a 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -7492,6 +7492,7 @@ int btrfs_verify_dev_extents(struct btrfs_fs_info *fs_info)
 	while (1) {
 		struct extent_buffer *leaf = path->nodes[0];
 		struct btrfs_dev_extent *dext;
+		struct btrfs_device *dev;
 		int slot = path->slots[0];
 		u64 chunk_offset;
 		u64 physical_offset;
@@ -7517,6 +7518,22 @@ int btrfs_verify_dev_extents(struct btrfs_fs_info *fs_info)
 			goto out;
 		}
 
+		/* Make sure no dev extent is beyond device bondary */
+		dev = btrfs_find_device(fs_info, devid, NULL, NULL);
+		if (!dev) {
+			btrfs_err(fs_info, "failed to find devid %llu", devid);
+			ret = -EUCLEAN;
+			goto out;
+		}
+		if (physical_offset + physical_len > dev->disk_total_bytes) {
+			btrfs_err(fs_info,
+"dev extent devid %llu physical offset %llu len %llu is beyond device boundary %llu",
+				  devid, physical_offset, physical_len,
+				  dev->disk_total_bytes);
+			ret = -EUCLEAN;
+			goto out;
+		}
+
 		ret = verify_one_dev_extent(fs_info, chunk_offset, devid,
 					    physical_offset, physical_len);
 		if (ret < 0)
-- 
2.19.0

Re: [PATCH 2/2] btrfs: volumes: Make sure no dev extent is beyond device boundary

[Nikolay Borisov]

On  5.10.2018 11:25, Qu Wenruo wrote:
> Add extra dev extent end check against device boundary.
> 
> Signed-off-by: Qu Wenruo <wqu@suse.com>
> ---
>  fs/btrfs/volumes.c | 17 +++++++++++++++++
>  1 file changed, 17 insertions(+)
> 
> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> index bf0b2c16847a..9fb40e30be6a 100644
> --- a/fs/btrfs/volumes.c
> +++ b/fs/btrfs/volumes.c
> @@ -7492,6 +7492,7 @@ int btrfs_verify_dev_extents(struct btrfs_fs_info *fs_info)
>  	while (1) {
>  		struct extent_buffer *leaf = path->nodes[0];
>  		struct btrfs_dev_extent *dext;
> +		struct btrfs_device *dev;
>  		int slot = path->slots[0];
>  		u64 chunk_offset;
>  		u64 physical_offset;
> @@ -7517,6 +7518,22 @@ int btrfs_verify_dev_extents(struct btrfs_fs_info *fs_info)
>  			goto out;
>  		}
>  
> +		/* Make sure no dev extent is beyond device bondary */
> +		dev = btrfs_find_device(fs_info, devid, NULL, NULL);
> +		if (!dev) {
> +			btrfs_err(fs_info, "failed to find devid %llu", devid);
> +			ret = -EUCLEAN;
> +			goto out;
> +		}
> +		if (physical_offset + physical_len > dev->disk_total_bytes) {
> +			btrfs_err(fs_info,
> +"dev extent devid %llu physical offset %llu len %llu is beyond device boundary %llu",
> +				  devid, physical_offset, physical_len,
> +				  dev->disk_total_bytes);
> +			ret = -EUCLEAN;
> +			goto out;
> +		}

Why not move this code in verify_one_dev_extent, you already have all
the necessary context in there - devid and physical_len/physical_offset,
let's avoid scattering logically related code in different places. ?

> +
>  		ret = verify_one_dev_extent(fs_info, chunk_offset, devid,
>  					    physical_offset, physical_len);
>  		if (ret < 0)
> 

Re: [PATCH 1/2] btrfs: volumes: Make sure there is no overlap dev extents at mount time

[Nikolay Borisov]

On  5.10.2018 11:25, Qu Wenruo wrote:
> Enhance btrfs_verify_dev_extents() to remember previous checked dev
> extents, so it can check if one dev extent overlaps with previous dev extent.
> 
> Reported-by: Hans van Kranenburg <hans.van.kranenburg@mendix.com>
> Signed-off-by: Qu Wenruo <wqu@suse.com>

If this is related to the initial report here:

https://lore.kernel.org/linux-btrfs/782f6000-30c0-0085-abd2-74ec5827c903@mendix.com/T/#m609ccb5d32998e8ba5cfa9901c1ab56a38a6f374

I'd rather have it as a link to the commit:

Link:
https://lore.kernel.org/linux-btrfs/b3461a38-e5f8-f41d-c67c-2efac8129054@mendix.com/

Since I wouldn't expect this occur but clearly there is a bug.


> ---
>  fs/btrfs/volumes.c | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
> 
> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> index da86706123ff..bf0b2c16847a 100644
> --- a/fs/btrfs/volumes.c
> +++ b/fs/btrfs/volumes.c
> @@ -7462,6 +7462,8 @@ int btrfs_verify_dev_extents(struct btrfs_fs_info *fs_info)
>  	struct btrfs_path *path;
>  	struct btrfs_root *root = fs_info->dev_root;
>  	struct btrfs_key key;
> +	u64 prev_devid = 0;
> +	u64 prev_dev_ext_end = 0;
>  	int ret = 0;
>  
>  	key.objectid = 1;
> @@ -7506,10 +7508,22 @@ int btrfs_verify_dev_extents(struct btrfs_fs_info *fs_info)
>  		chunk_offset = btrfs_dev_extent_chunk_offset(leaf, dext);
>  		physical_len = btrfs_dev_extent_length(leaf, dext);
>  
> +		/* Check if this dev extent over lap with previous one */
> +		if (devid == prev_devid && physical_offset < prev_dev_ext_end) {
> +			btrfs_err(fs_info,
> +"dev extent devid %llu physical offset %llu overlap with previous dev extent end %llu",
> +				  devid, physical_offset, prev_dev_ext_end);
> +			ret = -EUCLEAN;
> +			goto out;
> +		}
> +
>  		ret = verify_one_dev_extent(fs_info, chunk_offset, devid,
>  					    physical_offset, physical_len);
>  		if (ret < 0)
>  			goto out;
> +		prev_devid = devid;
> +		prev_dev_ext_end = physical_offset + physical_len;
> +
>  		ret = btrfs_next_item(root, path);
>  		if (ret < 0)
>  			goto out;
> 

Re: [PATCH 2/2] btrfs: volumes: Make sure no dev extent is beyond device boundary

[Qu Wenruo]

On 2018/10/5 下午4:55, Nikolay Borisov wrote:
> 
> 
> On  5.10.2018 11:25, Qu Wenruo wrote:
>> Add extra dev extent end check against device boundary.
>>
>> Signed-off-by: Qu Wenruo <wqu@suse.com>
>> ---
>>  fs/btrfs/volumes.c | 17 +++++++++++++++++
>>  1 file changed, 17 insertions(+)
>>
>> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
>> index bf0b2c16847a..9fb40e30be6a 100644
>> --- a/fs/btrfs/volumes.c
>> +++ b/fs/btrfs/volumes.c
>> @@ -7492,6 +7492,7 @@ int btrfs_verify_dev_extents(struct btrfs_fs_info *fs_info)
>>  	while (1) {
>>  		struct extent_buffer *leaf = path->nodes[0];
>>  		struct btrfs_dev_extent *dext;
>> +		struct btrfs_device *dev;
>>  		int slot = path->slots[0];
>>  		u64 chunk_offset;
>>  		u64 physical_offset;
>> @@ -7517,6 +7518,22 @@ int btrfs_verify_dev_extents(struct btrfs_fs_info *fs_info)
>>  			goto out;
>>  		}
>>  
>> +		/* Make sure no dev extent is beyond device bondary */
>> +		dev = btrfs_find_device(fs_info, devid, NULL, NULL);
>> +		if (!dev) {
>> +			btrfs_err(fs_info, "failed to find devid %llu", devid);
>> +			ret = -EUCLEAN;
>> +			goto out;
>> +		}
>> +		if (physical_offset + physical_len > dev->disk_total_bytes) {
>> +			btrfs_err(fs_info,
>> +"dev extent devid %llu physical offset %llu len %llu is beyond device boundary %llu",
>> +				  devid, physical_offset, physical_len,
>> +				  dev->disk_total_bytes);
>> +			ret = -EUCLEAN;
>> +			goto out;
>> +		}
> 
> Why not move this code in verify_one_dev_extent, you already have all
> the necessary context in there - devid and physical_len/physical_offset,
> let's avoid scattering logically related code in different places. ?

Right, this part could be moved into verify_one_dev_extent().

It will go that way in next update.

Thanks,
Qu

> 
>> +
>>  		ret = verify_one_dev_extent(fs_info, chunk_offset, devid,
>>  					    physical_offset, physical_len);
>>  		if (ret < 0)
>>

Re: [PATCH 1/2] btrfs: volumes: Make sure there is no overlap dev extents at mount time

[Qu Wenruo]

On 2018/10/5 下午4:56, Nikolay Borisov wrote:
> 
> 
> On  5.10.2018 11:25, Qu Wenruo wrote:
>> Enhance btrfs_verify_dev_extents() to remember previous checked dev
>> extents, so it can check if one dev extent overlaps with previous dev extent.
>>
>> Reported-by: Hans van Kranenburg <hans.van.kranenburg@mendix.com>
>> Signed-off-by: Qu Wenruo <wqu@suse.com>
> 
> If this is related to the initial report here:
> 
> https://lore.kernel.org/linux-btrfs/782f6000-30c0-0085-abd2-74ec5827c903@mendix.com/T/#m609ccb5d32998e8ba5cfa9901c1ab56a38a6f374
> 
> I'd rather have it as a link to the commit:
> 
> Link:
> https://lore.kernel.org/linux-btrfs/b3461a38-e5f8-f41d-c67c-2efac8129054@mendix.com/

I'd prefer something shorter, like.
Link: https://www.spinics.net/lists/linux-btrfs/msg82370.html

Is there any shorter URL version from lore.kernel.org?

Will add such tag in next version.

Thanks,
Qu

> 
> Since I wouldn't expect this occur but clearly there is a bug.
> 
> 
>> ---
>>  fs/btrfs/volumes.c | 14 ++++++++++++++
>>  1 file changed, 14 insertions(+)
>>
>> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
>> index da86706123ff..bf0b2c16847a 100644
>> --- a/fs/btrfs/volumes.c
>> +++ b/fs/btrfs/volumes.c
>> @@ -7462,6 +7462,8 @@ int btrfs_verify_dev_extents(struct btrfs_fs_info *fs_info)
>>  	struct btrfs_path *path;
>>  	struct btrfs_root *root = fs_info->dev_root;
>>  	struct btrfs_key key;
>> +	u64 prev_devid = 0;
>> +	u64 prev_dev_ext_end = 0;
>>  	int ret = 0;
>>  
>>  	key.objectid = 1;
>> @@ -7506,10 +7508,22 @@ int btrfs_verify_dev_extents(struct btrfs_fs_info *fs_info)
>>  		chunk_offset = btrfs_dev_extent_chunk_offset(leaf, dext);
>>  		physical_len = btrfs_dev_extent_length(leaf, dext);
>>  
>> +		/* Check if this dev extent over lap with previous one */
>> +		if (devid == prev_devid && physical_offset < prev_dev_ext_end) {
>> +			btrfs_err(fs_info,
>> +"dev extent devid %llu physical offset %llu overlap with previous dev extent end %llu",
>> +				  devid, physical_offset, prev_dev_ext_end);
>> +			ret = -EUCLEAN;
>> +			goto out;
>> +		}
>> +
>>  		ret = verify_one_dev_extent(fs_info, chunk_offset, devid,
>>  					    physical_offset, physical_len);
>>  		if (ret < 0)
>>  			goto out;
>> +		prev_devid = devid;
>> +		prev_dev_ext_end = physical_offset + physical_len;
>> +
>>  		ret = btrfs_next_item(root, path);
>>  		if (ret < 0)
>>  			goto out;
>>

Re: [PATCH 1/2] btrfs: volumes: Make sure there is no overlap dev extents at mount time

[David Sterba]

On Fri, Oct 05, 2018 at 05:22:40PM +0800, Qu Wenruo wrote:
> 
> 
> On 2018/10/5 下午4:56, Nikolay Borisov wrote:
> > 
> > 
> > On  5.10.2018 11:25, Qu Wenruo wrote:
> >> Enhance btrfs_verify_dev_extents() to remember previous checked dev
> >> extents, so it can check if one dev extent overlaps with previous dev extent.
> >>
> >> Reported-by: Hans van Kranenburg <hans.van.kranenburg@mendix.com>
> >> Signed-off-by: Qu Wenruo <wqu@suse.com>
> > 
> > If this is related to the initial report here:
> > 
> > https://lore.kernel.org/linux-btrfs/782f6000-30c0-0085-abd2-74ec5827c903@mendix.com/T/#m609ccb5d32998e8ba5cfa9901c1ab56a38a6f374
> > 
> > I'd rather have it as a link to the commit:
> > 
> > Link:
> > https://lore.kernel.org/linux-btrfs/b3461a38-e5f8-f41d-c67c-2efac8129054@mendix.com/
> 
> I'd prefer something shorter, like.
> Link: https://www.spinics.net/lists/linux-btrfs/msg82370.html
> 
> Is there any shorter URL version from lore.kernel.org?

https://lkml.kernel.org/r/<message-id>

or

https://lore.kernel.org/linux-btrfs/<message-id>