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

[Nikolay Borisov <nborisov@suse.com>]

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).


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