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

[David Sterba <dsterba@suse.cz>]

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. The format
now uses the 'key=value' style, while we have the 'key value' in many
other places, this should be consistent.

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

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