On Thu, Jul 25, 2019 at 02:58:08PM +0800, Qu Wenruo wrote:
> [snip]
> >> - if (dst_offset + len > dst->len) {
> >> - btrfs_err(fs_info,
> >> - "memmove bogus dst_offset %lu move len %lu dst len %lu",
> >> - dst_offset, len, dst->len);
> >> - BUG();
> >> - }
> >> + if (check_eb_range(dst, dst_offset, len) ||
> >> + check_eb_range(dst, src_offset, len))
> >> + return;
> >
> > I'm not sure about this. If the code expects memcpy_extent_buffer to
> > never fail then it will make more sense to do the range check outside of
> > this function. Otherwise it might silently fail and cause mayhem up the
> > call chain. Or just leave the BUG (I'd rather not).
>
> Yes, that's also what I'm concerned.
>
> But at least, for that case we're not making things worse.
> Furthermore, btrfs tree checker should have already rejected most
> invalid trees already.
> So I'm not so concerned.
I think this is the valid use of BUG, the check is supposed something
that's verified in advance and it must not happen inisdie the extent
buffer functions. Stress on the 'must not', so if it happens something
is seriously wrong, like a memory bitflip or accidental overwrite by
some other code and the BUG is there to stop propagating the error.
