Re: [PATCH v5] btrfs: trim: fix underflow in trim length to prevent access beyond device boundary

[Qu Wenruo <quwenruo.btrfs@gmx.com>]

On 2020/8/12 下午2:43, David Sterba wrote:
> The v5 changes were discussed but were not all trivial to be just
> committed. I need to add the patch to pull request branch soon so am
> not waiting for your v5
>
> v5:
>
> - add mask for chunk state bits and use that to clear the range a after
>   device shrink; on a second thought doing all ones did not look clean
>   to me

Extra idea inspired by this patch.

We can do extra extent_io_tree bits sanity check for DEBUG build.

In the past, extent_io_tree got its owner member, which each
extent_io_tree should have one. (Unfortunately, when alloc_state is
added, we didn't add a new entry for it)

With that, we can easily verify the set/clear bits against its owner to
ensure we don't set wrong bits for wrong extent_io_tree.
E.g. CHUNK_* bits are only for alloc_state, while
DELALLOC/QGROUP_RESERVED are only for inode io tree.

Of course, this would be in a new patch.

Thanks,
Qu
>
> - removed assert after clear_extent_bits - make it consistent with all
>   other calls where we don't check the return value for now
>
> - reworded comments
>
> ---
>
> From: Qu Wenruo <wqu@suse.com>
> Subject: [PATCH] btrfs: trim: fix underflow in trim length to prevent access
>  beyond device boundary
>
> [BUG]
> The following script can lead to tons of beyond device boundary access:
>
>   mkfs.btrfs -f $dev -b 10G
>   mount $dev $mnt
>   trimfs $mnt
>   btrfs filesystem resize 1:-1G $mnt
>   trimfs $mnt
>
> [CAUSE]
> Since commit 929be17a9b49 ("btrfs: Switch btrfs_trim_free_extents to
> find_first_clear_extent_bit"), we try to avoid trimming ranges that's
> already trimmed.
>
> So we check device->alloc_state by finding the first range which doesn't
> have CHUNK_TRIMMED and CHUNK_ALLOCATED not set.
>
> But if we shrunk the device, that bits are not cleared, thus we could
> easily got a range starts beyond the shrunk device size.
>
> This results the returned @start and @end are all beyond device size,
> then we call "end = min(end, device->total_bytes -1);" making @end
> smaller than device size.
>
> Then finally we goes "len = end - start + 1", totally underflow the
> result, and lead to the beyond-device-boundary access.
>
> [FIX]
> This patch will fix the problem in two ways:
>
> - Clear CHUNK_TRIMMED | CHUNK_ALLOCATED bits when shrinking device
>   This is the root fix
>
> - Add extra safety check when trimming free device extents
>   We check and warn if the returned range is already beyond current
>   device.
>
> Link: https://github.com/kdave/btrfs-progs/issues/282
> Fixes: 929be17a9b49 ("btrfs: Switch btrfs_trim_free_extents to find_first_clear_extent_bit")
> CC: stable@vger.kernel.org # 5.4+
> Signed-off-by: Qu Wenruo <wqu@suse.com>
> Reviewed-by: Filipe Manana <fdmanana@suse.com>
> Signed-off-by: David Sterba <dsterba@suse.com>
> ---
>  fs/btrfs/extent-io-tree.h |  2 ++
>  fs/btrfs/extent-tree.c    | 14 ++++++++++++++
>  fs/btrfs/volumes.c        |  4 ++++
>  3 files changed, 20 insertions(+)
>
> diff --git a/fs/btrfs/extent-io-tree.h b/fs/btrfs/extent-io-tree.h
> index f39d47a2d01a..219a09a2b734 100644
> --- a/fs/btrfs/extent-io-tree.h
> +++ b/fs/btrfs/extent-io-tree.h
> @@ -34,6 +34,8 @@ struct io_failure_record;
>   */
>  #define CHUNK_ALLOCATED				EXTENT_DIRTY
>  #define CHUNK_TRIMMED				EXTENT_DEFRAG
> +#define CHUNK_STATE_MASK			(CHUNK_ALLOCATED |		\
> +						 CHUNK_TRIMMED)
>
>  enum {
>  	IO_TREE_FS_PINNED_EXTENTS,
> diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
> index fa7d83051587..597505df90b4 100644
> --- a/fs/btrfs/extent-tree.c
> +++ b/fs/btrfs/extent-tree.c
> @@ -33,6 +33,7 @@
>  #include "delalloc-space.h"
>  #include "block-group.h"
>  #include "discard.h"
> +#include "rcu-string.h"
>
>  #undef SCRAMBLE_DELAYED_REFS
>
> @@ -5669,6 +5670,19 @@ static int btrfs_trim_free_extents(struct btrfs_device *device, u64 *trimmed)
>  					    &start, &end,
>  					    CHUNK_TRIMMED | CHUNK_ALLOCATED);
>
> +		/* Check if there are any CHUNK_* bits left */
> +		if (start > device->total_bytes) {
> +			WARN_ON(IS_ENABLED(CONFIG_BTRFS_DEBUG));
> +			btrfs_warn_in_rcu(fs_info,
> +"ignoring attempt to trim beyond device size: offset %llu length %llu device %s device size %llu",
> +					  start, end - start + 1,
> +					  rcu_str_deref(device->name),
> +					  device->total_bytes);
> +			mutex_unlock(&fs_info->chunk_mutex);
> +			ret = 0;
> +			break;
> +		}
> +
>  		/* Ensure we skip the reserved area in the first 1M */
>  		start = max_t(u64, start, SZ_1M);
>
> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> index d7670e2a9f39..ee96c5869f57 100644
> --- a/fs/btrfs/volumes.c
> +++ b/fs/btrfs/volumes.c
> @@ -4720,6 +4720,10 @@ int btrfs_shrink_device(struct btrfs_device *device, u64 new_size)
>  	}
>
>  	mutex_lock(&fs_info->chunk_mutex);
> +	/* Clear all state bits beyond the shrunk device size */
> +	clear_extent_bits(&device->alloc_state, new_size, (u64)-1,
> +			  CHUNK_STATE_MASK);
> +
>  	btrfs_device_set_disk_total_bytes(device, new_size);
>  	if (list_empty(&device->post_commit_list))
>  		list_add_tail(&device->post_commit_list,
>