Re: [PATCH v5 1/4] btrfs: extent_io: do extra check for extent buffer read write functions

[Qu Wenruo <wqu@suse.com>]

On 2020/8/20 上午1:11, David Sterba wrote:
> On Wed, Aug 19, 2020 at 02:35:47PM +0800, Qu Wenruo wrote:
>> --- a/fs/btrfs/extent_io.c
>> +++ b/fs/btrfs/extent_io.c
>> @@ -5620,6 +5620,34 @@ int read_extent_buffer_pages(struct extent_buffer *eb, int wait, int mirror_num)
>>  	return ret;
>>  }
>>  
>> +static void report_eb_range(const struct extent_buffer *eb, unsigned long start,
>> +			    unsigned long len)
>> +{
>> +	btrfs_warn(eb->fs_info,
>> +"btrfs: bad eb rw request, eb bytenr=%llu len=%lu rw start=%lu len=%lu\n",
> 
> No "btrfs:" prefix needed, no "\n" at the end of the string.

Oh sorry, should re-check the message.

> The format
> now uses the 'key=value' style, while we have the 'key value' in many
> other places, this should be consistent.

Indeed, I just checked tree-checker, which does use 'key value'.

> 
>> +		   eb->start, eb->len, start, len);
>> +	WARN_ON(IS_ENABLED(CONFIG_BTRFS_DEBUG));
>> +}
>> +
>> +/*
>> + * Check if the [start, start + len) range is valid before reading/writing
>> + * the eb.
>> + * NOTE: @start and @len are offset *INSIDE* the eb, *NOT* logical address.
>> + *
>> + * Caller should not touch the dst/src memory if this function returns error.
>> + */
>> +static inline int check_eb_range(const struct extent_buffer *eb,
>> +				 unsigned long start, unsigned long len)
>> +{
>> +	/* start, start + len should not go beyond eb->len nor overflow */
>> +	if (unlikely(start > eb->len || start + len > eb->len ||
>> +		     len > eb->len)) {
> 
> Can the number of condition be reduced? If 'start + len' overflows, then
> we don't need to check 'start > eb->len', and for the case where
> start = 1024 and len = -1024 the 'len > eb-len' would be enough.

I'm afraid not.
Although 'start > eb->len || len > eb->len' is enough to detect overflow
case, it no longer detects cases like 'start = 2k, len = 3k' while
eb->len == 4K case.

So we still need all 3 checks.

Thanks,
Qu

> 
>> +		report_eb_range(eb, start, len);
>> +		return -EINVAL;
> 
> This could be simply
> 
> 		return report_eb_range(...);
> 
> It's not a big difference though, compiler produces the same code.
>