[PATCH v2 0/7] xfs: fix [f]inobt magic value verification

[PATCH v2 0/7] xfs: fix [f]inobt magic value verification

[Brian Foster]

Hi all,

Here's v2 of the verifier magic value cleanup. This is generally the
same idea as the previous version. The primary difference is that v2
first converts verifiers that use cpu order magic comparisons to use
on-disk (big endian) order and then stores/compares the verifier magic
values in on-disk order. The purpose of this change is to reduce the
number of byte swaps required at runtime for verifier magic value
checks. Further changes include some cleanups, additional refactoring
and the inclusion of Darrick's scrub ->b_ops fix with some modifications
from the original version.

This survives fstests on v4 and v5 filesystems on both little and big
endian systems without regressions. Thoughts, reviews, flames
appreciated.

Brian

v2:
- Include djwong's ->b_ops patch w/ modifications.
- Added patch to fix up existing cpu endian magic checks, fold in typo
  fix.
- Replace static inline magic verifier helper with out of line variant,
  kill macro.
- Store on-disk byte order magics in ->b_ops.
- Added patch to refactor common xfs_da3_blkinfo checks.
v1: https://marc.info/?l=linux-xfs&m=154878684317178&w=2
- Remove endian conversion from helper.
- Drop finobt bad magic mitigation patch.
- Additional verifier magic fixups.
- Add verifier name typo fixup.
rfcv2: https://marc.info/?l=linux-xfs&m=154868884311668&w=2
- Split off finobt verifier into separate patch, assign it
  appropriately.
- Created helpers for xfs_buf_ops magic value verification.
- Added error mitigation patch for problematic finobt blocks.
rfcv1: https://marc.info/?l=linux-xfs&m=154834528212262&w=2

Brian Foster (6):
  xfs: always check magic values in on-disk byte order
  xfs: create a separate finobt verifier
  xfs: distinguish between inobt and finobt magic values
  xfs: use verifier magic field in dir2 leaf verifiers
  xfs: miscellaneous verifier magic value fixups
  xfs: factor xfs_da3_blkinfo verification into common helper

Darrick J. Wong (1):
  xfs: set buffer ops when repair probes for btree type

 fs/xfs/libxfs/xfs_ag.c             |   2 +-
 fs/xfs/libxfs/xfs_alloc.c          |  12 ++--
 fs/xfs/libxfs/xfs_attr_leaf.c      |  21 ++----
 fs/xfs/libxfs/xfs_attr_remote.c    |   8 ++-
 fs/xfs/libxfs/xfs_bmap_btree.c     |  13 ++--
 fs/xfs/libxfs/xfs_da_btree.c       |  50 ++++++++++-----
 fs/xfs/libxfs/xfs_da_format.h      |   2 +
 fs/xfs/libxfs/xfs_dir2_block.c     |  10 +--
 fs/xfs/libxfs/xfs_dir2_data.c      |  12 ++--
 fs/xfs/libxfs/xfs_dir2_leaf.c      | 100 ++++++-----------------------
 fs/xfs/libxfs/xfs_dir2_node.c      |  11 ++--
 fs/xfs/libxfs/xfs_ialloc.c         |   3 +-
 fs/xfs/libxfs/xfs_ialloc_btree.c   |  25 +++++---
 fs/xfs/libxfs/xfs_inode_buf.c      |   2 +-
 fs/xfs/libxfs/xfs_refcount_btree.c |   3 +-
 fs/xfs/libxfs/xfs_rmap_btree.c     |   3 +-
 fs/xfs/libxfs/xfs_sb.c             |   5 +-
 fs/xfs/libxfs/xfs_shared.h         |   1 +
 fs/xfs/libxfs/xfs_symlink_remote.c |   3 +-
 fs/xfs/scrub/agheader_repair.c     |   2 +-
 fs/xfs/scrub/repair.c              |  11 +++-
 fs/xfs/xfs_buf.c                   |  41 ++++++++++--
 fs/xfs/xfs_buf.h                   |   4 +-
 fs/xfs/xfs_log_recover.c           |   6 +-
 fs/xfs/xfs_trans_buf.c             |   2 +-
 25 files changed, 183 insertions(+), 169 deletions(-)

-- 
2.17.2

[PATCH v2 1/7] xfs: set buffer ops when repair probes for btree type

[Brian Foster]

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

In xrep_findroot_block, we work out the btree type and correctness of a
given block by calling different btree verifiers on root block
candidates.  However, we leave the NULL b_ops while ->verify_read
validates the block, which means that if the verifier calls
xfs_buf_verifier_error it'll crash on the null b_ops.  Fix it to set
b_ops before calling the verifier and unsetting it if the verifier
fails.

Furthermore, improve the documentation around xfs_buf_ensure_ops, which
is the function that is responsible for cleaning up the b_ops state of
buffers that go through xrep_findroot_block but don't match anything.

[bfoster: Renamed xfs_buf_ensure_ops() and modified comment.]

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
Signed-off-by: Brian Foster <bfoster@redhat.com>
---
 fs/xfs/scrub/repair.c  | 11 ++++++++---
 fs/xfs/xfs_buf.c       | 22 ++++++++++++++++------
 fs/xfs/xfs_buf.h       |  2 +-
 fs/xfs/xfs_trans_buf.c |  2 +-
 4 files changed, 26 insertions(+), 11 deletions(-)

diff --git a/fs/xfs/scrub/repair.c b/fs/xfs/scrub/repair.c
index 1c8eecfe52b8..6acf1bfa0bfe 100644
--- a/fs/xfs/scrub/repair.c
+++ b/fs/xfs/scrub/repair.c
@@ -768,18 +768,23 @@ xrep_findroot_block(
 		if (!uuid_equal(&btblock->bb_u.s.bb_uuid,
 				&mp->m_sb.sb_meta_uuid))
 			goto out;
+		/*
+		 * Read verifiers can reference b_ops, so we set the pointer
+		 * here.  If the verifier fails we'll reset the buffer state
+		 * to what it was before we touched the buffer.
+		 */
+		bp->b_ops = fab->buf_ops;
 		fab->buf_ops->verify_read(bp);
 		if (bp->b_error) {
+			bp->b_ops = NULL;
 			bp->b_error = 0;
 			goto out;
 		}
 
 		/*
 		 * Some read verifiers will (re)set b_ops, so we must be
-		 * careful not to blow away any such assignment.
+		 * careful not to change b_ops after running the verifier.
 		 */
-		if (!bp->b_ops)
-			bp->b_ops = fab->buf_ops;
 	}
 
 	/*
diff --git a/fs/xfs/xfs_buf.c b/fs/xfs/xfs_buf.c
index eedc5e0156ff..222b5260ed35 100644
--- a/fs/xfs/xfs_buf.c
+++ b/fs/xfs/xfs_buf.c
@@ -776,13 +776,23 @@ _xfs_buf_read(
 }
 
 /*
- * If the caller passed in an ops structure and the buffer doesn't have ops
- * assigned, set the ops and use them to verify the contents.  If the contents
- * cannot be verified, we'll clear XBF_DONE.  We assume the buffer has no
- * recorded errors and is already in XBF_DONE state.
+ * Reverify a buffer found in cache without an attached ->b_ops.
+ *
+ * If the caller passed an ops structure and the buffer doesn't have ops
+ * assigned, set the ops and use it to verify the contents. If verification
+ * fails, clear XBF_DONE. We assume the buffer has no recorded errors and is
+ * already in XBF_DONE state on entry.
+ *
+ * Under normal operations, every in-core buffer is verified on read I/O
+ * completion. There are two scenarios that can lead to in-core buffers without
+ * an assigned ->b_ops. The first is during log recovery of buffers on a V4
+ * filesystem, though these buffers are purged at the end of recovery. The
+ * other is online repair, which intentionally reads with a NULL buffer ops to
+ * run several verifiers across an in-core buffer in order to establish buffer
+ * type.
  */
 int
