Skip to contentSkip to conversation

Enhance btrfs_verify_dev_extents() to do more checks on dev extents

PATCH In review Qu Wenruo 3 participants 5 messages Oct 5, 2018 – Jan 7, 2019 Btrfs file system

lore
Share & tools
lore

Qu Wenruo

author
#0 lore raw

v2 cover Enhance btrfs_verify_dev_extents() to do more checks on dev extents

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(+)
Signature · 1 line
2.19.0
author
#1 lore raw

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

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 <[email protected]>
Signed-off-by Qu Wenruo <[email protected]>
---
 fs/btrfs/volumes.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)
fs/btrfs/volumes.c +14 -0
index da86706123ff..bf0b2c16847a 100644
@@ -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;
Signature · 1 line
2.19.0
author
#2 lore raw

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

In reply to Qu Wenruo

Add extra dev extent end check against device boundary.

Signed-off-by Qu Wenruo <[email protected]>
---
 fs/btrfs/volumes.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)
fs/btrfs/volumes.c +17 -0
index bf0b2c16847a..bc3ac4715694 100644
@@ -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;
Signature · 1 line
2.19.0
#3 lore raw

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

Quoted 21 lines · Qu Wenruo

Add extra dev extent end check against device boundary.

Signed-off-by Qu Wenruo <[email protected]>
---
 fs/btrfs/volumes.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)
fs/btrfs/volumes.c +1 -0
index bf0b2c16847a..bc3ac4715694 100644
@@ -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;

fs/btrfs/volumes.c static int verify_one_dev_extent(struct btrfs_fs_info *fs_info, · line 7426

Quoted 6 lines
fs/btrfs/volumes.c +3 -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.

Quoted 19 lines
fs/btrfs/volumes.c +13 -1
+       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

Signature · 3 lines
Filipe David Manana,

“Whether you think you can, or you think you can't — you're right.”
#4 lore raw
In reply to Qu Wenruo
Quoted 14 lines · 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.

Quoted 7 lines

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.