[PATCH] btrfs: fix double free of anon_dev after failure to create subvolume

[PATCH] btrfs: fix double free of anon_dev after failure to create subvolume

[fdmanana]

From: Filipe Manana <fdmanana@suse.com>

When creating a subvolume, at create_subvol(), we allocate an anonymous
device and later call btrfs_get_new_fs_root(), which in turn just calls
btrfs_get_root_ref(). There we call btrfs_init_fs_root() which assigns
the anonymous device to the root, but if after that call there's an error,
when we jump to 'fail' label, we call btrfs_put_root(), which frees the
anonymous device and then returns an error that is propagated back to
create_subvol(). Than create_subvol() frees the anonymous device again.

When this happens, if the anonymous device was not reallocated after
the first time it was freed with btrfs_put_root(), we get a kernel
message like the following:

  (...)
  [13950.282466] BTRFS: error (device dm-0) in create_subvol:663: errno=-5 IO failure
  [13950.283027] ida_free called for id=65 which is not allocated.
  [13950.285974] BTRFS info (device dm-0): forced readonly
  (...)

If the anonymous device gets reallocated by another btrfs filesystem
or any other kernel subsystem, then bad things can happen.

So fix this by setting the root's anonymous device to 0 at
btrfs_get_root_ref(), before we call btrfs_put_root(), if an error
happened.

Fixes: 2dfb1e43f57dd3 ("btrfs: preallocate anon block device at first phase of snapshot creation")
Signed-off-by: Filipe Manana <fdmanana@suse.com>
---
 fs/btrfs/disk-io.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
index 5c598e124c25..fc7dd5109806 100644
--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -1826,6 +1826,14 @@ static struct btrfs_root *btrfs_get_root_ref(struct btrfs_fs_info *fs_info,
 	}
 	return root;
 fail:
+	/*
+	 * If our caller provided us an anonymous device, then it's his
+	 * responsability to free it in case we fail. So we have to set our
+	 * root's anon_dev to 0 to avoid a double free, once by btrfs_put_root()
+	 * and once again by our caller.
+	 */
+	if (anon_dev)
+		root->anon_dev = 0;
 	btrfs_put_root(root);
 	return ERR_PTR(ret);
 }
-- 
2.33.0

Re: [PATCH] btrfs: fix double free of anon_dev after failure to create subvolume

[Qu Wenruo]

On 2021/12/11 03:02, fdmanana@kernel.org wrote:
> From: Filipe Manana <fdmanana@suse.com>
>
> When creating a subvolume, at create_subvol(), we allocate an anonymous
> device and later call btrfs_get_new_fs_root(), which in turn just calls
> btrfs_get_root_ref(). There we call btrfs_init_fs_root() which assigns
> the anonymous device to the root, but if after that call there's an error,
> when we jump to 'fail' label, we call btrfs_put_root(), which frees the
> anonymous device and then returns an error that is propagated back to
> create_subvol(). Than create_subvol() frees the anonymous device again.
>
> When this happens, if the anonymous device was not reallocated after
> the first time it was freed with btrfs_put_root(), we get a kernel
> message like the following:
>
>    (...)
>    [13950.282466] BTRFS: error (device dm-0) in create_subvol:663: errno=-5 IO failure
>    [13950.283027] ida_free called for id=65 which is not allocated.
>    [13950.285974] BTRFS info (device dm-0): forced readonly
>    (...)
>
> If the anonymous device gets reallocated by another btrfs filesystem
> or any other kernel subsystem, then bad things can happen.
>
> So fix this by setting the root's anonymous device to 0 at
> btrfs_get_root_ref(), before we call btrfs_put_root(), if an error
> happened.
>
> Fixes: 2dfb1e43f57dd3 ("btrfs: preallocate anon block device at first phase of snapshot creation")
> Signed-off-by: Filipe Manana <fdmanana@suse.com>

Reviewed-by: Qu Wenruo <wqu@suse.com>

Thanks,
Qu

> ---
>   fs/btrfs/disk-io.c | 8 ++++++++
>   1 file changed, 8 insertions(+)
>
> diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
> index 5c598e124c25..fc7dd5109806 100644
> --- a/fs/btrfs/disk-io.c
> +++ b/fs/btrfs/disk-io.c
> @@ -1826,6 +1826,14 @@ static struct btrfs_root *btrfs_get_root_ref(struct btrfs_fs_info *fs_info,
>   	}
>   	return root;
>   fail:
> +	/*
> +	 * If our caller provided us an anonymous device, then it's his
> +	 * responsability to free it in case we fail. So we have to set our
> +	 * root's anon_dev to 0 to avoid a double free, once by btrfs_put_root()
> +	 * and once again by our caller.
> +	 */
> +	if (anon_dev)
> +		root->anon_dev = 0;
>   	btrfs_put_root(root);
>   	return ERR_PTR(ret);
>   }

Re: [PATCH] btrfs: fix double free of anon_dev after failure to create subvolume

[David Sterba]

On Fri, Dec 10, 2021 at 07:02:18PM +0000, fdmanana@kernel.org wrote:
> From: Filipe Manana <fdmanana@suse.com>
> 
> When creating a subvolume, at create_subvol(), we allocate an anonymous
> device and later call btrfs_get_new_fs_root(), which in turn just calls
> btrfs_get_root_ref(). There we call btrfs_init_fs_root() which assigns
> the anonymous device to the root, but if after that call there's an error,
> when we jump to 'fail' label, we call btrfs_put_root(), which frees the
> anonymous device and then returns an error that is propagated back to
> create_subvol(). Than create_subvol() frees the anonymous device again.
> 
> When this happens, if the anonymous device was not reallocated after
> the first time it was freed with btrfs_put_root(), we get a kernel
> message like the following:
> 
>   (...)
>   [13950.282466] BTRFS: error (device dm-0) in create_subvol:663: errno=-5 IO failure
>   [13950.283027] ida_free called for id=65 which is not allocated.
>   [13950.285974] BTRFS info (device dm-0): forced readonly
>   (...)
> 
> If the anonymous device gets reallocated by another btrfs filesystem
> or any other kernel subsystem, then bad things can happen.
> 
> So fix this by setting the root's anonymous device to 0 at
> btrfs_get_root_ref(), before we call btrfs_put_root(), if an error
> happened.
> 
> Fixes: 2dfb1e43f57dd3 ("btrfs: preallocate anon block device at first phase of snapshot creation")
> Signed-off-by: Filipe Manana <fdmanana@suse.com>

Added to misc-next, thanks.