| Message ID | 3fc2e3dd35d339a7251a2ed48af3ba484769002e.1608135557.git.josef@toxicpanda.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Serious fixes for different error paths | expand |
Regarding the subject, you can write plain 'assert', no need to spell it exactly as it's in the code. On Wed, Dec 16, 2020 at 11:22:10AM -0500, Josef Bacik wrote: > A weird KASAN problem that Zygo reported Please add the relevant part of the report to the changelog and/or link to the report itself. > could have been easily caught > if we checked for basic things in our backref freeing code. We have two > methods of freeing a backref node > > - btrfs_backref_free_node: this just is kfree() essentially. > - btrfs_backref_drop_node: this actually unlinks the node and cleans up > everything and then calls btrfs_backref_free_node(). > > We should mostly be using btrfs_backref_drop_node(), to make sure the > node is properly unlinked from the backref cache, and only use > btrfs_backref_free_node() when we know the node isn't actually linked to > the backref cache. We made a mistake here and thus got the KASAN splat. > Make this style of issue easier to find by adding some ASSERT()'s to > btrfs_backref_free_node() and adjusting our deletion stuff to properly > init the list so we can rely on list_empty() checks working properly. > > Signed-off-by: Josef Bacik <josef@toxicpanda.com>
diff --git a/fs/btrfs/backref.h b/fs/btrfs/backref.h index ff705cc564a9..17abde7f794c 100644 --- a/fs/btrfs/backref.h +++ b/fs/btrfs/backref.h @@ -296,6 +296,9 @@ static inline void btrfs_backref_free_node(struct btrfs_backref_cache *cache, struct btrfs_backref_node *node) { if (node) { + ASSERT(list_empty(&node->list)); + ASSERT(list_empty(&node->lower)); + ASSERT(node->eb == NULL); cache->nr_nodes--; btrfs_put_root(node->root); kfree(node); @@ -340,11 +343,11 @@ static inline void btrfs_backref_drop_node_buffer( static inline void btrfs_backref_drop_node(struct btrfs_backref_cache *tree, struct btrfs_backref_node *node) { - BUG_ON(!list_empty(&node->upper)); + ASSERT(list_empty(&node->upper)); btrfs_backref_drop_node_buffer(node); - list_del(&node->list); - list_del(&node->lower); + list_del_init(&node->list); + list_del_init(&node->lower); if (!RB_EMPTY_NODE(&node->rb_node)) rb_erase(&node->rb_node, &tree->rb_root); btrfs_backref_free_node(tree, node);
A weird KASAN problem that Zygo reported could have been easily caught if we checked for basic things in our backref freeing code. We have two methods of freeing a backref node - btrfs_backref_free_node: this just is kfree() essentially. - btrfs_backref_drop_node: this actually unlinks the node and cleans up everything and then calls btrfs_backref_free_node(). We should mostly be using btrfs_backref_drop_node(), to make sure the node is properly unlinked from the backref cache, and only use btrfs_backref_free_node() when we know the node isn't actually linked to the backref cache. We made a mistake here and thus got the KASAN splat. Make this style of issue easier to find by adding some ASSERT()'s to btrfs_backref_free_node() and adjusting our deletion stuff to properly init the list so we can rely on list_empty() checks working properly. Signed-off-by: Josef Bacik <josef@toxicpanda.com> --- fs/btrfs/backref.h | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-)