Re: [PATCH 1/5] btrfs: extent_io: Do extra check for extent buffer read write functions

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

On 2019/7/25 上午12:00, David Sterba wrote:
> On Wed, Jul 10, 2019 at 10:58:40AM +0000, WenRuo Qu wrote:
>>
>>
>> On 2019/7/10 下午6:42, Nikolay Borisov wrote:
>>>
>>>
>> [...]
>>>>
>>>> +/*
>>>> + * Check if the [start, start + len) range is valid before reading/writing
>>>> + * the eb.
>>>> + *
>>>> + * Caller should not touch the dst/src memory if this function returns error.
>>>> + */
>>>> +static int check_eb_range(const struct extent_buffer *eb, unsigned long start,
>>>> +			  unsigned long len)
>>>> +{
>>>> +	unsigned long end;
>>>> +
>>>> +	/* start, start + len should not go beyond eb->len nor overflow */
>>>> +	if (unlikely(start > eb->len || start + len > eb->len ||
>>>
>>> I think your check here is wrong, it should be start + len > start +
>>> eb->len. start is the logical address hence it can be a lot bigger than
>>> the size of the eb which is 16k by default.
>>
>> Definitely NO.
>>
>> [start, start + len) must be in the range of [0, nodesize).
>> So think again.
>
> 'start' is the logical address, that's always larger than eb->len (16K),
> Nikolay is IMO right, the check

No. This @start is not eb->start. It's the offset inside the eb.

>
> 	start > eb->len
>
> does not make sense, neither does 'start + len > eb->len'.
>
> Your reference to nodesize probably means that the interval
> [start, start + len] is subset of [start, start + nodesize].
>
> The test condition in your patch must explode every time the function
> is called.

Nope, it doesn't explode.

>
>>>> +		     check_add_overflow(start, len, &end))) {
>>>> +		WARN(IS_ENABLED(CONFIG_BTRFS_DEBUG), KERN_ERR
>>>> +"btrfs: bad eb rw request, eb bytenr=%llu len=%lu rw start=%lu len=%lu\n",
>>>> +		     eb->start, eb->len, start, len);
>>>> +		btrfs_warn(eb->fs_info,
>>>> +"btrfs: bad eb rw request, eb bytenr=%llu len=%lu rw start=%lu len=%lu\n",
>>>> +			   eb->start, eb->len, start, len);
>>>
>>> If CONFIG_BTRFS_DEBUG is enabled then we will print the warning text
>>> twice. Simply make  it:
>>>
>>> WARN_ON(IS_ENABLED()) and leave the btrfs_Warn to always print the text.
>>
>> WARN_ON() doesn't contain any text to indicate the reason of the stack dump.
>> Thus I still prefer to show exact the reason other than takes developer
>> several seconds to combine the stack with the following btrfs_warn()
>> message.
>
> I agree that a message next to a WARN is a good thing. Plain WARN does
> not have the btrfs header (filesystem, device, ...) so we should use our
> helpers and do WARN_ON(IS_ENABLED()) after that. Why after? Because with
> panic-on-warn enabled the message won't be printed.
>
> Duplicating the message or even the code does not seem like a good
> practice to me.
>
OK, I'll just remove the duplicated error message.

Thanks,
Qu