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

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

[Brian Foster]

Hi all,

Here's another RFC of the [f]inobt magic value verifier fixups. This is
a bit more polished so I figured I'd send another to solicit any
feedback on the helpers and whatnot. I also put together a mitigation
patch to change over verifier and btree block verification errors into a
warning. I suppose it's debatable whether we want the latter but FWIW
the commit log description describes the reasoning for each mitigation.

Patch 1 is a straightforward creation of a finobt verifier, patch 2
implements the magic value verification and patch 3 implements the
aforementioned error mitigation. This has been tested, but note that
Darrick's recent scrub verifier patch[1] is required for patch 2. I had
a patch to address that in this series but dropped it since it's already
been posted. Thoughts, reviews, flames appreciated.

Brian

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

rfcv2:
- 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 (3):
  xfs: create a separate finobt verifier
  xfs: distinguish between inobt and finobt magic values
  xfs: detect and warn about finobt blocks with an inobt magic value

 fs/xfs/libxfs/xfs_ag.c           |  2 +-
 fs/xfs/libxfs/xfs_btree.c        | 12 +++++++--
 fs/xfs/libxfs/xfs_ialloc_btree.c | 43 ++++++++++++++++++++++++--------
 fs/xfs/libxfs/xfs_shared.h       |  1 +
 fs/xfs/scrub/agheader_repair.c   |  2 +-
 fs/xfs/xfs_buf.h                 | 19 ++++++++++++++
 fs/xfs/xfs_log_recover.c         |  6 +++--
 7 files changed, 69 insertions(+), 16 deletions(-)

-- 
2.17.2

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

[Brian Foster]

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

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

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

[PATCH RFC v2 2/3] xfs: distinguish between inobt and finobt magic values

[Brian Foster]

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

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

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

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

diff --git a/fs/xfs/libxfs/xfs_ialloc_btree.c b/fs/xfs/libxfs/xfs_ialloc_btree.c
index 798269eb4767..c57ecb6b1255 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 = { XFS_IBT_MAGIC, XFS_IBT_CRC_MAGIC },
 	.verify_read = xfs_inobt_read_verify,
 	.verify_write = xfs_inobt_write_verify,
 	.verify_struct = xfs_inobt_verify,
@@ -335,6 +331,7 @@ const struct xfs_buf_ops xfs_inobt_buf_ops = {
 
 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,
diff --git a/fs/xfs/xfs_buf.h b/fs/xfs/xfs_buf.h
index b9f5511ea998..d8757eafba71 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);
@@ -387,4 +388,22 @@ extern int xfs_setsize_buftarg(xfs_buftarg_t *, unsigned int);
 
 int xfs_buf_ensure_ops(struct xfs_buf *bp, const struct xfs_buf_ops *ops);
 
+/*
+ * Verify an on-disk magic value against the magic value specified in the
+ * verifier structure.
+ */
+static inline bool
+xfs_buf_ops_verify_magic(
+	struct xfs_buf		*bp,
+	__be32			dmagic,
+	bool			crc)
+{
+	if (unlikely(WARN_ON(!bp->b_ops || !bp->b_ops->magic[crc])))
+		return false;
+	return dmagic == cpu_to_be32(bp->b_ops->magic[crc]);
+}
+#define xfs_verify_magic(bp, dmagic)		\
+	xfs_buf_ops_verify_magic(bp, dmagic,	\
+			xfs_sb_version_hascrc(&bp->b_target->bt_mount->m_sb))
+
 #endif	/* __XFS_BUF_H__ */
-- 
2.17.2

[PATCH RFC v2 3/3] xfs: detect and warn about finobt blocks with an inobt magic value

[Brian Foster]

xfs_repair had a bug where finobt reconstruction could create
certain level > 0 nodes with an inobt magic. This went undetected by
kernel verifiers because the inode btree verifiers were common
between the inobt and finobt. Now that the verifiers enforce the
appropriate magic value based on btree type, a mitigation strategy
is necessary to avoid turning a low impact metadata inconsistency
into an immediate and fatal runtime error.

Add an explicit check for a finobt block with an inobt verifier to
the [f]inobt structure verifier function. In the event that this
occurs, report this as a warning rather than a verifier error to
indicate to the user to upgrade xfsprogs and run xfs_repair. Note
that without this change, an affected finobt filesystem will trigger
a verifier error at mount time due to the perag reservation finobt
scan and fail to mount.

Also filter out this error from the lower level btree verification
logic. This logic is what detected the problem in the first place,
but can also cause an otherwise functional filesystem to fail. Note
that while this logic has been around for some time, it hasn't
always been active on non-debug kernels or triggered a runtime
error. This means that the scarcity of error reports is not
necessarily indicative of the prevalence of the problem in the
field.

Signed-off-by: Brian Foster <bfoster@redhat.com>
---
 fs/xfs/libxfs/xfs_btree.c        | 12 ++++++++++--
 fs/xfs/libxfs/xfs_ialloc_btree.c | 25 ++++++++++++++++++++++---
 2 files changed, 32 insertions(+), 5 deletions(-)

