Enhance btrfs_verify_dev_extents() to do more checks on dev extents
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.
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
PATCH 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 check if one dev extent overlaps with previous dev extent.
--- fs/btrfs/volumes.c | 14 ++++++++++++++ 1 file changed, 14 insertions(+)
fs/btrfs/volumes.c +14 -0
@@ -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
Re: [PATCH 1/2] btrfs: volumes: Make sure there is no overlap dev extents at mount time
Quoted 5 lines · 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 <[email protected]>Signed-off-by Qu Wenruo <[email protected]>
If this is related to the initial report here:
https://lore.kernel.org/linux-btrfs/[email protected]/T/#m609ccb5d32998e8ba5cfa9901c1ab56a38a6f374
I'd rather have it as a link to the commit:
Link:
https://lore.kernel.org/linux-btrfs/[email protected]/
Since I wouldn't expect this occur but clearly there is a bug.
Quoted 41 lines
--- fs/btrfs/volumes.c | 14 ++++++++++++++ 1 file changed, 14 insertions(+)fs/btrfs/volumes.c +14
-0@@ -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
Quoted 9 lines · Nikolay Borisov
Quoted 5 lines · 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 <[email protected]>Signed-off-by Qu Wenruo <[email protected]>
Quoted 8 lines
If this is related to the initial report here:
https://lore.kernel.org/linux-btrfs/[email protected]/T/#m609ccb5d32998e8ba5cfa9901c1ab56a38a6f374
I'd rather have it as a link to the commit:
I'd prefer something shorter, like.
Is there any shorter URL version from lore.kernel.org?
Will add such tag in next version.
Thanks,
Qu
Quoted 45 lines
Since I wouldn't expect this occur but clearly there is a bug.
Quoted 41 lines
--- fs/btrfs/volumes.c | 14 ++++++++++++++ 1 file changed, 14 insertions(+)fs/btrfs/volumes.c +14
-0@@ -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
Quoted 18 lines · Qu Wenruo
Quoted 15 lines · Nikolay Borisov
Quoted 5 lines · 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 <[email protected]>Signed-off-by Qu Wenruo <[email protected]>If this is related to the initial report here:
https://lore.kernel.org/linux-btrfs/[email protected]/T/#m609ccb5d32998e8ba5cfa9901c1ab56a38a6f374
I'd rather have it as a link to the commit:
Quoted 7 lines
Quoted 2 lines
I'd prefer something shorter, like.
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>
PATCH 2/2 btrfs: volumes: Make sure no dev extent is beyond device boundary
In reply to Qu WenruoAdd extra dev extent end check against device boundary.
--- fs/btrfs/volumes.c | 17 +++++++++++++++++ 1 file changed, 17 insertions(+)
fs/btrfs/volumes.c +17 -0
@@ -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 )
Signature · 1 line
2.19.0
Re: [PATCH 2/2] btrfs: volumes: Make sure no dev extent is beyond device boundary
Quoted 28 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 +6
-0@@ -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 ;
fs/btrfs/volumes.c int btrfs_verify_dev_extents(struct btrfs_fs_info *fs_info) · line 7535
Quoted 10 lines
fs/btrfs/volumes.c +10
-0+ 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. ?
Quoted 5 lines
fs/btrfs/volumes.c +1
-0+ ret =verify_one_dev_extent (fs_info, chunk_offset, devid, physical_offset, physical_len);if (ret <0 )
Re: [PATCH 2/2] btrfs: volumes: Make sure no dev extent is beyond device boundary
Quoted 37 lines · Nikolay Borisov
Quoted 34 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 +12
-0@@ -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,
Quoted 8 lines
fs/btrfs/volumes.c int btrfs_verify_dev_extents(struct btrfs_fs_info *fs_info) · line 7535
Quoted 4 lines
fs/btrfs/volumes.c +4
-0+ 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
Quoted 6 lines
Quoted 6 lines
fs/btrfs/volumes.c +1
-0+ ret =verify_one_dev_extent (fs_info, chunk_offset, devid, physical_offset, physical_len);if (ret <0 )