Re: [PATCH 05/12] xfs: call xfs_dinode_verify from xfs_inode_from_disk

["Darrick J. Wong" <darrick.wong@oracle.com>]

On Fri, May 08, 2020 at 08:34:16AM +0200, Christoph Hellwig wrote:
> Keep the code dealing with the dinode together, and also ensure we verify
> the dinode in the owner change log recovery case as well.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> Reviewed-by: Brian Foster <bfoster@redhat.com>

Seems reasonable to me,
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>

--D

> ---
>  .../xfs-self-describing-metadata.txt           | 10 +++++-----
>  fs/xfs/libxfs/xfs_inode_buf.c                  | 18 ++++++++----------
>  2 files changed, 13 insertions(+), 15 deletions(-)
> 
> diff --git a/Documentation/filesystems/xfs-self-describing-metadata.txt b/Documentation/filesystems/xfs-self-describing-metadata.txt
> index 8db0121d0980c..e912699d74301 100644
> --- a/Documentation/filesystems/xfs-self-describing-metadata.txt
> +++ b/Documentation/filesystems/xfs-self-describing-metadata.txt
> @@ -337,11 +337,11 @@ buffer.
>  
>  The structure of the verifiers and the identifiers checks is very similar to the
>  buffer code described above. The only difference is where they are called. For
> -example, inode read verification is done in xfs_iread() when the inode is first
> -read out of the buffer and the struct xfs_inode is instantiated. The inode is
> -already extensively verified during writeback in xfs_iflush_int, so the only
> -addition here is to add the LSN and CRC to the inode as it is copied back into
> -the buffer.
> +example, inode read verification is done in xfs_inode_from_disk() when the inode
> +is first read out of the buffer and the struct xfs_inode is instantiated. The
> +inode is already extensively verified during writeback in xfs_iflush_int, so the
> +only addition here is to add the LSN and CRC to the inode as it is copied back
> +into the buffer.
>  
>  XXX: inode unlinked list modification doesn't recalculate the inode CRC! None of
>  the unlinked list modifications check or update CRCs, neither during unlink nor
> diff --git a/fs/xfs/libxfs/xfs_inode_buf.c b/fs/xfs/libxfs/xfs_inode_buf.c
> index 686a026b5f6ed..3aac22e892985 100644
> --- a/fs/xfs/libxfs/xfs_inode_buf.c
> +++ b/fs/xfs/libxfs/xfs_inode_buf.c
> @@ -188,10 +188,18 @@ xfs_inode_from_disk(
>  	struct xfs_icdinode	*to = &ip->i_d;
>  	struct inode		*inode = VFS_I(ip);
>  	int			error;
> +	xfs_failaddr_t		fa;
>  
>  	ASSERT(ip->i_cowfp == NULL);
>  	ASSERT(ip->i_afp == NULL);
>  
> +	fa = xfs_dinode_verify(ip->i_mount, ip->i_ino, from);
> +	if (fa) {
> +		xfs_inode_verifier_error(ip, -EFSCORRUPTED, "dinode", from,
> +				sizeof(*from), fa);
> +		return -EFSCORRUPTED;
> +	}
> +
>  	/*
>  	 * First get the permanent information that is needed to allocate an
>  	 * inode. If the inode is unused, mode is zero and we shouldn't mess
> @@ -627,7 +635,6 @@ xfs_iread(
>  {
>  	xfs_buf_t	*bp;
>  	xfs_dinode_t	*dip;
> -	xfs_failaddr_t	fa;
>  	int		error;
>  
>  	/*
> @@ -652,15 +659,6 @@ xfs_iread(
>  	if (error)
>  		return error;
>  
> -	/* even unallocated inodes are verified */
> -	fa = xfs_dinode_verify(mp, ip->i_ino, dip);
> -	if (fa) {
> -		xfs_inode_verifier_error(ip, -EFSCORRUPTED, "dinode", dip,
> -				sizeof(*dip), fa);
> -		error = -EFSCORRUPTED;
> -		goto out_brelse;
> -	}
> -
>  	error = xfs_inode_from_disk(ip, dip);
>  	if (error)
>  		goto out_brelse;
> -- 
> 2.26.2
>