[PATCHSET 0/2] xfs_db/xfs_repair: warn about bad btree heights

[PATCHSET 0/2] xfs_db/xfs_repair: warn about bad btree heights

[Darrick J. Wong]

Hi all,

While I was backporting 5.16 to libxfs, I noticed that repair and
metadump don't completely check the btree heights stored in the AGF/AGI
headers.  This series corrects that problem by making them all check.

If you're going to start using this mess, you probably ought to just
pull from my git trees, which are linked below.

This is an extraordinary way to destroy everything.  Enjoy!
Comments and questions are, as always, welcome.

--D

xfsprogs git tree:
https://git.kernel.org/cgit/linux/kernel/git/djwong/xfsprogs-dev.git/log/?h=warn-bad-btree-levels
---
 db/metadump.c |   15 +++++++++++++++
 repair/scan.c |   29 ++++++++++++++++++++++++++---
 2 files changed, 41 insertions(+), 3 deletions(-)

[PATCH 1/2] xfs_db: warn about suspicious finobt trees when metadumping

[Darrick J. Wong]

From: Darrick J. Wong <djwong@kernel.org>

We warn about suspicious roots and btree heights before metadumping the
inode btree, so do the same for the free inode btree.

Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
 db/metadump.c |   15 +++++++++++++++
 1 file changed, 15 insertions(+)


