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

[Dave Chinner <david@fromorbit.com>]

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