Re: [PATCH 1/1] xfs: make xchk_iget safer in the presence of corrupt inode btrees

From: "Darrick J. Wong" <djwong@kernel.org>
To: Dave Chinner <david@fromorbit.com>
Cc: linux-xfs@vger.kernel.org
Subject: Re: [PATCH 1/1] xfs: make xchk_iget safer in the presence of corrupt inode btrees
Date: Thu, 28 Sep 2023 10:01:00 -0700	[thread overview]
Message-ID: <20230928170100.GI11439@frogsfrogsfrogs> (raw)
In-Reply-To: <ZRUVL2okuLfNC6U1@dread.disaster.area>

On Thu, Sep 28, 2023 at 03:54:55PM +1000, Dave Chinner wrote:
> On Tue, Sep 26, 2023 at 04:31:07PM -0700, Darrick J. Wong wrote:
> > From: Darrick J. Wong <djwong@kernel.org>
> > 
> > When scrub is trying to iget na inode, ensure that it won't end up
> > deadlocked on a cycle in the inode btree by using an empty transaction
> > to store all the buffers.
> > 
> > Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> > ---
> >  fs/xfs/scrub/common.c |    6 ++++--
> >  fs/xfs/scrub/common.h |   19 +++++++++++++++++++
> >  fs/xfs/scrub/inode.c  |    4 ++--
> >  3 files changed, 25 insertions(+), 4 deletions(-)
> 
> Looks reasonable.
> 
> Reviewed-by: Dave Chinner <dchinner@redhat.com>
> 
> Further question on loops in btrees because I can't remember if this
> case is handled: if the loop goes round and round the same level,
> how is that detected? i.e. how do we avoid recursion count overflows
> if we keep walking over the same sibling block pattern?

I think regular btree walks are partially vulnerable to loops in a btree
level.  A while back, you patched the self-referential case of a block
that points back to itself, but there isn't any robust checking of
loops such as:

A -> B -> A

Loops involving blocks from multiple levels are caught by checking
whether the btree block level matches what we were expecting given the
direction of travel.

AFAICT nobody's systematically fixed the problem for btree users that
don't attach a transaction to the cursor.  Without a transaction there's
no central place to collect locked buffers, so the buffer cache will try
to re-lock a buffer that the caller already locked.  We've partially
fixed this for GETFSMAP, but I think we're still missing that when
reading the bmbt into memory and for looking things up in dir/attr
btrees.

Scrub, however, protects itself from loops in the btree structure.  The
tree walking in scrub/btree.c finds sibling blocks by looking at the
next pointer in the parent level.  Each block's sibling pointers are
easily checked, so it'll notice things like:

     A
 /   |    \
B -> C -+  D
^-------+

When we load C, xchk_btree_block_check_sibling will notice that
C.rightsib doesn't point at D.  It'll set the CORRUPT flag, and return
from the btree walk.

Scrub will also notice corruption like:
     A
 /   |   \
B    C    B

by computing the keyspace covered by the records in each block and
compares that to corresponding key in the parent.  It also checks that
the records of each level are in increasing order.  Just in case B and
C are both filled with the same record.

--D

> -Dave.
> -- 
> Dave Chinner
> david@fromorbit.com