diff --git a/db/metadump.c b/db/metadump.c
index c6f9d382..0d151bb8 100644
--- a/db/metadump.c
+++ b/db/metadump.c
@@ -2664,6 +2664,21 @@ copy_inodes(
 		root = be32_to_cpu(agi->agi_free_root);
 		levels = be32_to_cpu(agi->agi_free_level);
 
+		if (root == 0 || root > mp->m_sb.sb_agblocks) {
+			if (show_warnings)
+				print_warning("invalid block number (%u) in "
+						"finobt root in agi %u", root,
+						agno);
+			return 1;
+		}
+
+		if (levels > M_IGEO(mp)->inobt_maxlevels) {
+			if (show_warnings)
+				print_warning("invalid level (%u) in finobt "
+						"root in agi %u", levels, agno);
+			return 1;
+		}
+
 		finobt = 1;
 		if (!scan_btree(agno, root, levels, TYP_FINOBT, &finobt,
 				scanfunc_ino))

[PATCH 2/2] xfs_repair: warn about suspicious btree levels in AG headers

[Darrick J. Wong]

From: Darrick J. Wong <djwong@kernel.org>

Warn about suspicious AG btree levels in the AGF and AGI.

Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
 repair/scan.c |   29 ++++++++++++++++++++++++++---
 1 file changed, 26 insertions(+), 3 deletions(-)


diff --git a/repair/scan.c b/repair/scan.c
index 4a234ded..5a4b8dbd 100644
--- a/repair/scan.c
+++ b/repair/scan.c
@@ -2261,6 +2261,13 @@ validate_agf(
 {
 	xfs_agblock_t		bno;
 	uint32_t		magic;
+	unsigned int		levels;
+
+	levels = be32_to_cpu(agf->agf_levels[XFS_BTNUM_BNO]);
+	if (levels == 0 || levels > mp->m_alloc_maxlevels) {
+		do_warn(_("bad levels %u for btbno root, agno %d\n"),
+			levels, agno);
+	}
 
 	bno = be32_to_cpu(agf->agf_roots[XFS_BTNUM_BNO]);
 	if (libxfs_verify_agbno(mp, agno, bno)) {
@@ -2274,6 +2281,12 @@ validate_agf(
 			bno, agno);
 	}
 
+	levels = be32_to_cpu(agf->agf_levels[XFS_BTNUM_CNT]);
+	if (levels == 0 || levels > mp->m_alloc_maxlevels) {
+		do_warn(_("bad levels %u for btbcnt root, agno %d\n"),
+			levels, agno);
+	}
+
 	bno = be32_to_cpu(agf->agf_roots[XFS_BTNUM_CNT]);
 	if (libxfs_verify_agbno(mp, agno, bno)) {
 		magic = xfs_has_crc(mp) ? XFS_ABTC_CRC_MAGIC
@@ -2288,7 +2301,6 @@ validate_agf(
 
 	if (xfs_has_rmapbt(mp)) {
 		struct rmap_priv	priv;
-		unsigned int		levels;
 
 		memset(&priv.high_key, 0xFF, sizeof(priv.high_key));
 		priv.high_key.rm_blockcount = 0;
@@ -2320,8 +2332,6 @@ validate_agf(
 	}
 
 	if (xfs_has_reflink(mp)) {
-		unsigned int	levels;
-
 		levels = be32_to_cpu(agf->agf_refcount_level);
 		if (levels == 0 || levels > mp->m_refc_maxlevels) {
 			do_warn(_("bad levels %u for refcountbt root, agno %d\n"),
@@ -2378,6 +2388,13 @@ validate_agi(
 	xfs_agblock_t		bno;
 	int			i;
 	uint32_t		magic;
+	unsigned int		levels;
+
+	levels = be32_to_cpu(agi->agi_level);
+	if (levels == 0 || levels > M_IGEO(mp)->inobt_maxlevels) {
+		do_warn(_("bad levels %u for inobt root, agno %d\n"),
+			levels, agno);
+	}
 
 	bno = be32_to_cpu(agi->agi_root);
 	if (libxfs_verify_agbno(mp, agno, bno)) {
@@ -2392,6 +2409,12 @@ validate_agi(
 	}
 
 	if (xfs_has_finobt(mp)) {
+		levels = be32_to_cpu(agi->agi_free_level);
+		if (levels == 0 || levels > M_IGEO(mp)->inobt_maxlevels) {
+			do_warn(_("bad levels %u for finobt root, agno %d\n"),
+				levels, agno);
+		}
+
 		bno = be32_to_cpu(agi->agi_free_root);
 		if (libxfs_verify_agbno(mp, agno, bno)) {
 			magic = xfs_has_crc(mp) ?

Re: [PATCH 1/2] xfs_db: warn about suspicious finobt trees when metadumping

[Eric Sandeen]

On 5/5/22 11:04 AM, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> We warn about suspicious roots and btree heights before metadumping the
> inode btree, so do the same for the free inode btree.
> 
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>

LGTM. I wonder if we could do this without quite so much copied code
(like maybe call copy_inodes twice, once for inode tree once for free inode
tree?) but that's a patch for another day.

Reviewed-by: Eric Sandeen <sandeen@redhat.com>

===

maybe like this? worth it?

static int
copy_inodes(
        xfs_agnumber_t          agno,
        xfs_agi_t               *agi,
        int                     finobt)
{
        xfs_agblock_t           root;
        int                     levels;
        int                     type;

        if (finobt) {
                type = TYP_FINOBT;
                root = be32_to_cpu(agi->agi_root);
                levels = be32_to_cpu(agi->agi_level);
        } else {
                type = TYP_INOBT;
                root = be32_to_cpu(agi->agi_free_root);
                levels = be32_to_cpu(agi->agi_free_level);
        }

        /* validate root and levels before processing the tree */
        if (root == 0 || root > mp->m_sb.sb_agblocks) {
                if (show_warnings)
                        print_warning("invalid block number (%u) in %s"
                                        "root in agi %u", root,
                                        finobt ? "finobt" : "inobt", agno);
                return 1;
        }
        if (levels > M_IGEO(mp)->inobt_maxlevels) {
                if (show_warnings)
                        print_warning("invalid level (%u) in %s root "
                                        "in agi %u", levels,
                                        finobt ? "finobt" : "inobt", agno);
                return 1;
        }

        if (!scan_btree(agno, root, levels, type, &finobt, scanfunc_ino))
                return 0;

        return 1;
}

Re: [PATCH 2/2] xfs_repair: warn about suspicious btree levels in AG headers

[Eric Sandeen]

On 5/5/22 11:04 AM, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> Warn about suspicious AG btree levels in the AGF and AGI.
> 
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>

Reviewed-by: Eric Sandeen <sandeen@redhat.com>

Re: [PATCH 1/2] xfs_db: warn about suspicious finobt trees when metadumping

[Christoph Hellwig]

Looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH 2/2] xfs_repair: warn about suspicious btree levels in AG headers

[Christoph Hellwig]

On Thu, May 05, 2022 at 09:04:25AM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> Warn about suspicious AG btree levels in the AGF and AGI.

Looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>