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@xxxxxxxxxx> > --- > > 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@xxxxxxxxxx> (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 >
