On Fri, May 01, 2020 at 10:14:21AM +0200, Christoph Hellwig wrote:
> The split between xfs_inode_verify_forks and the two helpers
> implementing the actual functionality is a little strange. Reshuffle
> it so that xfs_inode_verify_forks verifies if the data and attr forks
> are actually in local format and only call the low-level helpers if
> that is the case. Handle the actual error reporting in the low-level
> handlers to streamline the caller.
>
> Signed-off-by: Christoph Hellwig <hch@xxxxxx>
> ---
> fs/xfs/libxfs/xfs_inode_fork.c | 47 ++++++++++++++++++++++------------
> fs/xfs/libxfs/xfs_inode_fork.h | 4 +--
> fs/xfs/xfs_inode.c | 21 +++------------
> 3 files changed, 37 insertions(+), 35 deletions(-)
>
> diff --git a/fs/xfs/libxfs/xfs_inode_fork.c b/fs/xfs/libxfs/xfs_inode_fork.c
> index f6dcee919f59e..7e129ed2f345f 100644
> --- a/fs/xfs/libxfs/xfs_inode_fork.c
> +++ b/fs/xfs/libxfs/xfs_inode_fork.c
> @@ -674,34 +674,49 @@ xfs_ifork_init_cow(
> }
>
...
>
> /* Verify the inline contents of the attr fork of an inode. */
> -xfs_failaddr_t
> -xfs_ifork_verify_attr(
> +int
> +xfs_ifork_verify_local_attr(
> struct xfs_inode *ip)
> {
> - /* There has to be an attr fork allocated if aformat is local. */
> - if (ip->i_d.di_aformat != XFS_DINODE_FMT_LOCAL)
> - return NULL;
> + xfs_failaddr_t fa;
> +
> if (!XFS_IFORK_PTR(ip, XFS_ATTR_FORK))
> - return __this_address;
> - return xfs_attr_shortform_verify(ip);
> + fa = __this_address;
> + else
> + fa = xfs_attr_shortform_verify(ip);
> +
> + if (fa) {
> + xfs_inode_verifier_error(ip, -EFSCORRUPTED, "attr fork",
> + ip->i_afp->if_u1.if_data, ip->i_afp->if_bytes, fa);
> + return -EFSCORRUPTED;
This explicitly makes !ip->i_afp one of the handled corruption cases for
XFS_DINODE_FMT_LOCAL, but then attempts to access it anyways. Otherwise
seems Ok modulo the comments on the previous patch...
Brian
> + }
> +
> + return 0;
> }
> diff --git a/fs/xfs/libxfs/xfs_inode_fork.h b/fs/xfs/libxfs/xfs_inode_fork.h
> index 3f84d33abd3b7..f46a8c1db5964 100644
> --- a/fs/xfs/libxfs/xfs_inode_fork.h
> +++ b/fs/xfs/libxfs/xfs_inode_fork.h
> @@ -176,7 +176,7 @@ extern struct kmem_zone *xfs_ifork_zone;
>
> extern void xfs_ifork_init_cow(struct xfs_inode *ip);
>
> -xfs_failaddr_t xfs_ifork_verify_data(struct xfs_inode *ip);
> -xfs_failaddr_t xfs_ifork_verify_attr(struct xfs_inode *ip);
> +int xfs_ifork_verify_local_data(struct xfs_inode *ip);
> +int xfs_ifork_verify_local_attr(struct xfs_inode *ip);
>
> #endif /* __XFS_INODE_FORK_H__ */
> diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
> index 93967278355de..2ec7789317133 100644
> --- a/fs/xfs/xfs_inode.c
> +++ b/fs/xfs/xfs_inode.c
> @@ -3766,25 +3766,12 @@ bool
> xfs_inode_verify_forks(
> struct xfs_inode *ip)
> {
> - struct xfs_ifork *ifp;
> - xfs_failaddr_t fa;
> -
> - fa = xfs_ifork_verify_data(ip);
> - if (fa) {
> - ifp = XFS_IFORK_PTR(ip, XFS_DATA_FORK);
> - xfs_inode_verifier_error(ip, -EFSCORRUPTED, "data fork",
> - ifp->if_u1.if_data, ifp->if_bytes, fa);
> + if (ip->i_d.di_format == XFS_DINODE_FMT_LOCAL &&
> + xfs_ifork_verify_local_data(ip))
> return false;
> - }
> -
> - fa = xfs_ifork_verify_attr(ip);
> - if (fa) {
> - ifp = XFS_IFORK_PTR(ip, XFS_ATTR_FORK);
> - xfs_inode_verifier_error(ip, -EFSCORRUPTED, "attr fork",
> - ifp ? ifp->if_u1.if_data : NULL,
> - ifp ? ifp->if_bytes : 0, fa);
> + if (ip->i_d.di_aformat == XFS_DINODE_FMT_LOCAL &&
> + xfs_ifork_verify_local_attr(ip))
> return false;
> - }
> return true;
> }
>
> --
> 2.26.2
>
