Re: [RFC PATCH] xfs: consolidate local format inode fork verifiers

[Brian Foster <bfoster@redhat.com>]

On Wed, Jul 19, 2017 at 09:03:18AM -0700, Darrick J. Wong wrote:
> Create a centralized function to verify local format inode forks, then
> migrate the existing short format directory verifier to use this
> framework and add verifiers for the other two things we support (attrs
> and symlinks).
> 
> Obviously this will get broken up into smaller pieces (one patch to
> refactor/move the existing verifier calls, another one to add the two
> new verifiers), but what do people think?
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---

I haven't combed through the details of the verifiers, but just a few
very quick thoughts..

>  fs/xfs/libxfs/xfs_attr_leaf.c      |   70 ++++++++++++++++++++++++++++++++++++
>  fs/xfs/libxfs/xfs_attr_leaf.h      |    1 +
>  fs/xfs/libxfs/xfs_inode_fork.c     |   10 -----
>  fs/xfs/libxfs/xfs_shared.h         |    1 +
>  fs/xfs/libxfs/xfs_symlink_remote.c |   29 +++++++++++++++
>  fs/xfs/xfs_icache.c                |   46 ++++++++++++++++++++++++
>  fs/xfs/xfs_icache.h                |    1 +
>  fs/xfs/xfs_inode.c                 |    6 +--
>  8 files changed, 150 insertions(+), 14 deletions(-)
> 
...
> diff --git a/fs/xfs/libxfs/xfs_inode_fork.c b/fs/xfs/libxfs/xfs_inode_fork.c
> index 0e80f34..5484211 100644
> --- a/fs/xfs/libxfs/xfs_inode_fork.c
> +++ b/fs/xfs/libxfs/xfs_inode_fork.c
> @@ -183,16 +183,6 @@ xfs_iformat_fork(
>  	if (error)
>  		return error;
>  
> -	/* Check inline dir contents. */
> -	if (S_ISDIR(VFS_I(ip)->i_mode) &&
> -	    dip->di_format == XFS_DINODE_FMT_LOCAL) {
> -		error = xfs_dir2_sf_verify(ip);
> -		if (error) {
> -			xfs_idestroy_fork(ip, XFS_DATA_FORK);
> -			return error;
> -		}
> -	}
> -

What's the purpose of moving this? Just to accommodate userspace? As it
is, it looks like this leaves a coverage gap in a log recovery case
where xfs_iget() is not used (xfs_recover_inode_owner_change()).

Also, I think this is best split up into two or three smaller patches.
E.g., one to move/rename the existing mechanism (with justification as
to why), one or more to add the additional verifiers for the other
formats.

>  	if (xfs_is_reflink_inode(ip)) {
>  		ASSERT(ip->i_cowfp == NULL);
>  		xfs_ifork_init_cow(ip);
...
> diff --git a/fs/xfs/libxfs/xfs_symlink_remote.c b/fs/xfs/libxfs/xfs_symlink_remote.c
> index c484877..96e2957 100644
> --- a/fs/xfs/libxfs/xfs_symlink_remote.c
> +++ b/fs/xfs/libxfs/xfs_symlink_remote.c
> @@ -207,3 +207,32 @@ xfs_symlink_local_to_remote(
>  	xfs_trans_log_buf(tp, bp, 0, sizeof(struct xfs_dsymlink_hdr) +
>  					ifp->if_bytes - 1);
>  }
> +
> +/* Verify the consistency of an inline symlink. */
> +int
> +xfs_symlink_sf_verify(
> +	struct xfs_inode	*ip)
> +{
> +	char			*sfp;
> +	char			*endp;
> +	struct xfs_ifork	*ifp;
> +	int			size;
> +
> +	ASSERT(ip->i_d.di_format == XFS_DINODE_FMT_LOCAL);
> +	ifp = XFS_IFORK_PTR(ip, XFS_DATA_FORK);
> +	sfp = (char *)ifp->if_u1.if_data;
> +	size = ifp->if_bytes;
> +	endp = sfp + size;
> +
> +	/* No negative sizes or overly long symlink targets. */
> +	if (size < 0 || size > XFS_SYMLINK_MAXLEN)
> +		return -EFSCORRUPTED;
> +
> +	/* No NULLs in the target either. */
> +	for (; sfp < endp; sfp++) {
> +		if (*sfp == 0)
> +			return -EFSCORRUPTED;
> +	}

	memchr() ?

Brian

> +
> +	return 0;
> +}
> diff --git a/fs/xfs/xfs_icache.c b/fs/xfs/xfs_icache.c
> index 0a9e698..fd1d430 100644
> --- a/fs/xfs/xfs_icache.c
> +++ b/fs/xfs/xfs_icache.c
> @@ -34,6 +34,11 @@
>  #include "xfs_dquot_item.h"
>  #include "xfs_dquot.h"
>  #include "xfs_reflink.h"
> +#include "xfs_da_format.h"
> +#include "xfs_da_btree.h"
> +#include "xfs_dir2_priv.h"
> +#include "xfs_attr_leaf.h"
> +#include "xfs_shared.h"
>  
>  #include <linux/kthread.h>
>  #include <linux/freezer.h>
> @@ -449,6 +454,43 @@ xfs_iget_cache_hit(
>  	return error;
>  }
>  
> +/* Check inline fork contents. */
> +int
> +xfs_iget_verify_forks(
> +	struct xfs_inode		*ip)
> +{
> +	int				error;
> +
> +	/* Check the attribute fork if there is one. */
> +	if (XFS_IFORK_PTR(ip, XFS_ATTR_FORK) &&
> +	    ip->i_d.di_aformat == XFS_DINODE_FMT_LOCAL) {
> +		error = xfs_attr_shortform_verify(ip);
> +		if (error) {
> +			xfs_alert(ip->i_mount,
> +					"%s: bad inode %Lu inline attr fork",
> +					__func__, ip->i_ino);
> +			return error;
> +		}
> +	}
> +
> +	/* Non-local data fork, jump out. */
> +	if (ip->i_d.di_format != XFS_DINODE_FMT_LOCAL)
> +		return 0;
> +
> +	/* Check the inline data fork if there is one. */
> +	if (S_ISDIR(VFS_I(ip)->i_mode))
> +		error = xfs_dir2_sf_verify(ip);
> +	else if (S_ISLNK(VFS_I(ip)->i_mode))
> +		error = xfs_symlink_sf_verify(ip);
> +	else
> +		error = -EFSCORRUPTED;
> +
> +	if (error)
> +		xfs_alert(ip->i_mount, "%s: bad inode %Lu inline data fork",
> +				__func__, ip->i_ino);
> +	return error;
> +}
> +
>  
>  static int
>  xfs_iget_cache_miss(
> @@ -473,6 +515,10 @@ xfs_iget_cache_miss(
>  	if (error)
>  		goto out_destroy;
>  
> +	error = xfs_iget_verify_forks(ip);
> +	if (error)
> +		goto out_destroy;
> +
>  	trace_xfs_iget_miss(ip);
>  
>  	if ((VFS_I(ip)->i_mode == 0) && !(flags & XFS_IGET_CREATE)) {
> diff --git a/fs/xfs/xfs_icache.h b/fs/xfs/xfs_icache.h
> index bff4d85..abf1cff 100644
> --- a/fs/xfs/xfs_icache.h
> +++ b/fs/xfs/xfs_icache.h
> @@ -129,5 +129,6 @@ xfs_fs_eofblocks_from_user(
>  
>  int xfs_icache_inode_is_allocated(struct xfs_mount *mp, struct xfs_trans *tp,
>  				  xfs_ino_t ino, bool *inuse);
> +int xfs_iget_verify_forks(struct xfs_inode *ip);
>  
>  #endif
> diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
> index ceef77c..7ca8336 100644
> --- a/fs/xfs/xfs_inode.c
> +++ b/fs/xfs/xfs_inode.c
> @@ -3547,10 +3547,8 @@ xfs_iflush_int(
>  	if (ip->i_d.di_version < 3)
>  		ip->i_d.di_flushiter++;
>  
> -	/* Check the inline directory data. */
> -	if (S_ISDIR(VFS_I(ip)->i_mode) &&
> -	    ip->i_d.di_format == XFS_DINODE_FMT_LOCAL &&
> -	    xfs_dir2_sf_verify(ip))
> +	/* Check the inline fork data. */
> +	if (xfs_iget_verify_forks(ip))
>  		goto corrupt_out;
>  
>  	/*
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html