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

[WenRuo Qu <wqu@suse.com>]

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.

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

Anyway, it's not something you'll see even in months, thus I don't
really think it would cause any difference.

Thanks,
Qu