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@xxxxxxxxxx>
> >
> > 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@xxxxxxxxxx>
> > ---
> > 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@xxxxxxxxxxxxx
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
