[PATCH v2 00/17] xfs_repair: catch things that xfs_check misses

[PATCH v2 00/17] xfs_repair: catch things that xfs_check misses

[Darrick J. Wong]

Hi all,

A long-time goal of mine is to get rid of xfs_check from fstests,
because it is deprecated, adds quite a bit of runtime to the test suite,
and consumes memory like crazy.  We've not been able to do that for lack
of even a basic field-by-field corruption detection comparison between
check and repair, so I temporarily modified the dangerous_repair tests
to warn when check finds something but repair says clean.

The patches below teach xfs_repair to complain about things that it
previously did not catch but xfs_check did.  The one remaining gap is
the lack of quota counter checking, which will be sent in a separate
series once I've worked out all the bugs.

v2: mostly minor updates and add a bunch of RVB tags.  Also pick up the
one quota fix because why not...

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=check-vs-repair
---
 libxfs/libxfs_api_defs.h |    4 +
 quota/edit.c             |   22 +++---
 repair/attr_repair.c     |    2 -
 repair/da_util.c         |   25 +++++--
 repair/dino_chunks.c     |    6 +-
 repair/dinode.c          |  109 +++----------------------------
 repair/dinode.h          |   14 ----
 repair/dir2.c            |   21 ++++++
 repair/phase4.c          |   11 ++-
 repair/phase6.c          |    3 +
 repair/prefetch.c        |    9 +--
 repair/sb.c              |    3 -
 repair/scan.c            |  160 ++++++++++++++++++++++++++++++++--------------
 13 files changed, 196 insertions(+), 193 deletions(-)

[PATCH 01/17] xfs_quota: fix unsigned int id comparisons

[Darrick J. Wong]

From: Darrick J. Wong <darrick.wong@oracle.com>

Fix compiler warnings about unsigned int comparisons by replacing them
with an explicit check for the one possible invalid value (-1U).
id_from_string sets exitcode to nonzero when it sees this value, so the
call sites don't have to do that.

Coverity-id: 1463855, 1463856, 1463857
Fixes: 67a73d6139d0 ("xfs_quota: refactor code to generate id from name")
Fixes: 36dc471cc9bb ("xfs_quota: allow individual timer extension")
Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 quota/edit.c |   22 +++++++++++-----------
 1 file changed, 11 insertions(+), 11 deletions(-)


diff --git a/quota/edit.c b/quota/edit.c
index cf9508bf..01d358f7 100644
--- a/quota/edit.c
+++ b/quota/edit.c
@@ -307,11 +307,11 @@ limit_f(
 
 
 	id = id_from_string(name, type);
-	if (id >= 0)
-		set_limits(id, type, mask, fs_path->fs_name,
-			   &bsoft, &bhard, &isoft, &ihard, &rtbsoft, &rtbhard);
-	else
-		exitcode = -1;
+	if (id == -1)
+		return 0;
+
+	set_limits(id, type, mask, fs_path->fs_name,
+		   &bsoft, &bhard, &isoft, &ihard, &rtbsoft, &rtbhard);
 	return 0;
 }
 
@@ -545,9 +545,10 @@ timer_f(
 	if (name)
 		id = id_from_string(name, type);
 
-	if (id >= 0)
-		set_timer(id, type, mask, fs_path->fs_name, value);
+	if (id == -1)
+		return 0;
 
+	set_timer(id, type, mask, fs_path->fs_name, value);
 	return 0;
 }
 
@@ -642,11 +643,10 @@ warn_f(
 	}
 
 	id = id_from_string(name, type);
-	if (id >= 0)
-		set_warnings(id, type, mask, fs_path->fs_name, value);
-	else
-		exitcode = -1;
+	if (id == -1)
+		return 0;
 
+	set_warnings(id, type, mask, fs_path->fs_name, value);
 	return 0;
 }
 

[PATCH 02/17] xfs_repair: fix missing dir buffer corruption checks

[Darrick J. Wong]

From: Darrick J. Wong <darrick.wong@oracle.com>

The da_read_buf() function operates in "salvage" mode, which means that
if the verifiers fail, it will return a buffer with b_error set.  The
callers of da_read_buf, however, do not adequately check for verifier
errors, which means that repair can fail to flag a corrupt filesystem.

Fix the callers to do this properly.  The dabtree block walker and the
dabtree path checker functions to complain any time the da node / leafn
verifiers fail.  Fix the directory block walking functions to complain
about EFSCORRUPTED, since they already dealt with EFSBADCRC.

Found by running xfs/496 against lhdr.stale = middlebit.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
Reviewed-by: Christoph Hellwig <hch@lst.de>
---
 repair/da_util.c |   25 ++++++++++++++++---------
 repair/dir2.c    |   21 +++++++++++++++++++++
 2 files changed, 37 insertions(+), 9 deletions(-)


diff --git a/repair/da_util.c b/repair/da_util.c
index 5061880f..7239c2e2 100644
--- a/repair/da_util.c
+++ b/repair/da_util.c
@@ -134,6 +134,15 @@ _("can't read %s block %u for inode %" PRIu64 "\n"),
 			goto error_out;
 		}
 
+		/* corrupt leafn/node; rebuild the dir. */
+		if (bp->b_error == -EFSBADCRC || bp->b_error == -EFSCORRUPTED) {
+			do_warn(
+_("corrupt %s tree block %u for inode %" PRIu64 "\n"),
+				FORKNAME(whichfork), bno, da_cursor->ino);
+			libxfs_buf_relse(bp);
+			goto error_out;
+		}
+
 		node = bp->b_addr;
 		libxfs_da3_node_hdr_from_disk(mp, &nodehdr, node);
 
@@ -160,15 +169,6 @@ _("bad %s magic number 0x%x in inode %" PRIu64 " bno = %u\n"),
 			goto error_out;
 		}
 
-		/* corrupt node; rebuild the dir. */
-		if (bp->b_error == -EFSBADCRC || bp->b_error == -EFSCORRUPTED) {
-			libxfs_buf_relse(bp);
-			do_warn(
-_("corrupt %s tree block %u for inode %" PRIu64 "\n"),
-				FORKNAME(whichfork), bno, da_cursor->ino);
-			goto error_out;
-		}
-
 		if (nodehdr.count > geo->node_ents) {
 			do_warn(
 _("bad %s record count in inode %" PRIu64 ", count = %d, max = %d\n"),
@@ -562,6 +562,13 @@ _("can't read %s block %u for inode %" PRIu64 "\n"),
 				FORKNAME(whichfork), dabno, cursor->ino);
 			return 1;
 		}
+		if (bp->b_error == -EFSCORRUPTED || bp->b_error == -EFSBADCRC) {
+			do_warn(
+_("corrupt %s tree block %u for inode %" PRIu64 "\n"),
+				FORKNAME(whichfork), dabno, cursor->ino);
+			libxfs_buf_relse(bp);
+			return 1;
+		}
 
 		newnode = bp->b_addr;
 		libxfs_da3_node_hdr_from_disk(mp, &nodehdr, newnode);
diff --git a/repair/dir2.c b/repair/dir2.c
index cbbce601..b374bc7b 100644
--- a/repair/dir2.c
+++ b/repair/dir2.c
@@ -983,6 +983,13 @@ _("can't read block %u for directory inode %" PRIu64 "\n"),
 			mp->m_dir_geo->datablk, ino);
 		return 1;
 	}
