[PATCH v4 0/8] xfs: fix [f]inobt magic value verification

[PATCH v4 0/8] xfs: fix [f]inobt magic value verification

[Brian Foster]

Hi all,

This is v4 of the buffer verifier magic value rework. The only changes
in this version are that patch 1 of v3 has been dropped as it is already
merged and that the magic value asserts that were added to several of
the refactored verifiers are replaced with compile-time magic value
offset checks starting from the top-level structures used in the
associated verifiers.

Brian

v4:
- Dropped set buf ops patch (already merged).
- Replace magic value asserts with build-time magic offset checks.
v3: https://marc.info/?l=linux-xfs&m=154929197225225&w=2
- Fix function declarations to include parameter names.
- Convert allocbt buffer verifier.
v2: https://marc.info/?l=linux-xfs&m=154894958207167&w=2
- 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 (8):
  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: split up allocation btree verifier
  xfs: distinguish between bnobt and cntbt 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

 fs/xfs/libxfs/xfs_ag.c             |   6 +-
 fs/xfs/libxfs/xfs_alloc.c          |  12 ++--
 fs/xfs/libxfs/xfs_alloc_btree.c    |  74 ++++++++++-----------
 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       |  49 +++++++++-----
 fs/xfs/libxfs/xfs_da_format.h      |   3 +
 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      |  10 +--
 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         |   4 +-
 fs/xfs/libxfs/xfs_symlink_remote.c |   3 +-
 fs/xfs/scrub/agheader_repair.c     |   6 +-
 fs/xfs/xfs_buf.c                   |  19 ++++++
 fs/xfs/xfs_buf.h                   |   2 +
 fs/xfs/xfs_log_recover.c           |  12 ++--
 fs/xfs/xfs_ondisk.h                |  17 +++++
 25 files changed, 218 insertions(+), 204 deletions(-)

-- 
2.17.2

[PATCH v4 1/8] 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>
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.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 v4 2/8] 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>
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.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 v4 3/8] 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>
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.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 e1c5be26bf9f..c9cf8c0c7d32 100644
--- a/fs/xfs/xfs_buf.c
+++ b/fs/xfs/xfs_buf.c
@@ -2203,3 +2203,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..44f9423a1861 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 *bp, uint32_t dmagic);
 
 #endif	/* __XFS_BUF_H__ */
-- 
2.17.2

[PATCH v4 4/8] xfs: split up allocation btree verifier

[Brian Foster]

Similar to the inode btree verifier, the same allocation btree
verifier structure is shared between the by-bno (bnobt) and by-size
(cntbt) btrees. This prevents the ability to distinguish magic
values between them. Separate the verifier into two, one for each
tree, and assign them appropriately. No functional changes.

Signed-off-by: Brian Foster <bfoster@redhat.com>
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 fs/xfs/libxfs/xfs_ag.c          |  4 ++--
 fs/xfs/libxfs/xfs_alloc_btree.c | 14 ++++++++++----
 fs/xfs/libxfs/xfs_shared.h      |  3 ++-
 fs/xfs/scrub/agheader_repair.c  |  4 ++--
 fs/xfs/xfs_log_recover.c        |  6 ++++--
 5 files changed, 20 insertions(+), 11 deletions(-)

