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

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

On 2020/8/11 下午3:22, David Sterba wrote:
> On Sat, Aug 01, 2020 at 07:35:26AM +0800, Qu Wenruo wrote:
>>
>>
>> On 2020/7/31 下午10:08, David Sterba wrote:
>>> On Fri, Jul 31, 2020 at 07:29:11PM +0800, Qu Wenruo wrote:
>>>> --- a/fs/btrfs/volumes.c
>>>> +++ b/fs/btrfs/volumes.c
>>>> @@ -4720,6 +4720,18 @@ int btrfs_shrink_device(struct btrfs_device *device, u64 new_size)
>>>>  	}
>>>>
>>>>  	mutex_lock(&fs_info->chunk_mutex);
>>>> +	/*
>>>> +	 * Also clear any CHUNK_TRIMMED and CHUNK_ALLOCATED bits beyond the
>>>> +	 * current device boundary.
>>>> +	 * This shouldn't fail, as alloc_state should only utilize those two
>>>> +	 * bits, thus we shouldn't alloc new memory for clearing the status.
>>>
>>> If this fails or not depends on implementation details of
>>> clear_extent_bits and this comment will get out of sync eventually, so I
>>> don't think it should be that specific.
>>>
>>> If the new_size is somewhere in the middle of an existing state, it'll
>>> need to be split anyway, no?
>>
>> Nope. Because in alloc_state we only have two bits utilized,
>> CHUNK_TRIMMED and CHUNK_ALLOCATED.
>>
>> Thus what we're doing is to clear all utilized bits.
>
> Which is true for now, adding a new bit would change that.

Yep, that's also why I had the comment here.

>
>>>
>>> alloc_state |-----+++++|
>>> clear             |------------------------- ... (u64)-1|
>>>
>>> So we'd need to keep the state "-" and unset bits only from "+", and
>>> this will require a split.
>>
>> In this case, we would only reduce the the size of the existing status,
>> or just remove it completely.
>
> I haven't found the 'only reduce the size' in the code, thre's always
> some split. The case in __clear_extent_bit is
>
>  773          *     | ---- desired range ---- |
>  774          *  | state | or
>  775          *  | ------------- state -------------- |
>
> the case on line 774 and followed by split_state.

You're right, we still need to allocate memory in this case.
And it's the BUG_ON() and the preallocated state saving us from the
memory failure.

>
>>> But I still have doubts about just clearing the range, why are there any
>>> device->alloc_state entries at all after device is shrunk?
>>
>> Because the alloc_state is mostly only utilized by trim facility, thus
>> existing functions won't bother clearing/setting it.
>>
>> In this particular case, previous fstrim run would set the CHUNK_TRIMMED
>> bit for all unallocated range (except the super reserve).
>> Then shrink doesn't clear the exceed range, and cause problem.
>
> So the unallocated range on a device is also represented in the
> alloc_state tree?

If the unallocated range has been trimmed, then it has an state, with
CHUNK_TRIMMED bit set.

>
>> Thus clearing the bit in btrfs_shrink_device() makes sense.
>>
>>> Using
>>> clear_extent_bits here is not wrong if we look at the end result of
>>> clearing the range, but otherwise it leaves some state information
>>> and allocated memory behind.
>>>
>> Not that complex case, just plain not fully considered corner case.
>
> So what to do about that? I expect the alloc_state tree to represent the
> device accurately and don't want to leave known issues unfixed.
>
The patch still stand as it is.

The reproducer is still the same:
- fstrim
  This marks the unallocated range of one device with CHUNK_TRIMMED bit
  And the range starts from the last dev extent end, to the end of the
device.

- shrink device
  We need to remove the CHUNK_* bits, or problems will happen in next step

- fstrim again
  Due to the bad check, we could underflow the length of the range, leads to
  access beyond boundary.

The patch fixes it in two locations.
When shrink, we clear the CHUNK_* bits, as these bits makes no sense for
range beyond device boundary.

When fstrim, we do extra check to ensure we don't underflow/overflow
anything.

Is there anything unclear that needs extra comments?

Thanks,
Qu