* [PATCH v2] btrfs: tree-checker: Fix wrong check on max devid
@ 2019-08-27 14:05 Qu Wenruo
2019-08-27 23:26 ` Anand Jain
0 siblings, 1 reply; 3+ messages in thread
From: Qu Wenruo @ 2019-08-27 14:05 UTC (permalink / raw)
To: linux-btrfs; +Cc: Anand Jain
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
^ permalink raw reply related [flat|nested] 3+ messages in thread* Re: [PATCH v2] btrfs: tree-checker: Fix wrong check on max devid
2019-08-27 14:05 [PATCH v2] btrfs: tree-checker: Fix wrong check on max devid Qu Wenruo
@ 2019-08-27 23:26 ` Anand Jain
2019-08-28 1:16 ` WenRuo Qu
0 siblings, 1 reply; 3+ messages in thread
From: Anand Jain @ 2019-08-27 23:26 UTC (permalink / raw)
To: Qu Wenruo, linux-btrfs
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
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [PATCH v2] btrfs: tree-checker: Fix wrong check on max devid
2019-08-27 23:26 ` Anand Jain
@ 2019-08-28 1:16 ` WenRuo Qu
0 siblings, 0 replies; 3+ messages in thread
From: WenRuo Qu @ 2019-08-28 1:16 UTC (permalink / raw)
To: Anand Jain, linux-btrfs@vger.kernel.org
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
>
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2019-08-28 1:16 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2019-08-27 14:05 [PATCH v2] btrfs: tree-checker: Fix wrong check on max devid Qu Wenruo
2019-08-27 23:26 ` Anand Jain
2019-08-28 1:16 ` WenRuo Qu
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox