[PATCH v2] btrfs: tree-checker: Fix wrong check on max devid

[PATCH v2] btrfs: tree-checker: Fix wrong check on max devid

[Qu Wenruo]

Btrfs doesn't reuse devid, thus if we add and delete device in a loop,
we can increase devid to higher value, triggering tree checker to give a
false alert.

Furthermore, we have dev extent verification already (after
tree-checker, at mount time).
So even if user had bitflip on some dev items, we can still detect it
and refuse to mount.

Reported-by: Anand Jain <anand.jain@oracle.com>
Fixes: ab4ba2e13346 ("btrfs: tree-checker: Verify dev item")
Signed-off-by: Qu Wenruo <wqu@suse.com>
---
Changelog:
v2:
- Remove devid check completely
  As we already have verify_one_dev_extent().
---
 fs/btrfs/tree-checker.c | 8 --------
 1 file changed, 8 deletions(-)

diff --git a/fs/btrfs/tree-checker.c b/fs/btrfs/tree-checker.c
index 43e488f5d063..076d5b8014fb 100644
--- a/fs/btrfs/tree-checker.c
+++ b/fs/btrfs/tree-checker.c
@@ -686,9 +686,7 @@ static void dev_item_err(const struct extent_buffer *eb, int slot,
 static int check_dev_item(struct extent_buffer *leaf,
 			  struct btrfs_key *key, int slot)
 {
-	struct btrfs_fs_info *fs_info = leaf->fs_info;
 	struct btrfs_dev_item *ditem;
-	u64 max_devid = max(BTRFS_MAX_DEVS(fs_info), BTRFS_MAX_DEVS_SYS_CHUNK);
 
 	if (key->objectid != BTRFS_DEV_ITEMS_OBJECTID) {
 		dev_item_err(leaf, slot,
@@ -696,12 +694,6 @@ static int check_dev_item(struct extent_buffer *leaf,
 			     key->objectid, BTRFS_DEV_ITEMS_OBJECTID);
 		return -EUCLEAN;
 	}
-	if (key->offset > max_devid) {
-		dev_item_err(leaf, slot,
-			     "invalid devid: has=%llu expect=[0, %llu]",
-			     key->offset, max_devid);
-		return -EUCLEAN;
-	}
 	ditem = btrfs_item_ptr(leaf, slot, struct btrfs_dev_item);
 	if (btrfs_device_id(leaf, ditem) != key->offset) {
 		dev_item_err(leaf, slot,
-- 
2.23.0

Re: [PATCH v2] btrfs: tree-checker: Fix wrong check on max devid

[Anand Jain]

On 27/8/19 10:05 PM, Qu Wenruo wrote:
> Btrfs doesn't reuse devid, thus if we add and delete device in a loop,
> we can increase devid to higher value, triggering tree checker to give a
> false alert.
> 
> Furthermore, we have dev extent verification already (after
> tree-checker, at mount time).
> So even if user had bitflip on some dev items, we can still detect it
> and refuse to mount.
> 
> Reported-by: Anand Jain <anand.jain@oracle.com>
> Fixes: ab4ba2e13346 ("btrfs: tree-checker: Verify dev item")
> Signed-off-by: Qu Wenruo <wqu@suse.com>
> ---
> Changelog:
> v2:
> - Remove devid check completely
>    As we already have verify_one_dev_extent().
> ---
>   fs/btrfs/tree-checker.c | 8 --------
>   1 file changed, 8 deletions(-)
> 
> diff --git a/fs/btrfs/tree-checker.c b/fs/btrfs/tree-checker.c
> index 43e488f5d063..076d5b8014fb 100644
> --- a/fs/btrfs/tree-checker.c
> +++ b/fs/btrfs/tree-checker.c
> @@ -686,9 +686,7 @@ static void dev_item_err(const struct extent_buffer *eb, int slot,
>   static int check_dev_item(struct extent_buffer *leaf,
>   			  struct btrfs_key *key, int slot)
>   {
> -	struct btrfs_fs_info *fs_info = leaf->fs_info;
>   	struct btrfs_dev_item *ditem;
> -	u64 max_devid = max(BTRFS_MAX_DEVS(fs_info), BTRFS_MAX_DEVS_SYS_CHUNK);
>   
>   	if (key->objectid != BTRFS_DEV_ITEMS_OBJECTID) {
>   		dev_item_err(leaf, slot,
> @@ -696,12 +694,6 @@ static int check_dev_item(struct extent_buffer *leaf,
>   			     key->objectid, BTRFS_DEV_ITEMS_OBJECTID);
>   		return -EUCLEAN;
>   	}
> -	if (key->offset > max_devid) {
> -		dev_item_err(leaf, slot,
> -			     "invalid devid: has=%llu expect=[0, %llu]",
> -			     key->offset, max_devid);
> -		return -EUCLEAN;
> -	}
>   	ditem = btrfs_item_ptr(leaf, slot, struct btrfs_dev_item);
>   	if (btrfs_device_id(leaf, ditem) != key->offset) {
>   		dev_item_err(leaf, slot,
> 

   Though ab4ba2e13346 didn't add BTRFS_MAX_DEVS_SYS_CHUNK,
   BTRFS_MAX_DEVS_SYS_CHUNK is unused now, can be deleted.

   The reproducer script and logs should rather be in this change log.

Thanks, Anand

Re: [PATCH v2] btrfs: tree-checker: Fix wrong check on max devid

[WenRuo Qu]

On 2019/8/28 上午7:26, Anand Jain wrote:
> On 27/8/19 10:05 PM, Qu Wenruo wrote:
>> Btrfs doesn't reuse devid, thus if we add and delete device in a loop,
>> we can increase devid to higher value, triggering tree checker to give a
>> false alert.
>>
>> Furthermore, we have dev extent verification already (after
>> tree-checker, at mount time).
>> So even if user had bitflip on some dev items, we can still detect it
>> and refuse to mount.
>>
>> Reported-by: Anand Jain <anand.jain@oracle.com>
>> Fixes: ab4ba2e13346 ("btrfs: tree-checker: Verify dev item")
>> Signed-off-by: Qu Wenruo <wqu@suse.com>
>> ---
>> Changelog:
>> v2:
>> - Remove devid check completely
>>    As we already have verify_one_dev_extent().
>> ---
>>   fs/btrfs/tree-checker.c | 8 --------
>>   1 file changed, 8 deletions(-)
>>
>> diff --git a/fs/btrfs/tree-checker.c b/fs/btrfs/tree-checker.c
>> index 43e488f5d063..076d5b8014fb 100644
>> --- a/fs/btrfs/tree-checker.c
>> +++ b/fs/btrfs/tree-checker.c
>> @@ -686,9 +686,7 @@ static void dev_item_err(const struct
>> extent_buffer *eb, int slot,
>>   static int check_dev_item(struct extent_buffer *leaf,
>>                 struct btrfs_key *key, int slot)
>>   {
>> -    struct btrfs_fs_info *fs_info = leaf->fs_info;
>>       struct btrfs_dev_item *ditem;
>> -    u64 max_devid = max(BTRFS_MAX_DEVS(fs_info),
>> BTRFS_MAX_DEVS_SYS_CHUNK);
>>         if (key->objectid != BTRFS_DEV_ITEMS_OBJECTID) {
>>           dev_item_err(leaf, slot,
>> @@ -696,12 +694,6 @@ static int check_dev_item(struct extent_buffer
>> *leaf,
>>                    key->objectid, BTRFS_DEV_ITEMS_OBJECTID);
>>           return -EUCLEAN;
>>       }
>> -    if (key->offset > max_devid) {
>> -        dev_item_err(leaf, slot,
>> -                 "invalid devid: has=%llu expect=[0, %llu]",
>> -                 key->offset, max_devid);
>> -        return -EUCLEAN;
>> -    }
>>       ditem = btrfs_item_ptr(leaf, slot, struct btrfs_dev_item);
>>       if (btrfs_device_id(leaf, ditem) != key->offset) {
>>           dev_item_err(leaf, slot,
>>
> 
>   Though ab4ba2e13346 didn't add BTRFS_MAX_DEVS_SYS_CHUNK,
>   BTRFS_MAX_DEVS_SYS_CHUNK is unused now, can be deleted.

Nope, they are still used to determine if we're at the max device limit.

So they are still needed.

Thanks,
Qu
> 
>   The reproducer script and logs should rather be in this change log.
> 
> Thanks, Anand
>