| Message ID | 1470286628-4123-1-git-send-email-bo.li.liu@oracle.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
On Wed, Aug 03, 2016 at 09:57:08PM -0700, Liu Bo wrote: > Right now we treat leaf which has zero item as a valid one > because we could have an empty tree, that is, a root that is > also a leaf without any item, however, in the same case but > when the leaf is not a root, we can end up with hitting the > BUG_ON(1) in btrfs_extend_item() called by > setup_inline_extent_backref(). > > This makes us check the situation as a corruption if leaf is > not its own root. > > Signed-off-by: Liu Bo <bo.li.liu@oracle.com> > --- > fs/btrfs/disk-io.c | 22 +++++++++++++++++++++- > 1 file changed, 21 insertions(+), 1 deletion(-) > > diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c > index a5a22be..dfaeb96 100644 > --- a/fs/btrfs/disk-io.c > +++ b/fs/btrfs/disk-io.c > @@ -559,8 +559,28 @@ static noinline int check_leaf(struct btrfs_root *root, > u32 nritems = btrfs_header_nritems(leaf); > int slot; > > - if (nritems == 0) > + if (nritems == 0) { > + struct btrfs_root *r; Please don't use single letter variables. > + > + key.objectid = btrfs_header_owner(leaf); > + key.type = BTRFS_ROOT_ITEM_KEY; > + key.offset = -1ULL; While -1ULL is fine, the common style is the (u64)-1, please use that. > + > + r = btrfs_get_fs_root(root->fs_info, &key, false); I wonder how expensive that could be. check_leaf is called at the end of the read so it's just the lookup time and the time the lock is held on the radix tree. All in all it seems acceptable. > + /* > + * The only reason we also check NULL here is that during > + * open_ctree() some roots has not yet been set up. > + */ > + if (!IS_ERR_OR_NULL(r)) { > + /* if leaf is the root, then it's fine */ > + if (leaf->start != btrfs_root_bytenr(&r->root_item)) { > + CORRUPT("non-root leaf's nritems is 0", > + leaf, root, 0); > + return -EIO; > + } > + } > return 0; > + } > > /* Check the 0 item */ > if (btrfs_item_offset_nr(leaf, 0) + btrfs_item_size_nr(leaf, 0) != > -- > 2.5.5 > > -- > 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 -- 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 Tue, Aug 16, 2016 at 07:07:28PM +0200, David Sterba wrote: > On Wed, Aug 03, 2016 at 09:57:08PM -0700, Liu Bo wrote: > > Right now we treat leaf which has zero item as a valid one > > because we could have an empty tree, that is, a root that is > > also a leaf without any item, however, in the same case but > > when the leaf is not a root, we can end up with hitting the > > BUG_ON(1) in btrfs_extend_item() called by > > setup_inline_extent_backref(). > > > > This makes us check the situation as a corruption if leaf is > > not its own root. > > > > Signed-off-by: Liu Bo <bo.li.liu@oracle.com> > > --- > > fs/btrfs/disk-io.c | 22 +++++++++++++++++++++- > > 1 file changed, 21 insertions(+), 1 deletion(-) > > > > diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c > > index a5a22be..dfaeb96 100644 > > --- a/fs/btrfs/disk-io.c > > +++ b/fs/btrfs/disk-io.c > > @@ -559,8 +559,28 @@ static noinline int check_leaf(struct btrfs_root *root, > > u32 nritems = btrfs_header_nritems(leaf); > > int slot; > > > > - if (nritems == 0) > > + if (nritems == 0) { > > + struct btrfs_root *r; > > Please don't use single letter variables. OK. > > > + > > + key.objectid = btrfs_header_owner(leaf); > > + key.type = BTRFS_ROOT_ITEM_KEY; > > + key.offset = -1ULL; > > While -1ULL is fine, the common style is the (u64)-1, please use that. OK. > > > + > > + r = btrfs_get_fs_root(root->fs_info, &key, false); > > I wonder how expensive that could be. check_leaf is called at the end of > the read so it's just the lookup time and the time the lock is held on > the radix tree. All in all it seems acceptable. It should be fine, it'd read a root leaf from searching fs radix tree if it's a fs/file root while it'd get root immediately if it's not, like tree_root/extent_root/chunk_root, etc. Thanks, -liubo > > > + /* > > + * The only reason we also check NULL here is that during > > + * open_ctree() some roots has not yet been set up. > > + */ > > + if (!IS_ERR_OR_NULL(r)) { > > + /* if leaf is the root, then it's fine */ > > + if (leaf->start != btrfs_root_bytenr(&r->root_item)) { > > + CORRUPT("non-root leaf's nritems is 0", > > + leaf, root, 0); > > + return -EIO; > > + } > > + } > > return 0; > > + } > > > > /* Check the 0 item */ > > if (btrfs_item_offset_nr(leaf, 0) + btrfs_item_size_nr(leaf, 0) != > > -- > > 2.5.5 > > > > -- > > 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 -- 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/disk-io.c b/fs/btrfs/disk-io.c index a5a22be..dfaeb96 100644 --- a/fs/btrfs/disk-io.c +++ b/fs/btrfs/disk-io.c @@ -559,8 +559,28 @@ static noinline int check_leaf(struct btrfs_root *root, u32 nritems = btrfs_header_nritems(leaf); int slot; - if (nritems == 0) + if (nritems == 0) { + struct btrfs_root *r; + + key.objectid = btrfs_header_owner(leaf); + key.type = BTRFS_ROOT_ITEM_KEY; + key.offset = -1ULL; + + r = btrfs_get_fs_root(root->fs_info, &key, false); + /* + * The only reason we also check NULL here is that during + * open_ctree() some roots has not yet been set up. + */ + if (!IS_ERR_OR_NULL(r)) { + /* if leaf is the root, then it's fine */ + if (leaf->start != btrfs_root_bytenr(&r->root_item)) { + CORRUPT("non-root leaf's nritems is 0", + leaf, root, 0); + return -EIO; + } + } return 0; + } /* Check the 0 item */ if (btrfs_item_offset_nr(leaf, 0) + btrfs_item_size_nr(leaf, 0) !=
Right now we treat leaf which has zero item as a valid one because we could have an empty tree, that is, a root that is also a leaf without any item, however, in the same case but when the leaf is not a root, we can end up with hitting the BUG_ON(1) in btrfs_extend_item() called by setup_inline_extent_backref(). This makes us check the situation as a corruption if leaf is not its own root. Signed-off-by: Liu Bo <bo.li.liu@oracle.com> --- fs/btrfs/disk-io.c | 22 +++++++++++++++++++++- 1 file changed, 21 insertions(+), 1 deletion(-)