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?
> +
> + /* 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....
> + 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...
> + 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....
> +
> + /* 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.
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
