Re: [PATCH 1/2] btrfs: Introduce mount time chunk <-> dev extent mapping check
On 2018年07月16日 21:06, David Sterba wrote:
> On Mon, Jul 09, 2018 at 02:42:02PM +0800, Qu Wenruo wrote:
>> This patch will introduce chunk <-> dev extent mapping check, to protect
>> us against invalid dev extents or chunks.
>>
>> Since chunk mapping is the fundamental infrastructure of btrfs, extra
>> check at mount time could prevent a lot of unexpected behavior (BUG_ON).
>>
>> Reported-by: Xu Wen
>> Links: https://bugzilla.kernel.org/show_bug.cgi?id=200403
>
> Link:
>
>> Links: https://bugzilla.kernel.org/show_bug.cgi?id=200407
>> Signed-off-by: Qu Wenruo
>> ---
>> fs/btrfs/disk-io.c | 7 ++
>> fs/btrfs/volumes.c | 173 +
>> fs/btrfs/volumes.h | 2 +
>> 3 files changed, 182 insertions(+)
>>
>> diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
>> index 205092dc9390..068ca7498e94 100644
>> --- a/fs/btrfs/disk-io.c
>> +++ b/fs/btrfs/disk-io.c
>> @@ -3075,6 +3075,13 @@ int open_ctree(struct super_block *sb,
>> fs_info->generation = generation;
>> fs_info->last_trans_committed = generation;
>>
>> +ret = btrfs_verify_dev_extents(fs_info);
>> +if (ret) {
>> +btrfs_err(fs_info,
>> + "failed to verify dev extents against chunks: %d",
>> + ret);
>> +goto fail_block_groups;
>> +}
>> ret = btrfs_recover_balance(fs_info);
>> if (ret) {
>> btrfs_err(fs_info, "failed to recover balance: %d", ret);
>> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
>> index e6a8e4aabc66..05e418cb37f3 100644
>> --- a/fs/btrfs/volumes.c
>> +++ b/fs/btrfs/volumes.c
>> @@ -6440,6 +6440,7 @@ static int read_one_chunk(struct btrfs_fs_info
>> *fs_info, struct btrfs_key *key,
>> map->stripe_len = btrfs_chunk_stripe_len(leaf, chunk);
>> map->type = btrfs_chunk_type(leaf, chunk);
>> map->sub_stripes = btrfs_chunk_sub_stripes(leaf, chunk);
>> +map->verified_stripes = 0;
>> for (i = 0; i < num_stripes; i++) {
>> map->stripes[i].physical =
>> btrfs_stripe_offset_nr(leaf, chunk, i);
>> @@ -7295,3 +7296,175 @@ void btrfs_reset_fs_info_ptr(struct btrfs_fs_info
>> *fs_info)
>> fs_devices = fs_devices->seed;
>> }
>> }
>> +
>> +static u64 calc_stripe_length(u64 type, u64 chunk_len, int num_stripes)
>> +{
>> +switch (type & BTRFS_BLOCK_GROUP_PROFILE_MASK) {
>> +case BTRFS_BLOCK_GROUP_RAID0:
>> +return div_u64(chunk_len, num_stripes);
>> +case BTRFS_BLOCK_GROUP_RAID10:
>> +return div_u64(chunk_len * 2, num_stripes);
>> +case BTRFS_BLOCK_GROUP_RAID5:
>> +return div_u64(chunk_len, num_stripes - 1);
>> +case BTRFS_BLOCK_GROUP_RAID6:
>> +return div_u64(chunk_len, num_stripes - 2);
>> +default:
>> +return chunk_len;
>> +}
>> +}
>
> There are already too many hardcoded values for the raid profiles,
> please don't add another one unless really necessary and use existing
> predefined constants or helpers (eg. nr_data_stripes or
> btrfs_raid_array).
OK, I'll try to reuse btrfs_raid_array in next version.
Thanks,
Qu
>
>> +static int verify_one_dev_extent(struct btrfs_fs_info *fs_info,
>> + u64 chunk_offset, u64 devid,
>> + u64 physical_offset, u64 physical_len)
>> +{
>> +struct extent_map_tree *em_tree = _info->mapping_tree.map_tree;
>> +struct extent_map *em;
>> +struct map_lookup *map;
>> +u64 stripe_len;
>> +bool found = false;
>
> This variable is only set and never checked.
>
>> +int ret = 0;
>> +int i;
>> +
>> +read_lock(_tree->lock);
>> +em = lookup_extent_mapping(em_tree, chunk_offset, 1);
>> +read_unlock(_tree->lock);
>> +
>> +if (!em) {
>> +ret = -EUCLEAN;
>> +btrfs_err(fs_info,
>> +"dev extent (%llu, %llu) doesn't have corresponding chunk",
>> + devid, physical_offset);
>> +goto out;
>> +}
>> +
>> +map = em->map_lookup;
>> +stripe_len = calc_stripe_length(map->type, em->len, map->num_stripes);
>> +if (physical_len != stripe_len) {
>> +btrfs_err(fs_info,
>> +"dev extent (%llu, %llu) length doesn't match with chunk %llu, have %llu
>> expect %llu",
>> + devid, physical_offset, em->start, physical_len,
>> + stripe_len);
>> +ret = -EUCLEAN;
>> +goto out;
>> +}
>> +
>> +for (i = 0; i < map->num_stripes; i++) {
>> +if (map->stripes[i].dev->devid == devid &&
>> +map->stripes[i].physical == physical_offset) {
>> +found = true;
>
> 2nd time set
>
>> +if (map->verified_stripes >= map->num_stripes) {
>> +btrfs_err(fs_info,
>> +"too many dev extent for chunk %llu is detected",
>> + em->start);
>> +
Re: [PATCH 1/2] btrfs: Introduce mount time chunk <-> dev extent mapping check
On Mon, Jul 09, 2018 at 02:42:02PM +0800, Qu Wenruo wrote:
> This patch will introduce chunk <-> dev extent mapping check, to protect
> us against invalid dev extents or chunks.
>
> Since chunk mapping is the fundamental infrastructure of btrfs, extra
> check at mount time could prevent a lot of unexpected behavior (BUG_ON).
>
> Reported-by: Xu Wen
> Links: https://bugzilla.kernel.org/show_bug.cgi?id=200403
Link:
> Links: https://bugzilla.kernel.org/show_bug.cgi?id=200407
> Signed-off-by: Qu Wenruo
> ---
> fs/btrfs/disk-io.c | 7 ++
> fs/btrfs/volumes.c | 173 +
> fs/btrfs/volumes.h | 2 +
> 3 files changed, 182 insertions(+)
>
> diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
> index 205092dc9390..068ca7498e94 100644
> --- a/fs/btrfs/disk-io.c
> +++ b/fs/btrfs/disk-io.c
> @@ -3075,6 +3075,13 @@ int open_ctree(struct super_block *sb,
> fs_info->generation = generation;
> fs_info->last_trans_committed = generation;
>
> + ret = btrfs_verify_dev_extents(fs_info);
> + if (ret) {
> + btrfs_err(fs_info,
> + "failed to verify dev extents against chunks: %d",
> + ret);
> + goto fail_block_groups;
> + }
> ret = btrfs_recover_balance(fs_info);
> if (ret) {
> btrfs_err(fs_info, "failed to recover balance: %d", ret);
> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> index e6a8e4aabc66..05e418cb37f3 100644
> --- a/fs/btrfs/volumes.c
> +++ b/fs/btrfs/volumes.c
> @@ -6440,6 +6440,7 @@ static int read_one_chunk(struct btrfs_fs_info
> *fs_info, struct btrfs_key *key,
> map->stripe_len = btrfs_chunk_stripe_len(leaf, chunk);
> map->type = btrfs_chunk_type(leaf, chunk);
> map->sub_stripes = btrfs_chunk_sub_stripes(leaf, chunk);
> + map->verified_stripes = 0;
> for (i = 0; i < num_stripes; i++) {
> map->stripes[i].physical =
> btrfs_stripe_offset_nr(leaf, chunk, i);
> @@ -7295,3 +7296,175 @@ void btrfs_reset_fs_info_ptr(struct btrfs_fs_info
> *fs_info)
> fs_devices = fs_devices->seed;
> }
> }
> +
> +static u64 calc_stripe_length(u64 type, u64 chunk_len, int num_stripes)
> +{
> + switch (type & BTRFS_BLOCK_GROUP_PROFILE_MASK) {
> + case BTRFS_BLOCK_GROUP_RAID0:
> + return div_u64(chunk_len, num_stripes);
> + case BTRFS_BLOCK_GROUP_RAID10:
> + return div_u64(chunk_len * 2, num_stripes);
> + case BTRFS_BLOCK_GROUP_RAID5:
> + return div_u64(chunk_len, num_stripes - 1);
> + case BTRFS_BLOCK_GROUP_RAID6:
> + return div_u64(chunk_len, num_stripes - 2);
> + default:
> + return chunk_len;
> + }
> +}
There are already too many hardcoded values for the raid profiles,
please don't add another one unless really necessary and use existing
predefined constants or helpers (eg. nr_data_stripes or
btrfs_raid_array).
> +static int verify_one_dev_extent(struct btrfs_fs_info *fs_info,
> + u64 chunk_offset, u64 devid,
> + u64 physical_offset, u64 physical_len)
> +{
> + struct extent_map_tree *em_tree = _info->mapping_tree.map_tree;
> + struct extent_map *em;
> + struct map_lookup *map;
> + u64 stripe_len;
> + bool found = false;
This variable is only set and never checked.
> + int ret = 0;
> + int i;
> +
> + read_lock(_tree->lock);
> + em = lookup_extent_mapping(em_tree, chunk_offset, 1);
> + read_unlock(_tree->lock);
> +
> + if (!em) {
> + ret = -EUCLEAN;
> + btrfs_err(fs_info,
> + "dev extent (%llu, %llu) doesn't have corresponding chunk",
> + devid, physical_offset);
> + goto out;
> + }
> +
> + map = em->map_lookup;
> + stripe_len = calc_stripe_length(map->type, em->len, map->num_stripes);
> + if (physical_len != stripe_len) {
> + btrfs_err(fs_info,
> +"dev extent (%llu, %llu) length doesn't match with chunk %llu, have %llu
> expect %llu",
> + devid, physical_offset, em->start, physical_len,
> + stripe_len);
> + ret = -EUCLEAN;
> + goto out;
> + }
> +
> + for (i = 0; i < map->num_stripes; i++) {
> + if (map->stripes[i].dev->devid == devid &&
> + map->stripes[i].physical == physical_offset) {
> + found = true;
2nd time set
> + if (map->verified_stripes >= map->num_stripes) {
> + btrfs_err(fs_info,
> + "too many dev extent for chunk %llu is detected",
> + em->start);
> + ret = -EUCLEAN;
> + goto out;
> + }
> + map->verified_stripes++;
> + break;
>
[PATCH 1/2] btrfs: Introduce mount time chunk <-> dev extent mapping check
This patch will introduce chunk <-> dev extent mapping check, to protect
us against invalid dev extents or chunks.
Since chunk mapping is the fundamental infrastructure of btrfs, extra
check at mount time could prevent a lot of unexpected behavior (BUG_ON).
Reported-by: Xu Wen
Links: https://bugzilla.kernel.org/show_bug.cgi?id=200403
Links: https://bugzilla.kernel.org/show_bug.cgi?id=200407
Signed-off-by: Qu Wenruo
---
fs/btrfs/disk-io.c | 7 ++
fs/btrfs/volumes.c | 173 +
fs/btrfs/volumes.h | 2 +
3 files changed, 182 insertions(+)
diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
index 205092dc9390..068ca7498e94 100644
--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -3075,6 +3075,13 @@ int open_ctree(struct super_block *sb,
fs_info->generation = generation;
fs_info->last_trans_committed = generation;
+ ret = btrfs_verify_dev_extents(fs_info);
+ if (ret) {
+ btrfs_err(fs_info,
+ "failed to verify dev extents against chunks: %d",
+ ret);
+ goto fail_block_groups;
+ }
ret = btrfs_recover_balance(fs_info);
if (ret) {
btrfs_err(fs_info, "failed to recover balance: %d", ret);
diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index e6a8e4aabc66..05e418cb37f3 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -6440,6 +6440,7 @@ static int read_one_chunk(struct btrfs_fs_info *fs_info,
struct btrfs_key *key,
map->stripe_len = btrfs_chunk_stripe_len(leaf, chunk);
map->type = btrfs_chunk_type(leaf, chunk);
map->sub_stripes = btrfs_chunk_sub_stripes(leaf, chunk);
+ map->verified_stripes = 0;
for (i = 0; i < num_stripes; i++) {
map->stripes[i].physical =
btrfs_stripe_offset_nr(leaf, chunk, i);
@@ -7295,3 +7296,175 @@ void btrfs_reset_fs_info_ptr(struct btrfs_fs_info
*fs_info)
fs_devices = fs_devices->seed;
}
}
+
+static u64 calc_stripe_length(u64 type, u64 chunk_len, int num_stripes)
+{
+ switch (type & BTRFS_BLOCK_GROUP_PROFILE_MASK) {
+ case BTRFS_BLOCK_GROUP_RAID0:
+ return div_u64(chunk_len, num_stripes);
+ case BTRFS_BLOCK_GROUP_RAID10:
+ return div_u64(chunk_len * 2, num_stripes);
+ case BTRFS_BLOCK_GROUP_RAID5:
+ return div_u64(chunk_len, num_stripes - 1);
+ case BTRFS_BLOCK_GROUP_RAID6:
+ return div_u64(chunk_len, num_stripes - 2);
+ default:
+ return chunk_len;
+ }
+}
+static int verify_one_dev_extent(struct btrfs_fs_info *fs_info,
+u64 chunk_offset, u64 devid,
+u64 physical_offset, u64 physical_len)
+{
+ struct extent_map_tree *em_tree = _info->mapping_tree.map_tree;
+ struct extent_map *em;
+ struct map_lookup *map;
+ u64 stripe_len;
+ bool found = false;
+ int ret = 0;
+ int i;
+
+ read_lock(_tree->lock);
+ em = lookup_extent_mapping(em_tree, chunk_offset, 1);
+ read_unlock(_tree->lock);
+
+ if (!em) {
+ ret = -EUCLEAN;
+ btrfs_err(fs_info,
+ "dev extent (%llu, %llu) doesn't have corresponding chunk",
+ devid, physical_offset);
+ goto out;
+ }
+
+ map = em->map_lookup;
+ stripe_len = calc_stripe_length(map->type, em->len, map->num_stripes);
+ if (physical_len != stripe_len) {
+ btrfs_err(fs_info,
+"dev extent (%llu, %llu) length doesn't match with chunk %llu, have %llu
expect %llu",
+ devid, physical_offset, em->start, physical_len,
+ stripe_len);
+ ret = -EUCLEAN;
+ goto out;
+ }
+
+ for (i = 0; i < map->num_stripes; i++) {
+ if (map->stripes[i].dev->devid == devid &&
+ map->stripes[i].physical == physical_offset) {
+ found = true;
+ if (map->verified_stripes >= map->num_stripes) {
+ btrfs_err(fs_info,
+ "too many dev extent for chunk %llu is detected",
+ em->start);
+ ret = -EUCLEAN;
+ goto out;
+ }
+ map->verified_stripes++;
+ break;
+ }
+ }
+out:
+ free_extent_map(em);
+ return ret;
+}
+
+static int verify_chunk_dev_extent_mapping(struct btrfs_fs_info *fs_info)
+{
+ struct extent_map_tree *em_tree = _info->mapping_tree.map_tree;
+ struct extent_map *em;
+ struct rb_node *node;
+ int ret = 0;
+
+ read_lock(_tree->lock);
+ for (node = rb_first(_tree->map); node; node = rb_next(node)) {
+
