xfs: compile time offset checks for common v4/v5 metadata
diff mbox series

Message ID 20190208152442.22862-1-bfoster@redhat.com
State Accepted
Headers show
Series
  • xfs: compile time offset checks for common v4/v5 metadata
Related show

Commit Message

Brian Foster Feb. 8, 2019, 3:24 p.m. UTC
The v5 superblock format added various metadata fields (such as crc,
metadata lsn, owner uuid, etc.) to v4 metadata headers or created
new v5 headers for blocks where no such headers existed on v4. Where
v4 headers did exist, the v5 structures are careful to place v4
metadata at the original location. For example, the magic value is
expected to be at the same location in certain blocks to facilitate
version detection.

While failure of this invariant is likely to cause severe and
obvious problems at runtime, we can detect this condition at compile
time via the more recently added on-disk format check
infrastructure. Since there is no runtime cost, add some offset
checks that start with v5 structure definitions, traverse down to
the first bit of common metadata with v4 and ensure that common
metadata is at the expected offset. Note that we don't care about
blocks which had no v4 header because there is no common metadata in
those cases. No functional changes.

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

Hi all,

This patch is inspired from debug checks that were initially in the
verifier magic fixup series. I originally had some assert checks to make
sure magic values were in the same location across v4 and v5 structures.
Darrick suggested that these were perhaps better suited as build time
offset checks. My subsequent attempt to convert the magic offset checks
resulted in somewhat confusing and convoluted build time checks.

As a result, I opted to remove this stuff from the verifier series to be
replaced with an independent patch that attempts to verify v5 structures
place v4 metadata at the expected location in general. This is not a
known problem or anything, just something that's easy to check for with
the on-disk format infrastructure already in place and helps validate
common code.

I think this covers all structures handled by the verifier series at
least (some are "internal" in that they are between the top-level v5
structs and v4 structs), but I could have missed some. Thoughts?

Brian

 fs/xfs/xfs_ondisk.h | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

Comments

Darrick J. Wong Feb. 8, 2019, 5:06 p.m. UTC | #1
On Fri, Feb 08, 2019 at 10:24:42AM -0500, Brian Foster wrote:
> The v5 superblock format added various metadata fields (such as crc,
> metadata lsn, owner uuid, etc.) to v4 metadata headers or created
> new v5 headers for blocks where no such headers existed on v4. Where
> v4 headers did exist, the v5 structures are careful to place v4
> metadata at the original location. For example, the magic value is
> expected to be at the same location in certain blocks to facilitate
> version detection.
> 
> While failure of this invariant is likely to cause severe and
> obvious problems at runtime, we can detect this condition at compile
> time via the more recently added on-disk format check
> infrastructure. Since there is no runtime cost, add some offset
> checks that start with v5 structure definitions, traverse down to
> the first bit of common metadata with v4 and ensure that common
> metadata is at the expected offset. Note that we don't care about
> blocks which had no v4 header because there is no common metadata in
> those cases. No functional changes.
> 
> Signed-off-by: Brian Foster <bfoster@redhat.com>
> ---
> 
> Hi all,
> 
> This patch is inspired from debug checks that were initially in the
> verifier magic fixup series. I originally had some assert checks to make
> sure magic values were in the same location across v4 and v5 structures.
> Darrick suggested that these were perhaps better suited as build time
> offset checks. My subsequent attempt to convert the magic offset checks
> resulted in somewhat confusing and convoluted build time checks.
> 
> As a result, I opted to remove this stuff from the verifier series to be
> replaced with an independent patch that attempts to verify v5 structures
> place v4 metadata at the expected location in general. This is not a
> known problem or anything, just something that's easy to check for with
> the on-disk format infrastructure already in place and helps validate
> common code.
> 
> I think this covers all structures handled by the verifier series at
> least (some are "internal" in that they are between the top-level v5
> structs and v4 structs), but I could have missed some. Thoughts?

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

(In my fantasyland we would have comprehensive build-time checks for
every field offset of every on-disk structure, but that's a totally
separate patchset.)

--D

