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

Qu Wenruo
2020-08-14T00:48:20Z

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