+	if (bp->b_error == -EFSCORRUPTED) {
+		do_warn(
+_("corrupt directory block %u for inode %" PRIu64 "\n"),
+			mp->m_dir_geo->datablk, ino);
+		libxfs_buf_relse(bp);
+		return 1;
+	}
 	/*
 	 * Verify the block
 	 */
@@ -1122,6 +1129,13 @@ _("can't read file block %u for directory inode %" PRIu64 "\n"),
 				da_bno, ino);
 			goto error_out;
 		}
+		if (bp->b_error == -EFSCORRUPTED) {
+			do_warn(
+_("corrupt directory leafn block %u for inode %" PRIu64 "\n"),
+				da_bno, ino);
+			libxfs_buf_relse(bp);
+			goto error_out;
+		}
 		leaf = bp->b_addr;
 		libxfs_dir2_leaf_hdr_from_disk(mp, &leafhdr, leaf);
 		/*
@@ -1324,6 +1338,13 @@ _("can't read block %" PRIu64 " for directory inode %" PRIu64 "\n"),
 				dbno, ino);
 			continue;
 		}
+		if (bp->b_error == -EFSCORRUPTED) {
+			do_warn(
+_("corrupt directory data block %lu for inode %" PRIu64 "\n"),
+				dbno, ino);
+			libxfs_buf_relse(bp);
+			continue;
+		}
 		data = bp->b_addr;
 		if (!(be32_to_cpu(data->magic) == XFS_DIR2_DATA_MAGIC ||
 		      be32_to_cpu(data->magic) == XFS_DIR3_DATA_MAGIC))

[PATCH 03/17] xfs_repair: warn when we would have rebuilt a directory

[Darrick J. Wong]

From: Darrick J. Wong <darrick.wong@oracle.com>

longform_dir2_entry_check should warn the user when we would have
rebuilt a directory had -n not been given on the command line.  The
missing warning results in repair returning 0 (all clean) when in fact
there were things that it would have fixed.

Found by running xfs/496 against lents[0].hashval = middlebit.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
Reviewed-by: Christoph Hellwig <hch@lst.de>
---
 repair/phase6.c |    3 +++
 1 file changed, 3 insertions(+)


diff --git a/repair/phase6.c b/repair/phase6.c
index 5e3b394a..b6391326 100644
--- a/repair/phase6.c
+++ b/repair/phase6.c
@@ -2425,6 +2425,9 @@ longform_dir2_entry_check(xfs_mount_t	*mp,
 		*num_illegal = 0;
 		*need_dot = 0;
 	} else {
+		if (fixit || dotdot_update)
+			do_warn(
+	_("would rebuild directory inode %" PRIu64 "\n"), ino);
 		for (i = 0; i < num_bps; i++)
 			if (bplist[i])
 				libxfs_buf_relse(bplist[i]);

[PATCH 04/17] xfs_repair: check for AG btree records that would wrap around

[Darrick J. Wong]

From: Darrick J. Wong <darrick.wong@oracle.com>

For AG btree types, make sure that each record's length is not so huge
that integer wraparound would happen.

Found via xfs/358 fuzzing recs[1].blockcount = ones.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
Reviewed-by: Christoph Hellwig <hch@lst.de>
---
 repair/scan.c |    9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)


diff --git a/repair/scan.c b/repair/scan.c
index 5c8d8b23..1ddb5763 100644
--- a/repair/scan.c
+++ b/repair/scan.c
@@ -684,7 +684,8 @@ _("%s freespace btree block claimed (state %d), agno %d, bno %d, suspect %d\n"),
 					b, i, name, agno, bno);
 				continue;
 			}
-			if (len == 0 || !verify_agbno(mp, agno, end - 1)) {
+			if (len == 0 || end <= b ||
+			    !verify_agbno(mp, agno, end - 1)) {
 				do_warn(
 	_("invalid length %u in record %u of %s btree block %u/%u\n"),
 					len, i, name, agno, bno);
@@ -1066,7 +1067,8 @@ _("%s rmap btree block claimed (state %d), agno %d, bno %d, suspect %d\n"),
 					b, i, name, agno, bno);
 				continue;
 			}
-			if (len == 0 || !verify_agbno(mp, agno, end - 1)) {
+			if (len == 0 || end <= b ||
+			    !verify_agbno(mp, agno, end - 1)) {
 				do_warn(
 	_("invalid length %u in record %u of %s btree block %u/%u\n"),
 					len, i, name, agno, bno);
@@ -1353,7 +1355,8 @@ _("leftover CoW extent has invalid startblock in record %u of %s btree block %u/
 					b, i, name, agno, bno);
 				continue;
 			}
-			if (len == 0 || !verify_agbno(mp, agno, end - 1)) {
+			if (len == 0 || end <= agb ||
+			    !verify_agbno(mp, agno, end - 1)) {
 				do_warn(
 	_("invalid length %u in record %u of %s btree block %u/%u\n"),
 					len, i, name, agno, bno);

[PATCH 05/17] xfs_repair: fix bnobt and refcountbt record order checks

[Darrick J. Wong]

From: Darrick J. Wong <darrick.wong@oracle.com>

The bnobt and refcountbt scanners attempt to check that records are in
the correct order.  However, the lastblock variable in both functions
ought to be set to the end of the previous record (instead of the start)
because otherwise we fail to catch overlapping records, which are not
allowed in either btree type.

Found by running xfs/410 with recs[1].blockcount = middlebit.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
Reviewed-by: Christoph Hellwig <hch@lst.de>
---
 repair/scan.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)


diff --git a/repair/scan.c b/repair/scan.c
index 1ddb5763..2d156d64 100644
--- a/repair/scan.c
+++ b/repair/scan.c
@@ -699,7 +699,7 @@ _("%s freespace btree block claimed (state %d), agno %d, bno %d, suspect %d\n"),
 	"out-of-order bno btree record %d (%u %u) block %u/%u\n"),
 						i, b, len, agno, bno);
 				} else {
-					lastblock = b;
+					lastblock = end - 1;
 				}
 			} else {
 				agcnts->fdblocks += len;
@@ -1396,7 +1396,7 @@ _("extent (%u/%u) len %u claimed, state is %d\n"),
 	"out-of-order %s btree record %d (%u %u) block %u/%u\n"),
 					name, i, b, len, agno, bno);
 			} else {
-				lastblock = b;
+				lastblock = end - 1;
 			}
 
 			/* Is this record mergeable with the last one? */

[PATCH 06/17] xfs_repair: check for out-of-order inobt records

[Darrick J. Wong]

From: Darrick J. Wong <darrick.wong@oracle.com>

Make sure that the inode btree records are in order.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
Reviewed-by: Christoph Hellwig <hch@lst.de>
---
 repair/scan.c |   10 ++++++++++
 1 file changed, 10 insertions(+)


