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

[PATCH v2 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.

Changelog:
v2:
  Add "Link:" tag for the first patch.
  Move the actual check into verify_one_dev_extent() for the 2nd patch.

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 v2 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 verify no dev extents can overlap.

Link: https://www.spinics.net/lists/linux-btrfs/msg82370.html
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 v2 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..bc3ac4715694 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -7371,6 +7371,7 @@ static int verify_one_dev_extent(struct btrfs_fs_info *fs_info,
 	struct extent_map_tree *em_tree = &fs_info->mapping_tree.map_tree;
 	struct extent_map *em;
 	struct map_lookup *map;
+	struct btrfs_device *dev;
 	u64 stripe_len;
 	bool found = false;
 	int ret = 0;
@@ -7420,6 +7421,22 @@ static int verify_one_dev_extent(struct btrfs_fs_info *fs_info,
 			physical_offset, devid);
 		ret = -EUCLEAN;
 	}
+
+	/* 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;
+	}
 out:
 	free_extent_map(em);
 	return ret;
-- 
2.19.0

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

[David Sterba]

On Fri, Oct 05, 2018 at 05:45:53PM +0800, Qu Wenruo wrote:
> 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.
> 
> Changelog:
> v2:
>   Add "Link:" tag for the first patch.
>   Move the actual check into verify_one_dev_extent() for the 2nd patch.

Moved from for-next topic branch to misc-next, with some changelog
updates. Thanks.

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

[Filipe Manana]

On Fri, Oct 5, 2018 at 10:46 AM Qu Wenruo <wqu@suse.com> 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..bc3ac4715694 100644
> --- a/fs/btrfs/volumes.c
> +++ b/fs/btrfs/volumes.c
> @@ -7371,6 +7371,7 @@ static int verify_one_dev_extent(struct btrfs_fs_info *fs_info,
>         struct extent_map_tree *em_tree = &fs_info->mapping_tree.map_tree;
>         struct extent_map *em;
>         struct map_lookup *map;
> +       struct btrfs_device *dev;
>         u64 stripe_len;
>         bool found = false;
>         int ret = 0;
> @@ -7420,6 +7421,22 @@ static int verify_one_dev_extent(struct btrfs_fs_info *fs_info,
>                         physical_offset, devid);
>                 ret = -EUCLEAN;
>         }
> +
> +       /* Make sure no dev extent is beyond device bondary */
> +       dev = btrfs_find_device(fs_info, devid, NULL, NULL);

Qu,

This breaks use cases including seed devices. In those cases we need
to find a device by ID and FSID (not not return the first device we
find with a specific ID, ignoring the FSID).
Fstest btrfs/163 now fails in 5.0-rc1 because of this change.

Thanks.

> +       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;
> +       }
>  out:
>         free_extent_map(em);
>         return ret;
> --
> 2.19.0
>


-- 
Filipe David Manana,

“Whether you think you can, or you think you can't — you're right.”