> 
> Brian
> 
>  fs/xfs/xfs_ondisk.h | 21 +++++++++++++++++++++
>  1 file changed, 21 insertions(+)
> 
> diff --git a/fs/xfs/xfs_ondisk.h b/fs/xfs/xfs_ondisk.h
> index d3e04d20d8d4..c8ba98fae30a 100644
> --- a/fs/xfs/xfs_ondisk.h
> +++ b/fs/xfs/xfs_ondisk.h
> @@ -125,6 +125,27 @@ xfs_check_ondisk_structs(void)
>  	XFS_CHECK_STRUCT_SIZE(struct xfs_inode_log_format,	56);
>  	XFS_CHECK_STRUCT_SIZE(struct xfs_qoff_logformat,	20);
>  	XFS_CHECK_STRUCT_SIZE(struct xfs_trans_header,		16);
> +
> +	/*
> +	 * The v5 superblock format extended several v4 header structures with
> +	 * additional data. While new fields are only accessible on v5
> +	 * superblocks, it's important that the v5 structures place original v4
> +	 * fields/headers in the correct location on-disk. For example, we must
> +	 * be able to find magic values at the same location in certain blocks
> +	 * regardless of superblock version.
> +	 *
> +	 * The following checks ensure that various v5 data structures place the
> +	 * subset of v4 metadata associated with the same type of block at the
> +	 * start of the on-disk block. If there is no data structure definition
> +	 * for certain types of v4 blocks, traverse down to the first field of
> +	 * common metadata (e.g., magic value) and make sure it is at offset
> +	 * zero.
> +	 */
> +	XFS_CHECK_OFFSET(struct xfs_dir3_leaf, hdr.info.hdr,	0);
> +	XFS_CHECK_OFFSET(struct xfs_da3_intnode, hdr.info.hdr,	0);
> +	XFS_CHECK_OFFSET(struct xfs_dir3_data_hdr, hdr.magic,	0);
> +	XFS_CHECK_OFFSET(struct xfs_dir3_free, hdr.hdr.magic,	0);
> +	XFS_CHECK_OFFSET(struct xfs_attr3_leafblock, hdr.info.hdr, 0);
>  }
>  
>  #endif /* __XFS_ONDISK_H */
> -- 
> 2.17.2
>
Christoph Hellwig Feb. 11, 2019, 8:10 a.m. UTC | #2
Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

Patch
diff mbox series

diff --git a/fs/xfs/xfs_ondisk.h b/fs/xfs/xfs_ondisk.h
index d3e04d20d8d4..c8ba98fae30a 100644
--- a/fs/xfs/xfs_ondisk.h
+++ b/fs/xfs/xfs_ondisk.h
@@ -125,6 +125,27 @@  xfs_check_ondisk_structs(void)
 	XFS_CHECK_STRUCT_SIZE(struct xfs_inode_log_format,	56);
 	XFS_CHECK_STRUCT_SIZE(struct xfs_qoff_logformat,	20);
 	XFS_CHECK_STRUCT_SIZE(struct xfs_trans_header,		16);
+
+	/*
+	 * The v5 superblock format extended several v4 header structures with
+	 * additional data. While new fields are only accessible on v5
+	 * superblocks, it's important that the v5 structures place original v4
+	 * fields/headers in the correct location on-disk. For example, we must
+	 * be able to find magic values at the same location in certain blocks
+	 * regardless of superblock version.
+	 *
+	 * The following checks ensure that various v5 data structures place the
+	 * subset of v4 metadata associated with the same type of block at the
+	 * start of the on-disk block. If there is no data structure definition
+	 * for certain types of v4 blocks, traverse down to the first field of
+	 * common metadata (e.g., magic value) and make sure it is at offset
+	 * zero.
+	 */
+	XFS_CHECK_OFFSET(struct xfs_dir3_leaf, hdr.info.hdr,	0);
+	XFS_CHECK_OFFSET(struct xfs_da3_intnode, hdr.info.hdr,	0);
+	XFS_CHECK_OFFSET(struct xfs_dir3_data_hdr, hdr.magic,	0);
+	XFS_CHECK_OFFSET(struct xfs_dir3_free, hdr.hdr.magic,	0);
+	XFS_CHECK_OFFSET(struct xfs_attr3_leafblock, hdr.info.hdr, 0);
 }
 
 #endif /* __XFS_ONDISK_H */