diff --git a/repair/scan.c b/repair/scan.c
index 2d156d64..7c46ab89 100644
--- a/repair/scan.c
+++ b/repair/scan.c
@@ -1947,6 +1947,7 @@ scan_inobt(
 	const struct xfs_buf_ops *ops)
 {
 	struct aghdr_cnts	*agcnts = priv;
+	xfs_agino_t		lastino = 0;
 	int			i;
 	int			numrecs;
 	int			state;
@@ -2029,7 +2030,16 @@ _("inode btree block claimed (state %d), agno %d, bno %d, suspect %d\n"),
 		 * the block.  skip processing of bogus records.
 		 */
 		for (i = 0; i < numrecs; i++) {
+			xfs_agino_t	startino;
+
 			freecount = inorec_get_freecount(mp, &rp[i]);
+			startino = be32_to_cpu(rp[i].ir_startino);
+			if (i > 0 && startino <= lastino)
+				do_warn(_(
+	"out-of-order ino btree record %d (%u) block %u/%u\n"),
+						i, startino, agno, bno);
+			else
+				lastino = startino + XFS_INODES_PER_CHUNK - 1;
 
 			if (magic == XFS_IBT_MAGIC ||
 			    magic == XFS_IBT_CRC_MAGIC) {

[PATCH 07/17] xfs_repair: fix rmapbt record order check

[Darrick J. Wong]

From: Darrick J. Wong <darrick.wong@oracle.com>

The rmapbt record order checks here don't quite work properly.  For
non-shared filesystems, we fail to check that the startblock of the nth
record comes entirely after the previous record.

However, for filesystems with shared blocks (reflink) we correctly check
that the startblock/owner/offset of the nth record comes after the
previous one.

Therefore, make the reflink fs checks use "laststartblock" to preserve
that functionality while making the non-reflink fs checks use
"lastblock" to fix the problem outlined above.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
Reviewed-by: Christoph Hellwig <hch@lst.de>
---
 repair/scan.c |   12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)


diff --git a/repair/scan.c b/repair/scan.c
index 7c46ab89..7508f7e8 100644
--- a/repair/scan.c
+++ b/repair/scan.c
@@ -925,15 +925,15 @@ struct rmap_priv {
 static bool
 rmap_in_order(
 	xfs_agblock_t	b,
-	xfs_agblock_t	lastblock,
+	xfs_agblock_t	laststartblock,
 	uint64_t	owner,
 	uint64_t	lastowner,
 	uint64_t	offset,
 	uint64_t	lastoffset)
 {
-	if (b > lastblock)
+	if (b > laststartblock)
 		return true;
-	else if (b < lastblock)
+	else if (b < laststartblock)
 		return false;
 
 	if (owner > lastowner)
@@ -964,6 +964,7 @@ scan_rmapbt(
 	int			hdr_errors = 0;
 	int			numrecs;
 	int			state;
+	xfs_agblock_t		laststartblock = 0;
 	xfs_agblock_t		lastblock = 0;
 	uint64_t		lastowner = 0;
 	uint64_t		lastoffset = 0;
@@ -1101,14 +1102,15 @@ _("%s rmap btree block claimed (state %d), agno %d, bno %d, suspect %d\n"),
 			/* Check for out of order records. */
 			if (i == 0) {
 advance:
-				lastblock = b;
+				laststartblock = b;
+				lastblock = end - 1;
 				lastowner = owner;
 				lastoffset = offset;
 			} else {
 				bool bad;
 
 				if (xfs_sb_version_hasreflink(&mp->m_sb))
-					bad = !rmap_in_order(b, lastblock,
+					bad = !rmap_in_order(b, laststartblock,
 							owner, lastowner,
 							offset, lastoffset);
 				else

[PATCH 08/17] xfs_repair: tag inobt vs finobt errors properly

[Darrick J. Wong]

From: Darrick J. Wong <darrick.wong@oracle.com>

Amend the generic inode btree block scanner function to tag correctly
which tree it's complaining about.  Previously, dubious finobt headers
would be attributed to the "inode btree", which is at best ambiguous
and misleading at worst.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
Reviewed-by: Christoph Hellwig <hch@lst.de>
---
 repair/scan.c |   36 ++++++++++++++++++++++++++----------
 1 file changed, 26 insertions(+), 10 deletions(-)


diff --git a/repair/scan.c b/repair/scan.c
index 7508f7e8..fff54ecf 100644
--- a/repair/scan.c
+++ b/repair/scan.c
@@ -1949,6 +1949,7 @@ scan_inobt(
 	const struct xfs_buf_ops *ops)
 {
 	struct aghdr_cnts	*agcnts = priv;
+	char			*name;
 	xfs_agino_t		lastino = 0;
 	int			i;
 	int			numrecs;
@@ -1961,17 +1962,32 @@ scan_inobt(
 
 	hdr_errors = 0;
 
+	switch (magic) {
+	case XFS_FIBT_MAGIC:
+	case XFS_FIBT_CRC_MAGIC:
+		name = "fino";
+		break;
+	case XFS_IBT_MAGIC:
+	case XFS_IBT_CRC_MAGIC:
+		name = "ino";
+		break;
+	default:
+		name = "(unknown)";
+		assert(0);
+		break;
+	}
+
 	if (be32_to_cpu(block->bb_magic) != magic) {
-		do_warn(_("bad magic # %#x in inobt block %d/%d\n"),
-			be32_to_cpu(block->bb_magic), agno, bno);
+		do_warn(_("bad magic # %#x in %sbt block %d/%d\n"),
+			be32_to_cpu(block->bb_magic), name, agno, bno);
 		hdr_errors++;
 		bad_ino_btree = 1;
 		if (suspect)
 			return;
 	}
 	if (be16_to_cpu(block->bb_level) != level) {
-		do_warn(_("expected level %d got %d in inobt block %d/%d\n"),
-			level, be16_to_cpu(block->bb_level), agno, bno);
+		do_warn(_("expected level %d got %d in %sbt block %d/%d\n"),
+			level, be16_to_cpu(block->bb_level), name, agno, bno);
 		hdr_errors++;
 		bad_ino_btree = 1;
 		if (suspect)
@@ -1993,8 +2009,8 @@ scan_inobt(
 	default:
 		set_bmap(agno, bno, XR_E_MULT);
 		do_warn(
-_("inode btree block claimed (state %d), agno %d, bno %d, suspect %d\n"),
-			state, agno, bno, suspect);
+_("%sbt btree block claimed (state %d), agno %d, bno %d, suspect %d\n"),
+			name, state, agno, bno, suspect);
 	}
 
 	numrecs = be16_to_cpu(block->bb_numrecs);
@@ -2016,8 +2032,8 @@ _("inode btree block claimed (state %d), agno %d, bno %d, suspect %d\n"),
 
 		if (hdr_errors)  {
 			bad_ino_btree = 1;
-			do_warn(_("dubious inode btree block header %d/%d\n"),
-				agno, bno);
+			do_warn(_("dubious %sbt btree block header %d/%d\n"),
+				name, agno, bno);
 			suspect++;
 		}
 
@@ -2038,8 +2054,8 @@ _("inode btree block claimed (state %d), agno %d, bno %d, suspect %d\n"),
 			startino = be32_to_cpu(rp[i].ir_startino);
 			if (i > 0 && startino <= lastino)
 				do_warn(_(
-	"out-of-order ino btree record %d (%u) block %u/%u\n"),
-						i, startino, agno, bno);
+	"out-of-order %s btree record %d (%u) block %u/%u\n"),
+						name, i, startino, agno, bno);
 			else
 				lastino = startino + XFS_INODES_PER_CHUNK - 1;
 

[PATCH 09/17] xfs_repair: complain about bad interior btree pointers

[Darrick J. Wong]

From: Darrick J. Wong <darrick.wong@oracle.com>

Actually complain about garbage btree node pointers, don't just silently
ignore them.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
Reviewed-by: Christoph Hellwig <hch@lst.de>
---
 libxfs/libxfs_api_defs.h |    1 +
 repair/scan.c            |   55 +++++++++++++++++++++++++++++++++-------------
 2 files changed, 41 insertions(+), 15 deletions(-)


diff --git a/libxfs/libxfs_api_defs.h b/libxfs/libxfs_api_defs.h
index 4462036b..7b264ff2 100644
--- a/libxfs/libxfs_api_defs.h
+++ b/libxfs/libxfs_api_defs.h
@@ -180,6 +180,7 @@
 #define xfs_trans_roll_inode		libxfs_trans_roll_inode
 #define xfs_trans_roll			libxfs_trans_roll
 
+#define xfs_verify_agbno		libxfs_verify_agbno
 #define xfs_verify_cksum		libxfs_verify_cksum
 #define xfs_verify_dir_ino		libxfs_verify_dir_ino
 #define xfs_verify_ino			libxfs_verify_ino
diff --git a/repair/scan.c b/repair/scan.c
index fff54ecf..719ad035 100644
--- a/repair/scan.c
+++ b/repair/scan.c
@@ -779,6 +779,14 @@ _("%s freespace btree block claimed (state %d), agno %d, bno %d, suspect %d\n"),
 	for (i = 0; i < numrecs; i++)  {
 		xfs_agblock_t		agbno = be32_to_cpu(pp[i]);
 
+		if (!libxfs_verify_agbno(mp, agno, agbno)) {
+			do_warn(
+	_("bad btree pointer (%u) in %sbt block %u/%u\n"),
+				agbno, name, agno, bno);
+			suspect++;
+			return;
+		}
+
 		/*
 		 * XXX - put sibling detection right here.
 		 * we know our sibling chain is good.  So as we go,
@@ -788,10 +796,8 @@ _("%s freespace btree block claimed (state %d), agno %d, bno %d, suspect %d\n"),
 		 * pointer mismatch, try and extract as much data
 		 * as possible.
 		 */
-		if (agbno != 0 && verify_agbno(mp, agno, agbno)) {
-			scan_sbtree(agbno, level, agno, suspect, scan_allocbt,
-				    0, magic, priv, ops);
-		}
+		scan_sbtree(agbno, level, agno, suspect, scan_allocbt, 0,
+				magic, priv, ops);
 	}
 }
 
@@ -1234,10 +1240,16 @@ _("%s rmap btree block claimed (state %d), agno %d, bno %d, suspect %d\n"),
 			continue;
 		}
 
-		if (agbno != 0 && verify_agbno(mp, agno, agbno)) {
-			scan_sbtree(agbno, level, agno, suspect, scan_rmapbt, 0,
-				    magic, priv, ops);
+		if (!libxfs_verify_agbno(mp, agno, agbno)) {
+			do_warn(
+	_("bad btree pointer (%u) in %sbt block %u/%u\n"),
+				agbno, name, agno, bno);
+			suspect++;
+			return;
 		}
+
+		scan_sbtree(agbno, level, agno, suspect, scan_rmapbt, 0, magic,
+				priv, ops);
 	}
 
 out:
@@ -1454,10 +1466,16 @@ _("extent (%u/%u) len %u claimed, state is %d\n"),
 	for (i = 0; i < numrecs; i++)  {
 		xfs_agblock_t		agbno = be32_to_cpu(pp[i]);
 
-		if (agbno != 0 && verify_agbno(mp, agno, agbno)) {
-			scan_sbtree(agbno, level, agno, suspect, scan_refcbt, 0,
-				    magic, priv, ops);
+		if (!libxfs_verify_agbno(mp, agno, agbno)) {
+			do_warn(
+	_("bad btree pointer (%u) in %sbt block %u/%u\n"),
+				agbno, name, agno, bno);
+			suspect++;
+			return;
 		}
+
+		scan_sbtree(agbno, level, agno, suspect, scan_refcbt, 0, magic,
+				priv, ops);
 	}
 out:
 	if (suspect)
@@ -2125,11 +2143,18 @@ _("%sbt btree block claimed (state %d), agno %d, bno %d, suspect %d\n"),
 	}
 
 	for (i = 0; i < numrecs; i++)  {
-		if (be32_to_cpu(pp[i]) != 0 && verify_agbno(mp, agno,
-							be32_to_cpu(pp[i])))
-			scan_sbtree(be32_to_cpu(pp[i]), level, agno,
-					suspect, scan_inobt, 0, magic, priv,
-					ops);
+		xfs_agblock_t	agbno = be32_to_cpu(pp[i]);
+
+		if (!libxfs_verify_agbno(mp, agno, agbno)) {
+			do_warn(
+	_("bad btree pointer (%u) in %sbt block %u/%u\n"),
+				agbno, name, agno, bno);
+			suspect++;
+			return;
+		}
+
+		scan_sbtree(be32_to_cpu(pp[i]), level, agno, suspect,
+				scan_inobt, 0, magic, priv, ops);
 	}
 }
 

[PATCH 10/17] xfs_repair: convert to libxfs_verify_agbno

[Darrick J. Wong]

From: Darrick J. Wong <darrick.wong@oracle.com>

Convert the homegrown verify_agbno callers to use the libxfs function,
as needed.  In some places we drop the "bno != 0" checks because those
conditionals are checking btree roots; btree roots should never be
zero if the corresponding feature bit is set; and repair skips the if
clause entirely if the feature bit is disabled.

In effect, this strengthens repair to validate that AG btree pointers
neither point to the AG headers nor past the end of the AG.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
Reviewed-by: Christoph Hellwig <hch@lst.de>
---
 libxfs/libxfs_api_defs.h |    1 +
 repair/dinode.c          |   11 -----------
 repair/dinode.h          |    5 -----
 repair/scan.c            |   36 +++++++++++++++++++++++-------------
 4 files changed, 24 insertions(+), 29 deletions(-)


diff --git a/libxfs/libxfs_api_defs.h b/libxfs/libxfs_api_defs.h
index 7b264ff2..c03f0efa 100644
--- a/libxfs/libxfs_api_defs.h
+++ b/libxfs/libxfs_api_defs.h
@@ -20,6 +20,7 @@
 #define xfs_agfl_walk			libxfs_agfl_walk
 
 #define xfs_ag_init_headers		libxfs_ag_init_headers
+#define xfs_ag_block_count		libxfs_ag_block_count
 
 #define xfs_alloc_ag_max_usable		libxfs_alloc_ag_max_usable
 #define xfs_allocbt_maxrecs		libxfs_allocbt_maxrecs
diff --git a/repair/dinode.c b/repair/dinode.c
index 1f1cc26b..b343534c 100644
--- a/repair/dinode.c
+++ b/repair/dinode.c
@@ -255,17 +255,6 @@ verify_dfsbno_range(xfs_mount_t	*mp,
 	return (XR_DFSBNORANGE_VALID);
 }
 
-int
-verify_agbno(xfs_mount_t	*mp,
-		xfs_agnumber_t	agno,
-		xfs_agblock_t	agbno)
-{
-	xfs_sb_t	*sbp = &mp->m_sb;;
-
-	/* range check ag #, ag block.  range-checking offset is pointless */
-	return verify_ag_bno(sbp, agno, agbno) == 0;
-}
-
 static int
 process_rt_rec(
 	xfs_mount_t		*mp,
diff --git a/repair/dinode.h b/repair/dinode.h
index 98238357..c8e563b5 100644
--- a/repair/dinode.h
+++ b/repair/dinode.h
@@ -9,11 +9,6 @@
 struct blkmap;
 struct prefetch_args;
 
-int
-verify_agbno(xfs_mount_t	*mp,
-		xfs_agnumber_t	agno,
-		xfs_agblock_t	agbno);
-
 int
 verify_dfsbno(xfs_mount_t	*mp,
 		xfs_fsblock_t	fsbno);
diff --git a/repair/scan.c b/repair/scan.c
index 719ad035..8e81c552 100644
--- a/repair/scan.c
+++ b/repair/scan.c
@@ -678,14 +678,14 @@ _("%s freespace btree block claimed (state %d), agno %d, bno %d, suspect %d\n"),
 			len = be32_to_cpu(rp[i].ar_blockcount);
 			end = b + len;
 
-			if (b == 0 || !verify_agbno(mp, agno, b)) {
+			if (!libxfs_verify_agbno(mp, agno, b)) {
 				do_warn(
 	_("invalid start block %u in record %u of %s btree block %u/%u\n"),
 					b, i, name, agno, bno);
 				continue;
 			}
 			if (len == 0 || end <= b ||
-			    !verify_agbno(mp, agno, end - 1)) {
+			    !libxfs_verify_agbno(mp, agno, end - 1)) {
 				do_warn(
 	_("invalid length %u in record %u of %s btree block %u/%u\n"),
 					len, i, name, agno, bno);
@@ -950,6 +950,16 @@ rmap_in_order(
 	return offset > lastoffset;
 }
 
+static inline bool
+verify_rmap_agbno(
+	struct xfs_mount	*mp,
+	xfs_agnumber_t		agno,
+	xfs_agblock_t		agbno)
+{
+	return agbno < libxfs_ag_block_count(mp, agno);
+}
+
+
 static void
 scan_rmapbt(
 	struct xfs_btree_block	*block,
@@ -1068,14 +1078,14 @@ _("%s rmap btree block claimed (state %d), agno %d, bno %d, suspect %d\n"),
 			end = key.rm_startblock + key.rm_blockcount;
 
 			/* Make sure agbno & len make sense. */
-			if (!verify_agbno(mp, agno, b)) {
+			if (!verify_rmap_agbno(mp, agno, b)) {
 				do_warn(
 	_("invalid start block %u in record %u of %s btree block %u/%u\n"),
 					b, i, name, agno, bno);
 				continue;
 			}
 			if (len == 0 || end <= b ||
-			    !verify_agbno(mp, agno, end - 1)) {
+			    !verify_rmap_agbno(mp, agno, end - 1)) {
 				do_warn(
 	_("invalid length %u in record %u of %s btree block %u/%u\n"),
 					len, i, name, agno, bno);
@@ -1363,14 +1373,14 @@ _("leftover CoW extent has invalid startblock in record %u of %s btree block %u/
 			}
 			end = agb + len;
 
-			if (!verify_agbno(mp, agno, agb)) {
+			if (!libxfs_verify_agbno(mp, agno, agb)) {
 				do_warn(
 	_("invalid start block %u in record %u of %s btree block %u/%u\n"),
 					b, i, name, agno, bno);
 				continue;
 			}
 			if (len == 0 || end <= agb ||
-			    !verify_agbno(mp, agno, end - 1)) {
+			    !libxfs_verify_agbno(mp, agno, end - 1)) {
 				do_warn(
 	_("invalid length %u in record %u of %s btree block %u/%u\n"),
 					len, i, name, agno, bno);
@@ -2171,7 +2181,7 @@ scan_agfl(
 {
 	struct agfl_state	*as = priv;
 
-	if (verify_agbno(mp, as->agno, bno))
+	if (libxfs_verify_agbno(mp, as->agno, bno))
 		set_bmap(as->agno, bno, XR_E_FREE);
 	else
 		do_warn(_("bad agbno %u in agfl, agno %d\n"),
@@ -2244,7 +2254,7 @@ validate_agf(
 	uint32_t		magic;
 
 	bno = be32_to_cpu(agf->agf_roots[XFS_BTNUM_BNO]);
-	if (bno != 0 && verify_agbno(mp, agno, bno)) {
+	if (libxfs_verify_agbno(mp, agno, bno)) {
 		magic = xfs_sb_version_hascrc(&mp->m_sb) ? XFS_ABTB_CRC_MAGIC
 							 : XFS_ABTB_MAGIC;
 		scan_sbtree(bno, be32_to_cpu(agf->agf_levels[XFS_BTNUM_BNO]),
@@ -2256,7 +2266,7 @@ validate_agf(
 	}
 
 	bno = be32_to_cpu(agf->agf_roots[XFS_BTNUM_CNT]);
-	if (bno != 0 && verify_agbno(mp, agno, bno)) {
+	if (libxfs_verify_agbno(mp, agno, bno)) {
 		magic = xfs_sb_version_hascrc(&mp->m_sb) ? XFS_ABTC_CRC_MAGIC
 							 : XFS_ABTC_MAGIC;
 		scan_sbtree(bno, be32_to_cpu(agf->agf_levels[XFS_BTNUM_CNT]),
@@ -2276,7 +2286,7 @@ validate_agf(
 		priv.last_rec.rm_owner = XFS_RMAP_OWN_UNKNOWN;
 		priv.nr_blocks = 0;
 		bno = be32_to_cpu(agf->agf_roots[XFS_BTNUM_RMAP]);
-		if (bno != 0 && verify_agbno(mp, agno, bno)) {
+		if (libxfs_verify_agbno(mp, agno, bno)) {
 			scan_sbtree(bno,
 				    be32_to_cpu(agf->agf_levels[XFS_BTNUM_RMAP]),
 				    agno, 0, scan_rmapbt, 1, XFS_RMAP_CRC_MAGIC,
@@ -2294,7 +2304,7 @@ validate_agf(
 
 	if (xfs_sb_version_hasreflink(&mp->m_sb)) {
 		bno = be32_to_cpu(agf->agf_refcount_root);
-		if (bno != 0 && verify_agbno(mp, agno, bno)) {
+		if (libxfs_verify_agbno(mp, agno, bno)) {
 			struct refc_priv	priv;
 
 			memset(&priv, 0, sizeof(priv));
@@ -2342,7 +2352,7 @@ validate_agi(
 	uint32_t		magic;
 
 	bno = be32_to_cpu(agi->agi_root);
-	if (bno != 0 && verify_agbno(mp, agno, bno)) {
+	if (libxfs_verify_agbno(mp, agno, bno)) {
 		magic = xfs_sb_version_hascrc(&mp->m_sb) ? XFS_IBT_CRC_MAGIC
 							 : XFS_IBT_MAGIC;
 		scan_sbtree(bno, be32_to_cpu(agi->agi_level),
@@ -2355,7 +2365,7 @@ validate_agi(
 
 	if (xfs_sb_version_hasfinobt(&mp->m_sb)) {
 		bno = be32_to_cpu(agi->agi_free_root);
-		if (bno != 0 && verify_agbno(mp, agno, bno)) {
+		if (libxfs_verify_agbno(mp, agno, bno)) {
 			magic = xfs_sb_version_hascrc(&mp->m_sb) ?
 					XFS_FIBT_CRC_MAGIC : XFS_FIBT_MAGIC;
 			scan_sbtree(bno, be32_to_cpu(agi->agi_free_level),

[PATCH 11/17] xfs_repair: refactor verify_dfsbno_range

[Darrick J. Wong]

From: Darrick J. Wong <darrick.wong@oracle.com>

Refactor this function to use libxfs type checking helpers.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
Reviewed-by: Christoph Hellwig <hch@lst.de>
---
 libxfs/libxfs_api_defs.h |    1 +
 repair/dinode.c          |   26 +++++++++-----------------
 2 files changed, 10 insertions(+), 17 deletions(-)


diff --git a/libxfs/libxfs_api_defs.h b/libxfs/libxfs_api_defs.h
index c03f0efa..69f79a08 100644
--- a/libxfs/libxfs_api_defs.h
+++ b/libxfs/libxfs_api_defs.h
@@ -184,6 +184,7 @@
 #define xfs_verify_agbno		libxfs_verify_agbno
 #define xfs_verify_cksum		libxfs_verify_cksum
 #define xfs_verify_dir_ino		libxfs_verify_dir_ino
+#define xfs_verify_fsbno		libxfs_verify_fsbno
 #define xfs_verify_ino			libxfs_verify_ino
 #define xfs_verify_rtbno		libxfs_verify_rtbno
 #define xfs_zero_extent			libxfs_zero_extent
diff --git a/repair/dinode.c b/repair/dinode.c
index b343534c..135703d9 100644
--- a/repair/dinode.c
+++ b/repair/dinode.c
@@ -228,31 +228,23 @@ verify_dfsbno(xfs_mount_t	*mp,
 #define XR_DFSBNORANGE_OVERFLOW	3
 
 static __inline int
-verify_dfsbno_range(xfs_mount_t	*mp,
-		xfs_fsblock_t	fsbno,
-		xfs_filblks_t	count)
+verify_dfsbno_range(
+	struct xfs_mount	*mp,
+	xfs_fsblock_t		fsbno,
+	xfs_filblks_t		count)
 {
-	xfs_agnumber_t	agno;
-	xfs_agblock_t	agbno;
-	xfs_sb_t	*sbp = &mp->m_sb;;
-
 	/* the start and end blocks better be in the same allocation group */
-	agno = XFS_FSB_TO_AGNO(mp, fsbno);
-	if (agno != XFS_FSB_TO_AGNO(mp, fsbno + count - 1)) {
+	if (XFS_FSB_TO_AGNO(mp, fsbno) !=
+	    XFS_FSB_TO_AGNO(mp, fsbno + count - 1)) {
 		return XR_DFSBNORANGE_OVERFLOW;
 	}
 
-	agbno = XFS_FSB_TO_AGBNO(mp, fsbno);
-	if (verify_ag_bno(sbp, agno, agbno)) {
+	if (!libxfs_verify_fsbno(mp, fsbno))
 		return XR_DFSBNORANGE_BADSTART;
-	}
-
-	agbno = XFS_FSB_TO_AGBNO(mp, fsbno + count - 1);
-	if (verify_ag_bno(sbp, agno, agbno)) {
+	if (!libxfs_verify_fsbno(mp, fsbno + count - 1))
 		return XR_DFSBNORANGE_BADEND;
-	}
 
-	return (XR_DFSBNORANGE_VALID);
+	return XR_DFSBNORANGE_VALID;
 }
 
 static int

[PATCH 12/17] xfs_repair: remove verify_dfsbno

[Darrick J. Wong]

From: Darrick J. Wong <darrick.wong@oracle.com>

Replace this homegrown helper with its libxfs equivalent.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
Reviewed-by: Christoph Hellwig <hch@lst.de>
---
 repair/attr_repair.c |    2 +-
 repair/dinode.c      |   21 +--------------------
 repair/dinode.h      |    4 ----
 repair/prefetch.c    |    9 +++++----
 repair/scan.c        |    2 +-
 5 files changed, 8 insertions(+), 30 deletions(-)


diff --git a/repair/attr_repair.c b/repair/attr_repair.c
index 5f884033..6cec0f70 100644
--- a/repair/attr_repair.c
+++ b/repair/attr_repair.c
@@ -1092,7 +1092,7 @@ process_longform_attr(
 	}
 
 	/* FIX FOR bug 653709 -- EKN */
-	if (!xfs_verify_fsbno(mp, bno)) {
+	if (!libxfs_verify_fsbno(mp, bno)) {
 		do_warn(
 	_("block in attribute fork of inode %" PRIu64 " is not valid\n"), ino);
 		return 1;
diff --git a/repair/dinode.c b/repair/dinode.c
index 135703d9..67adddd7 100644
--- a/repair/dinode.c
+++ b/repair/dinode.c
@@ -203,25 +203,6 @@ verify_aginum(xfs_mount_t	*mp,
 	return verify_ag_bno(sbp, agno, agbno);
 }
 
-/*
- * return 1 if block number is good, 0 if out of range
- */
-int
-verify_dfsbno(xfs_mount_t	*mp,
-		xfs_fsblock_t	fsbno)
-{
-	xfs_agnumber_t	agno;
-	xfs_agblock_t	agbno;
-	xfs_sb_t	*sbp = &mp->m_sb;;
-
-	/* range check ag #, ag block.  range-checking offset is pointless */
-
-	agno = XFS_FSB_TO_AGNO(mp, fsbno);
-	agbno = XFS_FSB_TO_AGBNO(mp, fsbno);
-
-	return verify_ag_bno(sbp, agno, agbno) == 0;
-}
-
 #define XR_DFSBNORANGE_VALID	0
 #define XR_DFSBNORANGE_BADSTART	1
 #define XR_DFSBNORANGE_BADEND	2
@@ -835,7 +816,7 @@ _("bad numrecs 0 in inode %" PRIu64 " bmap btree root block\n"),
 		 * btree, we'd do it right here.  For now, if there's a
 		 * problem, we'll bail out and presumably clear the inode.
 		 */
-		if (!verify_dfsbno(mp, get_unaligned_be64(&pp[i])))  {
+		if (!libxfs_verify_fsbno(mp, get_unaligned_be64(&pp[i])))  {
 			do_warn(
 _("bad bmap btree ptr 0x%" PRIx64 " in ino %" PRIu64 "\n"),
 				get_unaligned_be64(&pp[i]), lino);
diff --git a/repair/dinode.h b/repair/dinode.h
index c8e563b5..4bf7affd 100644
--- a/repair/dinode.h
+++ b/repair/dinode.h
@@ -9,10 +9,6 @@
 struct blkmap;
 struct prefetch_args;
 
-int
-verify_dfsbno(xfs_mount_t	*mp,
-		xfs_fsblock_t	fsbno);
-
 void
 convert_extent(
 	xfs_bmbt_rec_t		*rp,
diff --git a/repair/prefetch.c b/repair/prefetch.c
index 3ac49db1..686bf7be 100644
--- a/repair/prefetch.c
+++ b/repair/prefetch.c
@@ -188,8 +188,9 @@ pf_read_bmbt_reclist(
 				(irec.br_startoff >= fs_max_file_offset))
 			goto out_free;
 
-		if (!verify_dfsbno(mp, irec.br_startblock) || !verify_dfsbno(mp,
-				irec.br_startblock + irec.br_blockcount - 1))
+		if (!libxfs_verify_fsbno(mp, irec.br_startblock) ||
+		    !libxfs_verify_fsbno(mp, irec.br_startblock +
+					     irec.br_blockcount - 1))
 			goto out_free;
 
 		if (!args->dirs_only && ((irec.br_startoff +
@@ -337,7 +338,7 @@ pf_scanfunc_bmap(
 
 	for (i = 0; i < numrecs; i++) {
 		dbno = get_unaligned_be64(&pp[i]);
-		if (!verify_dfsbno(mp, dbno))
+		if (!libxfs_verify_fsbno(mp, dbno))
 			return 0;
 		if (!pf_scan_lbtree(dbno, level, isadir, args, pf_scanfunc_bmap))
 			return 0;
@@ -379,7 +380,7 @@ pf_read_btinode(
 
 	for (i = 0; i < numrecs; i++) {
 		dbno = get_unaligned_be64(&pp[i]);
-		if (!verify_dfsbno(mp, dbno))
+		if (!libxfs_verify_fsbno(mp, dbno))
 			break;
 		if (!pf_scan_lbtree(dbno, level, isadir, args, pf_scanfunc_bmap))
 			break;
diff --git a/repair/scan.c b/repair/scan.c
index 8e81c552..dcd4864d 100644
--- a/repair/scan.c
+++ b/repair/scan.c
@@ -491,7 +491,7 @@ _("inode %" PRIu64 " bad # of bmap records (%u, min - %u, max - %u)\n"),
 		 * we'd do it right here.  For now, if there's a problem,
 		 * we'll bail out and presumably clear the inode.
 		 */
-		if (!verify_dfsbno(mp, be64_to_cpu(pp[i])))  {
+		if (!libxfs_verify_fsbno(mp, be64_to_cpu(pp[i])))  {
 			do_warn(
 _("bad bmap btree ptr 0x%llx in ino %" PRIu64 "\n"),
 			       (unsigned long long) be64_to_cpu(pp[i]), ino);

[PATCH 13/17] xfs_repair: remove verify_aginum

[Darrick J. Wong]

From: Darrick J. Wong <darrick.wong@oracle.com>

Replace this homegrown inode pointer verification function with the
libxfs checking helper.  This one is a little tricky because this
function (unlike all of its verify_* siblings) returned 1 for bad and 0
for good, so we must invert the checking logic.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
Reviewed-by: Christoph Hellwig <hch@lst.de>
---
 libxfs/libxfs_api_defs.h |    1 +
 repair/dino_chunks.c     |    6 +++--
 repair/dinode.c          |   51 ----------------------------------------------
 repair/dinode.h          |    5 -----
 repair/scan.c            |    4 ++--
 5 files changed, 6 insertions(+), 61 deletions(-)


diff --git a/libxfs/libxfs_api_defs.h b/libxfs/libxfs_api_defs.h
index 69f79a08..be06c763 100644
--- a/libxfs/libxfs_api_defs.h
+++ b/libxfs/libxfs_api_defs.h
@@ -182,6 +182,7 @@
 #define xfs_trans_roll			libxfs_trans_roll
 
 #define xfs_verify_agbno		libxfs_verify_agbno
+#define xfs_verify_agino		libxfs_verify_agino
 #define xfs_verify_cksum		libxfs_verify_cksum
 #define xfs_verify_dir_ino		libxfs_verify_dir_ino
 #define xfs_verify_fsbno		libxfs_verify_fsbno
diff --git a/repair/dino_chunks.c b/repair/dino_chunks.c
index 6685a4d2..399d4998 100644
--- a/repair/dino_chunks.c
+++ b/repair/dino_chunks.c
@@ -1124,7 +1124,7 @@ check_uncertain_aginodes(xfs_mount_t *mp, xfs_agnumber_t agno)
 
 			agino = i + irec->ino_startnum;
 
-			if (verify_aginum(mp, agno, agino))
+			if (!libxfs_verify_agino(mp, agno, agino))
 				continue;
 
 			if (nrec != NULL && nrec->ino_startnum <= agino &&
@@ -1133,7 +1133,7 @@ check_uncertain_aginodes(xfs_mount_t *mp, xfs_agnumber_t agno)
 				continue;
 
 			if ((nrec = find_inode_rec(mp, agno, agino)) == NULL)
-				if (!verify_aginum(mp, agno, agino))
+				if (libxfs_verify_agino(mp, agno, agino))
 					if (verify_aginode_chunk(mp, agno,
 							agino, &start))
 						got_some = 1;
@@ -1215,7 +1215,7 @@ process_uncertain_aginodes(xfs_mount_t *mp, xfs_agnumber_t agno)
 			 * good tree), bad inode numbers, and inode numbers
 			 * pointing to bogus inodes
 			 */
-			if (verify_aginum(mp, agno, agino))
+			if (!libxfs_verify_agino(mp, agno, agino))
 				continue;
 
 			if (nrec != NULL && nrec->ino_startnum <= agino &&
diff --git a/repair/dinode.c b/repair/dinode.c
index 67adddd7..526ecde3 100644
--- a/repair/dinode.c
+++ b/repair/dinode.c
@@ -152,57 +152,6 @@ clear_dinode(xfs_mount_t *mp, xfs_dinode_t *dino, xfs_ino_t ino_num)
  * misc. inode-related utility routines
  */
 
-/*
- * verify_ag_bno is heavily used. In the common case, it
- * performs just two number of compares
- * Returns 1 for bad ag/bno pair or 0 if it's valid.
- */
-static __inline int
-verify_ag_bno(xfs_sb_t *sbp,
-		xfs_agnumber_t agno,
-		xfs_agblock_t agbno)
-{
-	if (agno < (sbp->sb_agcount - 1))
-		return (agbno >= sbp->sb_agblocks);
-	if (agno == (sbp->sb_agcount - 1))
-		return (agbno >= (sbp->sb_dblocks -
-				((xfs_rfsblock_t)(sbp->sb_agcount - 1) *
-				 sbp->sb_agblocks)));
-	return 1;
-}
-
-/*
- * have a separate routine to ensure that we don't accidentally
- * lose illegally set bits in the agino by turning it into an FSINO
- * to feed to the above routine
- */
-int
-verify_aginum(xfs_mount_t	*mp,
-		xfs_agnumber_t	agno,
-		xfs_agino_t	agino)
-{
-	xfs_agblock_t	agbno;
-	xfs_sb_t	*sbp = &mp->m_sb;;
-
-	/* range check ag #, ag block.  range-checking offset is pointless */
-
-	if (agino == 0 || agino == NULLAGINO)
-		return(1);
-
-	/*
-	 * agino's can't be too close to NULLAGINO because the min blocksize
-	 * is 9 bits and at most 1 bit of that gets used for the inode offset
-	 * so if the agino gets shifted by the # of offset bits and compared
-	 * to the legal agbno values, a bogus agino will be too large.  there
-	 * will be extra bits set at the top that shouldn't be set.
-	 */
-	agbno = XFS_AGINO_TO_AGBNO(mp, agino);
-	if (agbno == 0)
-		return 1;
-
-	return verify_ag_bno(sbp, agno, agbno);
-}
-
 #define XR_DFSBNORANGE_VALID	0
 #define XR_DFSBNORANGE_BADSTART	1
 #define XR_DFSBNORANGE_BADEND	2
diff --git a/repair/dinode.h b/repair/dinode.h
index 4bf7affd..1bd0e0b7 100644
--- a/repair/dinode.h
+++ b/repair/dinode.h
@@ -68,11 +68,6 @@ verify_uncertain_dinode(xfs_mount_t *mp,
 		xfs_agnumber_t agno,
 		xfs_agino_t ino);
 
-int
-verify_aginum(xfs_mount_t	*mp,
-		xfs_agnumber_t	agno,
-		xfs_agino_t	agino);
-
 int
 process_uncertain_aginodes(xfs_mount_t		*mp,
 				xfs_agnumber_t	agno);
diff --git a/repair/scan.c b/repair/scan.c
index dcd4864d..76079247 100644
--- a/repair/scan.c
+++ b/repair/scan.c
@@ -1561,7 +1561,7 @@ verify_single_ino_chunk_align(
 	 * (NULLAGINO). if it gets closer, the agino number will be illegal as
 	 * the agbno will be too large.
 	 */
-	if (verify_aginum(mp, agno, ino)) {
+	if (!libxfs_verify_agino(mp, agno, ino)) {
 		do_warn(
 _("bad starting inode # (%" PRIu64 " (0x%x 0x%x)) in %s rec, skipping rec\n"),
 			lino, agno, ino, inobt_name);
@@ -1569,7 +1569,7 @@ _("bad starting inode # (%" PRIu64 " (0x%x 0x%x)) in %s rec, skipping rec\n"),
 		return ++suspect;
 	}
 
-	if (verify_aginum(mp, agno,
+	if (!libxfs_verify_agino(mp, agno,
 			ino + XFS_INODES_PER_CHUNK - 1)) {
 		do_warn(
 _("bad ending inode # (%" PRIu64 " (0x%x 0x%zx)) in %s rec, skipping rec\n"),

[PATCH 14/17] xfs_repair: mark entire free space btree record as free1

[Darrick J. Wong]

From: Darrick J. Wong <darrick.wong@oracle.com>

In scan_allocbt, we iterate each free space btree record (of both bnobt
and cntbt) in the hopes of pushing all the free space from UNKNOWN to
FREE1 to FREE.  Unfortunately, the first time we see a free space record
we only set the first block of that record to FREE1, which means that
the second time we see the record, the first block will get set to FREE,
but the rest of the free space will only make it to FREE1.  This is
incorrect state, so we need to fix that.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
Reviewed-by: Christoph Hellwig <hch@lst.de>
---
 repair/scan.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)


diff --git a/repair/scan.c b/repair/scan.c
index 76079247..505cfc53 100644
--- a/repair/scan.c
+++ b/repair/scan.c
@@ -719,7 +719,7 @@ _("%s freespace btree block claimed (state %d), agno %d, bno %d, suspect %d\n"),
 				state = get_bmap_ext(agno, b, end, &blen);
 				switch (state) {
 				case XR_E_UNKNOWN:
-					set_bmap(agno, b, XR_E_FREE1);
+					set_bmap_ext(agno, b, blen, XR_E_FREE1);
 					break;
 				case XR_E_FREE1:
 					/*

[PATCH 15/17] xfs_repair: complain about free space only seen by one btree

[Darrick J. Wong]

From: Darrick J. Wong <darrick.wong@oracle.com>

During the free space btree walk, scan_allocbt claims in a comment that
we'll catch FREE1 blocks (i.e. blocks that were seen by only one free
space btree) later.  This never happens, with the result that xfs_repair
in dry-run mode can return 0 on a filesystem with corrupt free space
btrees.

Found by fuzzing xfs/358 with numrecs = middlebit (or sub).

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
Reviewed-by: Christoph Hellwig <hch@lst.de>
---
 repair/phase4.c |    7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)


diff --git a/repair/phase4.c b/repair/phase4.c
index 8197db06..a43413c7 100644
--- a/repair/phase4.c
+++ b/repair/phase4.c
@@ -306,6 +306,12 @@ phase4(xfs_mount_t *mp)
 		for (j = ag_hdr_block; j < ag_end; j += blen)  {
 			bstate = get_bmap_ext(i, j, ag_end, &blen);
 			switch (bstate) {
+			case XR_E_FREE1:
+				if (no_modify)
+					do_warn(
+	_("free space (%u,%u-%u) only seen by one free space btree\n"),
+						i, j, j + blen - 1);
+				break;
 			case XR_E_BAD_STATE:
 			default:
 				do_warn(
@@ -313,7 +319,6 @@ phase4(xfs_mount_t *mp)
 					i, j);
 				/* fall through .. */
 			case XR_E_UNKNOWN:
-			case XR_E_FREE1:
 			case XR_E_FREE:
 			case XR_E_INUSE:
 			case XR_E_INUSE_FS:

[PATCH 16/17] xfs_repair: complain about extents in unknown state

[Darrick J. Wong]

From: Darrick J. Wong <darrick.wong@oracle.com>

During phase 4, if we find any extents that are unaccounted for, report
the entire extent, not just the first block.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
Reviewed-by: Christoph Hellwig <hch@lst.de>
---
 repair/phase4.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)


diff --git a/repair/phase4.c b/repair/phase4.c
index a43413c7..191b4842 100644
--- a/repair/phase4.c
+++ b/repair/phase4.c
@@ -315,8 +315,8 @@ phase4(xfs_mount_t *mp)
 			case XR_E_BAD_STATE:
 			default:
 				do_warn(
-				_("unknown block state, ag %d, block %d\n"),
-					i, j);
+				_("unknown block state, ag %d, blocks %u-%u\n"),
+					i, j, j + blen - 1);
 				/* fall through .. */
 			case XR_E_UNKNOWN:
 			case XR_E_FREE:

[PATCH 17/17] xfs_repair: complain about any nonzero inprogress value, not just 1

[Darrick J. Wong]

From: Darrick J. Wong <darrick.wong@oracle.com>

Complain about the primary superblock having any non-zero sb_inprogress
value, not just 1.  This brings repair's behavior into alignment with
xfs_check and the kernel.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
Reviewed-by: Christoph Hellwig <hch@lst.de>
---
 repair/sb.c |    3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)


diff --git a/repair/sb.c b/repair/sb.c
index 91a36dd3..17ce43cc 100644
--- a/repair/sb.c
+++ b/repair/sb.c
@@ -369,8 +369,7 @@ verify_sb(char *sb_buf, xfs_sb_t *sb, int is_primary_sb)
 		return(XR_BAD_VERSION);
 
 	/* does sb think mkfs really finished ? */
-
-	if (is_primary_sb && sb->sb_inprogress == 1)
+	if (is_primary_sb && sb->sb_inprogress)
 		return(XR_BAD_INPROGRESS);
 
 	/*

Re: [PATCH 01/17] xfs_quota: fix unsigned int id comparisons

[Christoph Hellwig]

On Mon, Jun 01, 2020 at 09:25:07PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Fix compiler warnings about unsigned int comparisons by replacing them
> with an explicit check for the one possible invalid value (-1U).
> id_from_string sets exitcode to nonzero when it sees this value, so the
> call sites don't have to do that.
> 
> Coverity-id: 1463855, 1463856, 1463857
> Fixes: 67a73d6139d0 ("xfs_quota: refactor code to generate id from name")
> Fixes: 36dc471cc9bb ("xfs_quota: allow individual timer extension")
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>

Looks good,

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