Re: [PATCH RFC v2 2/3] xfs: distinguish between inobt and finobt magic values

[Brian Foster <bfoster@redhat.com>]

On Wed, Jan 30, 2019 at 01:15:29PM +1100, Dave Chinner wrote:
> On Tue, Jan 29, 2019 at 08:05:53PM -0500, Brian Foster wrote:
> > On Wed, Jan 30, 2019 at 08:16:55AM +1100, Dave Chinner wrote:
> > > On Tue, Jan 29, 2019 at 09:01:36AM -0500, Brian Foster wrote:
> > > > On Tue, Jan 29, 2019 at 09:54:26AM +1100, Dave Chinner wrote:
> > > > I agree that the magic value itself is a bit obfuscated with this
> > > > change, but that's still the case with a lookup table.
> > > 
> > > The difference with the lookup table is that you know what the magic
> > > number is supposed to be by looking at the code that calls it...
> > > 
> > 
> > Indeed. What I didn't realize until later today is that some verifiers
> > (xfs_sb_buf_ops, xfs_attr3_leaf_buf_ops, xfs_da3_node_buf_ops) check
> > already converted in-core structures and thus actually verify against
> > cpu endian magic values. This means said verifiers would require further
> > tweaks to either check the underlying buffer, another conversion back to
> > disk endian, or we'd otherwise need four of these arrays. :/
> 
> That was purely convenience, because we had to convert to the incore
> header to check a bunch of other stuff, so the magic number got
> converted for free.
> 

I think that applies to the first two cases noted above. The
xfs_da3_node_verify() case is a bit more involved conceptually because
we call out to another indirect function to do the conversion. I think
we can ultimately use hdr for the magic check just the same as the
others because either way the block is headed by an xfs_da_blkinfo, it
just takes some thought to grok from the verifier context (and thus adds
minor maintenance burden if this code changes again down the road). I'll
try to add a comment there..

> I'd prefer if we are going to use a generic method of checking magic
> numbers that it does it in on-disk format so that we don't need to
> convert just for the magic number check.
> 
> > > I'd like all the verifiers to use the same mechanism so we maintain
> > > consistency between them.
> > > 
> > 
> > I'd like that too, but I think we need to make some kind of tradeoff or
> > compromise to fix this problem given the current, rather ad-hoc nature
> > of the verifier code. Some check in-core structs, some don't and may or
> > may not use the compile time conversion optimization.
> 
> Ypup, so lets get them all on to checking the on-disk magic number
> before conversion.
> 
> > > > --- 8< ---
> > > > 
> > > > diff --git a/fs/xfs/libxfs/xfs_dir2_leaf.c b/fs/xfs/libxfs/xfs_dir2_leaf.c
> > > > index 1728a3e6f5cf..f602307d2fa0 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, be16_to_cpu(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);
> > > >  		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);
> > > >  }
> > > 
> > > .....
> > > 
> > > Ok, that removes a lot more existing code than I ever thought it
> > > would. If you clean up the macro mess and use encoded magic numbers
> > > in the ops structure, then consider my objections removed. :)
> > > 
> > 
> > I'll kill off the macro..
> > 
> > By encoded, I assume you mean on-disk order(?).
> 
> Yup.
> 
> > > (And that then leads to factoring of xfs_dablk_info_verify() as dir
> > > leaf, danode and attribute leaf blocks all use the same struct
> > > xfs_da3_blkinfo header, and now the magic number is abstracted they
> > > can use the same code....)
> > > 
> > 
> > Not sure I follow..?
> 
> They all do the same thing. Taking your converted code:
> 
> 	if (!xfs_verify_magic(bp, be16_to_cpu(leaf->hdr.info.magic)))
> 		return __this_address;
> 
> 	if (xfs_sb_version_hascrc(&mp->m_sb)) {
> 		struct xfs_dir3_leaf_hdr *leaf3 = bp->b_addr;
> 
> 		ASSERT(leaf3->info.hdr.magic == leaf->hdr.info.magic);
> 		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;
> 	}
> 
> The only thing they need is mp, &leaf->hdr, and bp. They don't
> actually need to know that its a dir2/dir3 leaf block now the magic
> number is encoded in bp->b_ops.
> 
> i.e. that boiler plate can be factored out of multiple verifiers...
> 

Ok, I thought you meant that there were other, existing functions being
shared rather than referring to a subset of the (modified) verifier
code. I'll take a closer look at this after the other fixups.

> > > Brian, to help prevent stupid people like me wasting your time in
> > > future, can you post the entire patch set you have so we can see the
> > > same picture you have for the overall change, even if there's only a
> > > small chunk you are proposing for merge? That way we'll be able to
> > > judge the change on the merits of the entire work, rather than just
> > > the small chunk that was posted? 
> > > 
> > 
> > That was the entire patchset at the time. ;) I intentionally made the
> > isolated finobt change and posted that to try and get big picture
> > feedback before making mechanical changes to the rest of the verifiers.
> > I probably had most of the rest done shortly after posting the rfcv2,
> > but it wasn't tested until today (re: the v1 post) so I just included
> > the above snippet to demonstrate the cleanup.
> 
> OK, so somewhat crossed wires while changes were still being made.
> Such is life...
> 

*nod*

Brian

> Cheers,
> 
> Dave.
> -- 
> Dave Chinner
> david@fromorbit.com