On Mon, Dec 10, 2018 at 10:00:08AM -0600, Eric Sandeen wrote:
> On 12/7/18 7:37 AM, Brian Foster wrote:
> > On Wed, Dec 05, 2018 at 03:09:48PM -0600, Eric Sandeen wrote:
> >> Modify CRC checking functions to set __this_address into the
> >> verifier context failaddr vc->fa using new macro XFS_BADCRC_RETURN,
> >> and pass that to failure handlers as well.
> >>
> >> Signed-off-by: Eric Sandeen <sandeen@xxxxxxxxxx>
> >> ---
> >> fs/xfs/libxfs/xfs_alloc.c | 4 ++--
> >> fs/xfs/libxfs/xfs_alloc_btree.c | 2 +-
> >> fs/xfs/libxfs/xfs_attr_leaf.c | 2 +-
> >> fs/xfs/libxfs/xfs_bmap_btree.c | 2 +-
> >> fs/xfs/libxfs/xfs_cksum.h | 5 ++++-
> >> fs/xfs/libxfs/xfs_da_btree.c | 3 +--
> >> fs/xfs/libxfs/xfs_dir2_block.c | 2 +-
> >> fs/xfs/libxfs/xfs_dir2_data.c | 2 +-
> >> fs/xfs/libxfs/xfs_dir2_leaf.c | 2 +-
> >> fs/xfs/libxfs/xfs_dir2_node.c | 2 +-
> >> fs/xfs/libxfs/xfs_ialloc.c | 2 +-
> >> fs/xfs/libxfs/xfs_ialloc_btree.c | 2 +-
> >> fs/xfs/libxfs/xfs_refcount_btree.c | 2 +-
> >> fs/xfs/libxfs/xfs_rmap_btree.c | 2 +-
> >> fs/xfs/libxfs/xfs_symlink_remote.c | 2 +-
> >> fs/xfs/libxfs/xfs_types.h | 12 ++++++++++--
> >> fs/xfs/xfs_linux.h | 7 -------
> >> 17 files changed, 29 insertions(+), 26 deletions(-)
> >>
> > ...
> >> diff --git a/fs/xfs/libxfs/xfs_types.h b/fs/xfs/libxfs/xfs_types.h
> >> index 29b0d354d9b7..ab045e8dfcb9 100644
> >> --- a/fs/xfs/libxfs/xfs_types.h
> >> +++ b/fs/xfs/libxfs/xfs_types.h
> >> @@ -45,8 +45,16 @@ struct xfs_vc {
> >> xfs_failaddr_t fa;
> >> };
> >>
> >> -#define XFS_CORRUPTED_RETURN(vc) ({(vc)->fa = __this_address; false;})
> >> -#define XFS_VERIFIED_RETURN(vc) ({(vc)->fa = NULL; true;})
> >> +/*
> >> + * Return the address of a label. Use barrier() so that the optimizer
> >> + * won't reorder code to refactor the error jumpouts into a single
> >> + * return, which throws off the reported address.
> >> + */
> >> +#define __this_address ({ __label__ __here; __here: barrier(); &&__here; })
> >> +
> >
> > FYI, minor whitespace damage on the line above.
> >
> >> +#define XFS_CORRUPTED_RETURN(vc) ({(vc)->fa = __this_address; false;})
> >> +#define XFS_BADCRC_RETURN(vc) ({(vc)->fa = __this_address; false;})
> >> +#define XFS_VERIFIED_RETURN(vc) ({(vc)->fa = NULL; true;})
> >>
> >
> > A couple high level comments..
> >
> > I don't particularly care that much whether we bury function returns in
> > the macro or open-code it, but the macro naming suggests the former
> > (based on precedent of other such macros in XFS) while we implement the
> > latter. If there's objection to a return within a macro, perhaps a
> > reasonable compromise between this and the common pattern of having to
> > return on a separate line is to tweak the macros to never clobber an
> > existing error and update the verifiers to check/return failure state at
> > opportune points. For example:
> >
> > ...
> > if (!uuid_equal(&agfl->agfl_uuid, &mp->m_sb.sb_meta_uuid))
> > XFS_VC_CORRUPT(vc);
> > if (be32_to_cpu(agfl->agfl_magicnum) != XFS_AGFL_MAGIC)
> > XFS_VC_CORRUPT(vc);
> > if (bp->b_pag && be32_to_cpu(agfl->agfl_seqno) != bp->b_pag->pag_agno)
> > XFS_VC_CORRUPT(vc);
> > ...
> >
> > return vc->fa ? false : true;
> >
> > Of course, that assumes it's safe to keep checking the structure(s) as
> > such in the event of corruption, which perhaps is not ideal. Anyways, we
> > could also just return on a separate line or rename the macros. Just
> > thinking out loud a bit.
I would be fine with changing the name but leaving the return, e.g.:
#define XFS_VC_CORRUPT(vc) ({vc->fa = __this_address; false;})
if (!uuid_equal(&agfl->agfl_uuid, &mp->m_sb.sb_meta_uuid))
return XFS_VC_CORRUPT(vc);
Since I don't see much point in continuing once we've decided the
metadata is garbage. Scrub used to try to continue to find all the
errors in a metadata, but Dave had good reasons for shooting down that
strategy (unnecessary work, slows down the checker, potential for
straying into places we shouldn't).
> > I'm also a little curious why we have the need for the success macro at
> > all. I've only made a cursory pass at this point, but is there a
> > particular need to set anything in the xfs_vc at the point of a
> > successful return as opposed to just leaving the structure in the
> > initialized state?
>
> yeah, it's probably not necessary; it seemed consistent tho. There is some
> risk to this whole framework that failing to initialize a vc would cause
> problems that might be difficult to debug, and always marking success
> upon success seemed "safe." But you're right, not not necessary if it's
> properly initialized to success at the top of the call chain.
Yeah, how about a good static initializer so we don't have to open code
struct xfs_vc vc = { NULL }; everywhere?
--D
> -Eric
