[v3,6/9] xfs: distinguish between bnobt and cntbt magic values
diff mbox series

Message ID 20190204145231.47034-7-bfoster@redhat.com
State New
Headers show
Series
  • xfs: fix [f]inobt magic value verification
Related show

Commit Message

Brian Foster Feb. 4, 2019, 2:52 p.m. UTC
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>
---
 fs/xfs/libxfs/xfs_alloc_btree.c | 60 ++++++++++++++-------------------
 1 file changed, 25 insertions(+), 35 deletions(-)

Comments

Darrick J. Wong Feb. 6, 2019, 6:15 p.m. UTC | #1
On Mon, Feb 04, 2019 at 09:52:28AM -0500, Brian Foster wrote:
> 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>

Looks ok,
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>

--D

> ---
>  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
diff mbox series

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,