| Message ID | 20190827132206.1483-1-wqu@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | btrfs: tree-checker: Fix wrong check on max devid | expand |
On 8/27/19 9:22 AM, 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. > > But we still don't want to give up the devid check, so here we > compromise by setting a larger devid upper limit, 1<<32. Is this really a useful check? There's no actual definition of what a devid can be, only what the kernel/tools does right now when it adds new devices. There's nothing in the format that requires it to be monotonic increments, which makes any check on read unreliable. Once we do read all the dev items, we can check for corruption on write, though. -Jeff > So crazy scripts can't bump devid to that high value easily, while we can > still detect obviously wrong devid. > > Reported-by: Anand Jain <anand.jain@oracle.com> > Fixes: ab4ba2e13346 ("btrfs: tree-checker: Verify dev item") > Signed-off-by: Qu Wenruo <wqu@suse.com> > --- > fs/btrfs/tree-checker.c | 9 +++++++-- > 1 file changed, 7 insertions(+), 2 deletions(-) > > diff --git a/fs/btrfs/tree-checker.c b/fs/btrfs/tree-checker.c > index 43e488f5d063..f9d24f01801e 100644 > --- a/fs/btrfs/tree-checker.c > +++ b/fs/btrfs/tree-checker.c > @@ -686,9 +686,14 @@ 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); > + /* > + * Btrfs doesn't really reuse devid, thus devid can increase to any > + * value, but we don't believe a devid higher than (1<<32) is really > + * valid. This could at least detect bitflip at the higher > + * 32 bits while still consider high devid valid. > + */ > + u64 max_devid = (1ULL << 32); > > if (key->objectid != BTRFS_DEV_ITEMS_OBJECTID) { > dev_item_err(leaf, slot, >
On 2019/8/27 下午9:37, Jeff Mahoney wrote: > On 8/27/19 9:22 AM, 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. >> >> But we still don't want to give up the devid check, so here we >> compromise by setting a larger devid upper limit, 1<<32. > > Is this really a useful check? There's no actual definition of what a > devid can be, only what the kernel/tools does right now when it adds new > devices. There's nothing in the format that requires it to be monotonic > increments, which makes any check on read unreliable. Right, that check makes no sense. > Once we do read > all the dev items, we can check for corruption on write, though. That could be too time consuming (we need to lookup each devid in fs_devices list) to be done at write time. So I'd prefer just to remove the devid check. We already have dev_extent verification, so even we have corrupted devid, we can detect it at mount time. Thus even we have a devid corrupted by bitflip, we can still detect it, although not by tree-checker. Thanks, Qu > > -Jeff > >> So crazy scripts can't bump devid to that high value easily, while we can >> still detect obviously wrong devid. >> >> Reported-by: Anand Jain <anand.jain@oracle.com> >> Fixes: ab4ba2e13346 ("btrfs: tree-checker: Verify dev item") >> Signed-off-by: Qu Wenruo <wqu@suse.com> >> --- >> fs/btrfs/tree-checker.c | 9 +++++++-- >> 1 file changed, 7 insertions(+), 2 deletions(-) >> >> diff --git a/fs/btrfs/tree-checker.c b/fs/btrfs/tree-checker.c >> index 43e488f5d063..f9d24f01801e 100644 >> --- a/fs/btrfs/tree-checker.c >> +++ b/fs/btrfs/tree-checker.c >> @@ -686,9 +686,14 @@ 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); >> + /* >> + * Btrfs doesn't really reuse devid, thus devid can increase to any >> + * value, but we don't believe a devid higher than (1<<32) is really >> + * valid. This could at least detect bitflip at the higher >> + * 32 bits while still consider high devid valid. >> + */ >> + u64 max_devid = (1ULL << 32); >> >> if (key->objectid != BTRFS_DEV_ITEMS_OBJECTID) { >> dev_item_err(leaf, slot, >> > >
On 8/27/19 9:56 AM, Qu Wenruo wrote: > > > On 2019/8/27 下午9:37, Jeff Mahoney wrote: >> On 8/27/19 9:22 AM, 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. >>> >>> But we still don't want to give up the devid check, so here we >>> compromise by setting a larger devid upper limit, 1<<32. >> >> Is this really a useful check? There's no actual definition of what a >> devid can be, only what the kernel/tools does right now when it adds new >> devices. There's nothing in the format that requires it to be monotonic >> increments, which makes any check on read unreliable. > > Right, that check makes no sense. > >> Once we do read >> all the dev items, we can check for corruption on write, though. > > That could be too time consuming (we need to lookup each devid in > fs_devices list) to be done at write time. > > So I'd prefer just to remove the devid check. > > We already have dev_extent verification, so even we have corrupted > devid, we can detect it at mount time. > Thus even we have a devid corrupted by bitflip, we can still detect it, > although not by tree-checker. That works too. I was just thinking we could use the actual observed max as the max value (and adjust it as we add devices). It grows less useful if users actually do create randomly assigned devids, but that's not what the official tools do so it should be effective in most cases. -Jeff > Thanks, > Qu > >> >> -Jeff >> >>> So crazy scripts can't bump devid to that high value easily, while we can >>> still detect obviously wrong devid. >>> >>> Reported-by: Anand Jain <anand.jain@oracle.com> >>> Fixes: ab4ba2e13346 ("btrfs: tree-checker: Verify dev item") >>> Signed-off-by: Qu Wenruo <wqu@suse.com> >>> --- >>> fs/btrfs/tree-checker.c | 9 +++++++-- >>> 1 file changed, 7 insertions(+), 2 deletions(-) >>> >>> diff --git a/fs/btrfs/tree-checker.c b/fs/btrfs/tree-checker.c >>> index 43e488f5d063..f9d24f01801e 100644 >>> --- a/fs/btrfs/tree-checker.c >>> +++ b/fs/btrfs/tree-checker.c >>> @@ -686,9 +686,14 @@ 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); >>> + /* >>> + * Btrfs doesn't really reuse devid, thus devid can increase to any >>> + * value, but we don't believe a devid higher than (1<<32) is really >>> + * valid. This could at least detect bitflip at the higher >>> + * 32 bits while still consider high devid valid. >>> + */ >>> + u64 max_devid = (1ULL << 32); >>> >>> if (key->objectid != BTRFS_DEV_ITEMS_OBJECTID) { >>> dev_item_err(leaf, slot, >>> >> >> >
diff --git a/fs/btrfs/tree-checker.c b/fs/btrfs/tree-checker.c index 43e488f5d063..f9d24f01801e 100644 --- a/fs/btrfs/tree-checker.c +++ b/fs/btrfs/tree-checker.c @@ -686,9 +686,14 @@ 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); + /* + * Btrfs doesn't really reuse devid, thus devid can increase to any + * value, but we don't believe a devid higher than (1<<32) is really + * valid. This could at least detect bitflip at the higher + * 32 bits while still consider high devid valid. + */ + u64 max_devid = (1ULL << 32); if (key->objectid != BTRFS_DEV_ITEMS_OBJECTID) { dev_item_err(leaf, slot,
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. But we still don't want to give up the devid check, so here we compromise by setting a larger devid upper limit, 1<<32. So crazy scripts can't bump devid to that high value easily, while we can still detect obviously wrong devid. Reported-by: Anand Jain <anand.jain@oracle.com> Fixes: ab4ba2e13346 ("btrfs: tree-checker: Verify dev item") Signed-off-by: Qu Wenruo <wqu@suse.com> --- fs/btrfs/tree-checker.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-)