Re: [PATCH] btrfs: fix extent buffer read/write range checks

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

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