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@xxxxxxxxxxxxx