-xfs_buf_ensure_ops(
+xfs_buf_reverify(
 	struct xfs_buf		*bp,
 	const struct xfs_buf_ops *ops)
 {
@@ -824,7 +834,7 @@ xfs_buf_read_map(
 		return bp;
 	}
 
-	xfs_buf_ensure_ops(bp, ops);
+	xfs_buf_reverify(bp, ops);
 
 	if (flags & XBF_ASYNC) {
 		/*
diff --git a/fs/xfs/xfs_buf.h b/fs/xfs/xfs_buf.h
index b9f5511ea998..1c436a85b71d 100644
--- a/fs/xfs/xfs_buf.h
+++ b/fs/xfs/xfs_buf.h
@@ -385,6 +385,6 @@ extern int xfs_setsize_buftarg(xfs_buftarg_t *, unsigned int);
 #define xfs_getsize_buftarg(buftarg)	block_size((buftarg)->bt_bdev)
 #define xfs_readonly_buftarg(buftarg)	bdev_read_only((buftarg)->bt_bdev)
 
-int xfs_buf_ensure_ops(struct xfs_buf *bp, const struct xfs_buf_ops *ops);
+int xfs_buf_reverify(struct xfs_buf *bp, const struct xfs_buf_ops *ops);
 
 #endif	/* __XFS_BUF_H__ */
diff --git a/fs/xfs/xfs_trans_buf.c b/fs/xfs/xfs_trans_buf.c
index 629f1479c9d2..7d65ebf1e847 100644
--- a/fs/xfs/xfs_trans_buf.c
+++ b/fs/xfs/xfs_trans_buf.c
@@ -277,7 +277,7 @@ xfs_trans_read_buf_map(
 		 * release this buffer when it kills the tranaction.
 		 */
 		ASSERT(bp->b_ops != NULL);
-		error = xfs_buf_ensure_ops(bp, ops);
+		error = xfs_buf_reverify(bp, ops);
 		if (error) {
 			xfs_buf_ioerror_alert(bp, __func__);
 
-- 
2.17.2

[PATCH v2 2/7] xfs: always check magic values in on-disk byte order

[Brian Foster]

Most verifiers that check on-disk magic values convert the CPU
endian magic value constant to disk endian to facilitate compile
time optimization of the byte swap and reduce the need for runtime
byte swaps in buffer verifiers. Several buffer verifiers do not
follow this pattern. Update those verifiers for consistency.

Also fix up a random typo in the inode readahead verifier name.

Signed-off-by: Brian Foster <bfoster@redhat.com>
---
 fs/xfs/libxfs/xfs_alloc.c     | 2 +-
 fs/xfs/libxfs/xfs_attr_leaf.c | 4 ++--
 fs/xfs/libxfs/xfs_da_btree.c  | 4 ++--
 fs/xfs/libxfs/xfs_inode_buf.c | 2 +-
 fs/xfs/libxfs/xfs_sb.c        | 3 ++-
 5 files changed, 8 insertions(+), 7 deletions(-)

diff --git a/fs/xfs/libxfs/xfs_alloc.c b/fs/xfs/libxfs/xfs_alloc.c
index b715668886a4..48aab07e7138 100644
--- a/fs/xfs/libxfs/xfs_alloc.c
+++ b/fs/xfs/libxfs/xfs_alloc.c
@@ -570,7 +570,7 @@ xfs_agfl_verify(
 
 	if (!uuid_equal(&agfl->agfl_uuid, &mp->m_sb.sb_meta_uuid))
 		return __this_address;
-	if (be32_to_cpu(agfl->agfl_magicnum) != XFS_AGFL_MAGIC)
+	if (agfl->agfl_magicnum != cpu_to_be32(XFS_AGFL_MAGIC))
 		return __this_address;
 	/*
 	 * during growfs operations, the perag is not fully initialised,
diff --git a/fs/xfs/libxfs/xfs_attr_leaf.c b/fs/xfs/libxfs/xfs_attr_leaf.c
index 2652d00842d6..e60eba7f129c 100644
--- a/fs/xfs/libxfs/xfs_attr_leaf.c
+++ b/fs/xfs/libxfs/xfs_attr_leaf.c
@@ -251,7 +251,7 @@ xfs_attr3_leaf_verify(
 	if (xfs_sb_version_hascrc(&mp->m_sb)) {
 		struct xfs_da3_node_hdr *hdr3 = bp->b_addr;
 
-		if (ichdr.magic != XFS_ATTR3_LEAF_MAGIC)
+		if (hdr3->info.hdr.magic != cpu_to_be16(XFS_ATTR3_LEAF_MAGIC))
 			return __this_address;
 
 		if (!uuid_equal(&hdr3->info.uuid, &mp->m_sb.sb_meta_uuid))
@@ -261,7 +261,7 @@ xfs_attr3_leaf_verify(
 		if (!xfs_log_check_lsn(mp, be64_to_cpu(hdr3->info.lsn)))
 			return __this_address;
 	} else {
-		if (ichdr.magic != XFS_ATTR_LEAF_MAGIC)
+		if (leaf->hdr.info.magic != cpu_to_be16(XFS_ATTR_LEAF_MAGIC))
 			return __this_address;
 	}
 	/*
diff --git a/fs/xfs/libxfs/xfs_da_btree.c b/fs/xfs/libxfs/xfs_da_btree.c
index 376bee94b5dd..355322688c9f 100644
--- a/fs/xfs/libxfs/xfs_da_btree.c
+++ b/fs/xfs/libxfs/xfs_da_btree.c
@@ -132,7 +132,7 @@ xfs_da3_node_verify(
 	if (xfs_sb_version_hascrc(&mp->m_sb)) {
 		struct xfs_da3_node_hdr *hdr3 = bp->b_addr;
 
-		if (ichdr.magic != XFS_DA3_NODE_MAGIC)
+		if (hdr3->info.hdr.magic != cpu_to_be16(XFS_DA3_NODE_MAGIC))
 			return __this_address;
 
 		if (!uuid_equal(&hdr3->info.uuid, &mp->m_sb.sb_meta_uuid))
@@ -142,7 +142,7 @@ xfs_da3_node_verify(
 		if (!xfs_log_check_lsn(mp, be64_to_cpu(hdr3->info.lsn)))
 			return __this_address;
 	} else {
-		if (ichdr.magic != XFS_DA_NODE_MAGIC)
+		if (hdr->hdr.info.magic != cpu_to_be16(XFS_DA_NODE_MAGIC))
 			return __this_address;
 	}
 	if (ichdr.level == 0)
diff --git a/fs/xfs/libxfs/xfs_inode_buf.c b/fs/xfs/libxfs/xfs_inode_buf.c
index 09d9c8cfa4a0..fd2df5747a3a 100644
--- a/fs/xfs/libxfs/xfs_inode_buf.c
+++ b/fs/xfs/libxfs/xfs_inode_buf.c
@@ -152,7 +152,7 @@ const struct xfs_buf_ops xfs_inode_buf_ops = {
 };
 
 const struct xfs_buf_ops xfs_inode_buf_ra_ops = {
-	.name = "xxfs_inode_ra",
+	.name = "xfs_inode_ra",
 	.verify_read = xfs_inode_buf_readahead_verify,
 	.verify_write = xfs_inode_buf_write_verify,
 };
diff --git a/fs/xfs/libxfs/xfs_sb.c b/fs/xfs/libxfs/xfs_sb.c
index b5a82acd7dfe..a2f52a958091 100644
--- a/fs/xfs/libxfs/xfs_sb.c
+++ b/fs/xfs/libxfs/xfs_sb.c
@@ -225,10 +225,11 @@ xfs_validate_sb_common(
 	struct xfs_buf		*bp,
 	struct xfs_sb		*sbp)
 {
+	struct xfs_dsb		*dsb = XFS_BUF_TO_SBP(bp);
 	uint32_t		agcount = 0;
 	uint32_t		rem;
 
-	if (sbp->sb_magicnum != XFS_SB_MAGIC) {
+	if (dsb->sb_magicnum != cpu_to_be32(XFS_SB_MAGIC)) {
 		xfs_warn(mp, "bad magic number");
 		return -EWRONGFS;
 	}
-- 
2.17.2

[PATCH v2 3/7] xfs: create a separate finobt verifier

[Brian Foster]

The inobt verifier is reused for the inobt and finobt, which
prevents the ability to distinguish between magic values on a
per-tree basis. Create a separate finobt structure in preparation
for changes to enforce the appropriate magic value for the
associated tree. This patch has no functional change.

Signed-off-by: Brian Foster <bfoster@redhat.com>
---
 fs/xfs/libxfs/xfs_ag.c           | 2 +-
 fs/xfs/libxfs/xfs_ialloc_btree.c | 9 ++++++++-
 fs/xfs/libxfs/xfs_shared.h       | 1 +
 fs/xfs/scrub/agheader_repair.c   | 2 +-
 fs/xfs/xfs_log_recover.c         | 6 ++++--
 5 files changed, 15 insertions(+), 5 deletions(-)

diff --git a/fs/xfs/libxfs/xfs_ag.c b/fs/xfs/libxfs/xfs_ag.c
index 999ad8d00d43..bde67ef3ff43 100644
--- a/fs/xfs/libxfs/xfs_ag.c
+++ b/fs/xfs/libxfs/xfs_ag.c
@@ -361,7 +361,7 @@ xfs_ag_init_headers(
 	{ /* FINO root block */
 		.daddr = XFS_AGB_TO_DADDR(mp, id->agno, XFS_FIBT_BLOCK(mp)),
 		.numblks = BTOBB(mp->m_sb.sb_blocksize),
-		.ops = &xfs_inobt_buf_ops,
+		.ops = &xfs_finobt_buf_ops,
 		.work = &xfs_btroot_init,
 		.type = XFS_BTNUM_FINO,
 		.need_init =  xfs_sb_version_hasfinobt(&mp->m_sb)
diff --git a/fs/xfs/libxfs/xfs_ialloc_btree.c b/fs/xfs/libxfs/xfs_ialloc_btree.c
index 9b25e7a0df47..798269eb4767 100644
--- a/fs/xfs/libxfs/xfs_ialloc_btree.c
+++ b/fs/xfs/libxfs/xfs_ialloc_btree.c
@@ -333,6 +333,13 @@ const struct xfs_buf_ops xfs_inobt_buf_ops = {
 	.verify_struct = xfs_inobt_verify,
 };
 
+const struct xfs_buf_ops xfs_finobt_buf_ops = {
+	.name = "xfs_finobt",
+	.verify_read = xfs_inobt_read_verify,
+	.verify_write = xfs_inobt_write_verify,
+	.verify_struct = xfs_inobt_verify,
+};
+
 STATIC int
 xfs_inobt_keys_inorder(
 	struct xfs_btree_cur	*cur,
@@ -389,7 +396,7 @@ static const struct xfs_btree_ops xfs_finobt_ops = {
 	.init_rec_from_cur	= xfs_inobt_init_rec_from_cur,
 	.init_ptr_from_cur	= xfs_finobt_init_ptr_from_cur,
 	.key_diff		= xfs_inobt_key_diff,
-	.buf_ops		= &xfs_inobt_buf_ops,
+	.buf_ops		= &xfs_finobt_buf_ops,
 	.diff_two_keys		= xfs_inobt_diff_two_keys,
 	.keys_inorder		= xfs_inobt_keys_inorder,
 	.recs_inorder		= xfs_inobt_recs_inorder,
diff --git a/fs/xfs/libxfs/xfs_shared.h b/fs/xfs/libxfs/xfs_shared.h
index 1c5debe748f0..9855f4d2f98f 100644
--- a/fs/xfs/libxfs/xfs_shared.h
+++ b/fs/xfs/libxfs/xfs_shared.h
@@ -36,6 +36,7 @@ extern const struct xfs_buf_ops xfs_dquot_buf_ops;
 extern const struct xfs_buf_ops xfs_symlink_buf_ops;
 extern const struct xfs_buf_ops xfs_agi_buf_ops;
 extern const struct xfs_buf_ops xfs_inobt_buf_ops;
+extern const struct xfs_buf_ops xfs_finobt_buf_ops;
 extern const struct xfs_buf_ops xfs_inode_buf_ops;
 extern const struct xfs_buf_ops xfs_inode_buf_ra_ops;
 extern const struct xfs_buf_ops xfs_dquot_buf_ops;
diff --git a/fs/xfs/scrub/agheader_repair.c b/fs/xfs/scrub/agheader_repair.c
index 03d1e15cceba..673be3cf7b8d 100644
--- a/fs/xfs/scrub/agheader_repair.c
+++ b/fs/xfs/scrub/agheader_repair.c
@@ -879,7 +879,7 @@ xrep_agi(
 		},
 		[XREP_AGI_FINOBT] = {
 			.rmap_owner = XFS_RMAP_OWN_INOBT,
-			.buf_ops = &xfs_inobt_buf_ops,
+			.buf_ops = &xfs_finobt_buf_ops,
 			.magic = XFS_FIBT_CRC_MAGIC,
 		},
 		[XREP_AGI_END] = {
diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c
index 9fe88d125f0a..228c754bb137 100644
--- a/fs/xfs/xfs_log_recover.c
+++ b/fs/xfs/xfs_log_recover.c
@@ -2445,11 +2445,13 @@ xlog_recover_validate_buf_type(
 			bp->b_ops = &xfs_allocbt_buf_ops;
 			break;
 		case XFS_IBT_CRC_MAGIC:
-		case XFS_FIBT_CRC_MAGIC:
 		case XFS_IBT_MAGIC:
-		case XFS_FIBT_MAGIC:
 			bp->b_ops = &xfs_inobt_buf_ops;
 			break;
+		case XFS_FIBT_CRC_MAGIC:
+		case XFS_FIBT_MAGIC:
+			bp->b_ops = &xfs_finobt_buf_ops;
+			break;
 		case XFS_BMAP_CRC_MAGIC:
 		case XFS_BMAP_MAGIC:
 			bp->b_ops = &xfs_bmbt_buf_ops;
-- 
2.17.2

[PATCH v2 4/7] xfs: distinguish between inobt and finobt magic values

[Brian Foster]

The inode btree verifier code is shared between the inode btree and
free inode btree because the underlying metadata formats are
essentially equivalent. A side effect of this is that the verifier
cannot determine whether a particular btree block should have an
inobt or finobt magic value.

This logic allows an unfortunate xfs_repair bug to escape detection
where certain level > 0 nodes of the finobt are stamped with inobt
magic by xfs_repair finobt reconstruction. This is fortunately not a
severe problem since the inode btree magic values do not contribute
to any changes in kernel behavior, but we do need a means to detect
and prevent this problem in the future.

Add a field to xfs_buf_ops to store the v4 and v5 superblock magic
values expected by a particular verifier. Add a helper to check an
on-disk magic value against the value expected by the verifier. Call
the helper from the shared [f]inobt verifier code for magic value
verification. This ensures that the inode btree blocks each have the
appropriate magic value based on specific tree type and superblock
version.

Signed-off-by: Brian Foster <bfoster@redhat.com>
---
 fs/xfs/libxfs/xfs_ialloc_btree.c | 16 +++++++---------
 fs/xfs/xfs_buf.c                 | 19 +++++++++++++++++++
 fs/xfs/xfs_buf.h                 |  2 ++
 3 files changed, 28 insertions(+), 9 deletions(-)

diff --git a/fs/xfs/libxfs/xfs_ialloc_btree.c b/fs/xfs/libxfs/xfs_ialloc_btree.c
index 798269eb4767..c2df1f89eec8 100644
--- a/fs/xfs/libxfs/xfs_ialloc_btree.c
+++ b/fs/xfs/libxfs/xfs_ialloc_btree.c
@@ -260,6 +260,9 @@ xfs_inobt_verify(
 	xfs_failaddr_t		fa;
 	unsigned int		level;
 
+	if (!xfs_verify_magic(bp, block->bb_magic))
+		return __this_address;
+
 	/*
 	 * During growfs operations, we can't verify the exact owner as the
 	 * perag is not fully initialised and hence not attached to the buffer.
@@ -270,18 +273,10 @@ xfs_inobt_verify(
 	 * but beware of the landmine (i.e. need to check pag->pagi_init) if we
 	 * ever do.
 	 */
-	switch (block->bb_magic) {
-	case cpu_to_be32(XFS_IBT_CRC_MAGIC):
-	case cpu_to_be32(XFS_FIBT_CRC_MAGIC):
+	if (xfs_sb_version_hascrc(&mp->m_sb)) {
 		fa = xfs_btree_sblock_v5hdr_verify(bp);
 		if (fa)
 			return fa;
-		/* fall through */
-	case cpu_to_be32(XFS_IBT_MAGIC):
-	case cpu_to_be32(XFS_FIBT_MAGIC):
-		break;
-	default:
-		return __this_address;
 	}
 
 	/* level verification */
@@ -328,6 +323,7 @@ xfs_inobt_write_verify(
 
 const struct xfs_buf_ops xfs_inobt_buf_ops = {
 	.name = "xfs_inobt",
+	.magic = { cpu_to_be32(XFS_IBT_MAGIC), cpu_to_be32(XFS_IBT_CRC_MAGIC) },
 	.verify_read = xfs_inobt_read_verify,
 	.verify_write = xfs_inobt_write_verify,
 	.verify_struct = xfs_inobt_verify,
@@ -335,6 +331,8 @@ const struct xfs_buf_ops xfs_inobt_buf_ops = {
 
 const struct xfs_buf_ops xfs_finobt_buf_ops = {
 	.name = "xfs_finobt",
+	.magic = { cpu_to_be32(XFS_FIBT_MAGIC),
+		   cpu_to_be32(XFS_FIBT_CRC_MAGIC) },
 	.verify_read = xfs_inobt_read_verify,
 	.verify_write = xfs_inobt_write_verify,
 	.verify_struct = xfs_inobt_verify,
diff --git a/fs/xfs/xfs_buf.c b/fs/xfs/xfs_buf.c
index 222b5260ed35..0481c19fe5ae 100644
--- a/fs/xfs/xfs_buf.c
+++ b/fs/xfs/xfs_buf.c
@@ -2204,3 +2204,22 @@ void xfs_buf_set_ref(struct xfs_buf *bp, int lru_ref)
 
 	atomic_set(&bp->b_lru_ref, lru_ref);
 }
+
+/*
+ * Verify an on-disk magic value against the magic value specified in the
+ * verifier structure. The verifier magic is in disk byte order so the caller is
+ * expected to pass the value directly from disk.
+ */
+bool
+xfs_verify_magic(
+	struct xfs_buf		*bp,
+	uint32_t		dmagic)
+{
+	struct xfs_mount	*mp = bp->b_target->bt_mount;
+	int			idx;
+
+	idx = xfs_sb_version_hascrc(&mp->m_sb);
+	if (unlikely(WARN_ON(!bp->b_ops || !bp->b_ops->magic[idx])))
+		return false;
+	return dmagic == bp->b_ops->magic[idx];
+}
diff --git a/fs/xfs/xfs_buf.h b/fs/xfs/xfs_buf.h
index 1c436a85b71d..8d8fbcf54dda 100644
--- a/fs/xfs/xfs_buf.h
+++ b/fs/xfs/xfs_buf.h
@@ -125,6 +125,7 @@ struct xfs_buf_map {
 
 struct xfs_buf_ops {
 	char *name;
+	uint32_t magic[2];		/* v4 and v5 on disk magic values */
 	void (*verify_read)(struct xfs_buf *);
 	void (*verify_write)(struct xfs_buf *);
 	xfs_failaddr_t (*verify_struct)(struct xfs_buf *bp);
@@ -386,5 +387,6 @@ extern int xfs_setsize_buftarg(xfs_buftarg_t *, unsigned int);
 #define xfs_readonly_buftarg(buftarg)	bdev_read_only((buftarg)->bt_bdev)
 
 int xfs_buf_reverify(struct xfs_buf *bp, const struct xfs_buf_ops *ops);
+bool xfs_verify_magic(struct xfs_buf *, uint32_t);
 
 #endif	/* __XFS_BUF_H__ */
-- 
2.17.2

[PATCH v2 5/7] xfs: use verifier magic field in dir2 leaf verifiers

[Brian Foster]

The dir2 leaf verifiers share the same underlying structure
verification code, but implement six accessor functions to multiplex
the code across the two verifiers. Further, the magic value isn't
sufficiently abstracted such that the common helper has to manually
fix up the magic from the caller on v5 filesystems.

Use the magic field in the verifier structure to eliminate the
duplicate code and clean this all up. No functional change.

Signed-off-by: Brian Foster <bfoster@redhat.com>
---
 fs/xfs/libxfs/xfs_dir2_leaf.c | 88 ++++++++---------------------------
 1 file changed, 20 insertions(+), 68 deletions(-)

diff --git a/fs/xfs/libxfs/xfs_dir2_leaf.c b/fs/xfs/libxfs/xfs_dir2_leaf.c
index 1728a3e6f5cf..a99ae2cd292e 100644
--- a/fs/xfs/libxfs/xfs_dir2_leaf.c
+++ b/fs/xfs/libxfs/xfs_dir2_leaf.c
@@ -142,41 +142,32 @@ xfs_dir3_leaf_check_int(
  */
 static xfs_failaddr_t
 xfs_dir3_leaf_verify(
-	struct xfs_buf		*bp,
-	uint16_t		magic)
+	struct xfs_buf		*bp)
 {
 	struct xfs_mount	*mp = bp->b_target->bt_mount;
 	struct xfs_dir2_leaf	*leaf = bp->b_addr;
 
-	ASSERT(magic == XFS_DIR2_LEAF1_MAGIC || magic == XFS_DIR2_LEAFN_MAGIC);
+	if (!xfs_verify_magic(bp, leaf->hdr.info.magic))
+		return __this_address;
 
 	if (xfs_sb_version_hascrc(&mp->m_sb)) {
 		struct xfs_dir3_leaf_hdr *leaf3 = bp->b_addr;
-		uint16_t		magic3;
 
-		magic3 = (magic == XFS_DIR2_LEAF1_MAGIC) ? XFS_DIR3_LEAF1_MAGIC
-							 : XFS_DIR3_LEAFN_MAGIC;
-
-		if (leaf3->info.hdr.magic != cpu_to_be16(magic3))
-			return __this_address;
+		ASSERT(leaf3->info.hdr.magic == leaf->hdr.info.magic);
 		if (!uuid_equal(&leaf3->info.uuid, &mp->m_sb.sb_meta_uuid))
 			return __this_address;
 		if (be64_to_cpu(leaf3->info.blkno) != bp->b_bn)
 			return __this_address;
 		if (!xfs_log_check_lsn(mp, be64_to_cpu(leaf3->info.lsn)))
 			return __this_address;
-	} else {
-		if (leaf->hdr.info.magic != cpu_to_be16(magic))
-			return __this_address;
 	}
 
 	return xfs_dir3_leaf_check_int(mp, NULL, NULL, leaf);
 }
 
 static void
-__read_verify(
-	struct xfs_buf  *bp,
-	uint16_t	magic)
+xfs_dir3_leaf_read_verify(
+	struct xfs_buf  *bp)
 {
 	struct xfs_mount	*mp = bp->b_target->bt_mount;
 	xfs_failaddr_t		fa;
@@ -185,23 +176,22 @@ __read_verify(
 	     !xfs_buf_verify_cksum(bp, XFS_DIR3_LEAF_CRC_OFF))
 		xfs_verifier_error(bp, -EFSBADCRC, __this_address);
 	else {
-		fa = xfs_dir3_leaf_verify(bp, magic);
+		fa = xfs_dir3_leaf_verify(bp);
 		if (fa)
 			xfs_verifier_error(bp, -EFSCORRUPTED, fa);
 	}
 }
 
 static void
-__write_verify(
-	struct xfs_buf  *bp,
-	uint16_t	magic)
+xfs_dir3_leaf_write_verify(
+	struct xfs_buf  *bp)
 {
 	struct xfs_mount	*mp = bp->b_target->bt_mount;
 	struct xfs_buf_log_item	*bip = bp->b_log_item;
 	struct xfs_dir3_leaf_hdr *hdr3 = bp->b_addr;
 	xfs_failaddr_t		fa;
 
-	fa = xfs_dir3_leaf_verify(bp, magic);
+	fa = xfs_dir3_leaf_verify(bp);
 	if (fa) {
 		xfs_verifier_error(bp, -EFSCORRUPTED, fa);
 		return;
@@ -216,60 +206,22 @@ __write_verify(
 	xfs_buf_update_cksum(bp, XFS_DIR3_LEAF_CRC_OFF);
 }
 
-static xfs_failaddr_t
-xfs_dir3_leaf1_verify(
-	struct xfs_buf	*bp)
-{
-	return xfs_dir3_leaf_verify(bp, XFS_DIR2_LEAF1_MAGIC);
-}
-
-static void
-xfs_dir3_leaf1_read_verify(
-	struct xfs_buf	*bp)
-{
-	__read_verify(bp, XFS_DIR2_LEAF1_MAGIC);
-}
-
-static void
-xfs_dir3_leaf1_write_verify(
-	struct xfs_buf	*bp)
-{
-	__write_verify(bp, XFS_DIR2_LEAF1_MAGIC);
-}
-
-static xfs_failaddr_t
-xfs_dir3_leafn_verify(
-	struct xfs_buf	*bp)
-{
-	return xfs_dir3_leaf_verify(bp, XFS_DIR2_LEAFN_MAGIC);
-}
-
-static void
-xfs_dir3_leafn_read_verify(
-	struct xfs_buf	*bp)
-{
-	__read_verify(bp, XFS_DIR2_LEAFN_MAGIC);
-}
-
-static void
-xfs_dir3_leafn_write_verify(
-	struct xfs_buf	*bp)
-{
-	__write_verify(bp, XFS_DIR2_LEAFN_MAGIC);
-}
-
 const struct xfs_buf_ops xfs_dir3_leaf1_buf_ops = {
 	.name = "xfs_dir3_leaf1",
-	.verify_read = xfs_dir3_leaf1_read_verify,
-	.verify_write = xfs_dir3_leaf1_write_verify,
-	.verify_struct = xfs_dir3_leaf1_verify,
+	.magic = { cpu_to_be16(XFS_DIR2_LEAF1_MAGIC),
+		   cpu_to_be16(XFS_DIR3_LEAF1_MAGIC) },
+	.verify_read = xfs_dir3_leaf_read_verify,
+	.verify_write = xfs_dir3_leaf_write_verify,
+	.verify_struct = xfs_dir3_leaf_verify,
 };
 
 const struct xfs_buf_ops xfs_dir3_leafn_buf_ops = {
 	.name = "xfs_dir3_leafn",
-	.verify_read = xfs_dir3_leafn_read_verify,
-	.verify_write = xfs_dir3_leafn_write_verify,
-	.verify_struct = xfs_dir3_leafn_verify,
+	.magic = { cpu_to_be16(XFS_DIR2_LEAFN_MAGIC),
+		   cpu_to_be16(XFS_DIR3_LEAFN_MAGIC) },
+	.verify_read = xfs_dir3_leaf_read_verify,
+	.verify_write = xfs_dir3_leaf_write_verify,
+	.verify_struct = xfs_dir3_leaf_verify,
 };
 
 int
-- 
2.17.2

[PATCH v2 6/7] xfs: miscellaneous verifier magic value fixups

[Brian Foster]

Most buffer verifiers have hardcoded magic value checks
conditionalized on the version of the filesystem. The magic value
field of the verifier structure facilitates abstraction of some of
this code. Populate the ->magic field of various verifiers to take
advantage of this abstraction. No functional changes.

Signed-off-by: Brian Foster <bfoster@redhat.com>
---
 fs/xfs/libxfs/xfs_alloc.c          | 12 ++++++++----
 fs/xfs/libxfs/xfs_attr_leaf.c      | 12 ++++++------
 fs/xfs/libxfs/xfs_attr_remote.c    |  8 +++++---
 fs/xfs/libxfs/xfs_bmap_btree.c     | 13 ++++++-------
 fs/xfs/libxfs/xfs_da_btree.c       | 12 ++++++------
 fs/xfs/libxfs/xfs_dir2_block.c     | 10 +++++-----
 fs/xfs/libxfs/xfs_dir2_data.c      | 12 +++++++-----
 fs/xfs/libxfs/xfs_dir2_node.c      | 11 ++++++-----
 fs/xfs/libxfs/xfs_ialloc.c         |  3 ++-
 fs/xfs/libxfs/xfs_refcount_btree.c |  3 ++-
 fs/xfs/libxfs/xfs_rmap_btree.c     |  3 ++-
 fs/xfs/libxfs/xfs_sb.c             |  4 +++-
 fs/xfs/libxfs/xfs_symlink_remote.c |  3 ++-
 13 files changed, 60 insertions(+), 46 deletions(-)

diff --git a/fs/xfs/libxfs/xfs_alloc.c b/fs/xfs/libxfs/xfs_alloc.c
index 48aab07e7138..bc3367b8b7bb 100644
--- a/fs/xfs/libxfs/xfs_alloc.c
+++ b/fs/xfs/libxfs/xfs_alloc.c
@@ -568,9 +568,9 @@ xfs_agfl_verify(
 	if (!xfs_sb_version_hascrc(&mp->m_sb))
 		return NULL;
 
-	if (!uuid_equal(&agfl->agfl_uuid, &mp->m_sb.sb_meta_uuid))
+	if (!xfs_verify_magic(bp, agfl->agfl_magicnum))
 		return __this_address;
-	if (agfl->agfl_magicnum != cpu_to_be32(XFS_AGFL_MAGIC))
+	if (!uuid_equal(&agfl->agfl_uuid, &mp->m_sb.sb_meta_uuid))
 		return __this_address;
 	/*
 	 * during growfs operations, the perag is not fully initialised,
@@ -643,6 +643,7 @@ xfs_agfl_write_verify(
 
 const struct xfs_buf_ops xfs_agfl_buf_ops = {
 	.name = "xfs_agfl",
+	.magic = { cpu_to_be32(XFS_AGFL_MAGIC), cpu_to_be32(XFS_AGFL_MAGIC) },
 	.verify_read = xfs_agfl_read_verify,
 	.verify_write = xfs_agfl_write_verify,
 	.verify_struct = xfs_agfl_verify,
@@ -2587,8 +2588,10 @@ xfs_agf_verify(
 			return __this_address;
 	}
 
-	if (!(agf->agf_magicnum == cpu_to_be32(XFS_AGF_MAGIC) &&
-	      XFS_AGF_GOOD_VERSION(be32_to_cpu(agf->agf_versionnum)) &&
+	if (!xfs_verify_magic(bp, agf->agf_magicnum))
+		return __this_address;
+
+	if (!(XFS_AGF_GOOD_VERSION(be32_to_cpu(agf->agf_versionnum)) &&
 	      be32_to_cpu(agf->agf_freeblks) <= be32_to_cpu(agf->agf_length) &&
 	      be32_to_cpu(agf->agf_flfirst) < xfs_agfl_size(mp) &&
 	      be32_to_cpu(agf->agf_fllast) < xfs_agfl_size(mp) &&
@@ -2670,6 +2673,7 @@ xfs_agf_write_verify(
 
 const struct xfs_buf_ops xfs_agf_buf_ops = {
 	.name = "xfs_agf",
+	.magic = { cpu_to_be32(XFS_AGF_MAGIC), cpu_to_be32(XFS_AGF_MAGIC) },
 	.verify_read = xfs_agf_read_verify,
 	.verify_write = xfs_agf_write_verify,
 	.verify_struct = xfs_agf_verify,
diff --git a/fs/xfs/libxfs/xfs_attr_leaf.c b/fs/xfs/libxfs/xfs_attr_leaf.c
index e60eba7f129c..dde3a550d7ce 100644
--- a/fs/xfs/libxfs/xfs_attr_leaf.c
+++ b/fs/xfs/libxfs/xfs_attr_leaf.c
@@ -248,21 +248,19 @@ xfs_attr3_leaf_verify(
 
 	xfs_attr3_leaf_hdr_from_disk(mp->m_attr_geo, &ichdr, leaf);
 
+	if (!xfs_verify_magic(bp, leaf->hdr.info.magic))
+		return __this_address;
+
 	if (xfs_sb_version_hascrc(&mp->m_sb)) {
 		struct xfs_da3_node_hdr *hdr3 = bp->b_addr;
 
-		if (hdr3->info.hdr.magic != cpu_to_be16(XFS_ATTR3_LEAF_MAGIC))
-			return __this_address;
-
+		ASSERT(leaf->hdr.info.magic == hdr3->info.hdr.magic);
 		if (!uuid_equal(&hdr3->info.uuid, &mp->m_sb.sb_meta_uuid))
 			return __this_address;
 		if (be64_to_cpu(hdr3->info.blkno) != bp->b_bn)
 			return __this_address;
 		if (!xfs_log_check_lsn(mp, be64_to_cpu(hdr3->info.lsn)))
 			return __this_address;
-	} else {
-		if (leaf->hdr.info.magic != cpu_to_be16(XFS_ATTR_LEAF_MAGIC))
-			return __this_address;
 	}
 	/*
 	 * In recovery there is a transient state where count == 0 is valid
@@ -369,6 +367,8 @@ xfs_attr3_leaf_read_verify(
 
 const struct xfs_buf_ops xfs_attr3_leaf_buf_ops = {
 	.name = "xfs_attr3_leaf",
+	.magic = { cpu_to_be16(XFS_ATTR_LEAF_MAGIC),
+		   cpu_to_be16(XFS_ATTR3_LEAF_MAGIC) },
 	.verify_read = xfs_attr3_leaf_read_verify,
 	.verify_write = xfs_attr3_leaf_write_verify,
 	.verify_struct = xfs_attr3_leaf_verify,
diff --git a/fs/xfs/libxfs/xfs_attr_remote.c b/fs/xfs/libxfs/xfs_attr_remote.c
index d89363c6b523..65ff600a8067 100644
--- a/fs/xfs/libxfs/xfs_attr_remote.c
+++ b/fs/xfs/libxfs/xfs_attr_remote.c
@@ -79,6 +79,7 @@ xfs_attr3_rmt_hdr_ok(
 static xfs_failaddr_t
 xfs_attr3_rmt_verify(
 	struct xfs_mount	*mp,
+	struct xfs_buf		*bp,
 	void			*ptr,
 	int			fsbsize,
 	xfs_daddr_t		bno)
@@ -87,7 +88,7 @@ xfs_attr3_rmt_verify(
 
 	if (!xfs_sb_version_hascrc(&mp->m_sb))
 		return __this_address;
-	if (rmt->rm_magic != cpu_to_be32(XFS_ATTR3_RMT_MAGIC))
+	if (!xfs_verify_magic(bp, rmt->rm_magic))
 		return __this_address;
 	if (!uuid_equal(&rmt->rm_uuid, &mp->m_sb.sb_meta_uuid))
 		return __this_address;
@@ -131,7 +132,7 @@ __xfs_attr3_rmt_read_verify(
 			*failaddr = __this_address;
 			return -EFSBADCRC;
 		}
-		*failaddr = xfs_attr3_rmt_verify(mp, ptr, blksize, bno);
+		*failaddr = xfs_attr3_rmt_verify(mp, bp, ptr, blksize, bno);
 		if (*failaddr)
 			return -EFSCORRUPTED;
 		len -= blksize;
@@ -193,7 +194,7 @@ xfs_attr3_rmt_write_verify(
 	while (len > 0) {
 		struct xfs_attr3_rmt_hdr *rmt = (struct xfs_attr3_rmt_hdr *)ptr;
 
-		fa = xfs_attr3_rmt_verify(mp, ptr, blksize, bno);
+		fa = xfs_attr3_rmt_verify(mp, bp, ptr, blksize, bno);
 		if (fa) {
 			xfs_verifier_error(bp, -EFSCORRUPTED, fa);
 			return;
@@ -220,6 +221,7 @@ xfs_attr3_rmt_write_verify(
 
 const struct xfs_buf_ops xfs_attr3_rmt_buf_ops = {
 	.name = "xfs_attr3_rmt",
+	.magic = { 0, cpu_to_be32(XFS_ATTR3_RMT_MAGIC) },
 	.verify_read = xfs_attr3_rmt_read_verify,
 	.verify_write = xfs_attr3_rmt_write_verify,
 	.verify_struct = xfs_attr3_rmt_verify_struct,
diff --git a/fs/xfs/libxfs/xfs_bmap_btree.c b/fs/xfs/libxfs/xfs_bmap_btree.c
index cdb74d2e2a43..aff82ed112c9 100644
--- a/fs/xfs/libxfs/xfs_bmap_btree.c
+++ b/fs/xfs/libxfs/xfs_bmap_btree.c
@@ -416,8 +416,10 @@ xfs_bmbt_verify(
 	xfs_failaddr_t		fa;
 	unsigned int		level;
 
-	switch (block->bb_magic) {
-	case cpu_to_be32(XFS_BMAP_CRC_MAGIC):
+	if (!xfs_verify_magic(bp, block->bb_magic))
+		return __this_address;
+
+	if (xfs_sb_version_hascrc(&mp->m_sb)) {
 		/*
 		 * XXX: need a better way of verifying the owner here. Right now
 		 * just make sure there has been one set.
@@ -425,11 +427,6 @@ xfs_bmbt_verify(
 		fa = xfs_btree_lblock_v5hdr_verify(bp, XFS_RMAP_OWN_UNKNOWN);
 		if (fa)
 			return fa;
-		/* fall through */
-	case cpu_to_be32(XFS_BMAP_MAGIC):
-		break;
-	default:
-		return __this_address;
 	}
 
 	/*
@@ -481,6 +478,8 @@ xfs_bmbt_write_verify(
 
 const struct xfs_buf_ops xfs_bmbt_buf_ops = {
 	.name = "xfs_bmbt",
+	.magic = { cpu_to_be32(XFS_BMAP_MAGIC),
+		   cpu_to_be32(XFS_BMAP_CRC_MAGIC) },
 	.verify_read = xfs_bmbt_read_verify,
 	.verify_write = xfs_bmbt_write_verify,
 	.verify_struct = xfs_bmbt_verify,
diff --git a/fs/xfs/libxfs/xfs_da_btree.c b/fs/xfs/libxfs/xfs_da_btree.c
index 355322688c9f..6b8ca390eecc 100644
--- a/fs/xfs/libxfs/xfs_da_btree.c
+++ b/fs/xfs/libxfs/xfs_da_btree.c
@@ -129,21 +129,19 @@ xfs_da3_node_verify(
 
 	ops->node_hdr_from_disk(&ichdr, hdr);
 
+	if (!xfs_verify_magic(bp, hdr->hdr.info.magic))
+		return __this_address;
+
 	if (xfs_sb_version_hascrc(&mp->m_sb)) {
 		struct xfs_da3_node_hdr *hdr3 = bp->b_addr;
 
-		if (hdr3->info.hdr.magic != cpu_to_be16(XFS_DA3_NODE_MAGIC))
-			return __this_address;
-
+		ASSERT(hdr3->info.hdr.magic == hdr->hdr.info.magic);
 		if (!uuid_equal(&hdr3->info.uuid, &mp->m_sb.sb_meta_uuid))
 			return __this_address;
 		if (be64_to_cpu(hdr3->info.blkno) != bp->b_bn)
 			return __this_address;
 		if (!xfs_log_check_lsn(mp, be64_to_cpu(hdr3->info.lsn)))
 			return __this_address;
-	} else {
-		if (hdr->hdr.info.magic != cpu_to_be16(XFS_DA_NODE_MAGIC))
-			return __this_address;
 	}
 	if (ichdr.level == 0)
 		return __this_address;
@@ -257,6 +255,8 @@ xfs_da3_node_verify_struct(
 
 const struct xfs_buf_ops xfs_da3_node_buf_ops = {
 	.name = "xfs_da3_node",
+	.magic = { cpu_to_be16(XFS_DA_NODE_MAGIC),
+		   cpu_to_be16(XFS_DA3_NODE_MAGIC) },
 	.verify_read = xfs_da3_node_read_verify,
 	.verify_write = xfs_da3_node_write_verify,
 	.verify_struct = xfs_da3_node_verify_struct,
diff --git a/fs/xfs/libxfs/xfs_dir2_block.c b/fs/xfs/libxfs/xfs_dir2_block.c
index 30ed5919da72..b7d6d78f4ce2 100644
--- a/fs/xfs/libxfs/xfs_dir2_block.c
+++ b/fs/xfs/libxfs/xfs_dir2_block.c
@@ -53,18 +53,16 @@ xfs_dir3_block_verify(
 	struct xfs_mount	*mp = bp->b_target->bt_mount;
 	struct xfs_dir3_blk_hdr	*hdr3 = bp->b_addr;
 
+	if (!xfs_verify_magic(bp, hdr3->magic))
+		return __this_address;
+
 	if (xfs_sb_version_hascrc(&mp->m_sb)) {
-		if (hdr3->magic != cpu_to_be32(XFS_DIR3_BLOCK_MAGIC))
-			return __this_address;
 		if (!uuid_equal(&hdr3->uuid, &mp->m_sb.sb_meta_uuid))
 			return __this_address;
 		if (be64_to_cpu(hdr3->blkno) != bp->b_bn)
 			return __this_address;
 		if (!xfs_log_check_lsn(mp, be64_to_cpu(hdr3->lsn)))
 			return __this_address;
-	} else {
-		if (hdr3->magic != cpu_to_be32(XFS_DIR2_BLOCK_MAGIC))
-			return __this_address;
 	}
 	return __xfs_dir3_data_check(NULL, bp);
 }
@@ -112,6 +110,8 @@ xfs_dir3_block_write_verify(
 
 const struct xfs_buf_ops xfs_dir3_block_buf_ops = {
 	.name = "xfs_dir3_block",
+	.magic = { cpu_to_be32(XFS_DIR2_BLOCK_MAGIC),
+		   cpu_to_be32(XFS_DIR3_BLOCK_MAGIC) },
 	.verify_read = xfs_dir3_block_read_verify,
 	.verify_write = xfs_dir3_block_write_verify,
 	.verify_struct = xfs_dir3_block_verify,
diff --git a/fs/xfs/libxfs/xfs_dir2_data.c b/fs/xfs/libxfs/xfs_dir2_data.c
index 01162c62ec8f..b7b9ce002cb9 100644
--- a/fs/xfs/libxfs/xfs_dir2_data.c
+++ b/fs/xfs/libxfs/xfs_dir2_data.c
@@ -252,18 +252,16 @@ xfs_dir3_data_verify(
 	struct xfs_mount	*mp = bp->b_target->bt_mount;
 	struct xfs_dir3_blk_hdr	*hdr3 = bp->b_addr;
 
+	if (!xfs_verify_magic(bp, hdr3->magic))
+		return __this_address;
+
 	if (xfs_sb_version_hascrc(&mp->m_sb)) {
-		if (hdr3->magic != cpu_to_be32(XFS_DIR3_DATA_MAGIC))
-			return __this_address;
 		if (!uuid_equal(&hdr3->uuid, &mp->m_sb.sb_meta_uuid))
 			return __this_address;
 		if (be64_to_cpu(hdr3->blkno) != bp->b_bn)
 			return __this_address;
 		if (!xfs_log_check_lsn(mp, be64_to_cpu(hdr3->lsn)))
 			return __this_address;
-	} else {
-		if (hdr3->magic != cpu_to_be32(XFS_DIR2_DATA_MAGIC))
-			return __this_address;
 	}
 	return __xfs_dir3_data_check(NULL, bp);
 }
@@ -339,6 +337,8 @@ xfs_dir3_data_write_verify(
 
 const struct xfs_buf_ops xfs_dir3_data_buf_ops = {
 	.name = "xfs_dir3_data",
+	.magic = { cpu_to_be32(XFS_DIR2_DATA_MAGIC),
+		   cpu_to_be32(XFS_DIR3_DATA_MAGIC) },
 	.verify_read = xfs_dir3_data_read_verify,
 	.verify_write = xfs_dir3_data_write_verify,
 	.verify_struct = xfs_dir3_data_verify,
@@ -346,6 +346,8 @@ const struct xfs_buf_ops xfs_dir3_data_buf_ops = {
 
 static const struct xfs_buf_ops xfs_dir3_data_reada_buf_ops = {
 	.name = "xfs_dir3_data_reada",
+	.magic = { cpu_to_be32(XFS_DIR2_DATA_MAGIC),
+		   cpu_to_be32(XFS_DIR3_DATA_MAGIC) },
 	.verify_read = xfs_dir3_data_reada_verify,
 	.verify_write = xfs_dir3_data_write_verify,
 };
diff --git a/fs/xfs/libxfs/xfs_dir2_node.c b/fs/xfs/libxfs/xfs_dir2_node.c
index f1bb3434f51c..9ccc8819e99f 100644
--- a/fs/xfs/libxfs/xfs_dir2_node.c
+++ b/fs/xfs/libxfs/xfs_dir2_node.c
@@ -87,20 +87,19 @@ xfs_dir3_free_verify(
 	struct xfs_mount	*mp = bp->b_target->bt_mount;
 	struct xfs_dir2_free_hdr *hdr = bp->b_addr;
 
+	if (!xfs_verify_magic(bp, hdr->magic))
+		return __this_address;
+
 	if (xfs_sb_version_hascrc(&mp->m_sb)) {
 		struct xfs_dir3_blk_hdr *hdr3 = bp->b_addr;
 
-		if (hdr3->magic != cpu_to_be32(XFS_DIR3_FREE_MAGIC))
-			return __this_address;
+		ASSERT(hdr3->magic == hdr->magic);
 		if (!uuid_equal(&hdr3->uuid, &mp->m_sb.sb_meta_uuid))
 			return __this_address;
 		if (be64_to_cpu(hdr3->blkno) != bp->b_bn)
 			return __this_address;
 		if (!xfs_log_check_lsn(mp, be64_to_cpu(hdr3->lsn)))
 			return __this_address;
-	} else {
-		if (hdr->magic != cpu_to_be32(XFS_DIR2_FREE_MAGIC))
-			return __this_address;
 	}
 
 	/* XXX: should bounds check the xfs_dir3_icfree_hdr here */
@@ -151,6 +150,8 @@ xfs_dir3_free_write_verify(
 
 const struct xfs_buf_ops xfs_dir3_free_buf_ops = {
 	.name = "xfs_dir3_free",
+	.magic = { cpu_to_be32(XFS_DIR2_FREE_MAGIC),
+		   cpu_to_be32(XFS_DIR3_FREE_MAGIC) },
 	.verify_read = xfs_dir3_free_read_verify,
 	.verify_write = xfs_dir3_free_write_verify,
 	.verify_struct = xfs_dir3_free_verify,
diff --git a/fs/xfs/libxfs/xfs_ialloc.c b/fs/xfs/libxfs/xfs_ialloc.c
index d32152fc8a6c..fe9898875097 100644
--- a/fs/xfs/libxfs/xfs_ialloc.c
+++ b/fs/xfs/libxfs/xfs_ialloc.c
@@ -2508,7 +2508,7 @@ xfs_agi_verify(
 	/*
 	 * Validate the magic number of the agi block.
 	 */
-	if (agi->agi_magicnum != cpu_to_be32(XFS_AGI_MAGIC))
+	if (!xfs_verify_magic(bp, agi->agi_magicnum))
 		return __this_address;
 	if (!XFS_AGI_GOOD_VERSION(be32_to_cpu(agi->agi_versionnum)))
 		return __this_address;
@@ -2582,6 +2582,7 @@ xfs_agi_write_verify(
 
 const struct xfs_buf_ops xfs_agi_buf_ops = {
 	.name = "xfs_agi",
+	.magic = { cpu_to_be32(XFS_AGI_MAGIC), cpu_to_be32(XFS_AGI_MAGIC) },
 	.verify_read = xfs_agi_read_verify,
 	.verify_write = xfs_agi_write_verify,
 	.verify_struct = xfs_agi_verify,
diff --git a/fs/xfs/libxfs/xfs_refcount_btree.c b/fs/xfs/libxfs/xfs_refcount_btree.c
index d9eab657b63e..6f47ab876d90 100644
--- a/fs/xfs/libxfs/xfs_refcount_btree.c
+++ b/fs/xfs/libxfs/xfs_refcount_btree.c
@@ -209,7 +209,7 @@ xfs_refcountbt_verify(
 	xfs_failaddr_t		fa;
 	unsigned int		level;
 
-	if (block->bb_magic != cpu_to_be32(XFS_REFC_CRC_MAGIC))
+	if (!xfs_verify_magic(bp, block->bb_magic))
 		return __this_address;
 
 	if (!xfs_sb_version_hasreflink(&mp->m_sb))
@@ -264,6 +264,7 @@ xfs_refcountbt_write_verify(
 
 const struct xfs_buf_ops xfs_refcountbt_buf_ops = {
 	.name			= "xfs_refcountbt",
+	.magic			= { 0, cpu_to_be32(XFS_REFC_CRC_MAGIC) },
 	.verify_read		= xfs_refcountbt_read_verify,
 	.verify_write		= xfs_refcountbt_write_verify,
 	.verify_struct		= xfs_refcountbt_verify,
diff --git a/fs/xfs/libxfs/xfs_rmap_btree.c b/fs/xfs/libxfs/xfs_rmap_btree.c
index f79cf040d745..5738e11055e6 100644
--- a/fs/xfs/libxfs/xfs_rmap_btree.c
+++ b/fs/xfs/libxfs/xfs_rmap_btree.c
@@ -310,7 +310,7 @@ xfs_rmapbt_verify(
 	 * from the on disk AGF. Again, we can only check against maximum limits
 	 * in this case.
 	 */
-	if (block->bb_magic != cpu_to_be32(XFS_RMAP_CRC_MAGIC))
+	if (!xfs_verify_magic(bp, block->bb_magic))
 		return __this_address;
 
 	if (!xfs_sb_version_hasrmapbt(&mp->m_sb))
@@ -365,6 +365,7 @@ xfs_rmapbt_write_verify(
 
 const struct xfs_buf_ops xfs_rmapbt_buf_ops = {
 	.name			= "xfs_rmapbt",
+	.magic			= { 0, cpu_to_be32(XFS_RMAP_CRC_MAGIC) },
 	.verify_read		= xfs_rmapbt_read_verify,
 	.verify_write		= xfs_rmapbt_write_verify,
 	.verify_struct		= xfs_rmapbt_verify,
diff --git a/fs/xfs/libxfs/xfs_sb.c b/fs/xfs/libxfs/xfs_sb.c
index a2f52a958091..4e5029c37966 100644
--- a/fs/xfs/libxfs/xfs_sb.c
+++ b/fs/xfs/libxfs/xfs_sb.c
@@ -229,7 +229,7 @@ xfs_validate_sb_common(
 	uint32_t		agcount = 0;
 	uint32_t		rem;
 
-	if (dsb->sb_magicnum != cpu_to_be32(XFS_SB_MAGIC)) {
+	if (!xfs_verify_magic(bp, dsb->sb_magicnum)) {
 		xfs_warn(mp, "bad magic number");
 		return -EWRONGFS;
 	}
@@ -782,12 +782,14 @@ xfs_sb_write_verify(
 
 const struct xfs_buf_ops xfs_sb_buf_ops = {
 	.name = "xfs_sb",
+	.magic = { cpu_to_be32(XFS_SB_MAGIC), cpu_to_be32(XFS_SB_MAGIC) },
 	.verify_read = xfs_sb_read_verify,
 	.verify_write = xfs_sb_write_verify,
 };
 
 const struct xfs_buf_ops xfs_sb_quiet_buf_ops = {
 	.name = "xfs_sb_quiet",
+	.magic = { cpu_to_be32(XFS_SB_MAGIC), cpu_to_be32(XFS_SB_MAGIC) },
 	.verify_read = xfs_sb_quiet_read_verify,
 	.verify_write = xfs_sb_write_verify,
 };
diff --git a/fs/xfs/libxfs/xfs_symlink_remote.c b/fs/xfs/libxfs/xfs_symlink_remote.c
index 77d80106f989..a0ccc253c43d 100644
--- a/fs/xfs/libxfs/xfs_symlink_remote.c
+++ b/fs/xfs/libxfs/xfs_symlink_remote.c
@@ -95,7 +95,7 @@ xfs_symlink_verify(
 
 	if (!xfs_sb_version_hascrc(&mp->m_sb))
 		return __this_address;
-	if (dsl->sl_magic != cpu_to_be32(XFS_SYMLINK_MAGIC))
+	if (!xfs_verify_magic(bp, dsl->sl_magic))
 		return __this_address;
 	if (!uuid_equal(&dsl->sl_uuid, &mp->m_sb.sb_meta_uuid))
 		return __this_address;
@@ -159,6 +159,7 @@ xfs_symlink_write_verify(
 
 const struct xfs_buf_ops xfs_symlink_buf_ops = {
 	.name = "xfs_symlink",
+	.magic = { 0, cpu_to_be32(XFS_SYMLINK_MAGIC) },
 	.verify_read = xfs_symlink_read_verify,
 	.verify_write = xfs_symlink_write_verify,
 	.verify_struct = xfs_symlink_verify,
-- 
2.17.2

[PATCH v2 7/7] xfs: factor xfs_da3_blkinfo verification into common helper

[Brian Foster]

With the verifier magic value helper in place, we've left a bit more
duplicate code across the verifiers that involve struct
xfs_da3_blkinfo. This includes the da node, xattr leaf and dir leaf
verifiers, all of which perform similar checks for v4 and v5
filesystems.

Create a common helper to verify an xfs_da3_blkinfo structure,
taking care to only access v5 fields where appropriate, and refactor
the aforementioned verifiers to use the helper. No functional
changes.

Signed-off-by: Brian Foster <bfoster@redhat.com>
---
 fs/xfs/libxfs/xfs_attr_leaf.c | 17 +++----------
 fs/xfs/libxfs/xfs_da_btree.c  | 46 +++++++++++++++++++++++++----------
 fs/xfs/libxfs/xfs_da_format.h |  2 ++
 fs/xfs/libxfs/xfs_dir2_leaf.c | 18 +++-----------
 4 files changed, 43 insertions(+), 40 deletions(-)

diff --git a/fs/xfs/libxfs/xfs_attr_leaf.c b/fs/xfs/libxfs/xfs_attr_leaf.c
index dde3a550d7ce..0c92987f57fc 100644
--- a/fs/xfs/libxfs/xfs_attr_leaf.c
+++ b/fs/xfs/libxfs/xfs_attr_leaf.c
@@ -245,23 +245,14 @@ xfs_attr3_leaf_verify(
 	struct xfs_attr_leaf_entry	*entries;
 	uint32_t			end;	/* must be 32bit - see below */
 	int				i;
+	xfs_failaddr_t			fa;
 
 	xfs_attr3_leaf_hdr_from_disk(mp->m_attr_geo, &ichdr, leaf);
 
-	if (!xfs_verify_magic(bp, leaf->hdr.info.magic))
-		return __this_address;
-
-	if (xfs_sb_version_hascrc(&mp->m_sb)) {
-		struct xfs_da3_node_hdr *hdr3 = bp->b_addr;
+	fa = xfs_da3_blkinfo_verify(bp, bp->b_addr);
+	if (fa)
+		return fa;
 
-		ASSERT(leaf->hdr.info.magic == hdr3->info.hdr.magic);
-		if (!uuid_equal(&hdr3->info.uuid, &mp->m_sb.sb_meta_uuid))
-			return __this_address;
-		if (be64_to_cpu(hdr3->info.blkno) != bp->b_bn)
-			return __this_address;
-		if (!xfs_log_check_lsn(mp, be64_to_cpu(hdr3->info.lsn)))
-			return __this_address;
-	}
 	/*
 	 * In recovery there is a transient state where count == 0 is valid
 	 * because we may have transitioned an empty shortform attr to a leaf
diff --git a/fs/xfs/libxfs/xfs_da_btree.c b/fs/xfs/libxfs/xfs_da_btree.c
index 6b8ca390eecc..622f88a1b1aa 100644
--- a/fs/xfs/libxfs/xfs_da_btree.c
+++ b/fs/xfs/libxfs/xfs_da_btree.c
@@ -116,6 +116,35 @@ xfs_da_state_free(xfs_da_state_t *state)
 	kmem_zone_free(xfs_da_state_zone, state);
 }
 
+/*
+ * Verify an xfs_da3_blkinfo structure. Note that the da3 fields are only
+ * accessible on v5 filesystems. This header format is common across da node,
+ * attr leaf and dir leaf blocks.
+ */
+xfs_failaddr_t
+xfs_da3_blkinfo_verify(
+	struct xfs_buf		*bp,
+	struct xfs_da3_blkinfo	*hdr3)
+{
+	struct xfs_mount	*mp = bp->b_target->bt_mount;
+	struct xfs_da_blkinfo	*hdr = &hdr3->hdr;
+
+	if (!xfs_verify_magic(bp, hdr->magic))
+		return __this_address;
+
+	if (xfs_sb_version_hascrc(&mp->m_sb)) {
+		ASSERT(hdr3->hdr.magic == hdr->magic);
+		if (!uuid_equal(&hdr3->uuid, &mp->m_sb.sb_meta_uuid))
+			return __this_address;
+		if (be64_to_cpu(hdr3->blkno) != bp->b_bn)
+			return __this_address;
+		if (!xfs_log_check_lsn(mp, be64_to_cpu(hdr3->lsn)))
+			return __this_address;
+	}
+
+	return 0;
+}
+
 static xfs_failaddr_t
 xfs_da3_node_verify(
 	struct xfs_buf		*bp)
@@ -124,25 +153,16 @@ xfs_da3_node_verify(
 	struct xfs_da_intnode	*hdr = bp->b_addr;
 	struct xfs_da3_icnode_hdr ichdr;
 	const struct xfs_dir_ops *ops;
+	xfs_failaddr_t		fa;
 
 	ops = xfs_dir_get_ops(mp, NULL);
 
 	ops->node_hdr_from_disk(&ichdr, hdr);
 
-	if (!xfs_verify_magic(bp, hdr->hdr.info.magic))
-		return __this_address;
+	fa = xfs_da3_blkinfo_verify(bp, bp->b_addr);
+	if (fa)
+		return fa;
 
-	if (xfs_sb_version_hascrc(&mp->m_sb)) {
-		struct xfs_da3_node_hdr *hdr3 = bp->b_addr;
-
-		ASSERT(hdr3->info.hdr.magic == hdr->hdr.info.magic);
-		if (!uuid_equal(&hdr3->info.uuid, &mp->m_sb.sb_meta_uuid))
-			return __this_address;
-		if (be64_to_cpu(hdr3->info.blkno) != bp->b_bn)
-			return __this_address;
-		if (!xfs_log_check_lsn(mp, be64_to_cpu(hdr3->info.lsn)))
-			return __this_address;
-	}
 	if (ichdr.level == 0)
 		return __this_address;
 	if (ichdr.level > XFS_DA_NODE_MAXDEPTH)
diff --git a/fs/xfs/libxfs/xfs_da_format.h b/fs/xfs/libxfs/xfs_da_format.h
index 5d5bf3bffc78..bf0c53bd2e60 100644
--- a/fs/xfs/libxfs/xfs_da_format.h
+++ b/fs/xfs/libxfs/xfs_da_format.h
@@ -869,4 +869,6 @@ static inline unsigned int xfs_dir2_dirblock_bytes(struct xfs_sb *sbp)
 	return 1 << (sbp->sb_blocklog + sbp->sb_dirblklog);
 }
 
+xfs_failaddr_t xfs_da3_blkinfo_verify(struct xfs_buf *, struct xfs_da3_blkinfo*);
+
 #endif /* __XFS_DA_FORMAT_H__ */
diff --git a/fs/xfs/libxfs/xfs_dir2_leaf.c b/fs/xfs/libxfs/xfs_dir2_leaf.c
index a99ae2cd292e..094028b7b162 100644
--- a/fs/xfs/libxfs/xfs_dir2_leaf.c
+++ b/fs/xfs/libxfs/xfs_dir2_leaf.c
@@ -146,21 +146,11 @@ xfs_dir3_leaf_verify(
 {
 	struct xfs_mount	*mp = bp->b_target->bt_mount;
 	struct xfs_dir2_leaf	*leaf = bp->b_addr;
+	xfs_failaddr_t		fa;
 
-	if (!xfs_verify_magic(bp, leaf->hdr.info.magic))
-		return __this_address;
-
-	if (xfs_sb_version_hascrc(&mp->m_sb)) {
-		struct xfs_dir3_leaf_hdr *leaf3 = bp->b_addr;
-
-		ASSERT(leaf3->info.hdr.magic == leaf->hdr.info.magic);
-		if (!uuid_equal(&leaf3->info.uuid, &mp->m_sb.sb_meta_uuid))
-			return __this_address;
-		if (be64_to_cpu(leaf3->info.blkno) != bp->b_bn)
-			return __this_address;
-		if (!xfs_log_check_lsn(mp, be64_to_cpu(leaf3->info.lsn)))
-			return __this_address;
-	}
+	fa = xfs_da3_blkinfo_verify(bp, bp->b_addr);
+	if (fa)
+		return fa;
 
 	return xfs_dir3_leaf_check_int(mp, NULL, NULL, leaf);
 }
-- 
2.17.2

Re: [PATCH v2 7/7] xfs: factor xfs_da3_blkinfo verification into common helper

[Darrick J. Wong]

On Thu, Jan 31, 2019 at 10:46:08AM -0500, Brian Foster wrote:
> With the verifier magic value helper in place, we've left a bit more
> duplicate code across the verifiers that involve struct
> xfs_da3_blkinfo. This includes the da node, xattr leaf and dir leaf
> verifiers, all of which perform similar checks for v4 and v5
> filesystems.
> 
> Create a common helper to verify an xfs_da3_blkinfo structure,
> taking care to only access v5 fields where appropriate, and refactor
> the aforementioned verifiers to use the helper. No functional
> changes.
> 
> Signed-off-by: Brian Foster <bfoster@redhat.com>
> ---
>  fs/xfs/libxfs/xfs_attr_leaf.c | 17 +++----------
>  fs/xfs/libxfs/xfs_da_btree.c  | 46 +++++++++++++++++++++++++----------
>  fs/xfs/libxfs/xfs_da_format.h |  2 ++
>  fs/xfs/libxfs/xfs_dir2_leaf.c | 18 +++-----------
>  4 files changed, 43 insertions(+), 40 deletions(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_attr_leaf.c b/fs/xfs/libxfs/xfs_attr_leaf.c
> index dde3a550d7ce..0c92987f57fc 100644
> --- a/fs/xfs/libxfs/xfs_attr_leaf.c
> +++ b/fs/xfs/libxfs/xfs_attr_leaf.c
> @@ -245,23 +245,14 @@ xfs_attr3_leaf_verify(
>  	struct xfs_attr_leaf_entry	*entries;
>  	uint32_t			end;	/* must be 32bit - see below */
>  	int				i;
> +	xfs_failaddr_t			fa;
>  
>  	xfs_attr3_leaf_hdr_from_disk(mp->m_attr_geo, &ichdr, leaf);
>  
> -	if (!xfs_verify_magic(bp, leaf->hdr.info.magic))
> -		return __this_address;
> -
> -	if (xfs_sb_version_hascrc(&mp->m_sb)) {
> -		struct xfs_da3_node_hdr *hdr3 = bp->b_addr;
> +	fa = xfs_da3_blkinfo_verify(bp, bp->b_addr);
> +	if (fa)
> +		return fa;
>  
> -		ASSERT(leaf->hdr.info.magic == hdr3->info.hdr.magic);
> -		if (!uuid_equal(&hdr3->info.uuid, &mp->m_sb.sb_meta_uuid))
> -			return __this_address;
> -		if (be64_to_cpu(hdr3->info.blkno) != bp->b_bn)
> -			return __this_address;
> -		if (!xfs_log_check_lsn(mp, be64_to_cpu(hdr3->info.lsn)))
> -			return __this_address;
> -	}
>  	/*
>  	 * In recovery there is a transient state where count == 0 is valid
>  	 * because we may have transitioned an empty shortform attr to a leaf
> diff --git a/fs/xfs/libxfs/xfs_da_btree.c b/fs/xfs/libxfs/xfs_da_btree.c
> index 6b8ca390eecc..622f88a1b1aa 100644
> --- a/fs/xfs/libxfs/xfs_da_btree.c
> +++ b/fs/xfs/libxfs/xfs_da_btree.c
> @@ -116,6 +116,35 @@ xfs_da_state_free(xfs_da_state_t *state)
>  	kmem_zone_free(xfs_da_state_zone, state);
>  }
>  
> +/*
> + * Verify an xfs_da3_blkinfo structure. Note that the da3 fields are only
> + * accessible on v5 filesystems. This header format is common across da node,
> + * attr leaf and dir leaf blocks.
> + */
> +xfs_failaddr_t
> +xfs_da3_blkinfo_verify(
> +	struct xfs_buf		*bp,
> +	struct xfs_da3_blkinfo	*hdr3)
> +{
> +	struct xfs_mount	*mp = bp->b_target->bt_mount;
> +	struct xfs_da_blkinfo	*hdr = &hdr3->hdr;
> +
> +	if (!xfs_verify_magic(bp, hdr->magic))
> +		return __this_address;
> +
> +	if (xfs_sb_version_hascrc(&mp->m_sb)) {
> +		ASSERT(hdr3->hdr.magic == hdr->magic);
> +		if (!uuid_equal(&hdr3->uuid, &mp->m_sb.sb_meta_uuid))
> +			return __this_address;
> +		if (be64_to_cpu(hdr3->blkno) != bp->b_bn)
> +			return __this_address;
> +		if (!xfs_log_check_lsn(mp, be64_to_cpu(hdr3->lsn)))
> +			return __this_address;
> +	}
> +
> +	return 0;
> +}
> +
>  static xfs_failaddr_t
>  xfs_da3_node_verify(
>  	struct xfs_buf		*bp)
> @@ -124,25 +153,16 @@ xfs_da3_node_verify(
>  	struct xfs_da_intnode	*hdr = bp->b_addr;
>  	struct xfs_da3_icnode_hdr ichdr;
>  	const struct xfs_dir_ops *ops;
> +	xfs_failaddr_t		fa;
>  
>  	ops = xfs_dir_get_ops(mp, NULL);
>  
>  	ops->node_hdr_from_disk(&ichdr, hdr);
>  
> -	if (!xfs_verify_magic(bp, hdr->hdr.info.magic))
> -		return __this_address;
> +	fa = xfs_da3_blkinfo_verify(bp, bp->b_addr);
> +	if (fa)
> +		return fa;
>  
> -	if (xfs_sb_version_hascrc(&mp->m_sb)) {
> -		struct xfs_da3_node_hdr *hdr3 = bp->b_addr;
> -
> -		ASSERT(hdr3->info.hdr.magic == hdr->hdr.info.magic);
> -		if (!uuid_equal(&hdr3->info.uuid, &mp->m_sb.sb_meta_uuid))
> -			return __this_address;
> -		if (be64_to_cpu(hdr3->info.blkno) != bp->b_bn)
> -			return __this_address;
> -		if (!xfs_log_check_lsn(mp, be64_to_cpu(hdr3->info.lsn)))
> -			return __this_address;
> -	}
>  	if (ichdr.level == 0)
>  		return __this_address;
>  	if (ichdr.level > XFS_DA_NODE_MAXDEPTH)
> diff --git a/fs/xfs/libxfs/xfs_da_format.h b/fs/xfs/libxfs/xfs_da_format.h
> index 5d5bf3bffc78..bf0c53bd2e60 100644
> --- a/fs/xfs/libxfs/xfs_da_format.h
> +++ b/fs/xfs/libxfs/xfs_da_format.h
> @@ -869,4 +869,6 @@ static inline unsigned int xfs_dir2_dirblock_bytes(struct xfs_sb *sbp)
>  	return 1 << (sbp->sb_blocklog + sbp->sb_dirblklog);
>  }
>  
> +xfs_failaddr_t xfs_da3_blkinfo_verify(struct xfs_buf *, struct xfs_da3_blkinfo*);

Specify parameter names in the declaration, please.

--D

> +
>  #endif /* __XFS_DA_FORMAT_H__ */
> diff --git a/fs/xfs/libxfs/xfs_dir2_leaf.c b/fs/xfs/libxfs/xfs_dir2_leaf.c
> index a99ae2cd292e..094028b7b162 100644
> --- a/fs/xfs/libxfs/xfs_dir2_leaf.c
> +++ b/fs/xfs/libxfs/xfs_dir2_leaf.c
> @@ -146,21 +146,11 @@ xfs_dir3_leaf_verify(
>  {
>  	struct xfs_mount	*mp = bp->b_target->bt_mount;
>  	struct xfs_dir2_leaf	*leaf = bp->b_addr;
> +	xfs_failaddr_t		fa;
>  
> -	if (!xfs_verify_magic(bp, leaf->hdr.info.magic))
> -		return __this_address;
> -
> -	if (xfs_sb_version_hascrc(&mp->m_sb)) {
> -		struct xfs_dir3_leaf_hdr *leaf3 = bp->b_addr;
> -
> -		ASSERT(leaf3->info.hdr.magic == leaf->hdr.info.magic);
> -		if (!uuid_equal(&leaf3->info.uuid, &mp->m_sb.sb_meta_uuid))
> -			return __this_address;
> -		if (be64_to_cpu(leaf3->info.blkno) != bp->b_bn)
> -			return __this_address;
> -		if (!xfs_log_check_lsn(mp, be64_to_cpu(leaf3->info.lsn)))
> -			return __this_address;
> -	}
> +	fa = xfs_da3_blkinfo_verify(bp, bp->b_addr);
> +	if (fa)
> +		return fa;
>  
>  	return xfs_dir3_leaf_check_int(mp, NULL, NULL, leaf);
>  }
> -- 
> 2.17.2
> 

Re: [PATCH v2 0/7] xfs: fix [f]inobt magic value verification

[Darrick J. Wong]

On Thu, Jan 31, 2019 at 10:46:01AM -0500, Brian Foster wrote:
> Hi all,
> 
> Here's v2 of the verifier magic value cleanup. This is generally the
> same idea as the previous version. The primary difference is that v2
> first converts verifiers that use cpu order magic comparisons to use
> on-disk (big endian) order and then stores/compares the verifier magic
> values in on-disk order. The purpose of this change is to reduce the
> number of byte swaps required at runtime for verifier magic value
> checks. Further changes include some cleanups, additional refactoring
> and the inclusion of Darrick's scrub ->b_ops fix with some modifications
> from the original version.
> 
> This survives fstests on v4 and v5 filesystems on both little and big
> endian systems without regressions. Thoughts, reviews, flames
> appreciated.

One thing I was expecting to see but didn't -- are you going to convert
the bnobt/cntbt verifiers so that we have tree-specific magic number
checking?

--D

> Brian
> 
> v2:
> - Include djwong's ->b_ops patch w/ modifications.
> - Added patch to fix up existing cpu endian magic checks, fold in typo
>   fix.
> - Replace static inline magic verifier helper with out of line variant,
>   kill macro.
> - Store on-disk byte order magics in ->b_ops.
> - Added patch to refactor common xfs_da3_blkinfo checks.
> v1: https://marc.info/?l=linux-xfs&m=154878684317178&w=2
> - Remove endian conversion from helper.
> - Drop finobt bad magic mitigation patch.
> - Additional verifier magic fixups.
> - Add verifier name typo fixup.
> rfcv2: https://marc.info/?l=linux-xfs&m=154868884311668&w=2
> - Split off finobt verifier into separate patch, assign it
>   appropriately.
> - Created helpers for xfs_buf_ops magic value verification.
> - Added error mitigation patch for problematic finobt blocks.
> rfcv1: https://marc.info/?l=linux-xfs&m=154834528212262&w=2
> 
> Brian Foster (6):
>   xfs: always check magic values in on-disk byte order
>   xfs: create a separate finobt verifier
>   xfs: distinguish between inobt and finobt magic values
>   xfs: use verifier magic field in dir2 leaf verifiers
>   xfs: miscellaneous verifier magic value fixups
>   xfs: factor xfs_da3_blkinfo verification into common helper
> 
> Darrick J. Wong (1):
>   xfs: set buffer ops when repair probes for btree type
> 
>  fs/xfs/libxfs/xfs_ag.c             |   2 +-
>  fs/xfs/libxfs/xfs_alloc.c          |  12 ++--
>  fs/xfs/libxfs/xfs_attr_leaf.c      |  21 ++----
>  fs/xfs/libxfs/xfs_attr_remote.c    |   8 ++-
>  fs/xfs/libxfs/xfs_bmap_btree.c     |  13 ++--
>  fs/xfs/libxfs/xfs_da_btree.c       |  50 ++++++++++-----
>  fs/xfs/libxfs/xfs_da_format.h      |   2 +
>  fs/xfs/libxfs/xfs_dir2_block.c     |  10 +--
>  fs/xfs/libxfs/xfs_dir2_data.c      |  12 ++--
>  fs/xfs/libxfs/xfs_dir2_leaf.c      | 100 ++++++-----------------------
>  fs/xfs/libxfs/xfs_dir2_node.c      |  11 ++--
>  fs/xfs/libxfs/xfs_ialloc.c         |   3 +-
>  fs/xfs/libxfs/xfs_ialloc_btree.c   |  25 +++++---
>  fs/xfs/libxfs/xfs_inode_buf.c      |   2 +-
>  fs/xfs/libxfs/xfs_refcount_btree.c |   3 +-
>  fs/xfs/libxfs/xfs_rmap_btree.c     |   3 +-
>  fs/xfs/libxfs/xfs_sb.c             |   5 +-
>  fs/xfs/libxfs/xfs_shared.h         |   1 +
>  fs/xfs/libxfs/xfs_symlink_remote.c |   3 +-
>  fs/xfs/scrub/agheader_repair.c     |   2 +-
>  fs/xfs/scrub/repair.c              |  11 +++-
>  fs/xfs/xfs_buf.c                   |  41 ++++++++++--
>  fs/xfs/xfs_buf.h                   |   4 +-
>  fs/xfs/xfs_log_recover.c           |   6 +-
>  fs/xfs/xfs_trans_buf.c             |   2 +-
>  25 files changed, 183 insertions(+), 169 deletions(-)
> 
> -- 
> 2.17.2
> 

Re: [PATCH v2 0/7] xfs: fix [f]inobt magic value verification

[Brian Foster]

On Thu, Jan 31, 2019 at 09:54:04AM -0800, Darrick J. Wong wrote:
> On Thu, Jan 31, 2019 at 10:46:01AM -0500, Brian Foster wrote:
> > Hi all,
> > 
> > Here's v2 of the verifier magic value cleanup. This is generally the
> > same idea as the previous version. The primary difference is that v2
> > first converts verifiers that use cpu order magic comparisons to use
> > on-disk (big endian) order and then stores/compares the verifier magic
> > values in on-disk order. The purpose of this change is to reduce the
> > number of byte swaps required at runtime for verifier magic value
> > checks. Further changes include some cleanups, additional refactoring
> > and the inclusion of Darrick's scrub ->b_ops fix with some modifications
> > from the original version.
> > 
> > This survives fstests on v4 and v5 filesystems on both little and big
> > endian systems without regressions. Thoughts, reviews, flames
> > appreciated.
> 
> One thing I was expecting to see but didn't -- are you going to convert
> the bnobt/cntbt verifiers so that we have tree-specific magic number
> checking?
> 

I originally skipped over that one because the hardcoded checks looked
convoluted. Looking again, we probably should include it simply because
it's a similar shared verifier case as for the inobt vs. finobt. We
might still have to keep around a couple hardcoded magic checks for the
pagf_levels thing, but I'll look into it and tack on a new patch for
xfs_allocbt_buf_ops.

Brian

> --D
> 
> > Brian
> > 
> > v2:
> > - Include djwong's ->b_ops patch w/ modifications.
> > - Added patch to fix up existing cpu endian magic checks, fold in typo
> >   fix.
> > - Replace static inline magic verifier helper with out of line variant,
> >   kill macro.
> > - Store on-disk byte order magics in ->b_ops.
> > - Added patch to refactor common xfs_da3_blkinfo checks.
> > v1: https://marc.info/?l=linux-xfs&m=154878684317178&w=2
> > - Remove endian conversion from helper.
> > - Drop finobt bad magic mitigation patch.
> > - Additional verifier magic fixups.
> > - Add verifier name typo fixup.
> > rfcv2: https://marc.info/?l=linux-xfs&m=154868884311668&w=2
> > - Split off finobt verifier into separate patch, assign it
> >   appropriately.
> > - Created helpers for xfs_buf_ops magic value verification.
> > - Added error mitigation patch for problematic finobt blocks.
> > rfcv1: https://marc.info/?l=linux-xfs&m=154834528212262&w=2
> > 
> > Brian Foster (6):
> >   xfs: always check magic values in on-disk byte order
> >   xfs: create a separate finobt verifier
> >   xfs: distinguish between inobt and finobt magic values
> >   xfs: use verifier magic field in dir2 leaf verifiers
> >   xfs: miscellaneous verifier magic value fixups
> >   xfs: factor xfs_da3_blkinfo verification into common helper
> > 
> > Darrick J. Wong (1):
> >   xfs: set buffer ops when repair probes for btree type
> > 
> >  fs/xfs/libxfs/xfs_ag.c             |   2 +-
> >  fs/xfs/libxfs/xfs_alloc.c          |  12 ++--
> >  fs/xfs/libxfs/xfs_attr_leaf.c      |  21 ++----
> >  fs/xfs/libxfs/xfs_attr_remote.c    |   8 ++-
> >  fs/xfs/libxfs/xfs_bmap_btree.c     |  13 ++--
> >  fs/xfs/libxfs/xfs_da_btree.c       |  50 ++++++++++-----
> >  fs/xfs/libxfs/xfs_da_format.h      |   2 +
> >  fs/xfs/libxfs/xfs_dir2_block.c     |  10 +--
> >  fs/xfs/libxfs/xfs_dir2_data.c      |  12 ++--
> >  fs/xfs/libxfs/xfs_dir2_leaf.c      | 100 ++++++-----------------------
> >  fs/xfs/libxfs/xfs_dir2_node.c      |  11 ++--
> >  fs/xfs/libxfs/xfs_ialloc.c         |   3 +-
> >  fs/xfs/libxfs/xfs_ialloc_btree.c   |  25 +++++---
> >  fs/xfs/libxfs/xfs_inode_buf.c      |   2 +-
> >  fs/xfs/libxfs/xfs_refcount_btree.c |   3 +-
> >  fs/xfs/libxfs/xfs_rmap_btree.c     |   3 +-
> >  fs/xfs/libxfs/xfs_sb.c             |   5 +-
> >  fs/xfs/libxfs/xfs_shared.h         |   1 +
> >  fs/xfs/libxfs/xfs_symlink_remote.c |   3 +-
> >  fs/xfs/scrub/agheader_repair.c     |   2 +-
> >  fs/xfs/scrub/repair.c              |  11 +++-
> >  fs/xfs/xfs_buf.c                   |  41 ++++++++++--
> >  fs/xfs/xfs_buf.h                   |   4 +-
> >  fs/xfs/xfs_log_recover.c           |   6 +-
> >  fs/xfs/xfs_trans_buf.c             |   2 +-
> >  25 files changed, 183 insertions(+), 169 deletions(-)
> > 
> > -- 
> > 2.17.2
> > 

Re: [PATCH v2 7/7] xfs: factor xfs_da3_blkinfo verification into common helper

[Brian Foster]

On Thu, Jan 31, 2019 at 09:51:13AM -0800, Darrick J. Wong wrote:
> On Thu, Jan 31, 2019 at 10:46:08AM -0500, Brian Foster wrote:
> > With the verifier magic value helper in place, we've left a bit more
> > duplicate code across the verifiers that involve struct
> > xfs_da3_blkinfo. This includes the da node, xattr leaf and dir leaf
> > verifiers, all of which perform similar checks for v4 and v5
> > filesystems.
> > 
> > Create a common helper to verify an xfs_da3_blkinfo structure,
> > taking care to only access v5 fields where appropriate, and refactor
> > the aforementioned verifiers to use the helper. No functional
> > changes.
> > 
> > Signed-off-by: Brian Foster <bfoster@redhat.com>
> > ---
> >  fs/xfs/libxfs/xfs_attr_leaf.c | 17 +++----------
> >  fs/xfs/libxfs/xfs_da_btree.c  | 46 +++++++++++++++++++++++++----------
> >  fs/xfs/libxfs/xfs_da_format.h |  2 ++
> >  fs/xfs/libxfs/xfs_dir2_leaf.c | 18 +++-----------
> >  4 files changed, 43 insertions(+), 40 deletions(-)
> > 
> > diff --git a/fs/xfs/libxfs/xfs_attr_leaf.c b/fs/xfs/libxfs/xfs_attr_leaf.c
> > index dde3a550d7ce..0c92987f57fc 100644
> > --- a/fs/xfs/libxfs/xfs_attr_leaf.c
> > +++ b/fs/xfs/libxfs/xfs_attr_leaf.c
> > @@ -245,23 +245,14 @@ xfs_attr3_leaf_verify(
> >  	struct xfs_attr_leaf_entry	*entries;
> >  	uint32_t			end;	/* must be 32bit - see below */
> >  	int				i;
> > +	xfs_failaddr_t			fa;
> >  
> >  	xfs_attr3_leaf_hdr_from_disk(mp->m_attr_geo, &ichdr, leaf);
> >  
> > -	if (!xfs_verify_magic(bp, leaf->hdr.info.magic))
> > -		return __this_address;
> > -
> > -	if (xfs_sb_version_hascrc(&mp->m_sb)) {
> > -		struct xfs_da3_node_hdr *hdr3 = bp->b_addr;
> > +	fa = xfs_da3_blkinfo_verify(bp, bp->b_addr);
> > +	if (fa)
> > +		return fa;
> >  
> > -		ASSERT(leaf->hdr.info.magic == hdr3->info.hdr.magic);
> > -		if (!uuid_equal(&hdr3->info.uuid, &mp->m_sb.sb_meta_uuid))
> > -			return __this_address;
> > -		if (be64_to_cpu(hdr3->info.blkno) != bp->b_bn)
> > -			return __this_address;
> > -		if (!xfs_log_check_lsn(mp, be64_to_cpu(hdr3->info.lsn)))
> > -			return __this_address;
> > -	}
> >  	/*
> >  	 * In recovery there is a transient state where count == 0 is valid
> >  	 * because we may have transitioned an empty shortform attr to a leaf
> > diff --git a/fs/xfs/libxfs/xfs_da_btree.c b/fs/xfs/libxfs/xfs_da_btree.c
> > index 6b8ca390eecc..622f88a1b1aa 100644
> > --- a/fs/xfs/libxfs/xfs_da_btree.c
> > +++ b/fs/xfs/libxfs/xfs_da_btree.c
> > @@ -116,6 +116,35 @@ xfs_da_state_free(xfs_da_state_t *state)
> >  	kmem_zone_free(xfs_da_state_zone, state);
> >  }
> >  
> > +/*
> > + * Verify an xfs_da3_blkinfo structure. Note that the da3 fields are only
> > + * accessible on v5 filesystems. This header format is common across da node,
> > + * attr leaf and dir leaf blocks.
> > + */
> > +xfs_failaddr_t
> > +xfs_da3_blkinfo_verify(
> > +	struct xfs_buf		*bp,
> > +	struct xfs_da3_blkinfo	*hdr3)
> > +{
> > +	struct xfs_mount	*mp = bp->b_target->bt_mount;
> > +	struct xfs_da_blkinfo	*hdr = &hdr3->hdr;
> > +
> > +	if (!xfs_verify_magic(bp, hdr->magic))
> > +		return __this_address;
> > +
> > +	if (xfs_sb_version_hascrc(&mp->m_sb)) {
> > +		ASSERT(hdr3->hdr.magic == hdr->magic);
> > +		if (!uuid_equal(&hdr3->uuid, &mp->m_sb.sb_meta_uuid))
> > +			return __this_address;
> > +		if (be64_to_cpu(hdr3->blkno) != bp->b_bn)
> > +			return __this_address;
> > +		if (!xfs_log_check_lsn(mp, be64_to_cpu(hdr3->lsn)))
> > +			return __this_address;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> >  static xfs_failaddr_t
> >  xfs_da3_node_verify(
> >  	struct xfs_buf		*bp)
> > @@ -124,25 +153,16 @@ xfs_da3_node_verify(
> >  	struct xfs_da_intnode	*hdr = bp->b_addr;
> >  	struct xfs_da3_icnode_hdr ichdr;
> >  	const struct xfs_dir_ops *ops;
> > +	xfs_failaddr_t		fa;
> >  
> >  	ops = xfs_dir_get_ops(mp, NULL);
> >  
> >  	ops->node_hdr_from_disk(&ichdr, hdr);
> >  
> > -	if (!xfs_verify_magic(bp, hdr->hdr.info.magic))
> > -		return __this_address;
> > +	fa = xfs_da3_blkinfo_verify(bp, bp->b_addr);
> > +	if (fa)
> > +		return fa;
> >  
> > -	if (xfs_sb_version_hascrc(&mp->m_sb)) {
> > -		struct xfs_da3_node_hdr *hdr3 = bp->b_addr;
> > -
> > -		ASSERT(hdr3->info.hdr.magic == hdr->hdr.info.magic);
> > -		if (!uuid_equal(&hdr3->info.uuid, &mp->m_sb.sb_meta_uuid))
> > -			return __this_address;
> > -		if (be64_to_cpu(hdr3->info.blkno) != bp->b_bn)
> > -			return __this_address;
> > -		if (!xfs_log_check_lsn(mp, be64_to_cpu(hdr3->info.lsn)))
> > -			return __this_address;
> > -	}
> >  	if (ichdr.level == 0)
> >  		return __this_address;
> >  	if (ichdr.level > XFS_DA_NODE_MAXDEPTH)
> > diff --git a/fs/xfs/libxfs/xfs_da_format.h b/fs/xfs/libxfs/xfs_da_format.h
> > index 5d5bf3bffc78..bf0c53bd2e60 100644
> > --- a/fs/xfs/libxfs/xfs_da_format.h
> > +++ b/fs/xfs/libxfs/xfs_da_format.h
> > @@ -869,4 +869,6 @@ static inline unsigned int xfs_dir2_dirblock_bytes(struct xfs_sb *sbp)
> >  	return 1 << (sbp->sb_blocklog + sbp->sb_dirblklog);
> >  }
> >  
> > +xfs_failaddr_t xfs_da3_blkinfo_verify(struct xfs_buf *, struct xfs_da3_blkinfo*);
> 
> Specify parameter names in the declaration, please.
> 

Ok, will fix.

Brian

> --D
> 
> > +
> >  #endif /* __XFS_DA_FORMAT_H__ */
> > diff --git a/fs/xfs/libxfs/xfs_dir2_leaf.c b/fs/xfs/libxfs/xfs_dir2_leaf.c
> > index a99ae2cd292e..094028b7b162 100644
> > --- a/fs/xfs/libxfs/xfs_dir2_leaf.c
> > +++ b/fs/xfs/libxfs/xfs_dir2_leaf.c
> > @@ -146,21 +146,11 @@ xfs_dir3_leaf_verify(
> >  {
> >  	struct xfs_mount	*mp = bp->b_target->bt_mount;
> >  	struct xfs_dir2_leaf	*leaf = bp->b_addr;
> > +	xfs_failaddr_t		fa;
> >  
> > -	if (!xfs_verify_magic(bp, leaf->hdr.info.magic))
> > -		return __this_address;
> > -
> > -	if (xfs_sb_version_hascrc(&mp->m_sb)) {
> > -		struct xfs_dir3_leaf_hdr *leaf3 = bp->b_addr;
> > -
> > -		ASSERT(leaf3->info.hdr.magic == leaf->hdr.info.magic);
> > -		if (!uuid_equal(&leaf3->info.uuid, &mp->m_sb.sb_meta_uuid))
> > -			return __this_address;
> > -		if (be64_to_cpu(leaf3->info.blkno) != bp->b_bn)
> > -			return __this_address;
> > -		if (!xfs_log_check_lsn(mp, be64_to_cpu(leaf3->info.lsn)))
> > -			return __this_address;
> > -	}
> > +	fa = xfs_da3_blkinfo_verify(bp, bp->b_addr);
> > +	if (fa)
> > +		return fa;
> >  
> >  	return xfs_dir3_leaf_check_int(mp, NULL, NULL, leaf);
> >  }
> > -- 
> > 2.17.2
> >