[PATCH RFC] xfs: support magic value in xfs_buf_ops

[Brian Foster <bfoster@redhat.com>]

Add a field to specify the v4 and v5 magic values in xfs_buf_ops.
This allows otherwise identical verifiers to distinguish between
and verify different magic values (inobt vs. finobt buffers). This
also facilitates verification of the appropriate magic value based
on superblock version.

The magic field is optional and is free to be used as appropriate
for each particular verifier.

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

Hi all,

What do folks think of something like this as a lightweight (and
untested) means to do proper [f]inobt magic verification? For reference,
the initial version of this put together to help root cause a user
report is here[1]. I was hoping to do the same thing with less code
duplication. A couple things that come to mind:

1. I know scrub has at least one place where we invoke the verifier with
->b_ops == NULL, which will cause this to explode. Could we fix that up
to assign and reset ->b_ops to accommodate something like this, or is
that problematic?

2. We have some other verifiers around that actually use the buffer
magic to set a more specific verifier. See xfs_da3_node_read_verify()
for an example. I'm not sure this is all that useful for such higher
level verifiers, but I think we'd at least be able to use it for the
underlying verifiers. That might provide some extra sb version vs. magic
sanity checking for places that might not already look at the sb version
(otherwise it's just refactoring).

Thoughts or other ideas before I try to apply this more broadly? Thanks.

Brian

[1] https://marc.info/?l=linux-xfs&m=154809431726435&w=2

 fs/xfs/libxfs/xfs_ialloc_btree.c | 27 +++++++++++++++++----------
 fs/xfs/xfs_buf.h                 |  1 +
 2 files changed, 18 insertions(+), 10 deletions(-)

diff --git a/fs/xfs/libxfs/xfs_ialloc_btree.c b/fs/xfs/libxfs/xfs_ialloc_btree.c
index 9b25e7a0df47..59b0cf1d759a 100644
--- a/fs/xfs/libxfs/xfs_ialloc_btree.c
+++ b/fs/xfs/libxfs/xfs_ialloc_btree.c
@@ -259,6 +259,12 @@ xfs_inobt_verify(
 	struct xfs_btree_block	*block = XFS_BUF_TO_BLOCK(bp);
 	xfs_failaddr_t		fa;
 	unsigned int		level;
+	uint32_t		magic;
+
+	ASSERT(bp->b_ops);
+	magic = bp->b_ops->magic[xfs_sb_version_hascrc(&mp->m_sb)];
+	if (block->bb_magic != cpu_to_be32(magic))
+		return __this_address;
 
 	/*
 	 * During growfs operations, we can't verify the exact owner as the
@@ -270,18 +276,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 +326,15 @@ xfs_inobt_write_verify(
 
 const struct xfs_buf_ops xfs_inobt_buf_ops = {
 	.name = "xfs_inobt",
+	.magic = { XFS_IBT_MAGIC, XFS_IBT_CRC_MAGIC },
+	.verify_read = xfs_inobt_read_verify,
+	.verify_write = xfs_inobt_write_verify,
+	.verify_struct = xfs_inobt_verify,
+};
+
+const struct xfs_buf_ops xfs_finobt_buf_ops = {
+	.name = "xfs_finobt",
+	.magic = { XFS_FIBT_MAGIC, XFS_FIBT_CRC_MAGIC },
 	.verify_read = xfs_inobt_read_verify,
 	.verify_write = xfs_inobt_write_verify,
 	.verify_struct = xfs_inobt_verify,
@@ -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/xfs_buf.h b/fs/xfs/xfs_buf.h
index b9f5511ea998..ce61e6b94725 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];
 	void (*verify_read)(struct xfs_buf *);
 	void (*verify_write)(struct xfs_buf *);
 	xfs_failaddr_t (*verify_struct)(struct xfs_buf *bp);
-- 
2.17.2