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'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?
Brian
> /*
> * Null values for the types.
> diff --git a/fs/xfs/xfs_linux.h b/fs/xfs/xfs_linux.h
> index edbd5a210df2..4141e70bb3fa 100644
> --- a/fs/xfs/xfs_linux.h
> +++ b/fs/xfs/xfs_linux.h
> @@ -131,13 +131,6 @@ typedef __u32 xfs_nlink_t;
> #define SYNCHRONIZE() barrier()
> #define __return_address __builtin_return_address(0)
>
> -/*
> - * 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; })
> -
> #define XFS_PROJID_DEFAULT 0
>
> #define howmany(x, y) (((x)+((y)-1))/(y))
> --
> 2.17.0
>
>
