Re: [PATCH 01/16] xfs: sb verifier doesn't handle uncached sb buffer

["Darrick J. Wong" <djwong@kernel.org>]

On Wed, Jul 14, 2021 at 02:18:57PM +1000, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
> 
> The verifier checks explicitly for bp->b_bn == XFS_SB_DADDR to match
> the primary superblock buffer, but the primary superblock is an
> uncached buffer and so bp->b_bn is always -1ULL. Hence this never
> matches and the CRC error reporting is wholly dependent on the
> mount superblock already being populated so CRC feature checks pass
> and allow CRC errors to be reported.
> 
> Fix this so that the primary superblock CRC error reporting is not
> dependent on already having read the superblock into memory.
> 
> Signed-off-by: Dave Chinner <dchinner@redhat.com>
> ---
>  fs/xfs/libxfs/xfs_sb.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_sb.c b/fs/xfs/libxfs/xfs_sb.c
> index 04f5386446db..4a4586bd2ba2 100644
> --- a/fs/xfs/libxfs/xfs_sb.c
> +++ b/fs/xfs/libxfs/xfs_sb.c
> @@ -636,7 +636,7 @@ xfs_sb_read_verify(
>  
>  		if (!xfs_buf_verify_cksum(bp, XFS_SB_CRC_OFF)) {
>  			/* Only fail bad secondaries on a known V5 filesystem */
> -			if (bp->b_bn == XFS_SB_DADDR ||
> +			if (bp->b_maps[0].bm_bn == XFS_SB_DADDR ||

I did not know that b_bn only applies to cached buffers.

Would you mind ... I dunno, updating the comment in the struct xfs_buf
declaration to make this clearer?

	/*
	 * Block number of buffer, when this buffer is cached.  For
	 * uncached buffers, only the buffer map (i.e. b_maps[0].bm_bn)
	 * contains the block number.
	 */
	xfs_daddr_t		b_bn;

With that changed, this looks reasonable to me.
Reviewed-by: Darrick J. Wong <djwong@kernel.org>

--D

>  			    xfs_sb_version_hascrc(&mp->m_sb)) {
>  				error = -EFSBADCRC;
>  				goto out_error;
> -- 
> 2.31.1
>