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

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

[-- Attachment #1.1: Type: text/plain, Size: 1743 bytes --]



On 2020/8/13 下午10:05, David Sterba wrote:
> On Wed, Aug 12, 2020 at 02:05:06PM +0800, Qu Wenruo wrote:
>> +/*
>> + * 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 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)) {
>> +		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);
>> +		WARN_ON(IS_ENABLED(CONFIG_BTRFS_DEBUG));
>> +		return -EINVAL;
>> +	}
>> +	return 0;
>> +}
> 
> This helper is similar to the check_setget_bounds that have some
> performance impact,
> https://lore.kernel.org/linux-btrfs/20200730110943.GE3703@twin.jikos.cz/
> .
> 
> The extent buffer helpers are not called that often as the setget
> helpers but still it could be improved to avoid the function call
> penalty on the hot path.
> 
> static inline in check_eb_range(...) {
> 	if (unlikely(out of range))
> 		return report_eb_range(...)
> 	return 0;
> }

I thought we should avoid manual inline, but let the compiler to
determine if it's needed.

Or in this particular case, we're better than the compiler?

Thanks,
Qu

> 
> In the original code the range check was open coded and the above will
> lead to the same asm output, while keeping the C code readable.
> 


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]