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

[David Sterba <dsterba@suse.cz>]

On Wed, Jul 10, 2019 at 10:58:40AM +0000, WenRuo Qu wrote:
> 
> 
> 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.

'start' is the logical address, that's always larger than eb->len (16K),
Nikolay is IMO right, the check

	start > eb->len

does not make sense, neither does 'start + len > eb->len'.

Your reference to nodesize probably means that the interval
[start, start + len] is subset of [start, start + nodesize].

The test condition in your patch must explode every time the function
is called.

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

I agree that a message next to a WARN is a good thing. Plain WARN does
not have the btrfs header (filesystem, device, ...) so we should use our
helpers and do WARN_ON(IS_ENABLED()) after that. Why after? Because with
panic-on-warn enabled the message won't be printed.

Duplicating the message or even the code does not seem like a good
practice to me.