Re: [PATCH 05/20] btrfs: handle invalid root reference found in btrfs_find_root()

[David Sterba <dsterba@suse.cz>]

On Thu, Jan 25, 2024 at 02:33:53PM +1030, Qu Wenruo wrote:
> 
> 
> On 2024/1/25 07:48, David Sterba wrote:
> > The btrfs_find_root() looks up a root by a key, allowing to do an
> > inexact search when key->offset is -1.  It's never expected to find such
> > item, as it would break allowed the range of a root id.
> >
> > Signed-off-by: David Sterba <dsterba@suse.com>
> > ---
> >   fs/btrfs/root-tree.c | 9 ++++++++-
> >   1 file changed, 8 insertions(+), 1 deletion(-)
> >
> > diff --git a/fs/btrfs/root-tree.c b/fs/btrfs/root-tree.c
> > index ba7e2181ff4e..326cd0d03287 100644
> > --- a/fs/btrfs/root-tree.c
> > +++ b/fs/btrfs/root-tree.c
> > @@ -82,7 +82,14 @@ int btrfs_find_root(struct btrfs_root *root, const struct btrfs_key *search_key,
> >   		if (ret > 0)
> >   			goto out;
> >   	} else {
> > -		BUG_ON(ret == 0);		/* Logical error */
> > +		/*
> > +		 * Key with offset -1 found, there would have to exist a root
> > +		 * with such id, but this is out of the valid range.
> > +		 */
> > +		if (ret == 0) {
> > +			ret = -EUCLEAN;
> > +			goto out;
> > +		}
> 
> Considering all root items would already be checked by tree-checker,
> I'd prefer to add an extra "key.offset == (u64)-1" check, and convert
> this to ASSERT(ret == 0), so that we won't need to bother the impossible
> case (nor its error messages).

I did not think about tree-checker in this context and it actually does
verify offsets of the item keys so it's already prevented, assuming such
corrupted issue would come from the outside.

Assertions are not very popular in release builds and distros turn them
off. A real error handling prevents propagating an error further to the
code, so this is for catching bugs that could happen after tree-checker
lets it pass with valid data but something sets wrong value to offset.

The reasoning I'm using is to have full coverage of the error values as
real handling with worst case throwing an EUCLEAN. Assertions are not
popular in release builds and distros turn them off so it's effectively
removing error handling and allowing silent errors.