diff --git a/fs/xfs/libxfs/xfs_ag.c b/fs/xfs/libxfs/xfs_ag.c
index bde67ef3ff43..1ef8acf35e7d 100644
--- a/fs/xfs/libxfs/xfs_ag.c
+++ b/fs/xfs/libxfs/xfs_ag.c
@@ -339,14 +339,14 @@ xfs_ag_init_headers(
 	{ /* BNO root block */
 		.daddr = XFS_AGB_TO_DADDR(mp, id->agno, XFS_BNO_BLOCK(mp)),
 		.numblks = BTOBB(mp->m_sb.sb_blocksize),
-		.ops = &xfs_allocbt_buf_ops,
+		.ops = &xfs_bnobt_buf_ops,
 		.work = &xfs_bnoroot_init,
 		.need_init = true
 	},
 	{ /* CNT root block */
 		.daddr = XFS_AGB_TO_DADDR(mp, id->agno, XFS_CNT_BLOCK(mp)),
 		.numblks = BTOBB(mp->m_sb.sb_blocksize),
-		.ops = &xfs_allocbt_buf_ops,
+		.ops = &xfs_cntbt_buf_ops,
 		.work = &xfs_cntroot_init,
 		.need_init = true
 	},
diff --git a/fs/xfs/libxfs/xfs_alloc_btree.c b/fs/xfs/libxfs/xfs_alloc_btree.c
index 4e59cc8a2802..480d0f52da64 100644
--- a/fs/xfs/libxfs/xfs_alloc_btree.c
+++ b/fs/xfs/libxfs/xfs_alloc_btree.c
@@ -377,13 +377,19 @@ xfs_allocbt_write_verify(
 
 }
 
-const struct xfs_buf_ops xfs_allocbt_buf_ops = {
-	.name = "xfs_allocbt",
+const struct xfs_buf_ops xfs_bnobt_buf_ops = {
+	.name = "xfs_bnobt",
 	.verify_read = xfs_allocbt_read_verify,
 	.verify_write = xfs_allocbt_write_verify,
 	.verify_struct = xfs_allocbt_verify,
 };
 
+const struct xfs_buf_ops xfs_cntbt_buf_ops = {
+	.name = "xfs_cntbt",
+	.verify_read = xfs_allocbt_read_verify,
+	.verify_write = xfs_allocbt_write_verify,
+	.verify_struct = xfs_allocbt_verify,
+};
 
 STATIC int
 xfs_bnobt_keys_inorder(
@@ -448,7 +454,7 @@ static const struct xfs_btree_ops xfs_bnobt_ops = {
 	.init_rec_from_cur	= xfs_allocbt_init_rec_from_cur,
 	.init_ptr_from_cur	= xfs_allocbt_init_ptr_from_cur,
 	.key_diff		= xfs_bnobt_key_diff,
-	.buf_ops		= &xfs_allocbt_buf_ops,
+	.buf_ops		= &xfs_bnobt_buf_ops,
 	.diff_two_keys		= xfs_bnobt_diff_two_keys,
 	.keys_inorder		= xfs_bnobt_keys_inorder,
 	.recs_inorder		= xfs_bnobt_recs_inorder,
@@ -470,7 +476,7 @@ static const struct xfs_btree_ops xfs_cntbt_ops = {
 	.init_rec_from_cur	= xfs_allocbt_init_rec_from_cur,
 	.init_ptr_from_cur	= xfs_allocbt_init_ptr_from_cur,
 	.key_diff		= xfs_cntbt_key_diff,
-	.buf_ops		= &xfs_allocbt_buf_ops,
+	.buf_ops		= &xfs_cntbt_buf_ops,
 	.diff_two_keys		= xfs_cntbt_diff_two_keys,
 	.keys_inorder		= xfs_cntbt_keys_inorder,
 	.recs_inorder		= xfs_cntbt_recs_inorder,
diff --git a/fs/xfs/libxfs/xfs_shared.h b/fs/xfs/libxfs/xfs_shared.h
index 9855f4d2f98f..4e909791aeac 100644
--- a/fs/xfs/libxfs/xfs_shared.h
+++ b/fs/xfs/libxfs/xfs_shared.h
@@ -25,7 +25,8 @@ extern const struct xfs_buf_ops xfs_agf_buf_ops;
 extern const struct xfs_buf_ops xfs_agi_buf_ops;
 extern const struct xfs_buf_ops xfs_agf_buf_ops;
 extern const struct xfs_buf_ops xfs_agfl_buf_ops;
-extern const struct xfs_buf_ops xfs_allocbt_buf_ops;
+extern const struct xfs_buf_ops xfs_bnobt_buf_ops;
+extern const struct xfs_buf_ops xfs_cntbt_buf_ops;
 extern const struct xfs_buf_ops xfs_rmapbt_buf_ops;
 extern const struct xfs_buf_ops xfs_refcountbt_buf_ops;
 extern const struct xfs_buf_ops xfs_attr3_leaf_buf_ops;
diff --git a/fs/xfs/scrub/agheader_repair.c b/fs/xfs/scrub/agheader_repair.c
index 673be3cf7b8d..2799d1531639 100644
--- a/fs/xfs/scrub/agheader_repair.c
+++ b/fs/xfs/scrub/agheader_repair.c
@@ -341,12 +341,12 @@ xrep_agf(
 	struct xrep_find_ag_btree	fab[XREP_AGF_MAX] = {
 		[XREP_AGF_BNOBT] = {
 			.rmap_owner = XFS_RMAP_OWN_AG,
-			.buf_ops = &xfs_allocbt_buf_ops,
+			.buf_ops = &xfs_bnobt_buf_ops,
 			.magic = XFS_ABTB_CRC_MAGIC,
 		},
 		[XREP_AGF_CNTBT] = {
 			.rmap_owner = XFS_RMAP_OWN_AG,
-			.buf_ops = &xfs_allocbt_buf_ops,
+			.buf_ops = &xfs_cntbt_buf_ops,
 			.magic = XFS_ABTC_CRC_MAGIC,
 		},
 		[XREP_AGF_RMAPBT] = {
diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c
index 228c754bb137..5ad42d598333 100644
--- a/fs/xfs/xfs_log_recover.c
+++ b/fs/xfs/xfs_log_recover.c
@@ -2439,10 +2439,12 @@ xlog_recover_validate_buf_type(
 	case XFS_BLFT_BTREE_BUF:
 		switch (magic32) {
 		case XFS_ABTB_CRC_MAGIC:
-		case XFS_ABTC_CRC_MAGIC:
 		case XFS_ABTB_MAGIC:
+			bp->b_ops = &xfs_bnobt_buf_ops;
+			break;
+		case XFS_ABTC_CRC_MAGIC:
 		case XFS_ABTC_MAGIC:
-			bp->b_ops = &xfs_allocbt_buf_ops;
+			bp->b_ops = &xfs_cntbt_buf_ops;
 			break;
 		case XFS_IBT_CRC_MAGIC:
 		case XFS_IBT_MAGIC:
-- 
2.17.2

[PATCH v4 5/8] xfs: distinguish between bnobt and cntbt magic values

[Brian Foster]

The allocation btree verifiers share code that is unable to detect
cross-tree magic value corruptions such as a bnobt block with a
cntbt magic value. Populate the b_ops->magic field of the associated
verifier structures such that the structure verifier can check the
magic value against the expected value based on tree type.

The btree level check requires knowledge of the tree type to
determine the appropriate maximum value. This was previously part of
the hardcoded magic value checks. With that code removed, peek at
the first magic value in the verifier to determine the expected tree
type of the current block.

Signed-off-by: Brian Foster <bfoster@redhat.com>
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 fs/xfs/libxfs/xfs_alloc_btree.c | 60 ++++++++++++++-------------------
 1 file changed, 25 insertions(+), 35 deletions(-)

diff --git a/fs/xfs/libxfs/xfs_alloc_btree.c b/fs/xfs/libxfs/xfs_alloc_btree.c
index 480d0f52da64..9fe949f6055e 100644
--- a/fs/xfs/libxfs/xfs_alloc_btree.c
+++ b/fs/xfs/libxfs/xfs_alloc_btree.c
@@ -297,48 +297,34 @@ xfs_allocbt_verify(
 	struct xfs_perag	*pag = bp->b_pag;
 	xfs_failaddr_t		fa;
 	unsigned int		level;
+	xfs_btnum_t		btnum = XFS_BTNUM_BNOi;
+
+	if (!xfs_verify_magic(bp, block->bb_magic))
+		return __this_address;
+
+	if (xfs_sb_version_hascrc(&mp->m_sb)) {
+		fa = xfs_btree_sblock_v5hdr_verify(bp);
+		if (fa)
+			return fa;
+	}
 
 	/*
-	 * magic number and level verification
-	 *
-	 * During growfs operations, we can't verify the exact level or owner as
-	 * the perag is not fully initialised and hence not attached to the
-	 * buffer.  In this case, check against the maximum tree depth.
+	 * The perag may not be attached during grow operations or fully
+	 * initialized from the AGF during log recovery. Therefore we can only
+	 * check against maximum tree depth from those contexts.
 	 *
-	 * Similarly, during log recovery we will have a perag structure
-	 * attached, but the agf information will not yet have been initialised
-	 * from the on disk AGF. Again, we can only check against maximum limits
-	 * in this case.
+	 * Otherwise check against the per-tree limit. Peek at one of the
+	 * verifier magic values to determine the type of tree we're verifying
+	 * against.
 	 */
 	level = be16_to_cpu(block->bb_level);
-	switch (block->bb_magic) {
-	case cpu_to_be32(XFS_ABTB_CRC_MAGIC):
-		fa = xfs_btree_sblock_v5hdr_verify(bp);
-		if (fa)
-			return fa;
-		/* fall through */
-	case cpu_to_be32(XFS_ABTB_MAGIC):
-		if (pag && pag->pagf_init) {
-			if (level >= pag->pagf_levels[XFS_BTNUM_BNOi])
-				return __this_address;
-		} else if (level >= mp->m_ag_maxlevels)
+	if (bp->b_ops->magic[0] == cpu_to_be32(XFS_ABTC_MAGIC))
+		btnum = XFS_BTNUM_CNTi;
+	if (pag && pag->pagf_init) {
+		if (level >= pag->pagf_levels[btnum])
 			return __this_address;
-		break;
-	case cpu_to_be32(XFS_ABTC_CRC_MAGIC):
-		fa = xfs_btree_sblock_v5hdr_verify(bp);
-		if (fa)
-			return fa;
-		/* fall through */
-	case cpu_to_be32(XFS_ABTC_MAGIC):
-		if (pag && pag->pagf_init) {
-			if (level >= pag->pagf_levels[XFS_BTNUM_CNTi])
-				return __this_address;
-		} else if (level >= mp->m_ag_maxlevels)
-			return __this_address;
-		break;
-	default:
+	} else if (level >= mp->m_ag_maxlevels)
 		return __this_address;
-	}
 
 	return xfs_btree_sblock_verify(bp, mp->m_alloc_mxr[level != 0]);
 }
@@ -379,6 +365,8 @@ xfs_allocbt_write_verify(
 
 const struct xfs_buf_ops xfs_bnobt_buf_ops = {
 	.name = "xfs_bnobt",
+	.magic = { cpu_to_be32(XFS_ABTB_MAGIC),
+		   cpu_to_be32(XFS_ABTB_CRC_MAGIC) },
 	.verify_read = xfs_allocbt_read_verify,
 	.verify_write = xfs_allocbt_write_verify,
 	.verify_struct = xfs_allocbt_verify,
@@ -386,6 +374,8 @@ const struct xfs_buf_ops xfs_bnobt_buf_ops = {
 
 const struct xfs_buf_ops xfs_cntbt_buf_ops = {
 	.name = "xfs_cntbt",
+	.magic = { cpu_to_be32(XFS_ABTC_MAGIC),
+		   cpu_to_be32(XFS_ABTC_CRC_MAGIC) },
 	.verify_read = xfs_allocbt_read_verify,
 	.verify_write = xfs_allocbt_write_verify,
 	.verify_struct = xfs_allocbt_verify,
-- 
2.17.2

[PATCH v4 6/8] 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 | 87 ++++++++---------------------------
 fs/xfs/xfs_ondisk.h           | 11 +++++
 2 files changed, 30 insertions(+), 68 deletions(-)

diff --git a/fs/xfs/libxfs/xfs_dir2_leaf.c b/fs/xfs/libxfs/xfs_dir2_leaf.c
index 1728a3e6f5cf..dee0fd333d9d 100644
--- a/fs/xfs/libxfs/xfs_dir2_leaf.c
+++ b/fs/xfs/libxfs/xfs_dir2_leaf.c
@@ -142,41 +142,31 @@ 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;
 		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 +175,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 +205,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
diff --git a/fs/xfs/xfs_ondisk.h b/fs/xfs/xfs_ondisk.h
index d3e04d20d8d4..0209f3e91254 100644
--- a/fs/xfs/xfs_ondisk.h
+++ b/fs/xfs/xfs_ondisk.h
@@ -125,6 +125,17 @@ xfs_check_ondisk_structs(void)
 	XFS_CHECK_STRUCT_SIZE(struct xfs_inode_log_format,	56);
 	XFS_CHECK_STRUCT_SIZE(struct xfs_qoff_logformat,	20);
 	XFS_CHECK_STRUCT_SIZE(struct xfs_trans_header,		16);
+
+	/*
+	 * Magic value offset checks. These are here because certain on-disk
+	 * structures are updated to include more information on v5 filesystems.
+	 * While different in-core data structures are used depending on fs
+	 * version, some buffer verifiers expect to be able to use either
+	 * structure to locate the magic value as it should always be in the
+	 * same place.
+	 */
+	XFS_CHECK_OFFSET(struct xfs_dir2_leaf, hdr.info.magic,		8);
+	XFS_CHECK_OFFSET(struct xfs_dir3_leaf_hdr, info.hdr.magic,	8);
 }
 
 #endif /* __XFS_ONDISK_H */
-- 
2.17.2

[PATCH v4 7/8] 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      | 11 +++++------
 fs/xfs/libxfs/xfs_attr_remote.c    |  8 +++++---
 fs/xfs/libxfs/xfs_bmap_btree.c     | 13 ++++++-------
 fs/xfs/libxfs/xfs_da_btree.c       | 11 +++++------
 fs/xfs/libxfs/xfs_dir2_block.c     | 10 +++++-----
 fs/xfs/libxfs/xfs_dir2_data.c      | 12 +++++++-----
 fs/xfs/libxfs/xfs_dir2_node.c      | 10 +++++-----
 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 ++-
 fs/xfs/xfs_ondisk.h                |  6 ++++++
 14 files changed, 63 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..f164f296f1b8 100644
--- a/fs/xfs/libxfs/xfs_attr_leaf.c
+++ b/fs/xfs/libxfs/xfs_attr_leaf.c
@@ -248,21 +248,18 @@ 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;
-
 		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 +366,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..e02d2f407e12 100644
--- a/fs/xfs/libxfs/xfs_da_btree.c
+++ b/fs/xfs/libxfs/xfs_da_btree.c
@@ -129,21 +129,18 @@ 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;
-
 		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 +254,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..3b03703c5c3d 100644
--- a/fs/xfs/libxfs/xfs_dir2_node.c
+++ b/fs/xfs/libxfs/xfs_dir2_node.c
@@ -87,20 +87,18 @@ 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;
 		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 +149,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,
diff --git a/fs/xfs/xfs_ondisk.h b/fs/xfs/xfs_ondisk.h
index 0209f3e91254..881af4f7ed89 100644
--- a/fs/xfs/xfs_ondisk.h
+++ b/fs/xfs/xfs_ondisk.h
@@ -136,6 +136,12 @@ xfs_check_ondisk_structs(void)
 	 */
 	XFS_CHECK_OFFSET(struct xfs_dir2_leaf, hdr.info.magic,		8);
 	XFS_CHECK_OFFSET(struct xfs_dir3_leaf_hdr, info.hdr.magic,	8);
+	XFS_CHECK_OFFSET(struct xfs_attr_leafblock, hdr.info.magic,	8);
+	XFS_CHECK_OFFSET(struct xfs_da3_node_hdr, info.hdr.magic,	8);
+	XFS_CHECK_OFFSET(struct xfs_da_intnode, hdr.info.magic,		8);
+	XFS_CHECK_OFFSET(struct xfs_da3_node_hdr, info.hdr.magic,	8);
+	XFS_CHECK_OFFSET(struct xfs_dir2_free_hdr, magic,		0);
+	XFS_CHECK_OFFSET(struct xfs_dir3_blk_hdr, magic,		0);
 }
 
 #endif /* __XFS_ONDISK_H */
-- 
2.17.2

[PATCH v4 8/8] 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>
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 fs/xfs/libxfs/xfs_attr_leaf.c | 16 ++++---------
 fs/xfs/libxfs/xfs_da_btree.c  | 44 +++++++++++++++++++++++++----------
 fs/xfs/libxfs/xfs_da_format.h |  3 +++
 fs/xfs/libxfs/xfs_dir2_leaf.c | 17 ++++----------
 4 files changed, 43 insertions(+), 37 deletions(-)

diff --git a/fs/xfs/libxfs/xfs_attr_leaf.c b/fs/xfs/libxfs/xfs_attr_leaf.c
index f164f296f1b8..0c92987f57fc 100644
--- a/fs/xfs/libxfs/xfs_attr_leaf.c
+++ b/fs/xfs/libxfs/xfs_attr_leaf.c
@@ -245,22 +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;
 
-		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 e02d2f407e12..eb2cee428f26 100644
--- a/fs/xfs/libxfs/xfs_da_btree.c
+++ b/fs/xfs/libxfs/xfs_da_btree.c
@@ -116,6 +116,34 @@ 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)) {
+		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,24 +152,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;
-
-		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..ae654e06b2fb 100644
--- a/fs/xfs/libxfs/xfs_da_format.h
+++ b/fs/xfs/libxfs/xfs_da_format.h
@@ -869,4 +869,7 @@ 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 *bp,
+				      struct xfs_da3_blkinfo *hdr3);
+
 #endif /* __XFS_DA_FORMAT_H__ */
diff --git a/fs/xfs/libxfs/xfs_dir2_leaf.c b/fs/xfs/libxfs/xfs_dir2_leaf.c
index dee0fd333d9d..094028b7b162 100644
--- a/fs/xfs/libxfs/xfs_dir2_leaf.c
+++ b/fs/xfs/libxfs/xfs_dir2_leaf.c
@@ -146,20 +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;
-
-		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 v4 3/8] xfs: distinguish between inobt and finobt magic values

[Darrick J. Wong]

On Thu, Feb 07, 2019 at 01:41:00PM -0500, Brian Foster wrote:
> 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>
> Reviewed-by: Darrick J. Wong <darrick.wong@oracle.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 e1c5be26bf9f..c9cf8c0c7d32 100644
> --- a/fs/xfs/xfs_buf.c
> +++ b/fs/xfs/xfs_buf.c
> @@ -2203,3 +2203,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)

It occurred to me that this probably ought to be __be32 since we're
encoding the magics in disk byte order, right?

> +{
> +	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..44f9423a1861 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 */

Here too?

--D

>  	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 *bp, uint32_t dmagic);
>  
>  #endif	/* __XFS_BUF_H__ */
> -- 
> 2.17.2
> 

[PATCH 0.5/8] xfs: clarify documentation for the function to reverify buffers

[Darrick J. Wong]

Errrr.... here's the patch to clean up the ensure_ops documentation and
rename the function:

--D

From: Brian Foster <bfoster@redhat.com>

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.  Rename
the function to xfs_buf_reverify.

[darrick: this started off as bfoster mods of a previous patch of mine,
but the renaming parts are now a separate patch.]

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

diff --git a/fs/xfs/xfs_buf.c b/fs/xfs/xfs_buf.c
index 4f5f2ff3f70f..d29246ac3721 100644
--- a/fs/xfs/xfs_buf.c
+++ b/fs/xfs/xfs_buf.c
@@ -776,29 +776,24 @@ _xfs_buf_read(
 }
 
 /*
- * Set buffer ops on an unchecked buffer and validate it, if possible.
+ * Reverify a buffer found in cache without an attached ->b_ops.
  *
- * 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.
+ * 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 must have buffer ops assigned
- * to them when the buffer is read in from disk so that we can validate the
- * metadata.
- *
- * However, there are two scenarios where one can encounter in-core buffers
- * that don't have buffer 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 tries to match arbitrary metadata blocks
- * with btree types in order to find the root.  If online repair doesn't match
- * the buffer with /any/ btree type, the buffer remains in memory in DONE state
- * with no ops, and a subsequent read_buf call from elsewhere will not set the
- * ops.  This function helps us fix this situation.
+ * 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.  If repair can't establish that, the buffer will be left in memory
+ * with NULL buffer ops.
  */
 int
-xfs_buf_ensure_ops(
+xfs_buf_reverify(
 	struct xfs_buf		*bp,
 	const struct xfs_buf_ops *ops)
 {
@@ -840,7 +835,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__);
 

[PATCH 9/8] xfs: add inode magic to inode verifier

[Darrick J. Wong]

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

Use xfs_verify_magic to check the magic numbers of inodes.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 fs/xfs/libxfs/xfs_inode_buf.c |    6 +++++-
 fs/xfs/xfs_log_recover.c      |    2 +-
 2 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/fs/xfs/libxfs/xfs_inode_buf.c b/fs/xfs/libxfs/xfs_inode_buf.c
index 01ebc9557f69..f92f14e93ad3 100644
--- a/fs/xfs/libxfs/xfs_inode_buf.c
+++ b/fs/xfs/libxfs/xfs_inode_buf.c
@@ -97,7 +97,7 @@ xfs_inode_buf_verify(
 
 		dip = xfs_buf_offset(bp, (i << mp->m_sb.sb_inodelog));
 		unlinked_ino = be32_to_cpu(dip->di_next_unlinked);
-		di_ok = dip->di_magic == cpu_to_be16(XFS_DINODE_MAGIC) &&
+		di_ok = xfs_verify_magic(bp, dip->di_magic) &&
 			xfs_dinode_good_version(mp, dip->di_version) &&
 			xfs_verify_agino_or_null(mp, agno, unlinked_ino);
 		if (unlikely(XFS_TEST_ERROR(!di_ok, mp,
@@ -146,12 +146,16 @@ xfs_inode_buf_write_verify(
 
 const struct xfs_buf_ops xfs_inode_buf_ops = {
 	.name = "xfs_inode",
+	.magic = { cpu_to_be16(XFS_DINODE_MAGIC),
+		   cpu_to_be16(XFS_DINODE_MAGIC) },
 	.verify_read = xfs_inode_buf_read_verify,
 	.verify_write = xfs_inode_buf_write_verify,
 };
 
 const struct xfs_buf_ops xfs_inode_buf_ra_ops = {
 	.name = "xfs_inode_ra",
+	.magic = { cpu_to_be16(XFS_DINODE_MAGIC),
+		   cpu_to_be16(XFS_DINODE_MAGIC) },
 	.verify_read = xfs_inode_buf_readahead_verify,
 	.verify_write = xfs_inode_buf_write_verify,
 };
diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c
index 5ad42d598333..f5948d16015b 100644
--- a/fs/xfs/xfs_log_recover.c
+++ b/fs/xfs/xfs_log_recover.c
@@ -3049,7 +3049,7 @@ xlog_recover_inode_pass2(
 	 * Make sure the place we're flushing out to really looks
 	 * like an inode!
 	 */
-	if (unlikely(dip->di_magic != cpu_to_be16(XFS_DINODE_MAGIC))) {
+	if (unlikely(!xfs_verify_magic(bp, dip->di_magic))) {
 		xfs_alert(mp,
 	"%s: Bad inode magic number, dip = "PTR_FMT", dino bp = "PTR_FMT", ino = %Ld",
 			__func__, dip, bp, in_f->ilf_ino);

[PATCH 10/8] xfs: add magic numbers to dquot buffer ops

[Darrick J. Wong]

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

Add dquot magic numbers to the buffer ops type, in case we ever want to
use them.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 fs/xfs/libxfs/xfs_dquot_buf.c |    4 ++++
 1 file changed, 4 insertions(+)

diff --git a/fs/xfs/libxfs/xfs_dquot_buf.c b/fs/xfs/libxfs/xfs_dquot_buf.c
index d293f371dd54..b956638a3532 100644
--- a/fs/xfs/libxfs/xfs_dquot_buf.c
+++ b/fs/xfs/libxfs/xfs_dquot_buf.c
@@ -277,6 +277,8 @@ xfs_dquot_buf_write_verify(
 
 const struct xfs_buf_ops xfs_dquot_buf_ops = {
 	.name = "xfs_dquot",
+	.magic = { cpu_to_be16(XFS_DQUOT_MAGIC),
+		   cpu_to_be16(XFS_DQUOT_MAGIC) },
 	.verify_read = xfs_dquot_buf_read_verify,
 	.verify_write = xfs_dquot_buf_write_verify,
 	.verify_struct = xfs_dquot_buf_verify_struct,
@@ -284,6 +286,8 @@ const struct xfs_buf_ops xfs_dquot_buf_ops = {
 
 const struct xfs_buf_ops xfs_dquot_buf_ra_ops = {
 	.name = "xfs_dquot_ra",
+	.magic = { cpu_to_be16(XFS_DQUOT_MAGIC),
+		   cpu_to_be16(XFS_DQUOT_MAGIC) },
 	.verify_read = xfs_dquot_buf_readahead_verify,
 	.verify_write = xfs_dquot_buf_write_verify,
 };

[PATCH 11/8] xfs: use buf ops magic to detect btree block type

[Darrick J. Wong]

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

Now that we encode block magic numbers in all the buffer ops, use that
for block type detection in the ag header repair code instead of
encoding magics directly in the repair code.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 fs/xfs/scrub/agheader_repair.c |    6 ------
 fs/xfs/scrub/repair.c          |    3 ++-
 fs/xfs/scrub/repair.h          |    3 ---
 3 files changed, 2 insertions(+), 10 deletions(-)

diff --git a/fs/xfs/scrub/agheader_repair.c b/fs/xfs/scrub/agheader_repair.c
index 2799d1531639..64e31f87d490 100644
--- a/fs/xfs/scrub/agheader_repair.c
+++ b/fs/xfs/scrub/agheader_repair.c
@@ -342,22 +342,18 @@ xrep_agf(
 		[XREP_AGF_BNOBT] = {
 			.rmap_owner = XFS_RMAP_OWN_AG,
 			.buf_ops = &xfs_bnobt_buf_ops,
-			.magic = XFS_ABTB_CRC_MAGIC,
 		},
 		[XREP_AGF_CNTBT] = {
 			.rmap_owner = XFS_RMAP_OWN_AG,
 			.buf_ops = &xfs_cntbt_buf_ops,
-			.magic = XFS_ABTC_CRC_MAGIC,
 		},
 		[XREP_AGF_RMAPBT] = {
 			.rmap_owner = XFS_RMAP_OWN_AG,
 			.buf_ops = &xfs_rmapbt_buf_ops,
-			.magic = XFS_RMAP_CRC_MAGIC,
 		},
 		[XREP_AGF_REFCOUNTBT] = {
 			.rmap_owner = XFS_RMAP_OWN_REFC,
 			.buf_ops = &xfs_refcountbt_buf_ops,
-			.magic = XFS_REFC_CRC_MAGIC,
 		},
 		[XREP_AGF_END] = {
 			.buf_ops = NULL,
@@ -875,12 +871,10 @@ xrep_agi(
 		[XREP_AGI_INOBT] = {
 			.rmap_owner = XFS_RMAP_OWN_INOBT,
 			.buf_ops = &xfs_inobt_buf_ops,
-			.magic = XFS_IBT_CRC_MAGIC,
 		},
 		[XREP_AGI_FINOBT] = {
 			.rmap_owner = XFS_RMAP_OWN_INOBT,
 			.buf_ops = &xfs_finobt_buf_ops,
-			.magic = XFS_FIBT_CRC_MAGIC,
 		},
 		[XREP_AGI_END] = {
 			.buf_ops = NULL
diff --git a/fs/xfs/scrub/repair.c b/fs/xfs/scrub/repair.c
index 6acf1bfa0bfe..f28f4bad317b 100644
--- a/fs/xfs/scrub/repair.c
+++ b/fs/xfs/scrub/repair.c
@@ -743,7 +743,8 @@ xrep_findroot_block(
 
 	/* Ensure the block magic matches the btree type we're looking for. */
 	btblock = XFS_BUF_TO_BLOCK(bp);
-	if (be32_to_cpu(btblock->bb_magic) != fab->magic)
+	ASSERT(fab->buf_ops->magic[1] != 0);
+	if (btblock->bb_magic != fab->buf_ops->magic[1])
 		goto out;
 
 	/*
diff --git a/fs/xfs/scrub/repair.h b/fs/xfs/scrub/repair.h
index f2fc18bb7605..d990314eb08b 100644
--- a/fs/xfs/scrub/repair.h
+++ b/fs/xfs/scrub/repair.h
@@ -42,9 +42,6 @@ struct xrep_find_ag_btree {
 	/* in: buffer ops */
 	const struct xfs_buf_ops	*buf_ops;
 
-	/* in: magic number of the btree */
-	uint32_t			magic;
-
 	/* out: the highest btree block found and the tree height */
 	xfs_agblock_t			root;
 	unsigned int			height;

Re: [PATCH v4 3/8] xfs: distinguish between inobt and finobt magic values

[Brian Foster]

On Thu, Feb 07, 2019 at 10:52:09AM -0800, Darrick J. Wong wrote:
> On Thu, Feb 07, 2019 at 01:41:00PM -0500, Brian Foster wrote:
> > 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>
> > Reviewed-by: Darrick J. Wong <darrick.wong@oracle.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 e1c5be26bf9f..c9cf8c0c7d32 100644
> > --- a/fs/xfs/xfs_buf.c
> > +++ b/fs/xfs/xfs_buf.c
> > @@ -2203,3 +2203,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)
> 
> It occurred to me that this probably ought to be __be32 since we're
> encoding the magics in disk byte order, right?
> 

This crossed my mind when I originally changed it from cpu endian to
on-disk endian, but not all of these magic values are even 32-bit. It
seemed more appropriate to me to just use something more opaque (which I
figured was still uint32_t, but could very well be something else).

Brian

> > +{
> > +	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..44f9423a1861 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 */
> 
> Here too?
> 
> --D
> 
> >  	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 *bp, uint32_t dmagic);
> >  
> >  #endif	/* __XFS_BUF_H__ */
> > -- 
> > 2.17.2
> > 

Re: [PATCH v4 6/8] xfs: use verifier magic field in dir2 leaf verifiers

[Darrick J. Wong]

On Thu, Feb 07, 2019 at 01:41:03PM -0500, Brian Foster wrote:
> 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 | 87 ++++++++---------------------------
>  fs/xfs/xfs_ondisk.h           | 11 +++++
>  2 files changed, 30 insertions(+), 68 deletions(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_dir2_leaf.c b/fs/xfs/libxfs/xfs_dir2_leaf.c
> index 1728a3e6f5cf..dee0fd333d9d 100644
> --- a/fs/xfs/libxfs/xfs_dir2_leaf.c
> +++ b/fs/xfs/libxfs/xfs_dir2_leaf.c
> @@ -142,41 +142,31 @@ 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;
>  		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 +175,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 +205,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
> diff --git a/fs/xfs/xfs_ondisk.h b/fs/xfs/xfs_ondisk.h
> index d3e04d20d8d4..0209f3e91254 100644
> --- a/fs/xfs/xfs_ondisk.h
> +++ b/fs/xfs/xfs_ondisk.h
> @@ -125,6 +125,17 @@ xfs_check_ondisk_structs(void)
>  	XFS_CHECK_STRUCT_SIZE(struct xfs_inode_log_format,	56);
>  	XFS_CHECK_STRUCT_SIZE(struct xfs_qoff_logformat,	20);
>  	XFS_CHECK_STRUCT_SIZE(struct xfs_trans_header,		16);
> +
> +	/*
> +	 * Magic value offset checks. These are here because certain on-disk
> +	 * structures are updated to include more information on v5 filesystems.
> +	 * While different in-core data structures are used depending on fs
> +	 * version, some buffer verifiers expect to be able to use either
> +	 * structure to locate the magic value as it should always be in the
> +	 * same place.
> +	 */
> +	XFS_CHECK_OFFSET(struct xfs_dir2_leaf, hdr.info.magic,		8);
> +	XFS_CHECK_OFFSET(struct xfs_dir3_leaf_hdr, info.hdr.magic,	8);

Sorry for the nitpick, but why not

XFS_CHECK_OFFSET(struct xfs_dir3_leaf, hdr.info.hdr.magic, 8) ?

--D

>  }
>  
>  #endif /* __XFS_ONDISK_H */
> -- 
> 2.17.2
> 

Re: [PATCH v4 7/8] xfs: miscellaneous verifier magic value fixups

[Darrick J. Wong]

On Thu, Feb 07, 2019 at 01:41:04PM -0500, Brian Foster wrote:
> 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      | 11 +++++------
>  fs/xfs/libxfs/xfs_attr_remote.c    |  8 +++++---
>  fs/xfs/libxfs/xfs_bmap_btree.c     | 13 ++++++-------
>  fs/xfs/libxfs/xfs_da_btree.c       | 11 +++++------
>  fs/xfs/libxfs/xfs_dir2_block.c     | 10 +++++-----
>  fs/xfs/libxfs/xfs_dir2_data.c      | 12 +++++++-----
>  fs/xfs/libxfs/xfs_dir2_node.c      | 10 +++++-----
>  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 ++-
>  fs/xfs/xfs_ondisk.h                |  6 ++++++
>  14 files changed, 63 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..f164f296f1b8 100644
> --- a/fs/xfs/libxfs/xfs_attr_leaf.c
> +++ b/fs/xfs/libxfs/xfs_attr_leaf.c
> @@ -248,21 +248,18 @@ 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;
> -
>  		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 +366,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..e02d2f407e12 100644
> --- a/fs/xfs/libxfs/xfs_da_btree.c
> +++ b/fs/xfs/libxfs/xfs_da_btree.c
> @@ -129,21 +129,18 @@ 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;
> -
>  		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 +254,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..3b03703c5c3d 100644
> --- a/fs/xfs/libxfs/xfs_dir2_node.c
> +++ b/fs/xfs/libxfs/xfs_dir2_node.c
> @@ -87,20 +87,18 @@ 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;
>  		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 +149,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,
> diff --git a/fs/xfs/xfs_ondisk.h b/fs/xfs/xfs_ondisk.h
> index 0209f3e91254..881af4f7ed89 100644
> --- a/fs/xfs/xfs_ondisk.h
> +++ b/fs/xfs/xfs_ondisk.h
> @@ -136,6 +136,12 @@ xfs_check_ondisk_structs(void)
>  	 */
>  	XFS_CHECK_OFFSET(struct xfs_dir2_leaf, hdr.info.magic,		8);
>  	XFS_CHECK_OFFSET(struct xfs_dir3_leaf_hdr, info.hdr.magic,	8);
> +	XFS_CHECK_OFFSET(struct xfs_attr_leafblock, hdr.info.magic,	8);

> +	XFS_CHECK_OFFSET(struct xfs_da3_node_hdr, info.hdr.magic,	8);
> +	XFS_CHECK_OFFSET(struct xfs_da_intnode, hdr.info.magic,		8);

Should there be a check for struct xfs_da3_intnode too?

> +	XFS_CHECK_OFFSET(struct xfs_da3_node_hdr, info.hdr.magic,	8);

Is one of these supposed to be struct xfs_da_node_hdr?

Granted these all end up comparing the same thing, so maybe it's
overkill to check da_node_hdr/da_intnode and da3_node_hdr/da3_intnode?

--D

> +	XFS_CHECK_OFFSET(struct xfs_dir2_free_hdr, magic,		0);
> +	XFS_CHECK_OFFSET(struct xfs_dir3_blk_hdr, magic,		0);
>  }
>  
>  #endif /* __XFS_ONDISK_H */
> -- 
> 2.17.2
> 

Re: [PATCH v4 6/8] xfs: use verifier magic field in dir2 leaf verifiers

[Brian Foster]

On Thu, Feb 07, 2019 at 11:10:06AM -0800, Darrick J. Wong wrote:
> On Thu, Feb 07, 2019 at 01:41:03PM -0500, Brian Foster wrote:
> > 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 | 87 ++++++++---------------------------
> >  fs/xfs/xfs_ondisk.h           | 11 +++++
> >  2 files changed, 30 insertions(+), 68 deletions(-)
> > 
...
> > diff --git a/fs/xfs/xfs_ondisk.h b/fs/xfs/xfs_ondisk.h
> > index d3e04d20d8d4..0209f3e91254 100644
> > --- a/fs/xfs/xfs_ondisk.h
> > +++ b/fs/xfs/xfs_ondisk.h
> > @@ -125,6 +125,17 @@ xfs_check_ondisk_structs(void)
> >  	XFS_CHECK_STRUCT_SIZE(struct xfs_inode_log_format,	56);
> >  	XFS_CHECK_STRUCT_SIZE(struct xfs_qoff_logformat,	20);
> >  	XFS_CHECK_STRUCT_SIZE(struct xfs_trans_header,		16);
> > +
> > +	/*
> > +	 * Magic value offset checks. These are here because certain on-disk
> > +	 * structures are updated to include more information on v5 filesystems.
> > +	 * While different in-core data structures are used depending on fs
> > +	 * version, some buffer verifiers expect to be able to use either
> > +	 * structure to locate the magic value as it should always be in the
> > +	 * same place.
> > +	 */
> > +	XFS_CHECK_OFFSET(struct xfs_dir2_leaf, hdr.info.magic,		8);
> > +	XFS_CHECK_OFFSET(struct xfs_dir3_leaf_hdr, info.hdr.magic,	8);
> 
> Sorry for the nitpick, but why not
> 
> XFS_CHECK_OFFSET(struct xfs_dir3_leaf, hdr.info.hdr.magic, 8) ?
> 

I just used the structure(s) as used in the verifiers. I have no real
preference as to whether we use that or the higher/top level structures.

Brian

> --D
> 
> >  }
> >  
> >  #endif /* __XFS_ONDISK_H */
> > -- 
> > 2.17.2
> > 

Re: [PATCH v4 7/8] xfs: miscellaneous verifier magic value fixups

[Brian Foster]

On Thu, Feb 07, 2019 at 11:13:13AM -0800, Darrick J. Wong wrote:
> On Thu, Feb 07, 2019 at 01:41:04PM -0500, Brian Foster wrote:
> > 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      | 11 +++++------
> >  fs/xfs/libxfs/xfs_attr_remote.c    |  8 +++++---
> >  fs/xfs/libxfs/xfs_bmap_btree.c     | 13 ++++++-------
> >  fs/xfs/libxfs/xfs_da_btree.c       | 11 +++++------
> >  fs/xfs/libxfs/xfs_dir2_block.c     | 10 +++++-----
> >  fs/xfs/libxfs/xfs_dir2_data.c      | 12 +++++++-----
> >  fs/xfs/libxfs/xfs_dir2_node.c      | 10 +++++-----
> >  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 ++-
> >  fs/xfs/xfs_ondisk.h                |  6 ++++++
> >  14 files changed, 63 insertions(+), 46 deletions(-)
> > 
...
> > diff --git a/fs/xfs/xfs_ondisk.h b/fs/xfs/xfs_ondisk.h
> > index 0209f3e91254..881af4f7ed89 100644
> > --- a/fs/xfs/xfs_ondisk.h
> > +++ b/fs/xfs/xfs_ondisk.h
> > @@ -136,6 +136,12 @@ xfs_check_ondisk_structs(void)
> >  	 */
> >  	XFS_CHECK_OFFSET(struct xfs_dir2_leaf, hdr.info.magic,		8);
> >  	XFS_CHECK_OFFSET(struct xfs_dir3_leaf_hdr, info.hdr.magic,	8);
> > +	XFS_CHECK_OFFSET(struct xfs_attr_leafblock, hdr.info.magic,	8);
> 
> > +	XFS_CHECK_OFFSET(struct xfs_da3_node_hdr, info.hdr.magic,	8);
> > +	XFS_CHECK_OFFSET(struct xfs_da_intnode, hdr.info.magic,		8);
> 
> Should there be a check for struct xfs_da3_intnode too?
> 

I think this is the same deal as the previous patch. The verifier uses
xfs_da_intnode (where I suppose it could use either).

> > +	XFS_CHECK_OFFSET(struct xfs_da3_node_hdr, info.hdr.magic,	8);
> 
> Is one of these supposed to be struct xfs_da_node_hdr?
> 

Is that used in one of the verifiers (as opposed to xfs_da_intnode I
suspect)? I replaced 3 asserts in this patch and replaced each with the
magic value as accessed via the associated verifier on v4 and v5.

> Granted these all end up comparing the same thing, so maybe it's
> overkill to check da_node_hdr/da_intnode and da3_node_hdr/da3_intnode?
> 

The intent is just to cover the fact that the verifiers expect these
magic values to reside at the same offset, not so much that they're at
some particular offset. That's the reason for the comment above.

Perhaps it's better just to remove all of this entirely since this is
1.) not going to change without breaking v4/v5 fs' 2.) still technically
doesn't encode the requirement of the verifier and 3.) is kind of
getting beyond the scope of this series. Some of these verifiers used
the magic value itself to distinguish between v4 and v5 already, after
all. The original asserts were really just added as a quick mechanism to
make sure nothing obvious was broken by the changes that collapsed a few
of the verifiers with separate but similar magic checks.

Brian

> --D
> 
> > +	XFS_CHECK_OFFSET(struct xfs_dir2_free_hdr, magic,		0);
> > +	XFS_CHECK_OFFSET(struct xfs_dir3_blk_hdr, magic,		0);
> >  }
> >  
> >  #endif /* __XFS_ONDISK_H */
> > -- 
> > 2.17.2
> > 

[PATCH v4.1] 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>
---

This v4.1 update drops the compile-time magic value offset checks.
Rather than attempt to retrofit the original asserts into the
xfs_ondisk.h infrastructure, it's probably more appropriate to add some
checks that ensure the v4 headers land at the expected offset of the v5
extended variants (where appropriate). This is outside the scope of this
series.

Brian

 fs/xfs/libxfs/xfs_dir2_leaf.c | 87 ++++++++---------------------------
 1 file changed, 19 insertions(+), 68 deletions(-)

diff --git a/fs/xfs/libxfs/xfs_dir2_leaf.c b/fs/xfs/libxfs/xfs_dir2_leaf.c
index 1728a3e6f5cf..dee0fd333d9d 100644
--- a/fs/xfs/libxfs/xfs_dir2_leaf.c
+++ b/fs/xfs/libxfs/xfs_dir2_leaf.c
@@ -142,41 +142,31 @@ 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;
 		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 +175,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 +205,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 v4.1] 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>
---

v4.1:
- Drop the compile-time magic offset checks.

 fs/xfs/libxfs/xfs_alloc.c          | 12 ++++++++----
 fs/xfs/libxfs/xfs_attr_leaf.c      | 11 +++++------
 fs/xfs/libxfs/xfs_attr_remote.c    |  8 +++++---
 fs/xfs/libxfs/xfs_bmap_btree.c     | 13 ++++++-------
 fs/xfs/libxfs/xfs_da_btree.c       | 11 +++++------
 fs/xfs/libxfs/xfs_dir2_block.c     | 10 +++++-----
 fs/xfs/libxfs/xfs_dir2_data.c      | 12 +++++++-----
 fs/xfs/libxfs/xfs_dir2_node.c      | 10 +++++-----
 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, 57 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..f164f296f1b8 100644
--- a/fs/xfs/libxfs/xfs_attr_leaf.c
+++ b/fs/xfs/libxfs/xfs_attr_leaf.c
@@ -248,21 +248,18 @@ 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;
-
 		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 +366,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..e02d2f407e12 100644
--- a/fs/xfs/libxfs/xfs_da_btree.c
+++ b/fs/xfs/libxfs/xfs_da_btree.c
@@ -129,21 +129,18 @@ 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;
-
 		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 +254,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..3b03703c5c3d 100644
--- a/fs/xfs/libxfs/xfs_dir2_node.c
+++ b/fs/xfs/libxfs/xfs_dir2_node.c
@@ -87,20 +87,18 @@ 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;
 		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 +149,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

Re: [PATCH 9/8] xfs: add inode magic to inode verifier

[Brian Foster]

On Thu, Feb 07, 2019 at 10:56:32AM -0800, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Use xfs_verify_magic to check the magic numbers of inodes.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---

Reviewed-by: Brian Foster <bfoster@redhat.com>

>  fs/xfs/libxfs/xfs_inode_buf.c |    6 +++++-
>  fs/xfs/xfs_log_recover.c      |    2 +-
>  2 files changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_inode_buf.c b/fs/xfs/libxfs/xfs_inode_buf.c
> index 01ebc9557f69..f92f14e93ad3 100644
> --- a/fs/xfs/libxfs/xfs_inode_buf.c
> +++ b/fs/xfs/libxfs/xfs_inode_buf.c
> @@ -97,7 +97,7 @@ xfs_inode_buf_verify(
>  
>  		dip = xfs_buf_offset(bp, (i << mp->m_sb.sb_inodelog));
>  		unlinked_ino = be32_to_cpu(dip->di_next_unlinked);
> -		di_ok = dip->di_magic == cpu_to_be16(XFS_DINODE_MAGIC) &&
> +		di_ok = xfs_verify_magic(bp, dip->di_magic) &&
>  			xfs_dinode_good_version(mp, dip->di_version) &&
>  			xfs_verify_agino_or_null(mp, agno, unlinked_ino);
>  		if (unlikely(XFS_TEST_ERROR(!di_ok, mp,
> @@ -146,12 +146,16 @@ xfs_inode_buf_write_verify(
>  
>  const struct xfs_buf_ops xfs_inode_buf_ops = {
>  	.name = "xfs_inode",
> +	.magic = { cpu_to_be16(XFS_DINODE_MAGIC),
> +		   cpu_to_be16(XFS_DINODE_MAGIC) },
>  	.verify_read = xfs_inode_buf_read_verify,
>  	.verify_write = xfs_inode_buf_write_verify,
>  };
>  
>  const struct xfs_buf_ops xfs_inode_buf_ra_ops = {
>  	.name = "xfs_inode_ra",
> +	.magic = { cpu_to_be16(XFS_DINODE_MAGIC),
> +		   cpu_to_be16(XFS_DINODE_MAGIC) },
>  	.verify_read = xfs_inode_buf_readahead_verify,
>  	.verify_write = xfs_inode_buf_write_verify,
>  };
> diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c
> index 5ad42d598333..f5948d16015b 100644
> --- a/fs/xfs/xfs_log_recover.c
> +++ b/fs/xfs/xfs_log_recover.c
> @@ -3049,7 +3049,7 @@ xlog_recover_inode_pass2(
>  	 * Make sure the place we're flushing out to really looks
>  	 * like an inode!
>  	 */
> -	if (unlikely(dip->di_magic != cpu_to_be16(XFS_DINODE_MAGIC))) {
> +	if (unlikely(!xfs_verify_magic(bp, dip->di_magic))) {
>  		xfs_alert(mp,
>  	"%s: Bad inode magic number, dip = "PTR_FMT", dino bp = "PTR_FMT", ino = %Ld",
>  			__func__, dip, bp, in_f->ilf_ino);

Re: [PATCH 10/8] xfs: add magic numbers to dquot buffer ops

[Brian Foster]

On Thu, Feb 07, 2019 at 10:56:52AM -0800, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Add dquot magic numbers to the buffer ops type, in case we ever want to
> use them.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---

Reviewed-by: Brian Foster <bfoster@redhat.com>

>  fs/xfs/libxfs/xfs_dquot_buf.c |    4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/fs/xfs/libxfs/xfs_dquot_buf.c b/fs/xfs/libxfs/xfs_dquot_buf.c
> index d293f371dd54..b956638a3532 100644
> --- a/fs/xfs/libxfs/xfs_dquot_buf.c
> +++ b/fs/xfs/libxfs/xfs_dquot_buf.c
> @@ -277,6 +277,8 @@ xfs_dquot_buf_write_verify(
>  
>  const struct xfs_buf_ops xfs_dquot_buf_ops = {
>  	.name = "xfs_dquot",
> +	.magic = { cpu_to_be16(XFS_DQUOT_MAGIC),
> +		   cpu_to_be16(XFS_DQUOT_MAGIC) },
>  	.verify_read = xfs_dquot_buf_read_verify,
>  	.verify_write = xfs_dquot_buf_write_verify,
>  	.verify_struct = xfs_dquot_buf_verify_struct,
> @@ -284,6 +286,8 @@ const struct xfs_buf_ops xfs_dquot_buf_ops = {
>  
>  const struct xfs_buf_ops xfs_dquot_buf_ra_ops = {
>  	.name = "xfs_dquot_ra",
> +	.magic = { cpu_to_be16(XFS_DQUOT_MAGIC),
> +		   cpu_to_be16(XFS_DQUOT_MAGIC) },
>  	.verify_read = xfs_dquot_buf_readahead_verify,
>  	.verify_write = xfs_dquot_buf_write_verify,
>  };

Re: [PATCH 11/8] xfs: use buf ops magic to detect btree block type

[Brian Foster]

On Thu, Feb 07, 2019 at 10:57:13AM -0800, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Now that we encode block magic numbers in all the buffer ops, use that
> for block type detection in the ag header repair code instead of
> encoding magics directly in the repair code.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---

Reviewed-by: Brian Foster <bfoster@redhat.com>

>  fs/xfs/scrub/agheader_repair.c |    6 ------
>  fs/xfs/scrub/repair.c          |    3 ++-
>  fs/xfs/scrub/repair.h          |    3 ---
>  3 files changed, 2 insertions(+), 10 deletions(-)
> 
> diff --git a/fs/xfs/scrub/agheader_repair.c b/fs/xfs/scrub/agheader_repair.c
> index 2799d1531639..64e31f87d490 100644
> --- a/fs/xfs/scrub/agheader_repair.c
> +++ b/fs/xfs/scrub/agheader_repair.c
> @@ -342,22 +342,18 @@ xrep_agf(
>  		[XREP_AGF_BNOBT] = {
>  			.rmap_owner = XFS_RMAP_OWN_AG,
>  			.buf_ops = &xfs_bnobt_buf_ops,
> -			.magic = XFS_ABTB_CRC_MAGIC,
>  		},
>  		[XREP_AGF_CNTBT] = {
>  			.rmap_owner = XFS_RMAP_OWN_AG,
>  			.buf_ops = &xfs_cntbt_buf_ops,
> -			.magic = XFS_ABTC_CRC_MAGIC,
>  		},
>  		[XREP_AGF_RMAPBT] = {
>  			.rmap_owner = XFS_RMAP_OWN_AG,
>  			.buf_ops = &xfs_rmapbt_buf_ops,
> -			.magic = XFS_RMAP_CRC_MAGIC,
>  		},
>  		[XREP_AGF_REFCOUNTBT] = {
>  			.rmap_owner = XFS_RMAP_OWN_REFC,
>  			.buf_ops = &xfs_refcountbt_buf_ops,
> -			.magic = XFS_REFC_CRC_MAGIC,
>  		},
>  		[XREP_AGF_END] = {
>  			.buf_ops = NULL,
> @@ -875,12 +871,10 @@ xrep_agi(
>  		[XREP_AGI_INOBT] = {
>  			.rmap_owner = XFS_RMAP_OWN_INOBT,
>  			.buf_ops = &xfs_inobt_buf_ops,
> -			.magic = XFS_IBT_CRC_MAGIC,
>  		},
>  		[XREP_AGI_FINOBT] = {
>  			.rmap_owner = XFS_RMAP_OWN_INOBT,
>  			.buf_ops = &xfs_finobt_buf_ops,
> -			.magic = XFS_FIBT_CRC_MAGIC,
>  		},
>  		[XREP_AGI_END] = {
>  			.buf_ops = NULL
> diff --git a/fs/xfs/scrub/repair.c b/fs/xfs/scrub/repair.c
> index 6acf1bfa0bfe..f28f4bad317b 100644
> --- a/fs/xfs/scrub/repair.c
> +++ b/fs/xfs/scrub/repair.c
> @@ -743,7 +743,8 @@ xrep_findroot_block(
>  
>  	/* Ensure the block magic matches the btree type we're looking for. */
>  	btblock = XFS_BUF_TO_BLOCK(bp);
> -	if (be32_to_cpu(btblock->bb_magic) != fab->magic)
> +	ASSERT(fab->buf_ops->magic[1] != 0);
> +	if (btblock->bb_magic != fab->buf_ops->magic[1])
>  		goto out;
>  
>  	/*
> diff --git a/fs/xfs/scrub/repair.h b/fs/xfs/scrub/repair.h
> index f2fc18bb7605..d990314eb08b 100644
> --- a/fs/xfs/scrub/repair.h
> +++ b/fs/xfs/scrub/repair.h
> @@ -42,9 +42,6 @@ struct xrep_find_ag_btree {
>  	/* in: buffer ops */
>  	const struct xfs_buf_ops	*buf_ops;
>  
> -	/* in: magic number of the btree */
> -	uint32_t			magic;
> -
>  	/* out: the highest btree block found and the tree height */
>  	xfs_agblock_t			root;
>  	unsigned int			height;