Message ID | 20191209105435.36041-2-wqu@suse.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | [1/4] btrfs: tree-checker: Clean up fs_info parameter from error message wrapper | expand |
On 9.12.19 г. 12:54 ч., Qu Wenruo wrote: > Inode key check is not as easy as several lines, and it will be called > in more than one location (INODE_ITEM check and > DIR_ITEM/DIR_INDEX/XATTR_ITEM location key check). > > So here refactor such check into check_inode_key(). > > And add extra checks for XATTR_ITEM. > > Signed-off-by: Qu Wenruo <wqu@suse.com> Reviewed-by: Nikolay Borisov <nborisov@suse.com>
On 2019/12/9 6:54 PM, Qu Wenruo wrote: > Inode key check is not as easy as several lines, and it will be called > in more than one location (INODE_ITEM check and > DIR_ITEM/DIR_INDEX/XATTR_ITEM location key check). > > So here refactor such check into check_inode_key(). > > And add extra checks for XATTR_ITEM. > > Signed-off-by: Qu Wenruo <wqu@suse.com> > --- > fs/btrfs/tree-checker.c | 77 +++++++++++++++++++++++++++++++---------- > 1 file changed, 59 insertions(+), 18 deletions(-) > > diff --git a/fs/btrfs/tree-checker.c b/fs/btrfs/tree-checker.c > index 6cb49c75c5e1..68dad9ec38dd 100644 > --- a/fs/btrfs/tree-checker.c > +++ b/fs/btrfs/tree-checker.c > @@ -359,6 +359,60 @@ static int check_csum_item(struct extent_buffer *leaf, struct btrfs_key *key, > return 0; > } > > +/* Inode item error output has the same format as dir_item_err() */ > +#define inode_item_err(eb, slot, fmt, ...) \ > + dir_item_err(eb, slot, fmt, __VA_ARGS__) > + > +static int check_inode_key(struct extent_buffer *leaf, struct btrfs_key *key, > + int slot) The function name is confusing to me. It checks xattr which is not inode related obviously. I saw the 4th patch. How about introduction of new function check_location_key(), then calls check_root_key() , check_inode_key() and checks xatrr case inside? Others in the patchset seem fine to me. Thanks > +{ > + struct btrfs_key item_key; > + bool is_inode_item; > + > + btrfs_item_key_to_cpu(leaf, &item_key, slot); > + is_inode_item = (item_key.type == BTRFS_INODE_ITEM_KEY); > + > + /* For XATTR_ITEM, location key should be all 0 */ > + if (item_key.type == BTRFS_XATTR_ITEM_KEY) { > + if (key->type != 0 || key->objectid != 0 || key->offset != 0) > + return -EUCLEAN; > + return 0; > + } > + > + if ((key->objectid < BTRFS_FIRST_FREE_OBJECTID || > + key->objectid > BTRFS_LAST_FREE_OBJECTID) && > + key->objectid != BTRFS_ROOT_TREE_DIR_OBJECTID && > + key->objectid != BTRFS_FREE_INO_OBJECTID) { > + if (is_inode_item) > + generic_err(leaf, slot, > + "invalid key objectid: has %llu expect %llu or [%llu, %llu] or %llu", > + key->objectid, BTRFS_ROOT_TREE_DIR_OBJECTID, > + BTRFS_FIRST_FREE_OBJECTID, > + BTRFS_LAST_FREE_OBJECTID, > + BTRFS_FREE_INO_OBJECTID); > + else > + dir_item_err(leaf, slot, > +"invalid location key objectid: has %llu expect %llu or [%llu, %llu] or %llu", > + key->objectid, BTRFS_ROOT_TREE_DIR_OBJECTID, > + BTRFS_FIRST_FREE_OBJECTID, > + BTRFS_LAST_FREE_OBJECTID, > + BTRFS_FREE_INO_OBJECTID); > + return -EUCLEAN; > + } > + if (key->offset != 0) { > + if (is_inode_item) > + inode_item_err(leaf, slot, > + "invalid key offset: has %llu expect 0", > + key->offset); > + else > + dir_item_err(leaf, slot, > + "invalid location key offset:has %llu expect 0", > + key->offset); > + return -EUCLEAN; > + } > + return 0; > +} > + > static int check_dir_item(struct extent_buffer *leaf, > struct btrfs_key *key, struct btrfs_key *prev_key, > int slot) > @@ -798,25 +852,12 @@ static int check_inode_item(struct extent_buffer *leaf, > u64 super_gen = btrfs_super_generation(fs_info->super_copy); > u32 valid_mask = (S_IFMT | S_ISUID | S_ISGID | S_ISVTX | 0777); > u32 mode; > + int ret; > + > + ret = check_inode_key(leaf, key, slot); > + if (ret < 0) > + return ret; > > - if ((key->objectid < BTRFS_FIRST_FREE_OBJECTID || > - key->objectid > BTRFS_LAST_FREE_OBJECTID) && > - key->objectid != BTRFS_ROOT_TREE_DIR_OBJECTID && > - key->objectid != BTRFS_FREE_INO_OBJECTID) { > - generic_err(leaf, slot, > - "invalid key objectid: has %llu expect %llu or [%llu, %llu] or %llu", > - key->objectid, BTRFS_ROOT_TREE_DIR_OBJECTID, > - BTRFS_FIRST_FREE_OBJECTID, > - BTRFS_LAST_FREE_OBJECTID, > - BTRFS_FREE_INO_OBJECTID); > - return -EUCLEAN; > - } > - if (key->offset != 0) { > - inode_item_err(leaf, slot, > - "invalid key offset: has %llu expect 0", > - key->offset); > - return -EUCLEAN; > - } > iitem = btrfs_item_ptr(leaf, slot, struct btrfs_inode_item); > > /* Here we use super block generation + 1 to handle log tree */ >
On 2019/12/10 下午10:42, Su Yue wrote: > > > On 2019/12/9 6:54 PM, Qu Wenruo wrote: >> Inode key check is not as easy as several lines, and it will be called >> in more than one location (INODE_ITEM check and >> DIR_ITEM/DIR_INDEX/XATTR_ITEM location key check). >> >> So here refactor such check into check_inode_key(). >> >> And add extra checks for XATTR_ITEM. >> >> Signed-off-by: Qu Wenruo <wqu@suse.com> >> --- >> fs/btrfs/tree-checker.c | 77 +++++++++++++++++++++++++++++++---------- >> 1 file changed, 59 insertions(+), 18 deletions(-) >> >> diff --git a/fs/btrfs/tree-checker.c b/fs/btrfs/tree-checker.c >> index 6cb49c75c5e1..68dad9ec38dd 100644 >> --- a/fs/btrfs/tree-checker.c >> +++ b/fs/btrfs/tree-checker.c >> @@ -359,6 +359,60 @@ static int check_csum_item(struct extent_buffer >> *leaf, struct btrfs_key *key, >> return 0; >> } >> >> +/* Inode item error output has the same format as dir_item_err() */ >> +#define inode_item_err(eb, slot, fmt, ...) \ >> + dir_item_err(eb, slot, fmt, __VA_ARGS__) >> + >> +static int check_inode_key(struct extent_buffer *leaf, struct >> btrfs_key *key, >> + int slot) > > The function name is confusing to me. It checks xattr which is not > inode related obviously. If renamed to check_location_key() then the call site in check_inode_item() will be very strange... Thanks, Qu > > I saw the 4th patch. How about introduction of new function > check_location_key(), then calls check_root_key() , check_inode_key() > and checks xatrr case inside? > > Others in the patchset seem fine to me. > > > Thanks >> +{ >> + struct btrfs_key item_key; >> + bool is_inode_item; >> + >> + btrfs_item_key_to_cpu(leaf, &item_key, slot); >> + is_inode_item = (item_key.type == BTRFS_INODE_ITEM_KEY); >> + >> + /* For XATTR_ITEM, location key should be all 0 */ >> + if (item_key.type == BTRFS_XATTR_ITEM_KEY) { >> + if (key->type != 0 || key->objectid != 0 || key->offset != 0) >> + return -EUCLEAN; >> + return 0; >> + } >> + >> + if ((key->objectid < BTRFS_FIRST_FREE_OBJECTID || >> + key->objectid > BTRFS_LAST_FREE_OBJECTID) && >> + key->objectid != BTRFS_ROOT_TREE_DIR_OBJECTID && >> + key->objectid != BTRFS_FREE_INO_OBJECTID) { >> + if (is_inode_item) >> + generic_err(leaf, slot, >> + "invalid key objectid: has %llu expect %llu or [%llu, %llu] or >> %llu", >> + key->objectid, BTRFS_ROOT_TREE_DIR_OBJECTID, >> + BTRFS_FIRST_FREE_OBJECTID, >> + BTRFS_LAST_FREE_OBJECTID, >> + BTRFS_FREE_INO_OBJECTID); >> + else >> + dir_item_err(leaf, slot, >> +"invalid location key objectid: has %llu expect %llu or [%llu, %llu] >> or %llu", >> + key->objectid, BTRFS_ROOT_TREE_DIR_OBJECTID, >> + BTRFS_FIRST_FREE_OBJECTID, >> + BTRFS_LAST_FREE_OBJECTID, >> + BTRFS_FREE_INO_OBJECTID); >> + return -EUCLEAN; >> + } >> + if (key->offset != 0) { >> + if (is_inode_item) >> + inode_item_err(leaf, slot, >> + "invalid key offset: has %llu expect 0", >> + key->offset); >> + else >> + dir_item_err(leaf, slot, >> + "invalid location key offset:has %llu expect 0", >> + key->offset); >> + return -EUCLEAN; >> + } >> + return 0; >> +} >> + >> static int check_dir_item(struct extent_buffer *leaf, >> struct btrfs_key *key, struct btrfs_key *prev_key, >> int slot) >> @@ -798,25 +852,12 @@ static int check_inode_item(struct extent_buffer >> *leaf, >> u64 super_gen = btrfs_super_generation(fs_info->super_copy); >> u32 valid_mask = (S_IFMT | S_ISUID | S_ISGID | S_ISVTX | 0777); >> u32 mode; >> + int ret; >> + >> + ret = check_inode_key(leaf, key, slot); >> + if (ret < 0) >> + return ret; >> >> - if ((key->objectid < BTRFS_FIRST_FREE_OBJECTID || >> - key->objectid > BTRFS_LAST_FREE_OBJECTID) && >> - key->objectid != BTRFS_ROOT_TREE_DIR_OBJECTID && >> - key->objectid != BTRFS_FREE_INO_OBJECTID) { >> - generic_err(leaf, slot, >> - "invalid key objectid: has %llu expect %llu or [%llu, %llu] or >> %llu", >> - key->objectid, BTRFS_ROOT_TREE_DIR_OBJECTID, >> - BTRFS_FIRST_FREE_OBJECTID, >> - BTRFS_LAST_FREE_OBJECTID, >> - BTRFS_FREE_INO_OBJECTID); >> - return -EUCLEAN; >> - } >> - if (key->offset != 0) { >> - inode_item_err(leaf, slot, >> - "invalid key offset: has %llu expect 0", >> - key->offset); >> - return -EUCLEAN; >> - } >> iitem = btrfs_item_ptr(leaf, slot, struct btrfs_inode_item); >> >> /* Here we use super block generation + 1 to handle log tree */ >>
diff --git a/fs/btrfs/tree-checker.c b/fs/btrfs/tree-checker.c index 6cb49c75c5e1..68dad9ec38dd 100644 --- a/fs/btrfs/tree-checker.c +++ b/fs/btrfs/tree-checker.c @@ -359,6 +359,60 @@ static int check_csum_item(struct extent_buffer *leaf, struct btrfs_key *key, return 0; } +/* Inode item error output has the same format as dir_item_err() */ +#define inode_item_err(eb, slot, fmt, ...) \ + dir_item_err(eb, slot, fmt, __VA_ARGS__) + +static int check_inode_key(struct extent_buffer *leaf, struct btrfs_key *key, + int slot) +{ + struct btrfs_key item_key; + bool is_inode_item; + + btrfs_item_key_to_cpu(leaf, &item_key, slot); + is_inode_item = (item_key.type == BTRFS_INODE_ITEM_KEY); + + /* For XATTR_ITEM, location key should be all 0 */ + if (item_key.type == BTRFS_XATTR_ITEM_KEY) { + if (key->type != 0 || key->objectid != 0 || key->offset != 0) + return -EUCLEAN; + return 0; + } + + if ((key->objectid < BTRFS_FIRST_FREE_OBJECTID || + key->objectid > BTRFS_LAST_FREE_OBJECTID) && + key->objectid != BTRFS_ROOT_TREE_DIR_OBJECTID && + key->objectid != BTRFS_FREE_INO_OBJECTID) { + if (is_inode_item) + generic_err(leaf, slot, + "invalid key objectid: has %llu expect %llu or [%llu, %llu] or %llu", + key->objectid, BTRFS_ROOT_TREE_DIR_OBJECTID, + BTRFS_FIRST_FREE_OBJECTID, + BTRFS_LAST_FREE_OBJECTID, + BTRFS_FREE_INO_OBJECTID); + else + dir_item_err(leaf, slot, +"invalid location key objectid: has %llu expect %llu or [%llu, %llu] or %llu", + key->objectid, BTRFS_ROOT_TREE_DIR_OBJECTID, + BTRFS_FIRST_FREE_OBJECTID, + BTRFS_LAST_FREE_OBJECTID, + BTRFS_FREE_INO_OBJECTID); + return -EUCLEAN; + } + if (key->offset != 0) { + if (is_inode_item) + inode_item_err(leaf, slot, + "invalid key offset: has %llu expect 0", + key->offset); + else + dir_item_err(leaf, slot, + "invalid location key offset:has %llu expect 0", + key->offset); + return -EUCLEAN; + } + return 0; +} + static int check_dir_item(struct extent_buffer *leaf, struct btrfs_key *key, struct btrfs_key *prev_key, int slot) @@ -798,25 +852,12 @@ static int check_inode_item(struct extent_buffer *leaf, u64 super_gen = btrfs_super_generation(fs_info->super_copy); u32 valid_mask = (S_IFMT | S_ISUID | S_ISGID | S_ISVTX | 0777); u32 mode; + int ret; + + ret = check_inode_key(leaf, key, slot); + if (ret < 0) + return ret; - if ((key->objectid < BTRFS_FIRST_FREE_OBJECTID || - key->objectid > BTRFS_LAST_FREE_OBJECTID) && - key->objectid != BTRFS_ROOT_TREE_DIR_OBJECTID && - key->objectid != BTRFS_FREE_INO_OBJECTID) { - generic_err(leaf, slot, - "invalid key objectid: has %llu expect %llu or [%llu, %llu] or %llu", - key->objectid, BTRFS_ROOT_TREE_DIR_OBJECTID, - BTRFS_FIRST_FREE_OBJECTID, - BTRFS_LAST_FREE_OBJECTID, - BTRFS_FREE_INO_OBJECTID); - return -EUCLEAN; - } - if (key->offset != 0) { - inode_item_err(leaf, slot, - "invalid key offset: has %llu expect 0", - key->offset); - return -EUCLEAN; - } iitem = btrfs_item_ptr(leaf, slot, struct btrfs_inode_item); /* Here we use super block generation + 1 to handle log tree */
Inode key check is not as easy as several lines, and it will be called in more than one location (INODE_ITEM check and DIR_ITEM/DIR_INDEX/XATTR_ITEM location key check). So here refactor such check into check_inode_key(). And add extra checks for XATTR_ITEM. Signed-off-by: Qu Wenruo <wqu@suse.com> --- fs/btrfs/tree-checker.c | 77 +++++++++++++++++++++++++++++++---------- 1 file changed, 59 insertions(+), 18 deletions(-)