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.
