| Message ID | 20201116190113.GT6756@twin.jikos.cz (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [RFC] btrfs: tree-checker: missing returns after data_ref alignment checks | expand |
On 2020/11/17 上午3:01, David Sterba wrote: > Hi Qu, > > I've found more missing return satetments in tree-checker but I'm not > sure if leaving them out was intentional. Both are for extent data_ref > item alignment checks. It could be that alignment is not a hard problem, > tough it could point to one, there's a check of the hash vs key->offset > that would catch inconsistent data. All my bad. The case is, alignment is the first sign of corruption. So if alignment is incorrect, there is no need to continue. So yeah, I forgot more return lines. > > As there's only one statement after if, this looks like it was > forgotten, but otherwise needs a comment why the returns are not there > given that the rest of the file follows the same pattern > "if / extent_err/ return -EUCLEAN". > > --- > fs/btrfs/tree-checker.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/fs/btrfs/tree-checker.c b/fs/btrfs/tree-checker.c > index 1b27242a9c0b..f3f666b343ef 100644 > --- a/fs/btrfs/tree-checker.c > +++ b/fs/btrfs/tree-checker.c > @@ -1424,6 +1424,7 @@ static int check_extent_data_ref(struct extent_buffer *leaf, > "invalid item size, have %u expect aligned to %zu for key type %u", > btrfs_item_size_nr(leaf, slot), > sizeof(*dref), key->type); > + return -EUCLEAN; For EXTENT_DATA_REF_KEY, it should only contains one or more btrfs_extent_data_ref, thus the alignment is a hard requirement. Thus the return is needed. > } > if (!IS_ALIGNED(key->objectid, leaf->fs_info->sectorsize)) { > generic_err(leaf, slot, > @@ -1452,6 +1453,7 @@ static int check_extent_data_ref(struct extent_buffer *leaf, > extent_err(leaf, slot, > "invalid extent data backref offset, have %llu expect aligned to %u", > offset, leaf->fs_info->sectorsize); > + return -EUCLEAN; Basic sector size alignment requirement. So yet another missing return. Thanks, Qu > } > } > return 0; >
On Tue, Nov 17, 2020 at 07:45:25AM +0800, Qu Wenruo wrote: > > > On 2020/11/17 上午3:01, David Sterba wrote: > > Hi Qu, > > > > I've found more missing return satetments in tree-checker but I'm not > > sure if leaving them out was intentional. Both are for extent data_ref > > item alignment checks. It could be that alignment is not a hard problem, > > tough it could point to one, there's a check of the hash vs key->offset > > that would catch inconsistent data. > > All my bad. > > The case is, alignment is the first sign of corruption. > So if alignment is incorrect, there is no need to continue. > > So yeah, I forgot more return lines. > > > > As there's only one statement after if, this looks like it was > > forgotten, but otherwise needs a comment why the returns are not there > > given that the rest of the file follows the same pattern > > "if / extent_err/ return -EUCLEAN". > > > > --- > > fs/btrfs/tree-checker.c | 2 ++ > > 1 file changed, 2 insertions(+) > > > > diff --git a/fs/btrfs/tree-checker.c b/fs/btrfs/tree-checker.c > > index 1b27242a9c0b..f3f666b343ef 100644 > > --- a/fs/btrfs/tree-checker.c > > +++ b/fs/btrfs/tree-checker.c > > @@ -1424,6 +1424,7 @@ static int check_extent_data_ref(struct extent_buffer *leaf, > > "invalid item size, have %u expect aligned to %zu for key type %u", > > btrfs_item_size_nr(leaf, slot), > > sizeof(*dref), key->type); > > + return -EUCLEAN; > > For EXTENT_DATA_REF_KEY, it should only contains one or more > btrfs_extent_data_ref, thus the alignment is a hard requirement. > > Thus the return is needed. > > } > > if (!IS_ALIGNED(key->objectid, leaf->fs_info->sectorsize)) { > > generic_err(leaf, slot, > > @@ -1452,6 +1453,7 @@ static int check_extent_data_ref(struct extent_buffer *leaf, > > extent_err(leaf, slot, > > "invalid extent data backref offset, have %llu expect aligned to %u", > > offset, leaf->fs_info->sectorsize); > > + return -EUCLEAN; > > Basic sector size alignment requirement. > So yet another missing return. Thanks, I'll resend it as a proper patch.
diff --git a/fs/btrfs/tree-checker.c b/fs/btrfs/tree-checker.c index 1b27242a9c0b..f3f666b343ef 100644 --- a/fs/btrfs/tree-checker.c +++ b/fs/btrfs/tree-checker.c @@ -1424,6 +1424,7 @@ static int check_extent_data_ref(struct extent_buffer *leaf, "invalid item size, have %u expect aligned to %zu for key type %u", btrfs_item_size_nr(leaf, slot), sizeof(*dref), key->type); + return -EUCLEAN; } if (!IS_ALIGNED(key->objectid, leaf->fs_info->sectorsize)) { generic_err(leaf, slot, @@ -1452,6 +1453,7 @@ static int check_extent_data_ref(struct extent_buffer *leaf, extent_err(leaf, slot, "invalid extent data backref offset, have %llu expect aligned to %u", offset, leaf->fs_info->sectorsize); + return -EUCLEAN; } } return 0;