On Fri, May 01, 2020 at 10:14:17AM +0200, Christoph Hellwig wrote:
> Keep the code dealing with the dinode together, and also ensure we verify
> the dinode in the onwer change log recovery case as well.
owner
>
> Signed-off-by: Christoph Hellwig <hch@xxxxxx>
> ---
Reviewed-by: Brian Foster <bfoster@xxxxxxxxxx>
> .../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 b136f29f7d9d3..a00001a2336ef 100644
> --- a/fs/xfs/libxfs/xfs_inode_buf.c
> +++ b/fs/xfs/libxfs/xfs_inode_buf.c
> @@ -194,10 +194,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;
> + }
> +
> /*
> * Get the truly permanent information first that is not overwritten by
> * xfs_ialloc first. This also includes i_mode so that a newly read
> @@ -637,7 +645,6 @@ xfs_iread(
> {
> xfs_buf_t *bp;
> xfs_dinode_t *dip;
> - xfs_failaddr_t fa;
> int error;
>
> /*
> @@ -662,15 +669,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
>
