Re: [PATCH 11/25] xfs: scrub the AGI

["Darrick J. Wong" <darrick.wong@oracle.com>]

On Wed, Oct 04, 2017 at 12:43:47PM +1100, Dave Chinner wrote:
> On Tue, Oct 03, 2017 at 01:41:59PM -0700, Darrick J. Wong wrote:
> > From: Darrick J. Wong <darrick.wong@oracle.com>
> > 
> > Add a forgotten check to the AGI verifier, then wire up the scrub
> > infrastructure to check the AGI contents.
> > 
> > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> > ---
> >  fs/xfs/libxfs/xfs_fs.h  |    3 +-
> >  fs/xfs/scrub/agheader.c |   88 +++++++++++++++++++++++++++++++++++++++++++++++
> >  fs/xfs/scrub/common.c   |    6 ++-
> >  fs/xfs/scrub/scrub.c    |    4 ++
> >  fs/xfs/scrub/scrub.h    |    1 +
> >  5 files changed, 99 insertions(+), 3 deletions(-)
> > 
> > 
> > diff --git a/fs/xfs/libxfs/xfs_fs.h b/fs/xfs/libxfs/xfs_fs.h
> > index aeb2a66..1e326dd 100644
> > --- a/fs/xfs/libxfs/xfs_fs.h
> > +++ b/fs/xfs/libxfs/xfs_fs.h
> > @@ -487,9 +487,10 @@ struct xfs_scrub_metadata {
> >  #define XFS_SCRUB_TYPE_SB	1	/* superblock */
> >  #define XFS_SCRUB_TYPE_AGF	2	/* AG free header */
> >  #define XFS_SCRUB_TYPE_AGFL	3	/* AG free list */
> > +#define XFS_SCRUB_TYPE_AGI	4	/* AG inode header */
> >  
> >  /* Number of scrub subcommands. */
> > -#define XFS_SCRUB_TYPE_NR	4
> > +#define XFS_SCRUB_TYPE_NR	5
> >  
> >  /* i: Repair this metadata. */
> >  #define XFS_SCRUB_IFLAG_REPAIR		(1 << 0)
> > diff --git a/fs/xfs/scrub/agheader.c b/fs/xfs/scrub/agheader.c
> > index 7fe6630..3d269c2 100644
> > --- a/fs/xfs/scrub/agheader.c
> > +++ b/fs/xfs/scrub/agheader.c
> > @@ -535,3 +535,91 @@ xfs_scrub_agfl(
> >  out:
> >  	return error;
> >  }
> > +
> > +/* AGI */
> > +
> > +/* Scrub the AGI. */
> > +int
> > +xfs_scrub_agi(
> > +	struct xfs_scrub_context	*sc)
> > +{
> > +	struct xfs_mount		*mp = sc->mp;
> > +	struct xfs_agi			*agi;
> > +	xfs_daddr_t			daddr;
> > +	xfs_daddr_t			eofs;
> > +	xfs_agnumber_t			agno;
> > +	xfs_agblock_t			agbno;
> > +	xfs_agblock_t			eoag;
> > +	xfs_agino_t			agino;
> > +	xfs_agino_t			first_agino;
> > +	xfs_agino_t			last_agino;
> > +	int				i;
> > +	int				level;
> > +	int				error = 0;
> > +
> > +	agno = sc->sm->sm_agno;
> > +	error = xfs_scrub_load_ag_headers(sc, agno, XFS_SCRUB_TYPE_AGI);
> > +	if (!xfs_scrub_op_ok(sc, agno, XFS_AGI_BLOCK(sc->mp), &error))
> > +		goto out;
> > +
> > +	agi = XFS_BUF_TO_AGI(sc->sa.agi_bp);
> > +	eofs = XFS_FSB_TO_BB(mp, mp->m_sb.sb_dblocks);
> > +
> > +	/* Check the AG length */
> > +	eoag = be32_to_cpu(agi->agi_length);
> > +	if (eoag != xfs_scrub_ag_blocks(mp, agno))
> > +		xfs_scrub_block_set_corrupt(sc, sc->sa.agi_bp);
> 
> Should we be cross checking that the AGI and AGF both have
> the same length here?

Isn't that what this does?  Albeit indirectly?

xfs_scrub_ag_blocks returns sb_agcount for every AG except the last one.
For the last AG it returns (sb_dblocks - (all blocks in the other AGs))
which should be the same as agf->agf_length, right?

> > +
> > +	/* Check btree roots and levels */
> > +	agbno = be32_to_cpu(agi->agi_root);
> > +	daddr = XFS_AGB_TO_DADDR(mp, agno, agbno);
> > +	if (agbno <= XFS_AGI_BLOCK(mp) || agbno >= mp->m_sb.sb_agblocks ||
> > +	    agbno >= eoag || daddr >= eofs)
> > +		xfs_scrub_block_set_corrupt(sc, sc->sa.agi_bp);
> 
> xfs_verify_agbno(), again :P
> 
> > +
> > +	level = be32_to_cpu(agi->agi_level);
> > +	if (level <= 0 || level > XFS_BTREE_MAXLEVELS)
> > +		xfs_scrub_block_set_corrupt(sc, sc->sa.agi_bp);
> > +
> > +	if (xfs_sb_version_hasfinobt(&mp->m_sb)) {
> > +		agbno = be32_to_cpu(agi->agi_free_root);
> > +		daddr = XFS_AGB_TO_DADDR(mp, agno, agbno);
> > +		if (agbno <= XFS_AGI_BLOCK(mp) ||
> > +		    agbno >= mp->m_sb.sb_agblocks ||
> > +		    agbno >= eoag ||
> > +		    daddr >= eofs)
> 
> Broken records are us....
> 
> > +			xfs_scrub_block_set_corrupt(sc, sc->sa.agi_bp);
> > +
> > +		level = be32_to_cpu(agi->agi_free_level);
> > +		if (level <= 0 || level > XFS_BTREE_MAXLEVELS)
> > +			xfs_scrub_block_set_corrupt(sc, sc->sa.agi_bp);
> > +	}
> > +
> > +	/* Check inode counters */
> > +	first_agino = XFS_OFFBNO_TO_AGINO(mp, XFS_AGI_BLOCK(mp) + 1, 0);
> 
> Don't think this is right. AGFL, not AGI....

Yes.

> > +	last_agino = XFS_OFFBNO_TO_AGINO(mp, eoag + 1, 0) - 1;
> 
> Not sure this is right, either, because inode chunks won't be
> allocated over the end of the AG. hence if the eoag is not chunk
> aligned, there will be up to (chunk size - 1) blocks inodes won't be
> allocated in...

Yes.

> > +	agino = be32_to_cpu(agi->agi_count);
> > +	if (agino > last_agino - first_agino + 1 ||
> > +	    agino < be32_to_cpu(agi->agi_freecount))
> > +		xfs_scrub_block_set_corrupt(sc, sc->sa.agi_bp);
> 
> Please don't use agino as a count of inodes - this confused me
> very much because I was wondering how these checks were in any way
> realted to valid AG inode numbers....

<nod> Sorry about that, will fix.

> > +
> > +	/* Check inode pointers */
> > +	agino = be32_to_cpu(agi->agi_newino);
> > +	if (agino != NULLAGINO && (agino < first_agino || agino > last_agino))
> > +		xfs_scrub_block_set_corrupt(sc, sc->sa.agi_bp);
> > +	agino = be32_to_cpu(agi->agi_dirino);
> > +	if (agino != NULLAGINO && (agino < first_agino || agino > last_agino))
> > +		xfs_scrub_block_set_corrupt(sc, sc->sa.agi_bp);
> 
> Perhaps we also need a xfs_verify_agino() helper here.
> 
> > +	/* Check unlinked inode buckets */
> > +	for (i = 0; i < XFS_AGI_UNLINKED_BUCKETS; i++) {
> > +		agino = be32_to_cpu(agi->agi_unlinked[i]);
> > +		if (agino == NULLAGINO)
> > +			continue;
> > +		if (agino < first_agino || agino > last_agino)
> > +			xfs_scrub_block_set_corrupt(sc, sc->sa.agi_bp);
> 
> This is effectively the same check as above:
> 
> 	if (agino != NULLAGINO && (agino < first_agino || agino > last_agino))
> 
> so all these checks could use the same helper to make it easier
> to read.

<nod> Will do.  Thank you for the review so far!

--D

> 
> Cheers,
> 
> Dave.
> -- 
> Dave Chinner
> david@fromorbit.com
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html