* [PATCH] btrfs: fix extent buffer read/write range checks
@ 2019-07-26 5:27 Naohiro Aota
2019-07-26 5:38 ` Nikolay Borisov
0 siblings, 1 reply; 9+ messages in thread
From: Naohiro Aota @ 2019-07-26 5:27 UTC (permalink / raw)
To: linux-btrfs; +Cc: David Sterba, Naohiro Aota
Several functions to read/write an extent buffer check if specified offset
range resides in the size of the extent buffer. However, those checks have
two problems:
(1) they don't catch "start == eb->len" case.
(2) it checks offset in extent buffer against logical address using
eb->start.
Generally, eb->start is much larger than the offset, so the second WARN_ON
was almost useless.
Fix these problems in read_extent_buffer_to_user(),
{memcmp,write,memzero}_extent_buffer().
Signed-off-by: Naohiro Aota <naohiro.aota@wdc.com>
---
fs/btrfs/extent_io.c | 16 ++++++++--------
1 file changed, 8 insertions(+), 8 deletions(-)
diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
index 50cbaf1dad5b..c0174f530568 100644
--- a/fs/btrfs/extent_io.c
+++ b/fs/btrfs/extent_io.c
@@ -5545,8 +5545,8 @@ int read_extent_buffer_to_user(const struct extent_buffer *eb,
unsigned long i = (start_offset + start) >> PAGE_SHIFT;
int ret = 0;
- WARN_ON(start > eb->len);
- WARN_ON(start + len > eb->start + eb->len);
+ WARN_ON(start >= eb->len);
+ WARN_ON(start + len > eb->len);
offset = offset_in_page(start_offset + start);
@@ -5623,8 +5623,8 @@ int memcmp_extent_buffer(const struct extent_buffer *eb, const void *ptrv,
unsigned long i = (start_offset + start) >> PAGE_SHIFT;
int ret = 0;
- WARN_ON(start > eb->len);
- WARN_ON(start + len > eb->start + eb->len);
+ WARN_ON(start >= eb->len);
+ WARN_ON(start + len > eb->len);
offset = offset_in_page(start_offset + start);
@@ -5678,8 +5678,8 @@ void write_extent_buffer(struct extent_buffer *eb, const void *srcv,
size_t start_offset = offset_in_page(eb->start);
unsigned long i = (start_offset + start) >> PAGE_SHIFT;
- WARN_ON(start > eb->len);
- WARN_ON(start + len > eb->start + eb->len);
+ WARN_ON(start >= eb->len);
+ WARN_ON(start + len > eb->len);
offset = offset_in_page(start_offset + start);
@@ -5708,8 +5708,8 @@ void memzero_extent_buffer(struct extent_buffer *eb, unsigned long start,
size_t start_offset = offset_in_page(eb->start);
unsigned long i = (start_offset + start) >> PAGE_SHIFT;
- WARN_ON(start > eb->len);
- WARN_ON(start + len > eb->start + eb->len);
+ WARN_ON(start >= eb->len);
+ WARN_ON(start + len > eb->len);
offset = offset_in_page(start_offset + start);
--
2.22.0
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] btrfs: fix extent buffer read/write range checks
2019-07-26 5:27 [PATCH] btrfs: fix extent buffer read/write range checks Naohiro Aota
@ 2019-07-26 5:38 ` Nikolay Borisov
2019-07-26 6:13 ` Naohiro Aota
0 siblings, 1 reply; 9+ messages in thread
From: Nikolay Borisov @ 2019-07-26 5:38 UTC (permalink / raw)
To: Naohiro Aota, linux-btrfs; +Cc: David Sterba
On 26.07.19 г. 8:27 ч., Naohiro Aota wrote:
> Several functions to read/write an extent buffer check if specified offset
> range resides in the size of the extent buffer. However, those checks have
> two problems:
>
> (1) they don't catch "start == eb->len" case.
> (2) it checks offset in extent buffer against logical address using
> eb->start.
>
> Generally, eb->start is much larger than the offset, so the second WARN_ON
> was almost useless.
>
> Fix these problems in read_extent_buffer_to_user(),
> {memcmp,write,memzero}_extent_buffer().
>
> Signed-off-by: Naohiro Aota <naohiro.aota@wdc.com>
Qu already sent similar patch:
[PATCH v2 1/5] btrfs: extent_io: Do extra check for extent buffer read
write functions
He centralised the checking code, your >= fixes though should be merged
there.
> ---
> fs/btrfs/extent_io.c | 16 ++++++++--------
> 1 file changed, 8 insertions(+), 8 deletions(-)
>
> diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
> index 50cbaf1dad5b..c0174f530568 100644
> --- a/fs/btrfs/extent_io.c
> +++ b/fs/btrfs/extent_io.c
> @@ -5545,8 +5545,8 @@ int read_extent_buffer_to_user(const struct extent_buffer *eb,
> unsigned long i = (start_offset + start) >> PAGE_SHIFT;
> int ret = 0;
>
> - WARN_ON(start > eb->len);
> - WARN_ON(start + len > eb->start + eb->len);
> + WARN_ON(start >= eb->len);
> + WARN_ON(start + len > eb->len);
>
> offset = offset_in_page(start_offset + start);
>
> @@ -5623,8 +5623,8 @@ int memcmp_extent_buffer(const struct extent_buffer *eb, const void *ptrv,
> unsigned long i = (start_offset + start) >> PAGE_SHIFT;
> int ret = 0;
>
> - WARN_ON(start > eb->len);
> - WARN_ON(start + len > eb->start + eb->len);
> + WARN_ON(start >= eb->len);
> + WARN_ON(start + len > eb->len);
>
> offset = offset_in_page(start_offset + start);
>
> @@ -5678,8 +5678,8 @@ void write_extent_buffer(struct extent_buffer *eb, const void *srcv,
> size_t start_offset = offset_in_page(eb->start);
> unsigned long i = (start_offset + start) >> PAGE_SHIFT;
>
> - WARN_ON(start > eb->len);
> - WARN_ON(start + len > eb->start + eb->len);
> + WARN_ON(start >= eb->len);
> + WARN_ON(start + len > eb->len);
>
> offset = offset_in_page(start_offset + start);
>
> @@ -5708,8 +5708,8 @@ void memzero_extent_buffer(struct extent_buffer *eb, unsigned long start,
> size_t start_offset = offset_in_page(eb->start);
> unsigned long i = (start_offset + start) >> PAGE_SHIFT;
>
> - WARN_ON(start > eb->len);
> - WARN_ON(start + len > eb->start + eb->len);
> + WARN_ON(start >= eb->len);
> + WARN_ON(start + len > eb->len);
>
> offset = offset_in_page(start_offset + start);
>
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] btrfs: fix extent buffer read/write range checks
2019-07-26 5:38 ` Nikolay Borisov
@ 2019-07-26 6:13 ` Naohiro Aota
2019-07-26 6:36 ` Qu Wenruo
0 siblings, 1 reply; 9+ messages in thread
From: Naohiro Aota @ 2019-07-26 6:13 UTC (permalink / raw)
To: Nikolay Borisov, Qu Wenruo; +Cc: linux-btrfs, David Sterba
On Fri, Jul 26, 2019 at 08:38:27AM +0300, Nikolay Borisov wrote:
>
>
>On 26.07.19 г. 8:27 ч., Naohiro Aota wrote:
>> Several functions to read/write an extent buffer check if specified offset
>> range resides in the size of the extent buffer. However, those checks have
>> two problems:
>>
>> (1) they don't catch "start == eb->len" case.
>> (2) it checks offset in extent buffer against logical address using
>> eb->start.
>>
>> Generally, eb->start is much larger than the offset, so the second WARN_ON
>> was almost useless.
>>
>> Fix these problems in read_extent_buffer_to_user(),
>> {memcmp,write,memzero}_extent_buffer().
>>
>> Signed-off-by: Naohiro Aota <naohiro.aota@wdc.com>
>
>Qu already sent similar patch:
>
>[PATCH v2 1/5] btrfs: extent_io: Do extra check for extent buffer read
>write functions
>
>
>He centralised the checking code, your >= fixes though should be merged
>there.
Oops, I missed that series. Thank you for pointing out. Then, this
should be merged into Qu's version.
Qu, could you pick the change from "start > eb->len" to "start >= eb->len"?
>
>
>> ---
>> fs/btrfs/extent_io.c | 16 ++++++++--------
>> 1 file changed, 8 insertions(+), 8 deletions(-)
>>
>> diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
>> index 50cbaf1dad5b..c0174f530568 100644
>> --- a/fs/btrfs/extent_io.c
>> +++ b/fs/btrfs/extent_io.c
>> @@ -5545,8 +5545,8 @@ int read_extent_buffer_to_user(const struct extent_buffer *eb,
>> unsigned long i = (start_offset + start) >> PAGE_SHIFT;
>> int ret = 0;
>>
>> - WARN_ON(start > eb->len);
>> - WARN_ON(start + len > eb->start + eb->len);
>> + WARN_ON(start >= eb->len);
>> + WARN_ON(start + len > eb->len);
>>
>> offset = offset_in_page(start_offset + start);
>>
>> @@ -5623,8 +5623,8 @@ int memcmp_extent_buffer(const struct extent_buffer *eb, const void *ptrv,
>> unsigned long i = (start_offset + start) >> PAGE_SHIFT;
>> int ret = 0;
>>
>> - WARN_ON(start > eb->len);
>> - WARN_ON(start + len > eb->start + eb->len);
>> + WARN_ON(start >= eb->len);
>> + WARN_ON(start + len > eb->len);
>>
>> offset = offset_in_page(start_offset + start);
>>
>> @@ -5678,8 +5678,8 @@ void write_extent_buffer(struct extent_buffer *eb, const void *srcv,
>> size_t start_offset = offset_in_page(eb->start);
>> unsigned long i = (start_offset + start) >> PAGE_SHIFT;
>>
>> - WARN_ON(start > eb->len);
>> - WARN_ON(start + len > eb->start + eb->len);
>> + WARN_ON(start >= eb->len);
>> + WARN_ON(start + len > eb->len);
>>
>> offset = offset_in_page(start_offset + start);
>>
>> @@ -5708,8 +5708,8 @@ void memzero_extent_buffer(struct extent_buffer *eb, unsigned long start,
>> size_t start_offset = offset_in_page(eb->start);
>> unsigned long i = (start_offset + start) >> PAGE_SHIFT;
>>
>> - WARN_ON(start > eb->len);
>> - WARN_ON(start + len > eb->start + eb->len);
>> + WARN_ON(start >= eb->len);
>> + WARN_ON(start + len > eb->len);
>>
>> offset = offset_in_page(start_offset + start);
>>
>>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] btrfs: fix extent buffer read/write range checks
2019-07-26 6:13 ` Naohiro Aota
@ 2019-07-26 6:36 ` Qu Wenruo
2019-07-26 8:15 ` Naohiro Aota
0 siblings, 1 reply; 9+ messages in thread
From: Qu Wenruo @ 2019-07-26 6:36 UTC (permalink / raw)
To: Naohiro Aota, Nikolay Borisov, Qu Wenruo; +Cc: linux-btrfs, David Sterba
On 2019/7/26 下午2:13, Naohiro Aota wrote:
> On Fri, Jul 26, 2019 at 08:38:27AM +0300, Nikolay Borisov wrote:
>>
>>
>> On 26.07.19 г. 8:27 ч., Naohiro Aota wrote:
>>> Several functions to read/write an extent buffer check if specified
>>> offset
>>> range resides in the size of the extent buffer. However, those checks
>>> have
>>> two problems:
>>>
>>> (1) they don't catch "start == eb->len" case.
>>> (2) it checks offset in extent buffer against logical address using
>>> eb->start.
>>>
>>> Generally, eb->start is much larger than the offset, so the second
>>> WARN_ON
>>> was almost useless.
>>>
>>> Fix these problems in read_extent_buffer_to_user(),
>>> {memcmp,write,memzero}_extent_buffer().
>>>
>>> Signed-off-by: Naohiro Aota <naohiro.aota@wdc.com>
>>
>> Qu already sent similar patch:
>>
>> [PATCH v2 1/5] btrfs: extent_io: Do extra check for extent buffer read
>> write functions
>>
>>
>> He centralised the checking code, your >= fixes though should be merged
>> there.
>
> Oops, I missed that series. Thank you for pointing out. Then, this
> should be merged into Qu's version.
>
> Qu, could you pick the change from "start > eb->len" to "start >= eb->len"?
start >= eb->len is not always invalid.
start == eb->len while len == 0 is still valid.
Or should we also warn such bad practice?
Thanks,
Qu
>
>>
>>
>>> ---
>>> fs/btrfs/extent_io.c | 16 ++++++++--------
>>> 1 file changed, 8 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
>>> index 50cbaf1dad5b..c0174f530568 100644
>>> --- a/fs/btrfs/extent_io.c
>>> +++ b/fs/btrfs/extent_io.c
>>> @@ -5545,8 +5545,8 @@ int read_extent_buffer_to_user(const struct
>>> extent_buffer *eb,
>>> unsigned long i = (start_offset + start) >> PAGE_SHIFT;
>>> int ret = 0;
>>>
>>> - WARN_ON(start > eb->len);
>>> - WARN_ON(start + len > eb->start + eb->len);
>>> + WARN_ON(start >= eb->len);
>>> + WARN_ON(start + len > eb->len);
>>>
>>> offset = offset_in_page(start_offset + start);
>>>
>>> @@ -5623,8 +5623,8 @@ int memcmp_extent_buffer(const struct
>>> extent_buffer *eb, const void *ptrv,
>>> unsigned long i = (start_offset + start) >> PAGE_SHIFT;
>>> int ret = 0;
>>>
>>> - WARN_ON(start > eb->len);
>>> - WARN_ON(start + len > eb->start + eb->len);
>>> + WARN_ON(start >= eb->len);
>>> + WARN_ON(start + len > eb->len);
>>>
>>> offset = offset_in_page(start_offset + start);
>>>
>>> @@ -5678,8 +5678,8 @@ void write_extent_buffer(struct extent_buffer
>>> *eb, const void *srcv,
>>> size_t start_offset = offset_in_page(eb->start);
>>> unsigned long i = (start_offset + start) >> PAGE_SHIFT;
>>>
>>> - WARN_ON(start > eb->len);
>>> - WARN_ON(start + len > eb->start + eb->len);
>>> + WARN_ON(start >= eb->len);
>>> + WARN_ON(start + len > eb->len);
>>>
>>> offset = offset_in_page(start_offset + start);
>>>
>>> @@ -5708,8 +5708,8 @@ void memzero_extent_buffer(struct extent_buffer
>>> *eb, unsigned long start,
>>> size_t start_offset = offset_in_page(eb->start);
>>> unsigned long i = (start_offset + start) >> PAGE_SHIFT;
>>>
>>> - WARN_ON(start > eb->len);
>>> - WARN_ON(start + len > eb->start + eb->len);
>>> + WARN_ON(start >= eb->len);
>>> + WARN_ON(start + len > eb->len);
>>>
>>> offset = offset_in_page(start_offset + start);
>>>
>>>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] btrfs: fix extent buffer read/write range checks
2019-07-26 6:36 ` Qu Wenruo
@ 2019-07-26 8:15 ` Naohiro Aota
2019-07-26 8:26 ` Qu Wenruo
2019-07-29 5:07 ` Qu Wenruo
0 siblings, 2 replies; 9+ messages in thread
From: Naohiro Aota @ 2019-07-26 8:15 UTC (permalink / raw)
To: Qu Wenruo; +Cc: Nikolay Borisov, linux-btrfs, David Sterba
On Fri, Jul 26, 2019 at 02:36:10PM +0800, Qu Wenruo wrote:
>
>
>On 2019/7/26 下午2:13, Naohiro Aota wrote:
>> On Fri, Jul 26, 2019 at 08:38:27AM +0300, Nikolay Borisov wrote:
>>>
>>>
>>> On 26.07.19 г. 8:27 ч., Naohiro Aota wrote:
>>>> Several functions to read/write an extent buffer check if specified
>>>> offset
>>>> range resides in the size of the extent buffer. However, those checks
>>>> have
>>>> two problems:
>>>>
>>>> (1) they don't catch "start == eb->len" case.
>>>> (2) it checks offset in extent buffer against logical address using
>>>> eb->start.
>>>>
>>>> Generally, eb->start is much larger than the offset, so the second
>>>> WARN_ON
>>>> was almost useless.
>>>>
>>>> Fix these problems in read_extent_buffer_to_user(),
>>>> {memcmp,write,memzero}_extent_buffer().
>>>>
>>>> Signed-off-by: Naohiro Aota <naohiro.aota@wdc.com>
>>>
>>> Qu already sent similar patch:
>>>
>>> [PATCH v2 1/5] btrfs: extent_io: Do extra check for extent buffer read
>>> write functions
>>>
>>>
>>> He centralised the checking code, your >= fixes though should be merged
>>> there.
>>
>> Oops, I missed that series. Thank you for pointing out. Then, this
>> should be merged into Qu's version.
>>
>> Qu, could you pick the change from "start > eb->len" to "start >= eb->len"?
>
> start >= eb->len is not always invalid.
>
> start == eb->len while len == 0 is still valid.
Correct.
But then, we can even say "start > eb->len" is valid if len == 0?
> Or should we also warn such bad practice?
Maybe...
Or how about let the callers bailing out by e.g. "if (!len) return 1;"
in the check function?
Regards,
Naohiro
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] btrfs: fix extent buffer read/write range checks
2019-07-26 8:15 ` Naohiro Aota
@ 2019-07-26 8:26 ` Qu Wenruo
2019-07-29 5:07 ` Qu Wenruo
1 sibling, 0 replies; 9+ messages in thread
From: Qu Wenruo @ 2019-07-26 8:26 UTC (permalink / raw)
To: Naohiro Aota; +Cc: Nikolay Borisov, linux-btrfs, David Sterba
On 2019/7/26 下午4:15, Naohiro Aota wrote:
> On Fri, Jul 26, 2019 at 02:36:10PM +0800, Qu Wenruo wrote:
>>
>>
>> On 2019/7/26 下午2:13, Naohiro Aota wrote:
>>> On Fri, Jul 26, 2019 at 08:38:27AM +0300, Nikolay Borisov wrote:
>>>>
>>>>
>>>> On 26.07.19 г. 8:27 ч., Naohiro Aota wrote:
>>>>> Several functions to read/write an extent buffer check if specified
>>>>> offset
>>>>> range resides in the size of the extent buffer. However, those checks
>>>>> have
>>>>> two problems:
>>>>>
>>>>> (1) they don't catch "start == eb->len" case.
>>>>> (2) it checks offset in extent buffer against logical address using
>>>>> eb->start.
>>>>>
>>>>> Generally, eb->start is much larger than the offset, so the second
>>>>> WARN_ON
>>>>> was almost useless.
>>>>>
>>>>> Fix these problems in read_extent_buffer_to_user(),
>>>>> {memcmp,write,memzero}_extent_buffer().
>>>>>
>>>>> Signed-off-by: Naohiro Aota <naohiro.aota@wdc.com>
>>>>
>>>> Qu already sent similar patch:
>>>>
>>>> [PATCH v2 1/5] btrfs: extent_io: Do extra check for extent buffer read
>>>> write functions
>>>>
>>>>
>>>> He centralised the checking code, your >= fixes though should be merged
>>>> there.
>>>
>>> Oops, I missed that series. Thank you for pointing out. Then, this
>>> should be merged into Qu's version.
>>>
>>> Qu, could you pick the change from "start > eb->len" to "start >=
>>> eb->len"?
>>
>> start >= eb->len is not always invalid.
>>
>> start == eb->len while len == 0 is still valid.
>
> Correct.
>
> But then, we can even say "start > eb->len" is valid if len == 0?
>
>> Or should we also warn such bad practice?
>
> Maybe...
>
> Or how about let the callers bailing out by e.g. "if (!len) return 1;"
> in the check function?
Well, let's forgot len == 0 case and make start >= eb->len invalid.
That len == 0 is making a lot of invalid use case valid, and making the
check more complex.
Thanks,
Qu
>
> Regards,
> Naohiro
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] btrfs: fix extent buffer read/write range checks
2019-07-26 8:15 ` Naohiro Aota
2019-07-26 8:26 ` Qu Wenruo
@ 2019-07-29 5:07 ` Qu Wenruo
2019-07-29 6:46 ` Nikolay Borisov
1 sibling, 1 reply; 9+ messages in thread
From: Qu Wenruo @ 2019-07-29 5:07 UTC (permalink / raw)
To: Naohiro Aota; +Cc: Nikolay Borisov, linux-btrfs, David Sterba
On 2019/7/26 下午4:15, Naohiro Aota wrote:
> On Fri, Jul 26, 2019 at 02:36:10PM +0800, Qu Wenruo wrote:
>>
>>
>> On 2019/7/26 下午2:13, Naohiro Aota wrote:
>>> On Fri, Jul 26, 2019 at 08:38:27AM +0300, Nikolay Borisov wrote:
>>>>
>>>>
>>>> On 26.07.19 г. 8:27 ч., Naohiro Aota wrote:
>>>>> Several functions to read/write an extent buffer check if specified
>>>>> offset
>>>>> range resides in the size of the extent buffer. However, those checks
>>>>> have
>>>>> two problems:
>>>>>
>>>>> (1) they don't catch "start == eb->len" case.
>>>>> (2) it checks offset in extent buffer against logical address using
>>>>> eb->start.
>>>>>
>>>>> Generally, eb->start is much larger than the offset, so the second
>>>>> WARN_ON
>>>>> was almost useless.
>>>>>
>>>>> Fix these problems in read_extent_buffer_to_user(),
>>>>> {memcmp,write,memzero}_extent_buffer().
>>>>>
>>>>> Signed-off-by: Naohiro Aota <naohiro.aota@wdc.com>
>>>>
>>>> Qu already sent similar patch:
>>>>
>>>> [PATCH v2 1/5] btrfs: extent_io: Do extra check for extent buffer read
>>>> write functions
>>>>
>>>>
>>>> He centralised the checking code, your >= fixes though should be merged
>>>> there.
>>>
>>> Oops, I missed that series. Thank you for pointing out. Then, this
>>> should be merged into Qu's version.
>>>
>>> Qu, could you pick the change from "start > eb->len" to "start >=
>>> eb->len"?
>>
>> start >= eb->len is not always invalid.
>>
>> start == eb->len while len == 0 is still valid.
>
> Correct.
>
> But then, we can even say "start > eb->len" is valid if len == 0?
Tried the "start >= eb->len" check in the centralized check_eb_range(),
and unfortunately it triggers a lot of warnings.
Some callers in fact pass start == eb->len and len == 0:
memmove_extent_buffer() in btrfs_del_items()
copy_extent_buffer() in __push_leaf_*()
Since the check of "start > eb->len || len > eb->len || start + len >
eb->len)" has already ensured we won't access anything beyond the eb
data, I'd prefer to let the start == eb->len && len == 0 case to pass.
Doing the extra len == 0 check in those callers seems a little
over-reacted IMHO.
Thanks,
Qu
>
>> Or should we also warn such bad practice?
>
> Maybe...
>
> Or how about let the callers bailing out by e.g. "if (!len) return 1;"
> in the check function?
>
> Regards,
> Naohiro
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] btrfs: fix extent buffer read/write range checks
2019-07-29 5:07 ` Qu Wenruo
@ 2019-07-29 6:46 ` Nikolay Borisov
2019-07-29 6:54 ` Qu Wenruo
0 siblings, 1 reply; 9+ messages in thread
From: Nikolay Borisov @ 2019-07-29 6:46 UTC (permalink / raw)
To: Qu Wenruo, Naohiro Aota; +Cc: David Sterba, linux-btrfs
<snip>
>>
>> But then, we can even say "start > eb->len" is valid if len == 0?
>
> Tried the "start >= eb->len" check in the centralized check_eb_range(),
> and unfortunately it triggers a lot of warnings.
>
> Some callers in fact pass start == eb->len and len == 0:
Isn't this a noop?
> memmove_extent_buffer() in btrfs_del_items()
> copy_extent_buffer() in __push_leaf_*()
>
> Since the check of "start > eb->len || len > eb->len || start + len >
> eb->len)" has already ensured we won't access anything beyond the eb
> data, I'd prefer to let the start == eb->len && len == 0 case to pass.
In an ideal world shouldn't callers detect their parameters are going to
be a NOOP and never execute the code in the first place? E.g. is it
posible that the math in btrfs_del_item is broken for some edge
condition hence calling those functions with such parameters?
>
> Doing the extra len == 0 check in those callers seems a little
> over-reacted IMHO.
>
> Thanks,
> Qu
>>
>>> Or should we also warn such bad practice?
>>
>> Maybe...
>>
>> Or how about let the callers bailing out by e.g. "if (!len) return 1;"
>> in the check function?
>>
>> Regards,
>> Naohiro
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] btrfs: fix extent buffer read/write range checks
2019-07-29 6:46 ` Nikolay Borisov
@ 2019-07-29 6:54 ` Qu Wenruo
0 siblings, 0 replies; 9+ messages in thread
From: Qu Wenruo @ 2019-07-29 6:54 UTC (permalink / raw)
To: Nikolay Borisov, Naohiro Aota; +Cc: David Sterba, linux-btrfs
On 2019/7/29 下午2:46, Nikolay Borisov wrote:
> <snip>
>
>>>
>>> But then, we can even say "start > eb->len" is valid if len == 0?
>>
>> Tried the "start >= eb->len" check in the centralized check_eb_range(),
>> and unfortunately it triggers a lot of warnings.
>>
>> Some callers in fact pass start == eb->len and len == 0:
>
> Isn't this a noop?
Yep.
>
>> memmove_extent_buffer() in btrfs_del_items()
>> copy_extent_buffer() in __push_leaf_*()
>>
>> Since the check of "start > eb->len || len > eb->len || start + len >
>> eb->len)" has already ensured we won't access anything beyond the eb
>> data, I'd prefer to let the start == eb->len && len == 0 case to pass.
>
> In an ideal world shouldn't callers detect their parameters are going to
> be a NOOP and never execute the code in the first place? E.g. is it
> posible that the math in btrfs_del_item is broken for some edge
> condition hence calling those functions with such parameters?
This depends.
Sometimes we can save unnecessary (len == 0) check depending on how the
loop is written.
In btrfs, leaf item 0 always ends at eb->len, thus I believe it's the
reason why we have some loop generating (start = eb->len len = 0) request.
As long as we're not accessing any range beyond [0, eb->len), I tend not
to touch all these callers.
Thanks,
Qu
>
>>
>> Doing the extra len == 0 check in those callers seems a little
>> over-reacted IMHO.
>>
>> Thanks,
>> Qu
>>>
>>>> Or should we also warn such bad practice?
>>>
>>> Maybe...
>>>
>>> Or how about let the callers bailing out by e.g. "if (!len) return 1;"
>>> in the check function?
>>>
>>> Regards,
>>> Naohiro
>>
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2019-07-29 6:54 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-07-26 5:27 [PATCH] btrfs: fix extent buffer read/write range checks Naohiro Aota
2019-07-26 5:38 ` Nikolay Borisov
2019-07-26 6:13 ` Naohiro Aota
2019-07-26 6:36 ` Qu Wenruo
2019-07-26 8:15 ` Naohiro Aota
2019-07-26 8:26 ` Qu Wenruo
2019-07-29 5:07 ` Qu Wenruo
2019-07-29 6:46 ` Nikolay Borisov
2019-07-29 6:54 ` Qu Wenruo
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).