On Wed, Mar 04, 2020 at 09:39:07AM +1100, Dave Chinner wrote:
> On Fri, Feb 28, 2020 at 05:49:22PM -0800, Darrick J. Wong wrote:
> > From: Darrick J. Wong <darrick.wong@xxxxxxxxxx>
> >
> > In xchk_dir_check_ftype, we should mark the directory corrupt if we try
> > to _iget a directory entry's inode pointer and the inode btree says the
> > inode is not allocated. This involves changing the IGET call to force
> > the inobt lookup to return EINVAL if the inode isn't allocated; and
> > rearranging the code so that we always perform the iget.
>
> There's also a bug fix in this that isn't mentioned...
>
> >
> > Signed-off-by: Darrick J. Wong <darrick.wong@xxxxxxxxxx>
> > ---
> > fs/xfs/scrub/dir.c | 43 ++++++++++++++++++++++++++-----------------
> > 1 file changed, 26 insertions(+), 17 deletions(-)
> >
> >
> > diff --git a/fs/xfs/scrub/dir.c b/fs/xfs/scrub/dir.c
> > index 54afa75c95d1..a775fbf49a0d 100644
> > --- a/fs/xfs/scrub/dir.c
> > +++ b/fs/xfs/scrub/dir.c
> > @@ -39,9 +39,12 @@ struct xchk_dir_ctx {
> > struct xfs_scrub *sc;
> > };
> >
> > -/* Check that an inode's mode matches a given DT_ type. */
> > +/*
> > + * Check that a directory entry's inode pointer directs us to an allocated
> > + * inode and (if applicable) the inode mode matches the entry's DT_ type.
> > + */
> > STATIC int
> > -xchk_dir_check_ftype(
> > +xchk_dir_check_iptr(
> > struct xchk_dir_ctx *sdc,
> > xfs_fileoff_t offset,
> > xfs_ino_t inum,
> > @@ -52,13 +55,6 @@ xchk_dir_check_ftype(
> > int ino_dtype;
> > int error = 0;
> >
> > - if (!xfs_sb_version_hasftype(&mp->m_sb)) {
> > - if (dtype != DT_UNKNOWN && dtype != DT_DIR)
> > - xchk_fblock_set_corrupt(sdc->sc, XFS_DATA_FORK,
> > - offset);
> > - goto out;
> > - }
> > -
> > /*
> > * Grab the inode pointed to by the dirent. We release the
> > * inode before we cancel the scrub transaction. Since we're
> > @@ -66,17 +62,30 @@ xchk_dir_check_ftype(
> > * eofblocks cleanup (which allocates what would be a nested
> > * transaction), we can't use DONTCACHE here because DONTCACHE
> > * inodes can trigger immediate inactive cleanup of the inode.
> > + *
> > + * We use UNTRUSTED here so that iget will return EINVAL if we have an
> > + * inode pointer that points to an unallocated inode.
>
> "We use UNTRUSTED here to force validation of the inode number
> before we look it up. If it fails validation for any reason we will
> get -EINVAL returned and that indicates a corrupt directory entry."
Ok, changed.
> > */
> > - error = xfs_iget(mp, sdc->sc->tp, inum, 0, 0, &ip);
> > + error = xfs_iget(mp, sdc->sc->tp, inum, XFS_IGET_UNTRUSTED, 0, &ip);
> > + if (error == -EINVAL) {
> > + xchk_fblock_set_corrupt(sdc->sc, XFS_DATA_FORK, offset);
> > + return -EFSCORRUPTED;
> > + }
> > if (!xchk_fblock_xref_process_error(sdc->sc, XFS_DATA_FORK, offset,
> > &error))
> > goto out;
>
> Also:
> if (error == -EINVAL)
> error = -EFSCORRUPTED;
Also changed.
>
> >
> > - /* Convert mode to the DT_* values that dir_emit uses. */
> > - ino_dtype = xfs_dir3_get_dtype(mp,
> > - xfs_mode_to_ftype(VFS_I(ip)->i_mode));
> > - if (ino_dtype != dtype)
> > - xchk_fblock_set_corrupt(sdc->sc, XFS_DATA_FORK, offset);
> > + if (xfs_sb_version_hasftype(&mp->m_sb)) {
> > + /* Convert mode to the DT_* values that dir_emit uses. */
> > + ino_dtype = xfs_dir3_get_dtype(mp,
> > + xfs_mode_to_ftype(VFS_I(ip)->i_mode));
> > + if (ino_dtype != dtype)
> > + xchk_fblock_set_corrupt(sdc->sc, XFS_DATA_FORK, offset);
> > + } else {
> > + if (dtype != DT_UNKNOWN && dtype != DT_DIR)
> > + xchk_fblock_set_corrupt(sdc->sc, XFS_DATA_FORK,
> > + offset);
> > + }
>
> What is this fixing? xfs_dir3_get_dtype() always returned DT_UNKNOWN
> for !hasftype filesystems, so I'm guessing this fixes validation
> against dtype == DT_DIR for "." and ".." entries, but didn't we
> already check this in xchk_dir_actor() before it calls this
> function?
Oh, right, we already checked those, so we can get rid of the !hasftype
code entirely. Good catch!
--D
> Cheers,
>
> Dave.
> --
> Dave Chinner
> david@xxxxxxxxxxxxx