diff --git a/fs/xfs/libxfs/xfs_btree.c b/fs/xfs/libxfs/xfs_btree.c
index bbdae2b4559f..6bb5882d69f3 100644
--- a/fs/xfs/libxfs/xfs_btree.c
+++ b/fs/xfs/libxfs/xfs_btree.c
@@ -132,6 +132,7 @@ __xfs_btree_check_sblock(
 	struct xfs_mount	*mp = cur->bc_mp;
 	xfs_btnum_t		btnum = cur->bc_btnum;
 	int			crc = xfs_sb_version_hascrc(&mp->m_sb);
+	uint32_t		magic = xfs_btree_magic(crc, btnum);
 
 	if (crc) {
 		if (!uuid_equal(&block->bb_u.s.bb_uuid, &mp->m_sb.sb_meta_uuid))
@@ -141,8 +142,15 @@ __xfs_btree_check_sblock(
 			return __this_address;
 	}
 
-	if (be32_to_cpu(block->bb_magic) != xfs_btree_magic(crc, btnum))
-		return __this_address;
+	/*
+	 * Exclude the case of a finobt block with inobt magic from being an
+	 * error. See the inobt verifier for further details.
+	 */
+	if (be32_to_cpu(block->bb_magic) != magic) {
+		if (!(magic == XFS_FIBT_CRC_MAGIC &&
+		     (be32_to_cpu(block->bb_magic) == XFS_IBT_CRC_MAGIC)))
+			return __this_address;
+	}
 	if (be16_to_cpu(block->bb_level) != level)
 		return __this_address;
 	if (be16_to_cpu(block->bb_numrecs) >
diff --git a/fs/xfs/libxfs/xfs_ialloc_btree.c b/fs/xfs/libxfs/xfs_ialloc_btree.c
index c57ecb6b1255..79aafef42df6 100644
--- a/fs/xfs/libxfs/xfs_ialloc_btree.c
+++ b/fs/xfs/libxfs/xfs_ialloc_btree.c
@@ -259,9 +259,28 @@ xfs_inobt_verify(
 	struct xfs_btree_block	*block = XFS_BUF_TO_BLOCK(bp);
 	xfs_failaddr_t		fa;
 	unsigned int		level;
+	bool			hascrc = xfs_sb_version_hascrc(&mp->m_sb);
 
-	if (!xfs_verify_magic(bp, block->bb_magic))
-		return __this_address;
+	/*
+	 * xfs_repair had a bug that created interior finobt nodes with inobt
+	 * magic values. This escaped verifier detection for some time because
+	 * the verifier originally passed so long as the magic matched one of
+	 * the several possible [f]inobt magic values.
+	 *
+	 * We now distinguish between magic values per tree, but we cannot
+	 * outright fail as this would cause otherwise working filesystems to
+	 * become unusable. Instead, warn the user to upgrade xfs_repair and fix
+	 * the problem.
+	 */
+	if (!xfs_verify_magic(bp, block->bb_magic)) {
+		if ((bp->b_ops->magic[hascrc] == XFS_FIBT_CRC_MAGIC) &&
+		    (block->bb_magic == cpu_to_be32(XFS_IBT_CRC_MAGIC)))
+			xfs_warn(mp,
+"WARNING: inobt magic on finobt block 0x%llx. This is caused by xfs_repair. "
+"Please upgrade xfsprogs, unmount and run xfs_repair.", bp->b_bn);
+		else
+			return __this_address;
+	}
 
 	/*
 	 * During growfs operations, we can't verify the exact owner as the
@@ -273,7 +292,7 @@ xfs_inobt_verify(
 	 * but beware of the landmine (i.e. need to check pag->pagi_init) if we
 	 * ever do.
 	 */
-	if (xfs_sb_version_hascrc(&mp->m_sb)) {
+	if (hascrc) {
 		fa = xfs_btree_sblock_v5hdr_verify(bp);
 		if (fa)
 			return fa;
-- 
2.17.2

Re: [PATCH RFC v2 2/3] xfs: distinguish between inobt and finobt magic values

[Dave Chinner]

On Mon, Jan 28, 2019 at 10:20:33AM -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.

I still really don't like this code :(

> @@ -387,4 +388,22 @@ extern int xfs_setsize_buftarg(xfs_buftarg_t *, unsigned int);
>  
>  int xfs_buf_ensure_ops(struct xfs_buf *bp, const struct xfs_buf_ops *ops);
>  
> +/*
> + * Verify an on-disk magic value against the magic value specified in the
> + * verifier structure.
> + */
> +static inline bool
> +xfs_buf_ops_verify_magic(
> +	struct xfs_buf		*bp,
> +	__be32			dmagic,
> +	bool			crc)
> +{
> +	if (unlikely(WARN_ON(!bp->b_ops || !bp->b_ops->magic[crc])))
> +		return false;
> +	return dmagic == cpu_to_be32(bp->b_ops->magic[crc]);
> +}
> +#define xfs_verify_magic(bp, dmagic)		\
> +	xfs_buf_ops_verify_magic(bp, dmagic,	\
> +			xfs_sb_version_hascrc(&bp->b_target->bt_mount->m_sb))

That, IMO, is even worse....

Ok, here's a different option. Store all the magic numbers in a pair
of tables - one for v4, one for v5. They can be static const and
in on-disk format.

Then use some simple 1-line wrappers for the verifier definitions to
specify the table index for the magic numbers. e.g:

__be32 xfs_disk_magic(mp, idx)
{
	if (xfs_sb_version_hascrc(&mp->m_sb))
		return xfs_v5_disk_magic[idx];
	return xfs_v4_disk_magic[idx];
}

[.....]

__xfs_inobt_read_verify(bp, magic_idx)
{
	magic = xfs_disk_magic(mp, magic_idx);
	.....
}

__xfs_inobt_write_verify(bp, magic_idx)
{
	magic = xfs_disk_magic(mp, magic_idx);
	.....
}

__xfs_inobt_struct_verify(bp, magic_idx)
{
	magic = xfs_disk_magic(mp, magic_idx);
	.....
}

[ or drive the magic number resolution further inwards to where it
is actually needed. ]

xfs_inobt_read_verify(bp)
{
	return __xfs_inobt_read_verify(bp, INOBT);
}

xfs_inobt_write_verify(bp)
{
	return __xfs_inobt_write_verify(bp, INOBT);
}

xfs_inobt_struct_verify(bp)
{
	return __xfs_inobt_struct_verify(bp, INOBT);
}

xfs_finobt_read_verify(bp)
{
	return __xfs_inobt_read_verify(bp, FINOBT);
}

xfs_finobt_write_verify(bp)
{
	return __xfs_inobt_write_verify(bp, FINOBT);
}

xfs_finobt_struct_verify(bp)
{
	return __xfs_inobt_struct_verify(bp, FINOBT);
}

And this can be extended to all the verifiers - it handles crc and
non CRC variants transparently, and can be used for the cnt/bno free
space btrees, too.

Yes, it's a bit more boiler plate code, but IMO it is easier to
follow and understand than encoding multiple magic numbers into the
verifier and adding a dependency on the buffer having an ops
structure attached to be able to check the magic number...

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH RFC v2 2/3] xfs: distinguish between inobt and finobt magic values

[Brian Foster]

On Tue, Jan 29, 2019 at 09:54:26AM +1100, Dave Chinner wrote:
> On Mon, Jan 28, 2019 at 10:20:33AM -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.
> 
> I still really don't like this code :(
> 

Enough to explain why, perhaps?

> > @@ -387,4 +388,22 @@ extern int xfs_setsize_buftarg(xfs_buftarg_t *, unsigned int);
> >  
> >  int xfs_buf_ensure_ops(struct xfs_buf *bp, const struct xfs_buf_ops *ops);
> >  
> > +/*
> > + * Verify an on-disk magic value against the magic value specified in the
> > + * verifier structure.
> > + */
> > +static inline bool
> > +xfs_buf_ops_verify_magic(
> > +	struct xfs_buf		*bp,
> > +	__be32			dmagic,
> > +	bool			crc)
> > +{
> > +	if (unlikely(WARN_ON(!bp->b_ops || !bp->b_ops->magic[crc])))
> > +		return false;
> > +	return dmagic == cpu_to_be32(bp->b_ops->magic[crc]);
> > +}
> > +#define xfs_verify_magic(bp, dmagic)		\
> > +	xfs_buf_ops_verify_magic(bp, dmagic,	\
> > +			xfs_sb_version_hascrc(&bp->b_target->bt_mount->m_sb))
> 
> That, IMO, is even worse....
> 

Worse than what and why?

Note that I've removed the endian conversion from here. Otherwise, this
is basically just a wrapper to factor out the sb version lookup and
provide some common error checking.

> Ok, here's a different option. Store all the magic numbers in a pair
> of tables - one for v4, one for v5. They can be static const and
> in on-disk format.
> 
> Then use some simple 1-line wrappers for the verifier definitions to
> specify the table index for the magic numbers. e.g:
> 
> __be32 xfs_disk_magic(mp, idx)
> {
> 	if (xfs_sb_version_hascrc(&mp->m_sb))
> 		return xfs_v5_disk_magic[idx];
> 	return xfs_v4_disk_magic[idx];
> }
> 

Seems reasonable enough... but where/how is the index encoded?

> [.....]
> 
> __xfs_inobt_read_verify(bp, magic_idx)
> {
> 	magic = xfs_disk_magic(mp, magic_idx);
> 	.....
> }
> 
> __xfs_inobt_write_verify(bp, magic_idx)
> {
> 	magic = xfs_disk_magic(mp, magic_idx);
> 	.....
> }
> 
> __xfs_inobt_struct_verify(bp, magic_idx)
> {
> 	magic = xfs_disk_magic(mp, magic_idx);
> 	.....
> }
> 
> [ or drive the magic number resolution further inwards to where it
> is actually needed. ]
> 
> xfs_inobt_read_verify(bp)
> {
> 	return __xfs_inobt_read_verify(bp, INOBT);
> }
> 
> xfs_inobt_write_verify(bp)
> {
> 	return __xfs_inobt_write_verify(bp, INOBT);
> }
> 
> xfs_inobt_struct_verify(bp)
> {
> 	return __xfs_inobt_struct_verify(bp, INOBT);
> }
> 
> xfs_finobt_read_verify(bp)
> {
> 	return __xfs_inobt_read_verify(bp, FINOBT);
> }
> 
> xfs_finobt_write_verify(bp)
> {
> 	return __xfs_inobt_write_verify(bp, FINOBT);
> }
> 
> xfs_finobt_struct_verify(bp)
> {
> 	return __xfs_inobt_struct_verify(bp, FINOBT);
> }
> 
> And this can be extended to all the verifiers - it handles crc and
> non CRC variants transparently, and can be used for the cnt/bno free
> space btrees, too.
> 
> Yes, it's a bit more boiler plate code, but IMO it is easier to
> follow and understand than encoding multiple magic numbers into the
> verifier and adding a dependency on the buffer having an ops
> structure attached to be able to check the magic number...
> 

This code duplication is what I was hoping to avoid. We already have
similar proliferation of boilerplate code in some of the verifiers that
handle multiple object types. See the appended hunk related to the dir
leaf verifier code, for example.

I agree that the magic value itself is a bit obfuscated with this
change, but that's still the case with a lookup table. I disagree that
an indirected magic value is more difficult to read than nearly a screen
full of similarly named verifier functions. The magic value is a simple
data value, the code above makes it unnecessarily more confusing (at a
glance) to grok which verifiers run which checks. That's just my
experience from folding the code below, fwiw.

Another angle to this is that we don't necessarily have to use the
xfs_buf_ops->magic field for every verifier. I could just add it to the
finobt case, perhaps the directory case below, and leave the rest alone
until we come up with something more agreeable. Then it basically just
supports a couple corner cases and is easy enough to remove down the
road.

Brian

--- 8< ---

diff --git a/fs/xfs/libxfs/xfs_dir2_leaf.c b/fs/xfs/libxfs/xfs_dir2_leaf.c
index 1728a3e6f5cf..f602307d2fa0 100644
--- a/fs/xfs/libxfs/xfs_dir2_leaf.c
+++ b/fs/xfs/libxfs/xfs_dir2_leaf.c
@@ -142,41 +142,32 @@ xfs_dir3_leaf_check_int(
  */
 static xfs_failaddr_t
 xfs_dir3_leaf_verify(
-	struct xfs_buf		*bp,
-	uint16_t		magic)
+	struct xfs_buf		*bp)
 {
 	struct xfs_mount	*mp = bp->b_target->bt_mount;
 	struct xfs_dir2_leaf	*leaf = bp->b_addr;
 
-	ASSERT(magic == XFS_DIR2_LEAF1_MAGIC || magic == XFS_DIR2_LEAFN_MAGIC);
+	if (!xfs_verify_magic(bp, be16_to_cpu(leaf->hdr.info.magic)))
+		return __this_address;
 
 	if (xfs_sb_version_hascrc(&mp->m_sb)) {
 		struct xfs_dir3_leaf_hdr *leaf3 = bp->b_addr;
-		uint16_t		magic3;
 
-		magic3 = (magic == XFS_DIR2_LEAF1_MAGIC) ? XFS_DIR3_LEAF1_MAGIC
-							 : XFS_DIR3_LEAFN_MAGIC;
-
-		if (leaf3->info.hdr.magic != cpu_to_be16(magic3))
-			return __this_address;
+		ASSERT(leaf3->info.hdr.magic == leaf->hdr.info.magic);
 		if (!uuid_equal(&leaf3->info.uuid, &mp->m_sb.sb_meta_uuid))
 			return __this_address;
 		if (be64_to_cpu(leaf3->info.blkno) != bp->b_bn)
 			return __this_address;
 		if (!xfs_log_check_lsn(mp, be64_to_cpu(leaf3->info.lsn)))
 			return __this_address;
-	} else {
-		if (leaf->hdr.info.magic != cpu_to_be16(magic))
-			return __this_address;
 	}
 
 	return xfs_dir3_leaf_check_int(mp, NULL, NULL, leaf);
 }
 
 static void
-__read_verify(
-	struct xfs_buf  *bp,
-	uint16_t	magic)
+xfs_dir3_leaf_read_verify(
+	struct xfs_buf  *bp)
 {
 	struct xfs_mount	*mp = bp->b_target->bt_mount;
 	xfs_failaddr_t		fa;
@@ -185,23 +176,22 @@ __read_verify(
 	     !xfs_buf_verify_cksum(bp, XFS_DIR3_LEAF_CRC_OFF))
 		xfs_verifier_error(bp, -EFSBADCRC, __this_address);
 	else {
-		fa = xfs_dir3_leaf_verify(bp, magic);
+		fa = xfs_dir3_leaf_verify(bp);
 		if (fa)
 			xfs_verifier_error(bp, -EFSCORRUPTED, fa);
 	}
 }
 
 static void
-__write_verify(
-	struct xfs_buf  *bp,
-	uint16_t	magic)
+xfs_dir3_leaf_write_verify(
+	struct xfs_buf  *bp)
 {
 	struct xfs_mount	*mp = bp->b_target->bt_mount;
 	struct xfs_buf_log_item	*bip = bp->b_log_item;
 	struct xfs_dir3_leaf_hdr *hdr3 = bp->b_addr;
 	xfs_failaddr_t		fa;
 
-	fa = xfs_dir3_leaf_verify(bp, magic);
+	fa = xfs_dir3_leaf_verify(bp);
 	if (fa) {
 		xfs_verifier_error(bp, -EFSCORRUPTED, fa);
 		return;
@@ -216,60 +206,20 @@ __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 = { XFS_DIR2_LEAF1_MAGIC, 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 = { XFS_DIR2_LEAFN_MAGIC, 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

Re: [PATCH RFC v2 2/3] xfs: distinguish between inobt and finobt magic values

[Dave Chinner]

On Tue, Jan 29, 2019 at 09:01:36AM -0500, Brian Foster wrote:
> On Tue, Jan 29, 2019 at 09:54:26AM +1100, Dave Chinner wrote:
> > On Mon, Jan 28, 2019 at 10:20:33AM -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.
> > 
> > I still really don't like this code :(
> > 
> 
> Enough to explain why, perhaps?

I did in the past thread - it adds runtime overhead in performance
critical paths, and it requires verfiers to have a dependecy on
bp->b_ops being set.

> > > @@ -387,4 +388,22 @@ extern int xfs_setsize_buftarg(xfs_buftarg_t *, unsigned int);
> > >  
> > >  int xfs_buf_ensure_ops(struct xfs_buf *bp, const struct xfs_buf_ops *ops);
> > >  
> > > +/*
> > > + * Verify an on-disk magic value against the magic value specified in the
> > > + * verifier structure.
> > > + */
> > > +static inline bool
> > > +xfs_buf_ops_verify_magic(
> > > +	struct xfs_buf		*bp,
> > > +	__be32			dmagic,
> > > +	bool			crc)
> > > +{
> > > +	if (unlikely(WARN_ON(!bp->b_ops || !bp->b_ops->magic[crc])))
> > > +		return false;
> > > +	return dmagic == cpu_to_be32(bp->b_ops->magic[crc]);
> > > +}
> > > +#define xfs_verify_magic(bp, dmagic)		\
> > > +	xfs_buf_ops_verify_magic(bp, dmagic,	\
> > > +			xfs_sb_version_hascrc(&bp->b_target->bt_mount->m_sb))
> > 
> > That, IMO, is even worse....
> > 
> 
> Worse than what and why?

Worse that the last patch, because it now adds a needless macro that
only serves to obfuscate the code. This:

static inline bool
xfs_verify_magic(
	struct xfs_mount	*mp,
	__be32			dmagic,
	int			idx)
{
	__be32			magic;

	if (xfs_sb_version_hascrc(&mp->m_sb))
		magic = xfs_v5_disk_magic[idx];
	magic = xfs_v4_disk_magic[idx];

	return dmagic == magic;
}

is much cleaner and easier to understand....

> Note that I've removed the endian conversion from here. Otherwise, this
> is basically just a wrapper to factor out the sb version lookup and
> provide some common error checking.
> 
> > Ok, here's a different option. Store all the magic numbers in a pair
> > of tables - one for v4, one for v5. They can be static const and
> > in on-disk format.
> > 
> > Then use some simple 1-line wrappers for the verifier definitions to
> > specify the table index for the magic numbers. e.g:
> > 
> > __be32 xfs_disk_magic(mp, idx)
> > {
> > 	if (xfs_sb_version_hascrc(&mp->m_sb))
> > 		return xfs_v5_disk_magic[idx];
> > 	return xfs_v4_disk_magic[idx];
> > }
> > 
> 
> Seems reasonable enough... but where/how is the index encoded?

I was thinking in fs/xfs/libxfs/xfs_types.[ch], via an index similar
to xfs_btnum_t indexes (could even use it to begin with).

static const xfs_v5_disk_magic[] = {
	cpu_to_be32(XFS_ABTB_CRC_MAGIC),
	cpu_to_be32(XFS_ABTC_CRC_MAGIC),
	cpu_to_be32(XFS_ITB_CRC_MAGIC),
	cpu_to_be32(XFS_FITB_CRC_MAGIC),
	.....
}

You could do the same thing to the verfier op definition to
remove the need on-the-fly endian conversion just for the magic
number checks, which gets rid of that concern.

> > And this can be extended to all the verifiers - it handles crc and
> > non CRC variants transparently, and can be used for the cnt/bno free
> > space btrees, too.
> > 
> > Yes, it's a bit more boiler plate code, but IMO it is easier to
> > follow and understand than encoding multiple magic numbers into the
> > verifier and adding a dependency on the buffer having an ops
> > structure attached to be able to check the magic number...
> 
> This code duplication is what I was hoping to avoid. We already have
> similar proliferation of boilerplate code in some of the verifiers that
> handle multiple object types. See the appended hunk related to the dir
> leaf verifier code, for example.

Personally I prefer code duplication first, then factor later once
the code settles down. In hindsight, we've probably factored the
verifiers too much too soon...

> I agree that the magic value itself is a bit obfuscated with this
> change, but that's still the case with a lookup table.

The difference with the lookup table is that you know what the magic
number is supposed to be by looking at the code that calls it...

> Another angle to this is that we don't necessarily have to use the
> xfs_buf_ops->magic field for every verifier. I could just add it to the
> finobt case, perhaps the directory case below, and leave the rest alone
> until we come up with something more agreeable. Then it basically just
> supports a couple corner cases and is easy enough to remove down the
> road.

I'd like all the verifiers to use the same mechanism so we maintain
consistency between them.

> --- 8< ---
> 
> diff --git a/fs/xfs/libxfs/xfs_dir2_leaf.c b/fs/xfs/libxfs/xfs_dir2_leaf.c
> index 1728a3e6f5cf..f602307d2fa0 100644
> --- a/fs/xfs/libxfs/xfs_dir2_leaf.c
> +++ b/fs/xfs/libxfs/xfs_dir2_leaf.c
> @@ -142,41 +142,32 @@ xfs_dir3_leaf_check_int(
>   */
>  static xfs_failaddr_t
>  xfs_dir3_leaf_verify(
> -	struct xfs_buf		*bp,
> -	uint16_t		magic)
> +	struct xfs_buf		*bp)
>  {
>  	struct xfs_mount	*mp = bp->b_target->bt_mount;
>  	struct xfs_dir2_leaf	*leaf = bp->b_addr;
>  
> -	ASSERT(magic == XFS_DIR2_LEAF1_MAGIC || magic == XFS_DIR2_LEAFN_MAGIC);
> +	if (!xfs_verify_magic(bp, be16_to_cpu(leaf->hdr.info.magic)))
> +		return __this_address;
>  
>  	if (xfs_sb_version_hascrc(&mp->m_sb)) {
>  		struct xfs_dir3_leaf_hdr *leaf3 = bp->b_addr;
> -		uint16_t		magic3;
>  
> -		magic3 = (magic == XFS_DIR2_LEAF1_MAGIC) ? XFS_DIR3_LEAF1_MAGIC
> -							 : XFS_DIR3_LEAFN_MAGIC;
> -
> -		if (leaf3->info.hdr.magic != cpu_to_be16(magic3))
> -			return __this_address;
> +		ASSERT(leaf3->info.hdr.magic == leaf->hdr.info.magic);
>  		if (!uuid_equal(&leaf3->info.uuid, &mp->m_sb.sb_meta_uuid))
>  			return __this_address;
>  		if (be64_to_cpu(leaf3->info.blkno) != bp->b_bn)
>  			return __this_address;
>  		if (!xfs_log_check_lsn(mp, be64_to_cpu(leaf3->info.lsn)))
>  			return __this_address;
> -	} else {
> -		if (leaf->hdr.info.magic != cpu_to_be16(magic))
> -			return __this_address;
>  	}
>  
>  	return xfs_dir3_leaf_check_int(mp, NULL, NULL, leaf);
>  }

.....

Ok, that removes a lot more existing code than I ever thought it
would. If you clean up the macro mess and use encoded magic numbers
in the ops structure, then consider my objections removed. :)

(And that then leads to factoring of xfs_dablk_info_verify() as dir
leaf, danode and attribute leaf blocks all use the same struct
xfs_da3_blkinfo header, and now the magic number is abstracted they
can use the same code....)

Brian, to help prevent stupid people like me wasting your time in
future, can you post the entire patch set you have so we can see the
same picture you have for the overall change, even if there's only a
small chunk you are proposing for merge? That way we'll be able to
judge the change on the merits of the entire work, rather than just
the small chunk that was posted? 

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH RFC v2 2/3] xfs: distinguish between inobt and finobt magic values

[Brian Foster]

On Wed, Jan 30, 2019 at 08:16:55AM +1100, Dave Chinner wrote:
> On Tue, Jan 29, 2019 at 09:01:36AM -0500, Brian Foster wrote:
> > On Tue, Jan 29, 2019 at 09:54:26AM +1100, Dave Chinner wrote:
> > > On Mon, Jan 28, 2019 at 10:20:33AM -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.
> > > 
> > > I still really don't like this code :(
> > > 
> > 
> > Enough to explain why, perhaps?
> 
> I did in the past thread - it adds runtime overhead in performance
> critical paths, and it requires verfiers to have a dependecy on
> bp->b_ops being set.
> 

Fair points, but seem like nits to me when you consider the unfortunate
lack of decent alternatives. And the ->b_ops thing is really just a
happenstance bit of scrub logic that needs to be tweaked.

> > > > @@ -387,4 +388,22 @@ extern int xfs_setsize_buftarg(xfs_buftarg_t *, unsigned int);
> > > >  
> > > >  int xfs_buf_ensure_ops(struct xfs_buf *bp, const struct xfs_buf_ops *ops);
> > > >  
> > > > +/*
> > > > + * Verify an on-disk magic value against the magic value specified in the
> > > > + * verifier structure.
> > > > + */
> > > > +static inline bool
> > > > +xfs_buf_ops_verify_magic(
> > > > +	struct xfs_buf		*bp,
> > > > +	__be32			dmagic,
> > > > +	bool			crc)
> > > > +{
> > > > +	if (unlikely(WARN_ON(!bp->b_ops || !bp->b_ops->magic[crc])))
> > > > +		return false;
> > > > +	return dmagic == cpu_to_be32(bp->b_ops->magic[crc]);
> > > > +}
> > > > +#define xfs_verify_magic(bp, dmagic)		\
> > > > +	xfs_buf_ops_verify_magic(bp, dmagic,	\
> > > > +			xfs_sb_version_hascrc(&bp->b_target->bt_mount->m_sb))
> > > 
> > > That, IMO, is even worse....
> > > 
> > 
> > Worse than what and why?
> 
> Worse that the last patch, because it now adds a needless macro that
> only serves to obfuscate the code. This:
> 

That is easy enough to address (using your logic below) regardless of
how we access the magic value.

> static inline bool
> xfs_verify_magic(
> 	struct xfs_mount	*mp,
> 	__be32			dmagic,
> 	int			idx)
> {
> 	__be32			magic;
> 
> 	if (xfs_sb_version_hascrc(&mp->m_sb))
> 		magic = xfs_v5_disk_magic[idx];
> 	magic = xfs_v4_disk_magic[idx];
> 
> 	return dmagic == magic;
> }
> 
> is much cleaner and easier to understand....
> 
> > Note that I've removed the endian conversion from here. Otherwise, this
> > is basically just a wrapper to factor out the sb version lookup and
> > provide some common error checking.
> > 
> > > Ok, here's a different option. Store all the magic numbers in a pair
> > > of tables - one for v4, one for v5. They can be static const and
> > > in on-disk format.
> > > 
> > > Then use some simple 1-line wrappers for the verifier definitions to
> > > specify the table index for the magic numbers. e.g:
> > > 
> > > __be32 xfs_disk_magic(mp, idx)
> > > {
> > > 	if (xfs_sb_version_hascrc(&mp->m_sb))
> > > 		return xfs_v5_disk_magic[idx];
> > > 	return xfs_v4_disk_magic[idx];
> > > }
> > > 
> > 
> > Seems reasonable enough... but where/how is the index encoded?
> 
> I was thinking in fs/xfs/libxfs/xfs_types.[ch], via an index similar
> to xfs_btnum_t indexes (could even use it to begin with).
> 
> static const xfs_v5_disk_magic[] = {
> 	cpu_to_be32(XFS_ABTB_CRC_MAGIC),
> 	cpu_to_be32(XFS_ABTC_CRC_MAGIC),
> 	cpu_to_be32(XFS_ITB_CRC_MAGIC),
> 	cpu_to_be32(XFS_FITB_CRC_MAGIC),
> 	.....
> }
> 
> You could do the same thing to the verfier op definition to
> remove the need on-the-fly endian conversion just for the magic
> number checks, which gets rid of that concern.
> 
> > > And this can be extended to all the verifiers - it handles crc and
> > > non CRC variants transparently, and can be used for the cnt/bno free
> > > space btrees, too.
> > > 
> > > Yes, it's a bit more boiler plate code, but IMO it is easier to
> > > follow and understand than encoding multiple magic numbers into the
> > > verifier and adding a dependency on the buffer having an ops
> > > structure attached to be able to check the magic number...
> > 
> > This code duplication is what I was hoping to avoid. We already have
> > similar proliferation of boilerplate code in some of the verifiers that
> > handle multiple object types. See the appended hunk related to the dir
> > leaf verifier code, for example.
> 
> Personally I prefer code duplication first, then factor later once
> the code settles down. In hindsight, we've probably factored the
> verifiers too much too soon...
> 
> > I agree that the magic value itself is a bit obfuscated with this
> > change, but that's still the case with a lookup table.
> 
> The difference with the lookup table is that you know what the magic
> number is supposed to be by looking at the code that calls it...
> 

Indeed. What I didn't realize until later today is that some verifiers
(xfs_sb_buf_ops, xfs_attr3_leaf_buf_ops, xfs_da3_node_buf_ops) check
already converted in-core structures and thus actually verify against
cpu endian magic values. This means said verifiers would require further
tweaks to either check the underlying buffer, another conversion back to
disk endian, or we'd otherwise need four of these arrays. :/

> > Another angle to this is that we don't necessarily have to use the
> > xfs_buf_ops->magic field for every verifier. I could just add it to the
> > finobt case, perhaps the directory case below, and leave the rest alone
> > until we come up with something more agreeable. Then it basically just
> > supports a couple corner cases and is easy enough to remove down the
> > road.
> 
> I'd like all the verifiers to use the same mechanism so we maintain
> consistency between them.
> 

I'd like that too, but I think we need to make some kind of tradeoff or
compromise to fix this problem given the current, rather ad-hoc nature
of the verifier code. Some check in-core structs, some don't and may or
may not use the compile time conversion optimization.

> > --- 8< ---
> > 
> > diff --git a/fs/xfs/libxfs/xfs_dir2_leaf.c b/fs/xfs/libxfs/xfs_dir2_leaf.c
> > index 1728a3e6f5cf..f602307d2fa0 100644
> > --- a/fs/xfs/libxfs/xfs_dir2_leaf.c
> > +++ b/fs/xfs/libxfs/xfs_dir2_leaf.c
> > @@ -142,41 +142,32 @@ xfs_dir3_leaf_check_int(
> >   */
> >  static xfs_failaddr_t
> >  xfs_dir3_leaf_verify(
> > -	struct xfs_buf		*bp,
> > -	uint16_t		magic)
> > +	struct xfs_buf		*bp)
> >  {
> >  	struct xfs_mount	*mp = bp->b_target->bt_mount;
> >  	struct xfs_dir2_leaf	*leaf = bp->b_addr;
> >  
> > -	ASSERT(magic == XFS_DIR2_LEAF1_MAGIC || magic == XFS_DIR2_LEAFN_MAGIC);
> > +	if (!xfs_verify_magic(bp, be16_to_cpu(leaf->hdr.info.magic)))
> > +		return __this_address;
> >  
> >  	if (xfs_sb_version_hascrc(&mp->m_sb)) {
> >  		struct xfs_dir3_leaf_hdr *leaf3 = bp->b_addr;
> > -		uint16_t		magic3;
> >  
> > -		magic3 = (magic == XFS_DIR2_LEAF1_MAGIC) ? XFS_DIR3_LEAF1_MAGIC
> > -							 : XFS_DIR3_LEAFN_MAGIC;
> > -
> > -		if (leaf3->info.hdr.magic != cpu_to_be16(magic3))
> > -			return __this_address;
> > +		ASSERT(leaf3->info.hdr.magic == leaf->hdr.info.magic);
> >  		if (!uuid_equal(&leaf3->info.uuid, &mp->m_sb.sb_meta_uuid))
> >  			return __this_address;
> >  		if (be64_to_cpu(leaf3->info.blkno) != bp->b_bn)
> >  			return __this_address;
> >  		if (!xfs_log_check_lsn(mp, be64_to_cpu(leaf3->info.lsn)))
> >  			return __this_address;
> > -	} else {
> > -		if (leaf->hdr.info.magic != cpu_to_be16(magic))
> > -			return __this_address;
> >  	}
> >  
> >  	return xfs_dir3_leaf_check_int(mp, NULL, NULL, leaf);
> >  }
> 
> .....
> 
> Ok, that removes a lot more existing code than I ever thought it
> would. If you clean up the macro mess and use encoded magic numbers
> in the ops structure, then consider my objections removed. :)
> 

I'll kill off the macro..

By encoded, I assume you mean on-disk order(?). Given that some
verifiers use the cpu endian value, I thought it more clear for the
helper to expect a cpu endian value. We could technically store any
endian we want, including different endian on a per verifier basis and
pass the values all the way through, but I'd find that rather confusing
(and a nightmare to review and maintain).

> (And that then leads to factoring of xfs_dablk_info_verify() as dir
> leaf, danode and attribute leaf blocks all use the same struct
> xfs_da3_blkinfo header, and now the magic number is abstracted they
> can use the same code....)
> 

Not sure I follow..?

> Brian, to help prevent stupid people like me wasting your time in
> future, can you post the entire patch set you have so we can see the
> same picture you have for the overall change, even if there's only a
> small chunk you are proposing for merge? That way we'll be able to
> judge the change on the merits of the entire work, rather than just
> the small chunk that was posted? 
> 

That was the entire patchset at the time. ;) I intentionally made the
isolated finobt change and posted that to try and get big picture
feedback before making mechanical changes to the rest of the verifiers.
I probably had most of the rest done shortly after posting the rfcv2,
but it wasn't tested until today (re: the v1 post) so I just included
the above snippet to demonstrate the cleanup.

Brian

> Cheers,
> 
> Dave.
> -- 
> Dave Chinner
> david@fromorbit.com

Re: [PATCH RFC v2 2/3] xfs: distinguish between inobt and finobt magic values

[Dave Chinner]

On Tue, Jan 29, 2019 at 08:05:53PM -0500, Brian Foster wrote:
> On Wed, Jan 30, 2019 at 08:16:55AM +1100, Dave Chinner wrote:
> > On Tue, Jan 29, 2019 at 09:01:36AM -0500, Brian Foster wrote:
> > > On Tue, Jan 29, 2019 at 09:54:26AM +1100, Dave Chinner wrote:
> > > I agree that the magic value itself is a bit obfuscated with this
> > > change, but that's still the case with a lookup table.
> > 
> > The difference with the lookup table is that you know what the magic
> > number is supposed to be by looking at the code that calls it...
> > 
> 
> Indeed. What I didn't realize until later today is that some verifiers
> (xfs_sb_buf_ops, xfs_attr3_leaf_buf_ops, xfs_da3_node_buf_ops) check
> already converted in-core structures and thus actually verify against
> cpu endian magic values. This means said verifiers would require further
> tweaks to either check the underlying buffer, another conversion back to
> disk endian, or we'd otherwise need four of these arrays. :/

That was purely convenience, because we had to convert to the incore
header to check a bunch of other stuff, so the magic number got
converted for free.

I'd prefer if we are going to use a generic method of checking magic
numbers that it does it in on-disk format so that we don't need to
convert just for the magic number check.

> > I'd like all the verifiers to use the same mechanism so we maintain
> > consistency between them.
> > 
> 
> I'd like that too, but I think we need to make some kind of tradeoff or
> compromise to fix this problem given the current, rather ad-hoc nature
> of the verifier code. Some check in-core structs, some don't and may or
> may not use the compile time conversion optimization.

Ypup, so lets get them all on to checking the on-disk magic number
before conversion.

> > > --- 8< ---
> > > 
> > > diff --git a/fs/xfs/libxfs/xfs_dir2_leaf.c b/fs/xfs/libxfs/xfs_dir2_leaf.c
> > > index 1728a3e6f5cf..f602307d2fa0 100644
> > > --- a/fs/xfs/libxfs/xfs_dir2_leaf.c
> > > +++ b/fs/xfs/libxfs/xfs_dir2_leaf.c
> > > @@ -142,41 +142,32 @@ xfs_dir3_leaf_check_int(
> > >   */
> > >  static xfs_failaddr_t
> > >  xfs_dir3_leaf_verify(
> > > -	struct xfs_buf		*bp,
> > > -	uint16_t		magic)
> > > +	struct xfs_buf		*bp)
> > >  {
> > >  	struct xfs_mount	*mp = bp->b_target->bt_mount;
> > >  	struct xfs_dir2_leaf	*leaf = bp->b_addr;
> > >  
> > > -	ASSERT(magic == XFS_DIR2_LEAF1_MAGIC || magic == XFS_DIR2_LEAFN_MAGIC);
> > > +	if (!xfs_verify_magic(bp, be16_to_cpu(leaf->hdr.info.magic)))
> > > +		return __this_address;
> > >  
> > >  	if (xfs_sb_version_hascrc(&mp->m_sb)) {
> > >  		struct xfs_dir3_leaf_hdr *leaf3 = bp->b_addr;
> > > -		uint16_t		magic3;
> > >  
> > > -		magic3 = (magic == XFS_DIR2_LEAF1_MAGIC) ? XFS_DIR3_LEAF1_MAGIC
> > > -							 : XFS_DIR3_LEAFN_MAGIC;
> > > -
> > > -		if (leaf3->info.hdr.magic != cpu_to_be16(magic3))
> > > -			return __this_address;
> > > +		ASSERT(leaf3->info.hdr.magic == leaf->hdr.info.magic);
> > >  		if (!uuid_equal(&leaf3->info.uuid, &mp->m_sb.sb_meta_uuid))
> > >  			return __this_address;
> > >  		if (be64_to_cpu(leaf3->info.blkno) != bp->b_bn)
> > >  			return __this_address;
> > >  		if (!xfs_log_check_lsn(mp, be64_to_cpu(leaf3->info.lsn)))
> > >  			return __this_address;
> > > -	} else {
> > > -		if (leaf->hdr.info.magic != cpu_to_be16(magic))
> > > -			return __this_address;
> > >  	}
> > >  
> > >  	return xfs_dir3_leaf_check_int(mp, NULL, NULL, leaf);
> > >  }
> > 
> > .....
> > 
> > Ok, that removes a lot more existing code than I ever thought it
> > would. If you clean up the macro mess and use encoded magic numbers
> > in the ops structure, then consider my objections removed. :)
> > 
> 
> I'll kill off the macro..
> 
> By encoded, I assume you mean on-disk order(?).

Yup.

> > (And that then leads to factoring of xfs_dablk_info_verify() as dir
> > leaf, danode and attribute leaf blocks all use the same struct
> > xfs_da3_blkinfo header, and now the magic number is abstracted they
> > can use the same code....)
> > 
> 
> Not sure I follow..?

They all do the same thing. Taking your converted code:

	if (!xfs_verify_magic(bp, be16_to_cpu(leaf->hdr.info.magic)))
		return __this_address;

	if (xfs_sb_version_hascrc(&mp->m_sb)) {
		struct xfs_dir3_leaf_hdr *leaf3 = bp->b_addr;

		ASSERT(leaf3->info.hdr.magic == leaf->hdr.info.magic);
		if (!uuid_equal(&leaf3->info.uuid, &mp->m_sb.sb_meta_uuid))
			return __this_address;
		if (be64_to_cpu(leaf3->info.blkno) != bp->b_bn)
			return __this_address;
		if (!xfs_log_check_lsn(mp, be64_to_cpu(leaf3->info.lsn)))
			return __this_address;
	}

The only thing they need is mp, &leaf->hdr, and bp. They don't
actually need to know that its a dir2/dir3 leaf block now the magic
number is encoded in bp->b_ops.

i.e. that boiler plate can be factored out of multiple verifiers...

> > Brian, to help prevent stupid people like me wasting your time in
> > future, can you post the entire patch set you have so we can see the
> > same picture you have for the overall change, even if there's only a
> > small chunk you are proposing for merge? That way we'll be able to
> > judge the change on the merits of the entire work, rather than just
> > the small chunk that was posted? 
> > 
> 
> That was the entire patchset at the time. ;) I intentionally made the
> isolated finobt change and posted that to try and get big picture
> feedback before making mechanical changes to the rest of the verifiers.
> I probably had most of the rest done shortly after posting the rfcv2,
> but it wasn't tested until today (re: the v1 post) so I just included
> the above snippet to demonstrate the cleanup.

OK, so somewhat crossed wires while changes were still being made.
Such is life...

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH RFC v2 2/3] xfs: distinguish between inobt and finobt magic values

[Brian Foster]

On Wed, Jan 30, 2019 at 01:15:29PM +1100, Dave Chinner wrote:
> On Tue, Jan 29, 2019 at 08:05:53PM -0500, Brian Foster wrote:
> > On Wed, Jan 30, 2019 at 08:16:55AM +1100, Dave Chinner wrote:
> > > On Tue, Jan 29, 2019 at 09:01:36AM -0500, Brian Foster wrote:
> > > > On Tue, Jan 29, 2019 at 09:54:26AM +1100, Dave Chinner wrote:
> > > > I agree that the magic value itself is a bit obfuscated with this
> > > > change, but that's still the case with a lookup table.
> > > 
> > > The difference with the lookup table is that you know what the magic
> > > number is supposed to be by looking at the code that calls it...
> > > 
> > 
> > Indeed. What I didn't realize until later today is that some verifiers
> > (xfs_sb_buf_ops, xfs_attr3_leaf_buf_ops, xfs_da3_node_buf_ops) check
> > already converted in-core structures and thus actually verify against
> > cpu endian magic values. This means said verifiers would require further
> > tweaks to either check the underlying buffer, another conversion back to
> > disk endian, or we'd otherwise need four of these arrays. :/
> 
> That was purely convenience, because we had to convert to the incore
> header to check a bunch of other stuff, so the magic number got
> converted for free.
> 

I think that applies to the first two cases noted above. The
xfs_da3_node_verify() case is a bit more involved conceptually because
we call out to another indirect function to do the conversion. I think
we can ultimately use hdr for the magic check just the same as the
others because either way the block is headed by an xfs_da_blkinfo, it
just takes some thought to grok from the verifier context (and thus adds
minor maintenance burden if this code changes again down the road). I'll
try to add a comment there..

> I'd prefer if we are going to use a generic method of checking magic
> numbers that it does it in on-disk format so that we don't need to
> convert just for the magic number check.
> 
> > > I'd like all the verifiers to use the same mechanism so we maintain
> > > consistency between them.
> > > 
> > 
> > I'd like that too, but I think we need to make some kind of tradeoff or
> > compromise to fix this problem given the current, rather ad-hoc nature
> > of the verifier code. Some check in-core structs, some don't and may or
> > may not use the compile time conversion optimization.
> 
> Ypup, so lets get them all on to checking the on-disk magic number
> before conversion.
> 
> > > > --- 8< ---
> > > > 
> > > > diff --git a/fs/xfs/libxfs/xfs_dir2_leaf.c b/fs/xfs/libxfs/xfs_dir2_leaf.c
> > > > index 1728a3e6f5cf..f602307d2fa0 100644
> > > > --- a/fs/xfs/libxfs/xfs_dir2_leaf.c
> > > > +++ b/fs/xfs/libxfs/xfs_dir2_leaf.c
> > > > @@ -142,41 +142,32 @@ xfs_dir3_leaf_check_int(
> > > >   */
> > > >  static xfs_failaddr_t
> > > >  xfs_dir3_leaf_verify(
> > > > -	struct xfs_buf		*bp,
> > > > -	uint16_t		magic)
> > > > +	struct xfs_buf		*bp)
> > > >  {
> > > >  	struct xfs_mount	*mp = bp->b_target->bt_mount;
> > > >  	struct xfs_dir2_leaf	*leaf = bp->b_addr;
> > > >  
> > > > -	ASSERT(magic == XFS_DIR2_LEAF1_MAGIC || magic == XFS_DIR2_LEAFN_MAGIC);
> > > > +	if (!xfs_verify_magic(bp, be16_to_cpu(leaf->hdr.info.magic)))
> > > > +		return __this_address;
> > > >  
> > > >  	if (xfs_sb_version_hascrc(&mp->m_sb)) {
> > > >  		struct xfs_dir3_leaf_hdr *leaf3 = bp->b_addr;
> > > > -		uint16_t		magic3;
> > > >  
> > > > -		magic3 = (magic == XFS_DIR2_LEAF1_MAGIC) ? XFS_DIR3_LEAF1_MAGIC
> > > > -							 : XFS_DIR3_LEAFN_MAGIC;
> > > > -
> > > > -		if (leaf3->info.hdr.magic != cpu_to_be16(magic3))
> > > > -			return __this_address;
> > > > +		ASSERT(leaf3->info.hdr.magic == leaf->hdr.info.magic);
> > > >  		if (!uuid_equal(&leaf3->info.uuid, &mp->m_sb.sb_meta_uuid))
> > > >  			return __this_address;
> > > >  		if (be64_to_cpu(leaf3->info.blkno) != bp->b_bn)
> > > >  			return __this_address;
> > > >  		if (!xfs_log_check_lsn(mp, be64_to_cpu(leaf3->info.lsn)))
> > > >  			return __this_address;
> > > > -	} else {
> > > > -		if (leaf->hdr.info.magic != cpu_to_be16(magic))
> > > > -			return __this_address;
> > > >  	}
> > > >  
> > > >  	return xfs_dir3_leaf_check_int(mp, NULL, NULL, leaf);
> > > >  }
> > > 
> > > .....
> > > 
> > > Ok, that removes a lot more existing code than I ever thought it
> > > would. If you clean up the macro mess and use encoded magic numbers
> > > in the ops structure, then consider my objections removed. :)
> > > 
> > 
> > I'll kill off the macro..
> > 
> > By encoded, I assume you mean on-disk order(?).
> 
> Yup.
> 
> > > (And that then leads to factoring of xfs_dablk_info_verify() as dir
> > > leaf, danode and attribute leaf blocks all use the same struct
> > > xfs_da3_blkinfo header, and now the magic number is abstracted they
> > > can use the same code....)
> > > 
> > 
> > Not sure I follow..?
> 
> They all do the same thing. Taking your converted code:
> 
> 	if (!xfs_verify_magic(bp, be16_to_cpu(leaf->hdr.info.magic)))
> 		return __this_address;
> 
> 	if (xfs_sb_version_hascrc(&mp->m_sb)) {
> 		struct xfs_dir3_leaf_hdr *leaf3 = bp->b_addr;
> 
> 		ASSERT(leaf3->info.hdr.magic == leaf->hdr.info.magic);
> 		if (!uuid_equal(&leaf3->info.uuid, &mp->m_sb.sb_meta_uuid))
> 			return __this_address;
> 		if (be64_to_cpu(leaf3->info.blkno) != bp->b_bn)
> 			return __this_address;
> 		if (!xfs_log_check_lsn(mp, be64_to_cpu(leaf3->info.lsn)))
> 			return __this_address;
> 	}
> 
> The only thing they need is mp, &leaf->hdr, and bp. They don't
> actually need to know that its a dir2/dir3 leaf block now the magic
> number is encoded in bp->b_ops.
> 
> i.e. that boiler plate can be factored out of multiple verifiers...
> 

Ok, I thought you meant that there were other, existing functions being
shared rather than referring to a subset of the (modified) verifier
code. I'll take a closer look at this after the other fixups.

> > > Brian, to help prevent stupid people like me wasting your time in
> > > future, can you post the entire patch set you have so we can see the
> > > same picture you have for the overall change, even if there's only a
> > > small chunk you are proposing for merge? That way we'll be able to
> > > judge the change on the merits of the entire work, rather than just
> > > the small chunk that was posted? 
> > > 
> > 
> > That was the entire patchset at the time. ;) I intentionally made the
> > isolated finobt change and posted that to try and get big picture
> > feedback before making mechanical changes to the rest of the verifiers.
> > I probably had most of the rest done shortly after posting the rfcv2,
> > but it wasn't tested until today (re: the v1 post) so I just included
> > the above snippet to demonstrate the cleanup.
> 
> OK, so somewhat crossed wires while changes were still being made.
> Such is life...
> 

*nod*

Brian

> Cheers,
> 
> Dave.
> -- 
> Dave Chinner
> david@fromorbit.com