Re: [PATCH 01/16] xfs: sb verifier doesn't handle uncached sb buffer

From: Dave Chinner <david@fromorbit.com>
To: "Darrick J. Wong" <djwong@kernel.org>
Cc: linux-xfs@vger.kernel.org
Subject: Re: [PATCH 01/16] xfs: sb verifier doesn't handle uncached sb buffer
Date: Thu, 15 Jul 2021 09:00:57 +1000	[thread overview]
Message-ID: <20210714230057.GA664593@dread.disaster.area> (raw)
In-Reply-To: <20210714224450.GT22402@magnolia>

On Wed, Jul 14, 2021 at 03:44:50PM -0700, Darrick J. Wong wrote:
> On Wed, Jul 14, 2021 at 02:18:57PM +1000, Dave Chinner wrote:
> > From: Dave Chinner <dchinner@redhat.com>
> > 
> > The verifier checks explicitly for bp->b_bn == XFS_SB_DADDR to match
> > the primary superblock buffer, but the primary superblock is an
> > uncached buffer and so bp->b_bn is always -1ULL. Hence this never
> > matches and the CRC error reporting is wholly dependent on the
> > mount superblock already being populated so CRC feature checks pass
> > and allow CRC errors to be reported.
> > 
> > Fix this so that the primary superblock CRC error reporting is not
> > dependent on already having read the superblock into memory.
> > 
> > Signed-off-by: Dave Chinner <dchinner@redhat.com>
> > ---
> >  fs/xfs/libxfs/xfs_sb.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/fs/xfs/libxfs/xfs_sb.c b/fs/xfs/libxfs/xfs_sb.c
> > index 04f5386446db..4a4586bd2ba2 100644
> > --- a/fs/xfs/libxfs/xfs_sb.c
> > +++ b/fs/xfs/libxfs/xfs_sb.c
> > @@ -636,7 +636,7 @@ xfs_sb_read_verify(
> >  
> >  		if (!xfs_buf_verify_cksum(bp, XFS_SB_CRC_OFF)) {
> >  			/* Only fail bad secondaries on a known V5 filesystem */
> > -			if (bp->b_bn == XFS_SB_DADDR ||
> > +			if (bp->b_maps[0].bm_bn == XFS_SB_DADDR ||
> 
> I did not know that b_bn only applies to cached buffers.
> 
> Would you mind ... I dunno, updating the comment in the struct xfs_buf
> declaration to make this clearer?
> 
> 	/*
> 	 * Block number of buffer, when this buffer is cached.  For
> 	 * uncached buffers, only the buffer map (i.e. b_maps[0].bm_bn)
> 	 * contains the block number.
> 	 */
> 	xfs_daddr_t		b_bn;

Even that isn't stating the whole truth. b_maps[0].bm_bn contains
the current daddr for all buffers, regardless of whether they are
cached or not. This is what the IO path uses to provide the physical
address for the bio we submit...

Looking at the kernel code, there is still a lot of users of
bp->b_bn, and we've still got inconsistent use of XFS_BUF_ADDR, too.
I think fixing this all up needs a separate patchset - there's relatively
little outside libxfs/ in userspace that uses bp->b_bn directly
(largely repair, which is a 50/50 mix of b_bn and XFS_BUF_ADDR(bp))
so we should be able to clean this up entirely.

I suspect that converting all the external users to a
xfs_buf_daddr(bp) helper is the way to go here, and then renaming
b_bn to something else and use it only in xfs_buf.c for cache
indexing...

I'll put together another patchset to clean this up - it's separate
to the mount features rework, and this patch is only here because
when I change the feature check in the || case of this check to use
mount flags, the primary superblock buffer verification on first
read no longer flags CRC errors....

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com