Re: [PATCH] btrfs: handle errors properly in update_inline_extent_backref()

[David Sterba <dsterba@suse.cz>]

On Wed, Aug 09, 2023 at 07:29:20PM +0800, Qu Wenruo wrote:
> 
> 
> On 2023/8/9 19:20, David Sterba wrote:
> > On Wed, Aug 09, 2023 at 03:08:21PM +0800, Qu Wenruo wrote:
> > > [PROBLEM]
> > > Inside function update_inline_extent_backref(), we have several
> > > BUG_ON()s along with some ASSERT()s which can be triggered by corrupted
> > > filesystem.
> > > 
> > > [ANAYLYSE]
> > > Most of those BUG_ON()s and ASSERT()s are just a way of handling
> > > unexpected on-disk data.
> > > 
> > > Although we have tree-checker to rule out obviously incorrect extent
> > > tree blocks, it's not enough for those ones.
> > > 
> > > Thus we need proper error handling for them.
> > > 
> > > [FIX]
> > > Thankfully all the callers of update_inline_extent_backref() would
> > > eventually handle the errror by aborting the current transaction.
> > > 
> > > So this patch would do the proper error handling by:
> > > 
> > > - Make update_inline_extent_backref() to return int
> > >    The return value would be either 0 or -EUCLEAN.
> > > 
> > > - Replace BUG_ON()s and ASSERT()s with proper error handling
> > >    This includes:
> > >    * Dump the bad extent tree leaf
> > >    * Output an error message for the cause
> > >      This would include the extent bytenr, num_bytes (if needed),
> > >      the bad values and expected good values.
> > >    * Return -EUCLEAN
> > > 
> > >    Note here we remove all the WARN_ON()s, as eventually the transaction
> > >    would be aborted, thus a backtrace would be triggered anyway.
> > > 
> > > - Better comments on why we expect refs == 1 and refs_to_mode == -1 for
> > >    tree blocks
> > > 
> > > Signed-off-by: Qu Wenruo <wqu@suse.com>
> > 
> > Is this fix for syzbot report
> > https://lore.kernel.org/linux-btrfs/000000000000287928060275b914@google.com/
> > ?
> 
> Unfortunately, I don't think this is the proper fix.
> 
> Since there is no reproducer, I'm not sure if it's really corrupted fs
> causing the bug.
> 
> If it's something else, then the patch won't help at all, except a
> little better debug output.

Ok, thanks. Adding more error handling instead of BUG_ON or ASSERT that
could be possibly hit by corrupted images is good on its own.