From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from userp1040.oracle.com ([156.151.31.81]:39525 "EHLO userp1040.oracle.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750945AbdJDEZH (ORCPT ); Wed, 4 Oct 2017 00:25:07 -0400 Date: Tue, 3 Oct 2017 21:25:01 -0700 From: "Darrick J. Wong" Subject: Re: [PATCH 11/25] xfs: scrub the AGI Message-ID: <20171004042501.GO6503@magnolia> References: <150706324963.19351.17715069858921948692.stgit@magnolia> <150706331918.19351.1010060377239825093.stgit@magnolia> <20171004014347.GX3666@dastard> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20171004014347.GX3666@dastard> Sender: linux-xfs-owner@vger.kernel.org List-ID: List-Id: xfs To: Dave Chinner Cc: linux-xfs@vger.kernel.org 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 > > > > 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 > > --- > > 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.... 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. 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