Re: [PATCH] btrfs: trim: Check the range passed into to prevent overflow

Re: [PATCH] btrfs: trim: Check the range passed into to prevent overflow

[Qu Wenruo]

[-- Attachment #1.1: Type: text/plain, Size: 4068 bytes --]


On 2019/5/31 下午1:56, Qu Wenruo wrote:
> 
> 
> On 2019/5/31 下午1:35, Anand Jain wrote:
>> On 5/28/19 4:21 PM, Qu Wenruo wrote:
>>> Normally the range->len is set to default value (U64_MAX), but when it's
>>> not default value, we should check if the range overflows.
>>>
>>> And if overflows, return -EINVAL before doing anything.
>>>
>>> Signed-off-by: Qu Wenruo <wqu@suse.com>
>>
>> fstests patch
>>    https://patchwork.kernel.org/patch/10964105/
>> makes the sub-test like [1] in generic/260 skipped
>>
>> [1]
>> -----
>> fssize=$($DF_PROG -k | grep "$SCRATCH_MNT" | grep "$SCRATCH_DEV"  | awk
>> '{print $3}')
>> beyond_eofs=$((fssize*2048))
>> fstrim -o $beyond_eofs $SCRATCH_MNT <-- should_fail
>> -----
> 
> As I mentioned in the commit message and offline, the idea of *end of
> filesystem* is not clear enough.
> 
> For regular fs, they have almost every byte mapped directly to its block device
> (except external journal).
> So its end of filesystem is easy to determine.
> But we can still argue, how to trim the external journal device? Or
> should the external journal device contribute to the end of the fs?
> 
> 
> Now for btrfs, it's a dm-linear space, then dm-raid/dm-linear for each
> chunk. Thus we can argue either the end of btrfs is U64MAX (from
> dm-linear view), or the end of last block group (from mapped chunk view).
> 
> Further more, how to define end of a filesystem when the fs spans across
> several devices?
> 
> I'd say this is a good timing for us to make the fstrim behavior more clear.

Also add fsdevel list into the discussion.

> 
> Thanks,
> Qu
> 
>>
>> Originally [1] reported expected EINVAL until the patch
>>   6ba9fc8e628b btrfs: Ensure btrfs_trim_fs can trim the whole filesystem
>>
>> Not sure how will some of the production machines will find this as,
>> not compatible with previous versions? Nevertheless in practical terms
>> things are fine.
>>
>>  Reviewed-by: Anand Jain <anand.jain@oracle.com>
>>
>> Thanks, Anand
>>
>>> ---
>>>   fs/btrfs/extent-tree.c | 14 +++++++++++---
>>>   1 file changed, 11 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
>>> index f79e477a378e..62bfba6d3c07 100644
>>> --- a/fs/btrfs/extent-tree.c
>>> +++ b/fs/btrfs/extent-tree.c
>>> @@ -11245,6 +11245,7 @@ int btrfs_trim_fs(struct btrfs_fs_info
>>> *fs_info, struct fstrim_range *range)
>>>       struct btrfs_device *device;
>>>       struct list_head *devices;
>>>       u64 group_trimmed;
>>> +    u64 range_end = U64_MAX;
>>>       u64 start;
>>>       u64 end;
>>>       u64 trimmed = 0;
>>> @@ -11254,16 +11255,23 @@ int btrfs_trim_fs(struct btrfs_fs_info
>>> *fs_info, struct fstrim_range *range)
>>>       int dev_ret = 0;
>>>       int ret = 0;
>>>   +    /*
>>> +     * Check range overflow if range->len is set.
>>> +     * The default range->len is U64_MAX.
>>> +     */
>>> +    if (range->len != U64_MAX && check_add_overflow(range->start,
>>> +                range->len, &range_end))
>>> +        return -EINVAL;
>>> +
>>>       cache = btrfs_lookup_first_block_group(fs_info, range->start);
>>>       for (; cache; cache = next_block_group(cache)) {
>>> -        if (cache->key.objectid >= (range->start + range->len)) {
>>> +        if (cache->key.objectid >= range_end) {
>>>               btrfs_put_block_group(cache);
>>>               break;
>>>           }
>>>             start = max(range->start, cache->key.objectid);
>>> -        end = min(range->start + range->len,
>>> -                cache->key.objectid + cache->key.offset);
>>> +        end = min(range_end, cache->key.objectid + cache->key.offset);
>>>             if (end - start >= range->minlen) {
>>>               if (!block_group_cache_done(cache)) {
>>>
>>
> 


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]