| Message ID | 158294096213.1730101.1870315264682758950.stgit@magnolia (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | xfs: fix errors in attr/directory scrubbers | expand |
On 2/28/20 6:49 PM, Darrick J. Wong wrote: > From: Darrick J. Wong <darrick.wong@oracle.com> > > 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. > > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com> Ok, I followed it through, and didn't see any obvious issues Reviewed-by: Allison Collins <allison.henderson@oracle.com> > --- > 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. > */ > - 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; > > - /* 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); > + } > xfs_irele(ip); > out: > return error; > @@ -168,8 +177,8 @@ xchk_dir_actor( > goto out; > } > > - /* Verify the file type. This function absorbs error codes. */ > - error = xchk_dir_check_ftype(sdc, offset, lookup_ino, type); > + /* Verify the inode pointer. This function absorbs error codes. */ > + error = xchk_dir_check_iptr(sdc, offset, lookup_ino, type); > if (error) > goto out; > out: >
On Fri, Feb 28, 2020 at 05:49:22PM -0800, Darrick J. Wong wrote: > From: Darrick J. Wong <darrick.wong@oracle.com> > > 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@oracle.com> > --- > 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." > */ > - 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; > > - /* 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? Cheers, Dave.
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@oracle.com> > > > > 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@oracle.com> > > --- > > 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@fromorbit.com
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. */ - 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; - /* 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); + } xfs_irele(ip); out: return error; @@ -168,8 +177,8 @@ xchk_dir_actor( goto out; } - /* Verify the file type. This function absorbs error codes. */ - error = xchk_dir_check_ftype(sdc, offset, lookup_ino, type); + /* Verify the inode pointer. This function absorbs error codes. */ + error = xchk_dir_check_iptr(sdc, offset, lookup_ino, type); if (error) goto out; out: