Re: [PATCH v3 7/9] xfs: use verifier magic field in dir2 leaf verifiers

[Brian Foster <bfoster@redhat.com>]

On Wed, Feb 06, 2019 at 11:03:43AM -0800, Darrick J. Wong wrote:
> On Wed, Feb 06, 2019 at 01:41:22PM -0500, Brian Foster wrote:
> > On Wed, Feb 06, 2019 at 09:45:26AM -0800, Darrick J. Wong wrote:
> > > On Mon, Feb 04, 2019 at 09:52:29AM -0500, Brian Foster wrote:
> > > > The dir2 leaf verifiers share the same underlying structure
> > > > verification code, but implement six accessor functions to multiplex
> > > > the code across the two verifiers. Further, the magic value isn't
> > > > sufficiently abstracted such that the common helper has to manually
> > > > fix up the magic from the caller on v5 filesystems.
> > > > 
> > > > Use the magic field in the verifier structure to eliminate the
> > > > duplicate code and clean this all up. No functional change.
> > > > 
> > > > Signed-off-by: Brian Foster <bfoster@redhat.com>
> > > > ---
> > > >  fs/xfs/libxfs/xfs_dir2_leaf.c | 88 ++++++++---------------------------
> > > >  1 file changed, 20 insertions(+), 68 deletions(-)
> > > > 
> > > > diff --git a/fs/xfs/libxfs/xfs_dir2_leaf.c b/fs/xfs/libxfs/xfs_dir2_leaf.c
> > > > index 1728a3e6f5cf..a99ae2cd292e 100644
> > > > --- a/fs/xfs/libxfs/xfs_dir2_leaf.c
> > > > +++ b/fs/xfs/libxfs/xfs_dir2_leaf.c
> > > > @@ -142,41 +142,32 @@ xfs_dir3_leaf_check_int(
> > > >   */
> > > >  static xfs_failaddr_t
> > > >  xfs_dir3_leaf_verify(
> > > > -	struct xfs_buf		*bp,
> > > > -	uint16_t		magic)
> > > > +	struct xfs_buf		*bp)
> > > >  {
> > > >  	struct xfs_mount	*mp = bp->b_target->bt_mount;
> > > >  	struct xfs_dir2_leaf	*leaf = bp->b_addr;
> > > >  
> > > > -	ASSERT(magic == XFS_DIR2_LEAF1_MAGIC || magic == XFS_DIR2_LEAFN_MAGIC);
> > > > +	if (!xfs_verify_magic(bp, leaf->hdr.info.magic))
> > > > +		return __this_address;
> > > >  
> > > >  	if (xfs_sb_version_hascrc(&mp->m_sb)) {
> > > >  		struct xfs_dir3_leaf_hdr *leaf3 = bp->b_addr;
> > > > -		uint16_t		magic3;
> > > >  
> > > > -		magic3 = (magic == XFS_DIR2_LEAF1_MAGIC) ? XFS_DIR3_LEAF1_MAGIC
> > > > -							 : XFS_DIR3_LEAFN_MAGIC;
> > > > -
> > > > -		if (leaf3->info.hdr.magic != cpu_to_be16(magic3))
> > > > -			return __this_address;
> > > > +		ASSERT(leaf3->info.hdr.magic == leaf->hdr.info.magic);
> > > 
> > > leaf2 and leaf3 directory block headers are supposed to have the magic
> > > at the same offset in the buffer, right?  When would this assert fail?
> > > 
> > 
> > Hopefully never.. ;P I added the assert as a mechanical defense measure
> > simply because these are technically different structures and this
> > refactoring dictates that we access the magic value through one of the
> > two rather than both independently. I just wanted to make sure that this
> > dependency was encoded somewhere because it's not obvious in the code.
> 
> <nod>
> 
> > > Could we replace this with a BUILD_BUG_ON check of the leaf2/leaf3 magic
> > > number buffer offset in xfs_ondisk.h?
> > > 
> > 
> > BUILD_BUG_ON() probably makes more sense for this than an assert in
> > principle. Is there a clean enough way to encode the offset checks
> > through multiple structures though? We could do something with NULL
> > pointers:
> > 
> > 	BUILD_BUG_ON(&((struct xfs_da3_blkinfo *)NULL)->hdr.magic !=
> > 		     &((struct xfs_da_blkinfo *)NULL)->magic);
> > 
> > ... to check the offsets, but that's ugly. I'm not sure manually adding
> > up the offsetof() results is any better. That said, after this code is
> > refactored by the last patch this particular instance could probably be
> > reduced to a simple:
> > 
> > 	BUILD_BUG_ON(offsetof(struct xfs_da3_blkinfo, hdr) != 0);
> > 
> > ... in xfs_da3_blkinfo_verify(). So perhaps the right approach is just
> > to add a separate BUILD_BUG_ON() for each such layer in the
> > encapsulating structures. I'll take a closer look at that and see how
> > far I get.
> 
> <nod> afaict offsetof can look through substructures, since it all just
> turns into a bunch of pointer arithmetic goop.
> 
> > Also, any particular reason to put it in xfs_ondisk.h vs. where the
> > asserts currently are?
> 
> Hmm... we established xfs_ondisk.h to contain all the offset and
> structure size checks so that they'd all be in one place instead of
> scattered around in the verifiers and whatnot.  The macro soup emits
> prettier diagnostic data in case someone on an unfamiliar arch hits a
> build error and reports it.  So if we add this to xfs_ondisk.h:
> 
> XFS_CHECK_OFFSET(struct xfs_dir3_leaf_hdr, info.hdr.magic, 8);
> XFS_CHECK_OFFSET(struct xfs_dir2_leaf_hdr, info.magic, 8);
> 
> Then the compiler will emit:
> 
> include/linux/compiler.h:344:38: error: call to
> ‘__compiletime_assert_58’ declared with attribute error: XFS:
> offsetof(struct xfs_dir3_leaf_hdr, info.hdr.magic) is wrong, expected 8
> 
> I guess the strongest argument I have for xfs_ondisk.h is because
> "that's where all the other offset and size build checks go".
> 

Ok. Maybe it's just simplest to hardcode the relevant offsets like this.
A comment that explains that the verifier magic checks are the primary
motivation for the magic-specific offset checks mostly addresses my
concern over context. I'll give that a shot.

> > To me this is more context for the verifier code
> > than some broader requirement (since we're explicitly checking the magic
> > field), but maybe there are broader header alignment/offset expectations
> > elsewhere too.
> 
> Heh, I think this is a hair-splitting thing: are we checking that the
> offsets are the same (which indeed is a relational thing that ought to
> be in the verifier), or checking that ttwo offsets equal some value,
> where the value just happens to be the same number?
> 

The intent was really to check the offsets. The implementation did that
indirectly/hackily via the value comparison.

Brian

> --D
> 
> > Brian
> > 
> > > --D
> > > 
> > > >  		if (!uuid_equal(&leaf3->info.uuid, &mp->m_sb.sb_meta_uuid))
> > > >  			return __this_address;
> > > >  		if (be64_to_cpu(leaf3->info.blkno) != bp->b_bn)
> > > >  			return __this_address;
> > > >  		if (!xfs_log_check_lsn(mp, be64_to_cpu(leaf3->info.lsn)))
> > > >  			return __this_address;
> > > > -	} else {
> > > > -		if (leaf->hdr.info.magic != cpu_to_be16(magic))
> > > > -			return __this_address;
> > > >  	}
> > > >  
> > > >  	return xfs_dir3_leaf_check_int(mp, NULL, NULL, leaf);
> > > >  }
> > > >  
> > > >  static void
> > > > -__read_verify(
> > > > -	struct xfs_buf  *bp,
> > > > -	uint16_t	magic)
> > > > +xfs_dir3_leaf_read_verify(
> > > > +	struct xfs_buf  *bp)
> > > >  {
> > > >  	struct xfs_mount	*mp = bp->b_target->bt_mount;
> > > >  	xfs_failaddr_t		fa;
> > > > @@ -185,23 +176,22 @@ __read_verify(
> > > >  	     !xfs_buf_verify_cksum(bp, XFS_DIR3_LEAF_CRC_OFF))
> > > >  		xfs_verifier_error(bp, -EFSBADCRC, __this_address);
> > > >  	else {
> > > > -		fa = xfs_dir3_leaf_verify(bp, magic);
> > > > +		fa = xfs_dir3_leaf_verify(bp);
> > > >  		if (fa)
> > > >  			xfs_verifier_error(bp, -EFSCORRUPTED, fa);
> > > >  	}
> > > >  }
> > > >  
> > > >  static void
> > > > -__write_verify(
> > > > -	struct xfs_buf  *bp,
> > > > -	uint16_t	magic)
> > > > +xfs_dir3_leaf_write_verify(
> > > > +	struct xfs_buf  *bp)
> > > >  {
> > > >  	struct xfs_mount	*mp = bp->b_target->bt_mount;
> > > >  	struct xfs_buf_log_item	*bip = bp->b_log_item;
> > > >  	struct xfs_dir3_leaf_hdr *hdr3 = bp->b_addr;
> > > >  	xfs_failaddr_t		fa;
> > > >  
> > > > -	fa = xfs_dir3_leaf_verify(bp, magic);
> > > > +	fa = xfs_dir3_leaf_verify(bp);
> > > >  	if (fa) {
> > > >  		xfs_verifier_error(bp, -EFSCORRUPTED, fa);
> > > >  		return;
> > > > @@ -216,60 +206,22 @@ __write_verify(
> > > >  	xfs_buf_update_cksum(bp, XFS_DIR3_LEAF_CRC_OFF);
> > > >  }
> > > >  
> > > > -static xfs_failaddr_t
> > > > -xfs_dir3_leaf1_verify(
> > > > -	struct xfs_buf	*bp)
> > > > -{
> > > > -	return xfs_dir3_leaf_verify(bp, XFS_DIR2_LEAF1_MAGIC);
> > > > -}
> > > > -
> > > > -static void
> > > > -xfs_dir3_leaf1_read_verify(
> > > > -	struct xfs_buf	*bp)
> > > > -{
> > > > -	__read_verify(bp, XFS_DIR2_LEAF1_MAGIC);
> > > > -}
> > > > -
> > > > -static void
> > > > -xfs_dir3_leaf1_write_verify(
> > > > -	struct xfs_buf	*bp)
> > > > -{
> > > > -	__write_verify(bp, XFS_DIR2_LEAF1_MAGIC);
> > > > -}
> > > > -
> > > > -static xfs_failaddr_t
> > > > -xfs_dir3_leafn_verify(
> > > > -	struct xfs_buf	*bp)
> > > > -{
> > > > -	return xfs_dir3_leaf_verify(bp, XFS_DIR2_LEAFN_MAGIC);
> > > > -}
> > > > -
> > > > -static void
> > > > -xfs_dir3_leafn_read_verify(
> > > > -	struct xfs_buf	*bp)
> > > > -{
> > > > -	__read_verify(bp, XFS_DIR2_LEAFN_MAGIC);
> > > > -}
> > > > -
> > > > -static void
> > > > -xfs_dir3_leafn_write_verify(
> > > > -	struct xfs_buf	*bp)
> > > > -{
> > > > -	__write_verify(bp, XFS_DIR2_LEAFN_MAGIC);
> > > > -}
> > > > -
> > > >  const struct xfs_buf_ops xfs_dir3_leaf1_buf_ops = {
> > > >  	.name = "xfs_dir3_leaf1",
> > > > -	.verify_read = xfs_dir3_leaf1_read_verify,
> > > > -	.verify_write = xfs_dir3_leaf1_write_verify,
> > > > -	.verify_struct = xfs_dir3_leaf1_verify,
> > > > +	.magic = { cpu_to_be16(XFS_DIR2_LEAF1_MAGIC),
> > > > +		   cpu_to_be16(XFS_DIR3_LEAF1_MAGIC) },
> > > > +	.verify_read = xfs_dir3_leaf_read_verify,
> > > > +	.verify_write = xfs_dir3_leaf_write_verify,
> > > > +	.verify_struct = xfs_dir3_leaf_verify,
> > > >  };
> > > >  
> > > >  const struct xfs_buf_ops xfs_dir3_leafn_buf_ops = {
> > > >  	.name = "xfs_dir3_leafn",
> > > > -	.verify_read = xfs_dir3_leafn_read_verify,
> > > > -	.verify_write = xfs_dir3_leafn_write_verify,
> > > > -	.verify_struct = xfs_dir3_leafn_verify,
> > > > +	.magic = { cpu_to_be16(XFS_DIR2_LEAFN_MAGIC),
> > > > +		   cpu_to_be16(XFS_DIR3_LEAFN_MAGIC) },
> > > > +	.verify_read = xfs_dir3_leaf_read_verify,
> > > > +	.verify_write = xfs_dir3_leaf_write_verify,
> > > > +	.verify_struct = xfs_dir3_leaf_verify,
> > > >  };
> > > >  
> > > >  int
> > > > -- 
> > > > 2.17.2
> > > >