| Message ID | 20190826074039.28517-3-wqu@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | btrfs: tree-checker: Add checks to detect missing INODE_ITEM | expand |
On 26.08.19 г. 10:40 ч., Qu Wenruo wrote: > For INODE_REF we will check: > - Objectid (ino) against previous key > To detect missing INODE_ITEM. > > - No overflow/padding in the data payload > Much like DIR_ITEM, but with less members to check. > > Signed-off-by: Qu Wenruo <wqu@suse.com> > --- > fs/btrfs/tree-checker.c | 53 +++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 53 insertions(+) > > diff --git a/fs/btrfs/tree-checker.c b/fs/btrfs/tree-checker.c > index 636ce1b4566e..3ce447eb591c 100644 > --- a/fs/btrfs/tree-checker.c > +++ b/fs/btrfs/tree-checker.c > @@ -842,6 +842,56 @@ static int check_inode_item(struct extent_buffer *leaf, > return 0; > } > > +#define inode_ref_err(fs_info, eb, slot, fmt, ...) \ > + inode_item_err(fs_info, eb, slot, fmt, __VA_ARGS__) This define doesn't bring anything, just opencode the call to inode_item_err directly. > +static int check_inode_ref(struct extent_buffer *leaf, > + struct btrfs_key *key, struct btrfs_key *prev_key, > + int slot) > +{ > + struct btrfs_inode_ref *iref; > + unsigned long ptr; > + unsigned long end; > + > + /* namelen can't be 0, so item_size == sizeof() is also invalid */ > + if (btrfs_item_size_nr(leaf, slot) <= sizeof(*iref)) { > + inode_ref_err(fs_info, leaf, slot, > + "invalid item size, have %u expect (%zu, %u)", > + btrfs_item_size_nr(leaf, slot), > + sizeof(*iref), BTRFS_LEAF_DATA_SIZE(leaf->fs_info)); > + return -EUCLEAN; > + } > + > + ptr = btrfs_item_ptr_offset(leaf, slot); > + end = ptr + btrfs_item_size_nr(leaf, slot); > + while (ptr < end) { > + u16 namelen; > + > + if (ptr + sizeof(iref) > end) { > + inode_ref_err(fs_info, leaf, slot, > + "inode ref overflow, ptr %lu end %lu inode_ref_size %zu", > + ptr, end, sizeof(iref)); > + return -EUCLEAN; > + } > + > + iref = (struct btrfs_inode_ref *)ptr; > + namelen = btrfs_inode_ref_name_len(leaf, iref); > + if (ptr + sizeof(*iref) + namelen > end) { > + inode_ref_err(fs_info, leaf, slot, > + "inode ref overflow, ptr %lu end %lu namelen %u", > + ptr, end, namelen); > + return -EUCLEAN; > + } > + > + /* > + * NOTE: In theory we should record all found index number > + * to find any duplicated index. But that will be too time > + * consuming for inodes with too many hard links. > + */ > + ptr += sizeof(*iref) + namelen; > + } > + return 0; > +} > + > /* > * Common point to switch the item-specific validation. > */ > @@ -864,6 +914,9 @@ static int check_leaf_item(struct extent_buffer *leaf, > case BTRFS_XATTR_ITEM_KEY: > ret = check_dir_item(leaf, key, prev_key, slot); > break; > + case BTRFS_INODE_REF_KEY: > + ret = check_inode_ref(leaf, key, prev_key, slot); > + break; > case BTRFS_BLOCK_GROUP_ITEM_KEY: > ret = check_block_group_item(leaf, key, slot); > break; >
On 2019/8/26 下午7:45, Nikolay Borisov wrote: > > > On 26.08.19 г. 10:40 ч., Qu Wenruo wrote: >> For INODE_REF we will check: >> - Objectid (ino) against previous key >> To detect missing INODE_ITEM. >> >> - No overflow/padding in the data payload >> Much like DIR_ITEM, but with less members to check. >> >> Signed-off-by: Qu Wenruo <wqu@suse.com> >> --- >> fs/btrfs/tree-checker.c | 53 +++++++++++++++++++++++++++++++++++++++++ >> 1 file changed, 53 insertions(+) >> >> diff --git a/fs/btrfs/tree-checker.c b/fs/btrfs/tree-checker.c >> index 636ce1b4566e..3ce447eb591c 100644 >> --- a/fs/btrfs/tree-checker.c >> +++ b/fs/btrfs/tree-checker.c >> @@ -842,6 +842,56 @@ static int check_inode_item(struct extent_buffer *leaf, >> return 0; >> } >> >> +#define inode_ref_err(fs_info, eb, slot, fmt, ...) \ >> + inode_item_err(fs_info, eb, slot, fmt, __VA_ARGS__) > > This define doesn't bring anything, just opencode the call to > inode_item_err directly. I could argue we that in an inode ref context, using a inode_item_err() doesn't look right. And since it's doesn't do any hurt, I prefer to make the error message parse to match the context. Thanks, Qu > >> +static int check_inode_ref(struct extent_buffer *leaf, >> + struct btrfs_key *key, struct btrfs_key *prev_key, >> + int slot) >> +{ >> + struct btrfs_inode_ref *iref; >> + unsigned long ptr; >> + unsigned long end; >> + >> + /* namelen can't be 0, so item_size == sizeof() is also invalid */ >> + if (btrfs_item_size_nr(leaf, slot) <= sizeof(*iref)) { >> + inode_ref_err(fs_info, leaf, slot, >> + "invalid item size, have %u expect (%zu, %u)", >> + btrfs_item_size_nr(leaf, slot), >> + sizeof(*iref), BTRFS_LEAF_DATA_SIZE(leaf->fs_info)); >> + return -EUCLEAN; >> + } >> + >> + ptr = btrfs_item_ptr_offset(leaf, slot); >> + end = ptr + btrfs_item_size_nr(leaf, slot); >> + while (ptr < end) { >> + u16 namelen; >> + >> + if (ptr + sizeof(iref) > end) { >> + inode_ref_err(fs_info, leaf, slot, >> + "inode ref overflow, ptr %lu end %lu inode_ref_size %zu", >> + ptr, end, sizeof(iref)); >> + return -EUCLEAN; >> + } >> + >> + iref = (struct btrfs_inode_ref *)ptr; >> + namelen = btrfs_inode_ref_name_len(leaf, iref); >> + if (ptr + sizeof(*iref) + namelen > end) { >> + inode_ref_err(fs_info, leaf, slot, >> + "inode ref overflow, ptr %lu end %lu namelen %u", >> + ptr, end, namelen); >> + return -EUCLEAN; >> + } >> + >> + /* >> + * NOTE: In theory we should record all found index number >> + * to find any duplicated index. But that will be too time >> + * consuming for inodes with too many hard links. >> + */ >> + ptr += sizeof(*iref) + namelen; >> + } >> + return 0; >> +} >> + >> /* >> * Common point to switch the item-specific validation. >> */ >> @@ -864,6 +914,9 @@ static int check_leaf_item(struct extent_buffer *leaf, >> case BTRFS_XATTR_ITEM_KEY: >> ret = check_dir_item(leaf, key, prev_key, slot); >> break; >> + case BTRFS_INODE_REF_KEY: >> + ret = check_inode_ref(leaf, key, prev_key, slot); >> + break; >> case BTRFS_BLOCK_GROUP_ITEM_KEY: >> ret = check_block_group_item(leaf, key, slot); >> break; >>
On Mon, Aug 26, 2019 at 07:50:03PM +0800, Qu Wenruo wrote: > > > On 2019/8/26 下午7:45, Nikolay Borisov wrote: > > > > > > On 26.08.19 г. 10:40 ч., Qu Wenruo wrote: > >> For INODE_REF we will check: > >> - Objectid (ino) against previous key > >> To detect missing INODE_ITEM. > >> > >> - No overflow/padding in the data payload > >> Much like DIR_ITEM, but with less members to check. > >> > >> Signed-off-by: Qu Wenruo <wqu@suse.com> > >> --- > >> fs/btrfs/tree-checker.c | 53 +++++++++++++++++++++++++++++++++++++++++ > >> 1 file changed, 53 insertions(+) > >> > >> diff --git a/fs/btrfs/tree-checker.c b/fs/btrfs/tree-checker.c > >> index 636ce1b4566e..3ce447eb591c 100644 > >> --- a/fs/btrfs/tree-checker.c > >> +++ b/fs/btrfs/tree-checker.c > >> @@ -842,6 +842,56 @@ static int check_inode_item(struct extent_buffer *leaf, > >> return 0; > >> } > >> > >> +#define inode_ref_err(fs_info, eb, slot, fmt, ...) \ > >> + inode_item_err(fs_info, eb, slot, fmt, __VA_ARGS__) > > > > This define doesn't bring anything, just opencode the call to > > inode_item_err directly. > > I could argue we that in an inode ref context, using a inode_item_err() > doesn't look right. > > And since it's doesn't do any hurt, I prefer to make the error message > parse to match the context. I agree the alias inode_ref_err does not hurt, there's no penatly in the code so for sake of readability let's do it.
On Mon, Aug 26, 2019 at 03:40:39PM +0800, Qu Wenruo wrote: > For INODE_REF we will check: > - Objectid (ino) against previous key > To detect missing INODE_ITEM. > > - No overflow/padding in the data payload > Much like DIR_ITEM, but with less members to check. > > Signed-off-by: Qu Wenruo <wqu@suse.com> > --- > fs/btrfs/tree-checker.c | 53 +++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 53 insertions(+) > > diff --git a/fs/btrfs/tree-checker.c b/fs/btrfs/tree-checker.c > index 636ce1b4566e..3ce447eb591c 100644 > --- a/fs/btrfs/tree-checker.c > +++ b/fs/btrfs/tree-checker.c > @@ -842,6 +842,56 @@ static int check_inode_item(struct extent_buffer *leaf, > return 0; > } > > +#define inode_ref_err(fs_info, eb, slot, fmt, ...) \ > + inode_item_err(fs_info, eb, slot, fmt, __VA_ARGS__) I've changed that to #define inode_ref_err(fs_info, eb, slot, fmt, args...) \ inode_item_err(fs_info, eb, slot, fmt, ##args) as this is the common style for the variable macro args used in btrfs.
diff --git a/fs/btrfs/tree-checker.c b/fs/btrfs/tree-checker.c index 636ce1b4566e..3ce447eb591c 100644 --- a/fs/btrfs/tree-checker.c +++ b/fs/btrfs/tree-checker.c @@ -842,6 +842,56 @@ static int check_inode_item(struct extent_buffer *leaf, return 0; } +#define inode_ref_err(fs_info, eb, slot, fmt, ...) \ + inode_item_err(fs_info, eb, slot, fmt, __VA_ARGS__) +static int check_inode_ref(struct extent_buffer *leaf, + struct btrfs_key *key, struct btrfs_key *prev_key, + int slot) +{ + struct btrfs_inode_ref *iref; + unsigned long ptr; + unsigned long end; + + /* namelen can't be 0, so item_size == sizeof() is also invalid */ + if (btrfs_item_size_nr(leaf, slot) <= sizeof(*iref)) { + inode_ref_err(fs_info, leaf, slot, + "invalid item size, have %u expect (%zu, %u)", + btrfs_item_size_nr(leaf, slot), + sizeof(*iref), BTRFS_LEAF_DATA_SIZE(leaf->fs_info)); + return -EUCLEAN; + } + + ptr = btrfs_item_ptr_offset(leaf, slot); + end = ptr + btrfs_item_size_nr(leaf, slot); + while (ptr < end) { + u16 namelen; + + if (ptr + sizeof(iref) > end) { + inode_ref_err(fs_info, leaf, slot, + "inode ref overflow, ptr %lu end %lu inode_ref_size %zu", + ptr, end, sizeof(iref)); + return -EUCLEAN; + } + + iref = (struct btrfs_inode_ref *)ptr; + namelen = btrfs_inode_ref_name_len(leaf, iref); + if (ptr + sizeof(*iref) + namelen > end) { + inode_ref_err(fs_info, leaf, slot, + "inode ref overflow, ptr %lu end %lu namelen %u", + ptr, end, namelen); + return -EUCLEAN; + } + + /* + * NOTE: In theory we should record all found index number + * to find any duplicated index. But that will be too time + * consuming for inodes with too many hard links. + */ + ptr += sizeof(*iref) + namelen; + } + return 0; +} + /* * Common point to switch the item-specific validation. */ @@ -864,6 +914,9 @@ static int check_leaf_item(struct extent_buffer *leaf, case BTRFS_XATTR_ITEM_KEY: ret = check_dir_item(leaf, key, prev_key, slot); break; + case BTRFS_INODE_REF_KEY: + ret = check_inode_ref(leaf, key, prev_key, slot); + break; case BTRFS_BLOCK_GROUP_ITEM_KEY: ret = check_block_group_item(leaf, key, slot); break;
For INODE_REF we will check: - Objectid (ino) against previous key To detect missing INODE_ITEM. - No overflow/padding in the data payload Much like DIR_ITEM, but with less members to check. Signed-off-by: Qu Wenruo <wqu@suse.com> --- fs/btrfs/tree-checker.c | 53 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 53 insertions(+)