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 下午7:24, Nikolay Borisov wrote:
>
>
> On 12.08.20 г. 14:14 ч., Qu Wenruo wrote:
>>
>>
>> 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.
>
> Will this work given the CHUNK_* bits are defined to 2 existing flags,
> chosen such that to not clash with the special logic in bit management
> functions? (check comment above CHUNK_* bits defines).

No problem.

We're setting a allowed mask for each owner, although we're reusing
bits, it doesn't really matter that much.

Thanks,
Qu
>
>
>>
>> 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,
>>>
>>