* Re: [PATCH] btrfs: trim: Check the range passed into to prevent overflow
2019-05-28 8:21 [PATCH] btrfs: trim: Check the range passed into to prevent overflow Qu Wenruo
@ 2019-05-28 12:22 ` Nikolay Borisov
2019-05-30 12:04 ` David Sterba
` (2 subsequent siblings)
3 siblings, 0 replies; 10+ messages in thread
From: Nikolay Borisov @ 2019-05-28 12:22 UTC (permalink / raw)
To: Qu Wenruo, linux-btrfs
On 28.05.19 г. 11:21 ч., 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>
Reviewed-by: Nikolay Borisov <nborisov@suse.com>
> ---
> 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)) {
>
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH] btrfs: trim: Check the range passed into to prevent overflow
2019-05-28 8:21 [PATCH] btrfs: trim: Check the range passed into to prevent overflow Qu Wenruo
2019-05-28 12:22 ` Nikolay Borisov
@ 2019-05-30 12:04 ` David Sterba
2019-05-30 12:05 ` Qu Wenruo
2019-05-30 12:13 ` Nikolay Borisov
2019-05-31 5:35 ` Anand Jain
2019-08-07 12:03 ` Qu Wenruo
3 siblings, 2 replies; 10+ messages in thread
From: David Sterba @ 2019-05-30 12:04 UTC (permalink / raw)
To: Qu Wenruo; +Cc: linux-btrfs
On Tue, May 28, 2019 at 04:21:54PM +0800, 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>
The range support of TRIM will be reverted so this patch won't be
needed.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] btrfs: trim: Check the range passed into to prevent overflow
2019-05-30 12:04 ` David Sterba
@ 2019-05-30 12:05 ` Qu Wenruo
2019-05-30 12:13 ` Nikolay Borisov
1 sibling, 0 replies; 10+ messages in thread
From: Qu Wenruo @ 2019-05-30 12:05 UTC (permalink / raw)
To: dsterba, Qu Wenruo, linux-btrfs
[-- Attachment #1.1: Type: text/plain, Size: 804 bytes --]
On 2019/5/30 下午8:04, David Sterba wrote:
> On Tue, May 28, 2019 at 04:21:54PM +0800, 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>
>
> The range support of TRIM will be reverted so this patch won't be
> needed.
No, this patch is an independent one.
The objective is to behavior the same when @range->start + @range->len
overflows when @range->len is not default value (U64MAX).
With the trim range patch reverted, we should still detect overflow and
return -EINVAL to meet the new generic/260 check.
So please still include this patch.
Thanks,
Qu
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] btrfs: trim: Check the range passed into to prevent overflow
2019-05-30 12:04 ` David Sterba
2019-05-30 12:05 ` Qu Wenruo
@ 2019-05-30 12:13 ` Nikolay Borisov
1 sibling, 0 replies; 10+ messages in thread
From: Nikolay Borisov @ 2019-05-30 12:13 UTC (permalink / raw)
To: dsterba, Qu Wenruo, linux-btrfs
On 30.05.19 г. 15:04 ч., David Sterba wrote:
> On Tue, May 28, 2019 at 04:21:54PM +0800, 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>
>
> The range support of TRIM will be reverted so this patch won't be
> needed.
>
Range support is going to be removed only from unallocated device space
trimming, not block group trimming. So it's still required.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] btrfs: trim: Check the range passed into to prevent overflow
2019-05-28 8:21 [PATCH] btrfs: trim: Check the range passed into to prevent overflow Qu Wenruo
2019-05-28 12:22 ` Nikolay Borisov
2019-05-30 12:04 ` David Sterba
@ 2019-05-31 5:35 ` Anand Jain
2019-05-31 5:56 ` Qu Wenruo
2019-08-07 12:03 ` Qu Wenruo
3 siblings, 1 reply; 10+ messages in thread
From: Anand Jain @ 2019-05-31 5:35 UTC (permalink / raw)
To: Qu Wenruo, dsterba; +Cc: linux-btrfs
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
-----
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)) {
>
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH] btrfs: trim: Check the range passed into to prevent overflow
2019-05-31 5:35 ` Anand Jain
@ 2019-05-31 5:56 ` Qu Wenruo
2019-05-31 6:01 ` Qu Wenruo
0 siblings, 1 reply; 10+ messages in thread
From: Qu Wenruo @ 2019-05-31 5:56 UTC (permalink / raw)
To: Anand Jain, Qu Wenruo, dsterba; +Cc: linux-btrfs
[-- Attachment #1.1: Type: text/plain, Size: 3845 bytes --]
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 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.
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 --]
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH] btrfs: trim: Check the range passed into to prevent overflow
2019-05-31 5:56 ` Qu Wenruo
@ 2019-05-31 6:01 ` Qu Wenruo
0 siblings, 0 replies; 10+ messages in thread
From: Qu Wenruo @ 2019-05-31 6:01 UTC (permalink / raw)
To: Anand Jain, Qu Wenruo, dsterba; +Cc: linux-btrfs, Linux FS Devel
[-- 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 --]
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] btrfs: trim: Check the range passed into to prevent overflow
2019-05-28 8:21 [PATCH] btrfs: trim: Check the range passed into to prevent overflow Qu Wenruo
` (2 preceding siblings ...)
2019-05-31 5:35 ` Anand Jain
@ 2019-08-07 12:03 ` Qu Wenruo
2019-08-07 12:30 ` David Sterba
3 siblings, 1 reply; 10+ messages in thread
From: Qu Wenruo @ 2019-08-07 12:03 UTC (permalink / raw)
To: Qu Wenruo, linux-btrfs
[-- Attachment #1.1: Type: text/plain, Size: 1956 bytes --]
Gentle ping?
Thanks to the discussion with Anand, I find this patch is not merged yet.
Thanks,
Qu
On 2019/5/28 下午4:21, 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>
> ---
> 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 --]
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH] btrfs: trim: Check the range passed into to prevent overflow
2019-08-07 12:03 ` Qu Wenruo
@ 2019-08-07 12:30 ` David Sterba
0 siblings, 0 replies; 10+ messages in thread
From: David Sterba @ 2019-08-07 12:30 UTC (permalink / raw)
To: Qu Wenruo; +Cc: Qu Wenruo, linux-btrfs
On Wed, Aug 07, 2019 at 08:03:41PM +0800, Qu Wenruo wrote:
> Gentle ping?
>
> Thanks to the discussion with Anand, I find this patch is not merged yet.
I had the patch marked as dropped as it was during the trim range
changes. Reading the discussion again it's ok to merge and I'll add it
to the queue.
^ permalink raw reply [flat|nested] 10+ messages in thread