| Message ID | 20171108005426.17903-3-wqu@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 8.11.2017 02:54, Qu Wenruo wrote: > Add checker for dir item, for key types DIR_ITEM, DIR_INDEX and > XATTR_ITEM. > > This checker does comprehensive check for: > 1) dir_item header and its data size > Against item boundary and maximum name/xattr length. > This part is mostly the same as old verify_dir_item(). > > 2) dir_type > Against maximum file types, and against key type. > Since XATTR key should only have FT_XATTR dir item, and normal dir > item type should not have XATTR key. > > The check between key->type and dir_type is newly introduced by this > patch. > > 3) name hash > For XATTR and DIR_ITEM key, key->offset is name hash (crc32). > Check the hash of name against key to ensure it's correct. > > The name hash check is only found in btrfs-progs before this patch. > > Signed-off-by: Qu Wenruo <wqu@suse.com> > --- > fs/btrfs/tree-checker.c | 141 ++++++++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 141 insertions(+) > I'm gonna 'hijack' this patch to discuss something - we do return the correct EUCLEAN status from the leaf checker, however the only place where it's called is in btree_readpage_end_io_hook and if the check fails we only return -EIO. I wonder whether want to propagate the EUCLEAN from there, any thoughts? -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 2017年11月08日 15:59, Nikolay Borisov wrote: > > > On 8.11.2017 02:54, Qu Wenruo wrote: >> Add checker for dir item, for key types DIR_ITEM, DIR_INDEX and >> XATTR_ITEM. >> >> This checker does comprehensive check for: >> 1) dir_item header and its data size >> Against item boundary and maximum name/xattr length. >> This part is mostly the same as old verify_dir_item(). >> >> 2) dir_type >> Against maximum file types, and against key type. >> Since XATTR key should only have FT_XATTR dir item, and normal dir >> item type should not have XATTR key. >> >> The check between key->type and dir_type is newly introduced by this >> patch. >> >> 3) name hash >> For XATTR and DIR_ITEM key, key->offset is name hash (crc32). >> Check the hash of name against key to ensure it's correct. >> >> The name hash check is only found in btrfs-progs before this patch. >> >> Signed-off-by: Qu Wenruo <wqu@suse.com> >> --- >> fs/btrfs/tree-checker.c | 141 ++++++++++++++++++++++++++++++++++++++++++++++++ >> 1 file changed, 141 insertions(+) >> > > > I'm gonna 'hijack' this patch to discuss something - we do return the > correct EUCLEAN status from the leaf checker, however the only place > where it's called is in btree_readpage_end_io_hook and if the check > fails we only return -EIO. I wonder whether want to propagate the > EUCLEAN from there, any thoughts? In this particular case, I think returning -EUCLEAN is *marginally* better. Much more detailed info from block layer/csum verifier/tree-checker makes more sense than just a single return value. Returning any error to make the reading fail, and cause a graceful error is good enough. The returning values between EUCLEAN and EIO makes difference, but less meaningful than specific output from tree-checker. On the other hand, if in case where return value is the only meaningful output to user-space (that's to say no much meaningful extra info like kernel message), then return value makes sense. But even in that case, the return value can't give much detailed info. Here are several common errno: EIO ENOSPC EUCLEAN ENOMEM ENOENT We can only express at most less than 10 error types. There are tons of places where thing can go wrong. What we can do is just to try our best to classify these errors, and hopes it makes sense for end user. While more meaningful error/warning message will make both developer and end user happier. Thanks, Qu > > > -- > To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html >
On Wed, Nov 08, 2017 at 08:54:25AM +0800, Qu Wenruo wrote: > Add checker for dir item, for key types DIR_ITEM, DIR_INDEX and > XATTR_ITEM. > > This checker does comprehensive check for: > 1) dir_item header and its data size > Against item boundary and maximum name/xattr length. > This part is mostly the same as old verify_dir_item(). > > 2) dir_type > Against maximum file types, and against key type. > Since XATTR key should only have FT_XATTR dir item, and normal dir > item type should not have XATTR key. > > The check between key->type and dir_type is newly introduced by this > patch. > > 3) name hash > For XATTR and DIR_ITEM key, key->offset is name hash (crc32). > Check the hash of name against key to ensure it's correct. > > The name hash check is only found in btrfs-progs before this patch. > > Signed-off-by: Qu Wenruo <wqu@suse.com> Reviewed-by: David Sterba <dsterba@suse.com> -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Nov 08, 2017 at 09:59:39AM +0200, Nikolay Borisov wrote: > > > On 8.11.2017 02:54, Qu Wenruo wrote: > > Add checker for dir item, for key types DIR_ITEM, DIR_INDEX and > > XATTR_ITEM. > > > > This checker does comprehensive check for: > > 1) dir_item header and its data size > > Against item boundary and maximum name/xattr length. > > This part is mostly the same as old verify_dir_item(). > > > > 2) dir_type > > Against maximum file types, and against key type. > > Since XATTR key should only have FT_XATTR dir item, and normal dir > > item type should not have XATTR key. > > > > The check between key->type and dir_type is newly introduced by this > > patch. > > > > 3) name hash > > For XATTR and DIR_ITEM key, key->offset is name hash (crc32). > > Check the hash of name against key to ensure it's correct. > > > > The name hash check is only found in btrfs-progs before this patch. > > > > Signed-off-by: Qu Wenruo <wqu@suse.com> > > --- > > fs/btrfs/tree-checker.c | 141 ++++++++++++++++++++++++++++++++++++++++++++++++ > > 1 file changed, 141 insertions(+) > > > > > I'm gonna 'hijack' this patch to discuss something - we do return the > correct EUCLEAN status from the leaf checker, however the only place > where it's called is in btree_readpage_end_io_hook and if the check > fails we only return -EIO. I wonder whether want to propagate the > EUCLEAN from there, any thoughts? The @ret from readpage_end_io_hook won't go upper layer as it's wrapped in end_bio_extent_readpage(). The readpage/readpages will just check page's uptodate bit and error bit. Thanks, -liubo -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/fs/btrfs/tree-checker.c b/fs/btrfs/tree-checker.c index ce4ed6ec8f39..66dac0a4b01f 100644 --- a/fs/btrfs/tree-checker.c +++ b/fs/btrfs/tree-checker.c @@ -30,6 +30,7 @@ #include "tree-checker.h" #include "disk-io.h" #include "compression.h" +#include "hash.h" /* * Error message should follow the following format: @@ -222,6 +223,141 @@ static int check_csum_item(struct btrfs_root *root, struct extent_buffer *leaf, return 0; } +/* + * Customized reported for dir_item, only important new info is key->objectid, + * which represents inode number + */ +__printf(4, 5) +static void dir_item_err(const struct btrfs_root *root, + const struct extent_buffer *eb, int slot, + const char *fmt, ...) +{ + struct btrfs_key key; + struct va_format vaf; + va_list args; + + btrfs_item_key_to_cpu(eb, &key, slot); + va_start(args, fmt); + + vaf.fmt = fmt; + vaf.va = &args; + + btrfs_crit(root->fs_info, + "corrupt %s: root=%llu block=%llu slot=%d ino=%llu, %pV", + btrfs_header_level(eb) == 0 ? "leaf" : "node", root->objectid, + btrfs_header_bytenr(eb), slot, key.objectid, &vaf); + va_end(args); +} + +static int check_dir_item(struct btrfs_root *root, + struct extent_buffer *leaf, + struct btrfs_key *key, int slot) +{ + struct btrfs_dir_item *di; + u32 item_size = btrfs_item_size_nr(leaf, slot); + u32 cur = 0; + + di = btrfs_item_ptr(leaf, slot, struct btrfs_dir_item); + while (cur < item_size) { + char namebuf[max(BTRFS_NAME_LEN, XATTR_NAME_MAX)]; + u32 name_len; + u32 data_len; + u32 max_name_len; + u32 total_size; + u32 name_hash; + u8 dir_type; + + /* header itself should not cross item boundary */ + if (cur + sizeof(*di) > item_size) { + dir_item_err(root, leaf, slot, + "dir item header crosses item boundary, have %lu boundary %u", + cur + sizeof(*di), item_size); + return -EUCLEAN; + } + + /* dir type check */ + dir_type = btrfs_dir_type(leaf, di); + if (dir_type >= BTRFS_FT_MAX) { + dir_item_err(root, leaf, slot, + "invalid dir item type, have %u expect [0, %u)", + dir_type, BTRFS_FT_MAX); + return -EUCLEAN; + } + + if (key->type == BTRFS_XATTR_ITEM_KEY && + dir_type != BTRFS_FT_XATTR) { + dir_item_err(root, leaf, slot, + "invalid dir item type for XATTR key, have %u expect %u", + dir_type, BTRFS_FT_XATTR); + return -EUCLEAN; + } + if (dir_type == BTRFS_FT_XATTR && + key->type != BTRFS_XATTR_ITEM_KEY) { + dir_item_err(root, leaf, slot, + "xattr dir type found for non-XATTR key"); + return -EUCLEAN; + } + if (dir_type == BTRFS_FT_XATTR) + max_name_len = XATTR_NAME_MAX; + else + max_name_len = BTRFS_NAME_LEN; + + /* Name/data length check */ + name_len = btrfs_dir_name_len(leaf, di); + data_len = btrfs_dir_data_len(leaf, di); + if (name_len > max_name_len) { + dir_item_err(root, leaf, slot, + "dir item name len too long, have %u max %u", + name_len, max_name_len); + return -EUCLEAN; + } + if (name_len + data_len > BTRFS_MAX_XATTR_SIZE(root->fs_info)) { + dir_item_err(root, leaf, slot, + "dir item name and data len too long, have %u max %u", + name_len + data_len, + BTRFS_MAX_XATTR_SIZE(root->fs_info)); + return -EUCLEAN; + } + + if (data_len && dir_type != BTRFS_FT_XATTR) { + dir_item_err(root, leaf, slot, + "dir item with invalid data len, have %u expect 0", + data_len); + return -EUCLEAN; + } + + total_size = sizeof(*di) + name_len + data_len; + + /* header and name/data should not cross item boundary */ + if (cur + total_size > item_size) { + dir_item_err(root, leaf, slot, + "dir item data crosses item boundary, have %u boundary %u", + cur + total_size, item_size); + return -EUCLEAN; + } + + /* + * Special check for XATTR/DIR_ITEM, as key->offset is name + * hash, should match its name + */ + if (key->type == BTRFS_DIR_ITEM_KEY || + key->type == BTRFS_XATTR_ITEM_KEY) { + read_extent_buffer(leaf, namebuf, + (unsigned long)(di + 1), name_len); + name_hash = btrfs_name_hash(namebuf, name_len); + if (key->offset != name_hash) { + dir_item_err(root, leaf, slot, + "name hash mismatch with key, have 0x%016x expect 0x%016llx", + name_hash, key->offset); + return -EUCLEAN; + } + } + cur += total_size; + di = (struct btrfs_dir_item *)((void *)di + total_size); + } + return 0; +} + /* * Common point to switch the item-specific validation. */ @@ -238,6 +374,11 @@ static int check_leaf_item(struct btrfs_root *root, case BTRFS_EXTENT_CSUM_KEY: ret = check_csum_item(root, leaf, key, slot); break; + case BTRFS_DIR_ITEM_KEY: + case BTRFS_DIR_INDEX_KEY: + case BTRFS_XATTR_ITEM_KEY: + ret = check_dir_item(root, leaf, key, slot); + break; } return ret; }
Add checker for dir item, for key types DIR_ITEM, DIR_INDEX and XATTR_ITEM. This checker does comprehensive check for: 1) dir_item header and its data size Against item boundary and maximum name/xattr length. This part is mostly the same as old verify_dir_item(). 2) dir_type Against maximum file types, and against key type. Since XATTR key should only have FT_XATTR dir item, and normal dir item type should not have XATTR key. The check between key->type and dir_type is newly introduced by this patch. 3) name hash For XATTR and DIR_ITEM key, key->offset is name hash (crc32). Check the hash of name against key to ensure it's correct. The name hash check is only found in btrfs-progs before this patch. Signed-off-by: Qu Wenruo <wqu@suse.com> --- fs/btrfs/tree-checker.c | 141 ++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 141 insertions(+)