diff mbox series

[09/16] xfs_repair: convert to libxfs_verify_agbno

Message ID 158904185435.982941.16817943726460132539.stgit@magnolia (mailing list archive)
State Superseded
Headers show
Series xfs_repair: catch things that xfs_check misses | expand

Commit Message

Darrick J. Wong May 9, 2020, 4:30 p.m. UTC 
From: Darrick J. Wong <darrick.wong@oracle.com>

Convert the homegrown verify_agbno callers to use the libxfs function,
as needed.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 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(-)

Comments

Christoph Hellwig May 9, 2020, 5:18 p.m. UTC | #1 
On Sat, May 09, 2020 at 09:30:54AM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Convert the homegrown verify_agbno callers to use the libxfs function,
> as needed.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>

This looks mostly good, but there is one thing I wonder about:

>  	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)) {

Various of these block is non-zero check are going away.  Did you
audit if they weren't used as intentional escapes in a few places?

Either way this should probably be documented in the changelog.
Darrick J. Wong May 11, 2020, 4:22 p.m. UTC | #2 
On Sat, May 09, 2020 at 10:18:30AM -0700, Christoph Hellwig wrote:
> On Sat, May 09, 2020 at 09:30:54AM -0700, Darrick J. Wong wrote:
> > From: Darrick J. Wong <darrick.wong@oracle.com>
> > 
> > Convert the homegrown verify_agbno callers to use the libxfs function,
> > as needed.
> > 
> > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> 
> This looks mostly good, but there is one thing I wonder about:
> 
> >  	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)) {
> 
> Various of these block is non-zero check are going away.  Did you
> audit if they weren't used as intentional escapes in a few places?

Yes.  Each of those "bno != 0" checks occurs in the context of checking
an AG header's pointer to a btree root.  The roots should never be zero
if the corresponding feature is enabled, and we're careful to check the
feature bits first.

AFAICT that bno != 0 check is actually there to cover a deficiency in
the verify_agbno function, which is that it only checked that the
supplied argument didn't go past the end of the AG and did not check
that the pointer didn't point into the AG header block(s).

Checking for a nonzero value is also insufficient, since on a
blocksize < sectorsize * 4 filesystem, the AGFL can end up in a nonzero
agbno.  libxfs_verify_agbno covers all of these cases.

> Either way this should probably be documented in the changelog.

Ok, how about this for a commit message:

"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 point past the end of the AG."

--D
Christoph Hellwig May 12, 2020, 8:07 a.m. UTC | #3 
On Mon, May 11, 2020 at 09:22:03AM -0700, Darrick J. Wong wrote:
> Yes.  Each of those "bno != 0" checks occurs in the context of checking
> an AG header's pointer to a btree root.  The roots should never be zero
> if the corresponding feature is enabled, and we're careful to check the
> feature bits first.
> 
> AFAICT that bno != 0 check is actually there to cover a deficiency in
> the verify_agbno function, which is that it only checked that the
> supplied argument didn't go past the end of the AG and did not check
> that the pointer didn't point into the AG header block(s).
> 
> Checking for a nonzero value is also insufficient, since on a
> blocksize < sectorsize * 4 filesystem, the AGFL can end up in a nonzero
> agbno.  libxfs_verify_agbno covers all of these cases.
> 
> > Either way this should probably be documented in the changelog.
> 
> Ok, how about this for a commit message:
> 
> "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 point past the end of the AG."

With the additional explanation in the commit log:

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

Patch

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),