| Message ID | 20200603101112.23369-1-fdmanana@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2,1/3] Btrfs: fix a block group ref counter leak after failure to remove block group | expand |
(Except for the changelog needs a careful reading. It keeps confused
until it is mentioned that 'out_put_group' is renamed to 'out').
Otherwise looks good.
Reviewed-by: Anand Jain <anand.jain@oracle.com>
On Wed, Jun 03, 2020 at 11:11:12AM +0100, fdmanana@kernel.org wrote: > From: Filipe Manana <fdmanana@suse.com> > > When removing a block group, if we fail to delete the block group's item > from the extent tree, we jump to the 'out' label and end up decrementing > the block group's reference count once only (by 1), resulting in a counter > leak because the block group at that point was already removed from the > block group cache rbtree - so we have to decrement the reference count > twice, once for the rbtree and once for our lookup at the start of the > function. > > There is a second bug where if removing the free space tree entries (the > call to remove_block_group_free_space()) fails we end up jumping to the > 'out_put_group' label but end up decrementing the reference count only > once, when we should have done it twice, since we have already removed > the block group from the block group cache rbtree. This happens because > the reference count decrement for the rbtree reference happens after > attempting to remove the free space tree entries, which is far away from > the place where we remove the block group from the rbtree. > > To make things less error prone, decrement the reference count for the > rbtree immediately after removing the block group from it. This also > eleminates the need for two different exit labels on error, renaming > 'out_put_label' to just 'out' and removing the old 'out'. > > Fixes: f6033c5e333238 ("btrfs: fix block group leak when removing fails") > Signed-off-by: Filipe Manana <fdmanana@suse.com> > --- > > V2: Updated changelog to describe a second bug the patch fixes, pointed > out by Nikolay. V2 updated in misc-next, thanks.
diff --git a/fs/btrfs/block-group.c b/fs/btrfs/block-group.c index 176e8a292fd1..5bb76a437f5b 100644 --- a/fs/btrfs/block-group.c +++ b/fs/btrfs/block-group.c @@ -940,7 +940,7 @@ int btrfs_remove_block_group(struct btrfs_trans_handle *trans, path = btrfs_alloc_path(); if (!path) { ret = -ENOMEM; - goto out_put_group; + goto out; } /* @@ -978,7 +978,7 @@ int btrfs_remove_block_group(struct btrfs_trans_handle *trans, ret = btrfs_orphan_add(trans, BTRFS_I(inode)); if (ret) { btrfs_add_delayed_iput(inode); - goto out_put_group; + goto out; } clear_nlink(inode); /* One for the block groups ref */ @@ -1001,13 +1001,13 @@ int btrfs_remove_block_group(struct btrfs_trans_handle *trans, ret = btrfs_search_slot(trans, tree_root, &key, path, -1, 1); if (ret < 0) - goto out_put_group; + goto out; if (ret > 0) btrfs_release_path(path); if (ret == 0) { ret = btrfs_del_item(trans, tree_root, path); if (ret) - goto out_put_group; + goto out; btrfs_release_path(path); } @@ -1016,6 +1016,9 @@ int btrfs_remove_block_group(struct btrfs_trans_handle *trans, &fs_info->block_group_cache_tree); RB_CLEAR_NODE(&block_group->cache_node); + /* Once for the block groups rbtree. */ + btrfs_put_block_group(block_group); + if (fs_info->first_logical_byte == block_group->start) fs_info->first_logical_byte = (u64)-1; spin_unlock(&fs_info->block_group_cache_lock); @@ -1125,10 +1128,7 @@ int btrfs_remove_block_group(struct btrfs_trans_handle *trans, ret = remove_block_group_free_space(trans, block_group); if (ret) - goto out_put_group; - - /* Once for the block groups rbtree */ - btrfs_put_block_group(block_group); + goto out; ret = remove_block_group_item(trans, path, block_group); if (ret < 0) @@ -1145,10 +1145,9 @@ int btrfs_remove_block_group(struct btrfs_trans_handle *trans, free_extent_map(em); } -out_put_group: +out: /* Once for the lookup reference */ btrfs_put_block_group(block_group); -out: if (remove_rsv) btrfs_delayed_refs_rsv_release(fs_info, 1); btrfs_free_path(path);