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@xxxxxx> > Reviewed-by: Brian Foster <bfoster@xxxxxxxxxx> Seems reasonable to me, Reviewed-by: Darrick J. Wong <darrick.wong@xxxxxxxxxx> --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 >