[v2,03/16] btrfs: fix anon_dev leak in create_subvol()

Commit Message

Omar Sandoval March 10, 2022, 1:31 a.m. UTC

From: Omar Sandoval <[e-mail]>

When btrfs_qgroup_inherit(), btrfs_alloc_tree_block, or
btrfs_insert_root() fail in create_subvol(), we return without freeing
anon_dev. Reorganize the error handling in create_subvol() to fix this.

Signed-off-by: Omar Sandoval <[e-mail]>
---
 fs/btrfs/ioctl.c | 49 +++++++++++++++++++++++-------------------------
 1 file changed, 23 insertions(+), 26 deletions(-)

Comments

Sweet Tea Dorminy March 11, 2022, 3:42 p.m. UTC | #1

> +out_anon_dev:
>   	if (anon_dev)
>   		free_anon_bdev(anon_dev);

It looks to me as though the finer-grained cleanup means 
free_anon_bdev() can always be called with no conditional; if the code 
reaches this point in cleanup, anon_dev was populated by get_anon_bdev() 
(which must have returned zero, indicating successfully populating 
anon_dev).


Otherwise,

Reviewed-by: Sweet Tea Dorminy <[e-mail]>

and thanks.

Omar Sandoval March 12, 2022, 12:29 a.m. UTC | #2

On Fri, Mar 11, 2022 at 10:42:14AM -0500, Sweet Tea Dorminy wrote:
> 
> > +out_anon_dev:
> >   	if (anon_dev)
> >   		free_anon_bdev(anon_dev);
> 
> It looks to me as though the finer-grained cleanup means free_anon_bdev()
> can always be called with no conditional; if the code reaches this point in
> cleanup, anon_dev was populated by get_anon_bdev() (which must have returned
> zero, indicating successfully populating anon_dev).

The conditional is required because halfway through the function, we
assign the anon_dev to a struct btrfs_root, which means that the
anon_dev will be freed when the root is freed:

	new_root = btrfs_get_new_fs_root(fs_info, objectid, anon_dev);
	if (IS_ERR(new_root)) {
		ret = PTR_ERR(new_root);
		btrfs_abort_transaction(trans, ret);
		goto out;
	}
	/* anon_dev is owned by new_root now. */
	anon_dev = 0;

> Otherwise,
> 
> Reviewed-by: Sweet Tea Dorminy <[e-mail]>
> 
> and thanks.

Thanks!

Sweet Tea Dorminy March 12, 2022, 1:43 a.m. UTC | #3

On 3/11/22 19:29, Omar Sandoval wrote:
> On Fri, Mar 11, 2022 at 10:42:14AM -0500, Sweet Tea Dorminy wrote:
>>> +out_anon_dev:
>>>    	if (anon_dev)
>>>    		free_anon_bdev(anon_dev);
>> It looks to me as though the finer-grained cleanup means free_anon_bdev()
>> can always be called with no conditional; if the code reaches this point in
>> cleanup, anon_dev was populated by get_anon_bdev() (which must have returned
>> zero, indicating successfully populating anon_dev).
> The conditional is required because halfway through the function, we
> assign the anon_dev to a struct btrfs_root, which means that the
> anon_dev will be freed when the root is freed:
>
> 	new_root = btrfs_get_new_fs_root(fs_info, objectid, anon_dev);
> 	if (IS_ERR(new_root)) {
> 		ret = PTR_ERR(new_root);
> 		btrfs_abort_transaction(trans, ret);
> 		goto out;
> 	}
> 	/* anon_dev is owned by new_root now. */
> 	anon_dev = 0;

Makes sense, thanks for pointing that out!

Patch

diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
index 238cee5b5254..d04870ea6a21 100644
--- a/fs/btrfs/ioctl.c
+++ b/fs/btrfs/ioctl.c
@@ -561,7 +561,7 @@  static noinline int create_subvol(struct user_namespace *mnt_userns,
 	struct timespec64 cur_time = current_time(dir);
 	struct inode *inode;
 	int ret;
-	dev_t anon_dev = 0;
+	dev_t anon_dev;
 	u64 objectid;
 	u64 index = 0;
 
@@ -571,11 +571,7 @@  static noinline int create_subvol(struct user_namespace *mnt_userns,
 
 	ret = btrfs_get_free_objectid(fs_info->tree_root, &objectid);
 	if (ret)
-		goto fail_free;
-
-	ret = get_anon_bdev(&anon_dev);
-	if (ret < 0)
-		goto fail_free;
+		goto out_root_item;
 
 	/*
 	 * Don't create subvolume whose level is not zero. Or qgroup will be
@@ -583,9 +579,13 @@  static noinline int create_subvol(struct user_namespace *mnt_userns,
 	 */
 	if (btrfs_qgroup_level(objectid)) {
 		ret = -ENOSPC;
-		goto fail_free;
+		goto out_root_item;
 	}
 
+	ret = get_anon_bdev(&anon_dev);
+	if (ret < 0)
+		goto out_root_item;
+
 	btrfs_init_block_rsv(&block_rsv, BTRFS_BLOCK_RSV_TEMP);
 	/*
 	 * The same as the snapshot creation, please see the comment
@@ -593,26 +593,26 @@  static noinline int create_subvol(struct user_namespace *mnt_userns,
 	 */
 	ret = btrfs_subvolume_reserve_metadata(root, &block_rsv, 8, false);
 	if (ret)
-		goto fail_free;
+		goto out_anon_dev;
 
 	trans = btrfs_start_transaction(root, 0);
 	if (IS_ERR(trans)) {
 		ret = PTR_ERR(trans);
 		btrfs_subvolume_release_metadata(root, &block_rsv);
-		goto fail_free;
+		goto out_anon_dev;
 	}
 	trans->block_rsv = &block_rsv;
 	trans->bytes_reserved = block_rsv.size;
 
 	ret = btrfs_qgroup_inherit(trans, 0, objectid, inherit);
 	if (ret)
-		goto fail;
+		goto out;
 
 	leaf = btrfs_alloc_tree_block(trans, root, 0, objectid, NULL, 0, 0, 0,
 				      BTRFS_NESTING_NORMAL);
 	if (IS_ERR(leaf)) {
 		ret = PTR_ERR(leaf);
-		goto fail;
+		goto out;
 	}
 
 	btrfs_mark_buffer_dirty(leaf);
@@ -667,7 +667,7 @@  static noinline int create_subvol(struct user_namespace *mnt_userns,
 		btrfs_tree_unlock(leaf);
 		btrfs_free_tree_block(trans, objectid, leaf, 0, 1);
 		free_extent_buffer(leaf);
-		goto fail;
+		goto out;
 	}
 
 	free_extent_buffer(leaf);
@@ -676,19 +676,18 @@  static noinline int create_subvol(struct user_namespace *mnt_userns,
 	key.offset = (u64)-1;
 	new_root = btrfs_get_new_fs_root(fs_info, objectid, anon_dev);
 	if (IS_ERR(new_root)) {
-		free_anon_bdev(anon_dev);
 		ret = PTR_ERR(new_root);
 		btrfs_abort_transaction(trans, ret);
-		goto fail;
+		goto out;
 	}
-	/* Freeing will be done in btrfs_put_root() of new_root */
+	/* anon_dev is owned by new_root now. */
 	anon_dev = 0;
 
 	ret = btrfs_record_root_in_trans(trans, new_root);
 	if (ret) {
 		btrfs_put_root(new_root);
 		btrfs_abort_transaction(trans, ret);
-		goto fail;
+		goto out;
 	}
 
 	ret = btrfs_create_subvol_root(trans, new_root, root, mnt_userns);
@@ -696,7 +695,7 @@  static noinline int create_subvol(struct user_namespace *mnt_userns,
 	if (ret) {
 		/* We potentially lose an unused inode item here */
 		btrfs_abort_transaction(trans, ret);
-		goto fail;
+		goto out;
 	}
 
 	/*
@@ -705,28 +704,28 @@  static noinline int create_subvol(struct user_namespace *mnt_userns,
 	ret = btrfs_set_inode_index(BTRFS_I(dir), &index);
 	if (ret) {
 		btrfs_abort_transaction(trans, ret);
-		goto fail;
+		goto out;
 	}
 
 	ret = btrfs_insert_dir_item(trans, name, namelen, BTRFS_I(dir), &key,
 				    BTRFS_FT_DIR, index);
 	if (ret) {
 		btrfs_abort_transaction(trans, ret);
-		goto fail;
+		goto out;
 	}
 
 	btrfs_i_size_write(BTRFS_I(dir), dir->i_size + namelen * 2);
 	ret = btrfs_update_inode(trans, root, BTRFS_I(dir));
 	if (ret) {
 		btrfs_abort_transaction(trans, ret);
-		goto fail;
+		goto out;
 	}
 
 	ret = btrfs_add_root_ref(trans, objectid, root->root_key.objectid,
 				 btrfs_ino(BTRFS_I(dir)), index, name, namelen);
 	if (ret) {
 		btrfs_abort_transaction(trans, ret);
-		goto fail;
+		goto out;
 	}
 
 	ret = btrfs_uuid_tree_add(trans, root_item->uuid,
@@ -734,8 +733,7 @@  static noinline int create_subvol(struct user_namespace *mnt_userns,
 	if (ret)
 		btrfs_abort_transaction(trans, ret);
 
-fail:
-	kfree(root_item);
+out:
 	trans->block_rsv = NULL;
 	trans->bytes_reserved = 0;
 	btrfs_subvolume_release_metadata(root, &block_rsv);
@@ -751,11 +749,10 @@  static noinline int create_subvol(struct user_namespace *mnt_userns,
 			return PTR_ERR(inode);
 		d_instantiate(dentry, inode);
 	}
-	return ret;
-
-fail_free:
+out_anon_dev:
 	if (anon_dev)
 		free_anon_bdev(anon_dev);
+out_root_item:
 	kfree(root_item);
 	return ret;
 }