[PATCH 0/3] xfs: fix errors in attr/directory scrubbers

[PATCH 0/3] xfs: fix errors in attr/directory scrubbers

[Darrick J. Wong]

Hi all,

During a code audit of the effectiveness of the online repair code,
several deficiencies were discovered in the extended attribute and
directory scrubbing code.  The first two patches change both scrubbers
to note corruption when the name-based lookups (which test the hash
tree) fail to return any information.  The final patch amends the
directory checker to note corruption the inode target of a directory
entry points to an unallocated inode.

If you're going to start using this mess, you probably ought to just
pull from my git trees, which are linked below.

This is an extraordinary way to destroy everything.  Enjoy!
Comments and questions are, as always, welcome.

--D

kernel git tree:
https://git.kernel.org/cgit/linux/kernel/git/djwong/xfs-linux.git/log/?h=scrub-fixes

[PATCH 1/3] xfs: mark dir corrupt when lookup-by-hash fails

[Darrick J. Wong]

From: Darrick J. Wong <darrick.wong@oracle.com>

In xchk_dir_actor, we attempt to validate the directory hash structures
by performing a directory entry lookup by (hashed) name.  If the lookup
returns ENOENT, that means that the hash information is corrupt.  The
_process_error functions don't catch this, so we have to add that
explicitly.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 fs/xfs/scrub/dir.c |    5 +++++
 1 file changed, 5 insertions(+)


diff --git a/fs/xfs/scrub/dir.c b/fs/xfs/scrub/dir.c
index 266da4e4bde6..54afa75c95d1 100644
--- a/fs/xfs/scrub/dir.c
+++ b/fs/xfs/scrub/dir.c
@@ -155,6 +155,11 @@ xchk_dir_actor(
 	xname.type = XFS_DIR3_FT_UNKNOWN;
 
 	error = xfs_dir_lookup(sdc->sc->tp, ip, &xname, &lookup_ino, NULL);
+	if (error == -ENOENT) {
+		/* ENOENT means the hash lookup failed and the dir is corrupt */
+		xchk_fblock_set_corrupt(sdc->sc, XFS_DATA_FORK, offset);
+		return -EFSCORRUPTED;
+	}
 	if (!xchk_fblock_process_error(sdc->sc, XFS_DATA_FORK, offset,
 			&error))
 		goto out;

[PATCH 2/3] xfs: mark extended attr corrupt when lookup-by-hash fails

[Darrick J. Wong]

From: Darrick J. Wong <darrick.wong@oracle.com>

In xchk_xattr_listent, we attempt to validate the extended attribute
hash structures by performing a attr lookup by (hashed) name.  If the
lookup returns ENODATA, that means that the hash information is corrupt.
The _process_error functions don't catch this, so we have to add that
explicitly.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 fs/xfs/scrub/attr.c |    5 +++++
 1 file changed, 5 insertions(+)


diff --git a/fs/xfs/scrub/attr.c b/fs/xfs/scrub/attr.c
index d9f0dd444b80..54ea1efa7ddc 100644
--- a/fs/xfs/scrub/attr.c
+++ b/fs/xfs/scrub/attr.c
@@ -163,6 +163,11 @@ xchk_xattr_listent(
 	args.valuelen = valuelen;
 
 	error = xfs_attr_get_ilocked(context->dp, &args);
+	if (error == -ENODATA) {
+		/* ENODATA means the hash lookup failed and the attr is bad */
+		xchk_fblock_set_corrupt(sx->sc, XFS_ATTR_FORK, args.blkno);
+		goto fail_xref;
+	}
 	if (!xchk_fblock_process_error(sx->sc, XFS_ATTR_FORK, args.blkno,
 			&error))
 		goto fail_xref;

[PATCH 3/3] xfs: scrub should mark dir corrupt if entry points to unallocated inode

[Darrick J. Wong]

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>
---
 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:

Re: [PATCH 1/3] xfs: mark dir corrupt when lookup-by-hash fails

[Allison Collins]

On 2/28/20 6:49 PM, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> In xchk_dir_actor, we attempt to validate the directory hash structures
> by performing a directory entry lookup by (hashed) name.  If the lookup
> returns ENOENT, that means that the hash information is corrupt.  The
> _process_error functions don't catch this, so we have to add that
> explicitly.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
Ok, looks ok to me
Reviewed-by: Allison Collins <allison.henderson@oracle.com>

> ---
>   fs/xfs/scrub/dir.c |    5 +++++
>   1 file changed, 5 insertions(+)
> 
> 
> diff --git a/fs/xfs/scrub/dir.c b/fs/xfs/scrub/dir.c
> index 266da4e4bde6..54afa75c95d1 100644
> --- a/fs/xfs/scrub/dir.c
> +++ b/fs/xfs/scrub/dir.c
> @@ -155,6 +155,11 @@ xchk_dir_actor(
>   	xname.type = XFS_DIR3_FT_UNKNOWN;
>   
>   	error = xfs_dir_lookup(sdc->sc->tp, ip, &xname, &lookup_ino, NULL);
> +	if (error == -ENOENT) {
> +		/* ENOENT means the hash lookup failed and the dir is corrupt */
> +		xchk_fblock_set_corrupt(sdc->sc, XFS_DATA_FORK, offset);
> +		return -EFSCORRUPTED;
> +	}
>   	if (!xchk_fblock_process_error(sdc->sc, XFS_DATA_FORK, offset,
>   			&error))
>   		goto out;
> 

Re: [PATCH 2/3] xfs: mark extended attr corrupt when lookup-by-hash fails

[Allison Collins]

On 2/28/20 6:49 PM, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> In xchk_xattr_listent, we attempt to validate the extended attribute
> hash structures by performing a attr lookup by (hashed) name.  If the
> lookup returns ENODATA, that means that the hash information is corrupt.
> The _process_error functions don't catch this, so we have to add that
> explicitly.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
Looks good:
Reviewed-by: Allison Collins <allison.henderson@oracle.com>

> ---
>   fs/xfs/scrub/attr.c |    5 +++++
>   1 file changed, 5 insertions(+)
> 
> 
> diff --git a/fs/xfs/scrub/attr.c b/fs/xfs/scrub/attr.c
> index d9f0dd444b80..54ea1efa7ddc 100644
> --- a/fs/xfs/scrub/attr.c
> +++ b/fs/xfs/scrub/attr.c
> @@ -163,6 +163,11 @@ xchk_xattr_listent(
>   	args.valuelen = valuelen;
>   
>   	error = xfs_attr_get_ilocked(context->dp, &args);
> +	if (error == -ENODATA) {
> +		/* ENODATA means the hash lookup failed and the attr is bad */
> +		xchk_fblock_set_corrupt(sx->sc, XFS_ATTR_FORK, args.blkno);
> +		goto fail_xref;
> +	}
>   	if (!xchk_fblock_process_error(sx->sc, XFS_ATTR_FORK, args.blkno,
>   			&error))
>   		goto fail_xref;
> 

Re: [PATCH 3/3] xfs: scrub should mark dir corrupt if entry points to unallocated inode

[Allison Collins]

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:
> 

Re: [PATCH 1/3] xfs: mark dir corrupt when lookup-by-hash fails

[Dave Chinner]

On Fri, Feb 28, 2020 at 05:49:09PM -0800, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> In xchk_dir_actor, we attempt to validate the directory hash structures
> by performing a directory entry lookup by (hashed) name.  If the lookup
> returns ENOENT, that means that the hash information is corrupt.  The
> _process_error functions don't catch this, so we have to add that
> explicitly.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
>  fs/xfs/scrub/dir.c |    5 +++++
>  1 file changed, 5 insertions(+)
> 
> 
> diff --git a/fs/xfs/scrub/dir.c b/fs/xfs/scrub/dir.c
> index 266da4e4bde6..54afa75c95d1 100644
> --- a/fs/xfs/scrub/dir.c
> +++ b/fs/xfs/scrub/dir.c
> @@ -155,6 +155,11 @@ xchk_dir_actor(
>  	xname.type = XFS_DIR3_FT_UNKNOWN;
>  
>  	error = xfs_dir_lookup(sdc->sc->tp, ip, &xname, &lookup_ino, NULL);
> +	if (error == -ENOENT) {
> +		/* ENOENT means the hash lookup failed and the dir is corrupt */
> +		xchk_fblock_set_corrupt(sdc->sc, XFS_DATA_FORK, offset);
> +		return -EFSCORRUPTED;
> +	}
>  	if (!xchk_fblock_process_error(sdc->sc, XFS_DATA_FORK, offset,
>  			&error))
>  		goto out;

So why is this error handling open coded rather than doing something
like this to use the generic, pre-existing corruption handling:

	if (error == -ENOENT) {
		/* ENOENT means the hash lookup failed and the dir is corrupt */
		error = -EFSCORRUPTED;
	}
	if (!xchk_fblock_process_error(sdc->sc, XFS_DATA_FORK, offset,
			&error))
		goto out;

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH 2/3] xfs: mark extended attr corrupt when lookup-by-hash fails

[Dave Chinner]

On Fri, Feb 28, 2020 at 05:49:15PM -0800, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> In xchk_xattr_listent, we attempt to validate the extended attribute
> hash structures by performing a attr lookup by (hashed) name.  If the
> lookup returns ENODATA, that means that the hash information is corrupt.
> The _process_error functions don't catch this, so we have to add that
> explicitly.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
>  fs/xfs/scrub/attr.c |    5 +++++
>  1 file changed, 5 insertions(+)
> 
> 
> diff --git a/fs/xfs/scrub/attr.c b/fs/xfs/scrub/attr.c
> index d9f0dd444b80..54ea1efa7ddc 100644
> --- a/fs/xfs/scrub/attr.c
> +++ b/fs/xfs/scrub/attr.c
> @@ -163,6 +163,11 @@ xchk_xattr_listent(
>  	args.valuelen = valuelen;
>  
>  	error = xfs_attr_get_ilocked(context->dp, &args);
> +	if (error == -ENODATA) {
> +		/* ENODATA means the hash lookup failed and the attr is bad */
> +		xchk_fblock_set_corrupt(sx->sc, XFS_ATTR_FORK, args.blkno);
> +		goto fail_xref;
> +	}
>  	if (!xchk_fblock_process_error(sx->sc, XFS_ATTR_FORK, args.blkno,
>  			&error))
>  		goto fail_xref;

Same question as the first patch.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH 3/3] xfs: scrub should mark dir corrupt if entry points to unallocated inode

[Dave Chinner]

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.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH 1/3] xfs: mark dir corrupt when lookup-by-hash fails

[Darrick J. Wong]

On Wed, Mar 04, 2020 at 09:26:10AM +1100, Dave Chinner wrote:
> On Fri, Feb 28, 2020 at 05:49:09PM -0800, Darrick J. Wong wrote:
> > From: Darrick J. Wong <darrick.wong@oracle.com>
> > 
> > In xchk_dir_actor, we attempt to validate the directory hash structures
> > by performing a directory entry lookup by (hashed) name.  If the lookup
> > returns ENOENT, that means that the hash information is corrupt.  The
> > _process_error functions don't catch this, so we have to add that
> > explicitly.
> > 
> > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> > ---
> >  fs/xfs/scrub/dir.c |    5 +++++
> >  1 file changed, 5 insertions(+)
> > 
> > 
> > diff --git a/fs/xfs/scrub/dir.c b/fs/xfs/scrub/dir.c
> > index 266da4e4bde6..54afa75c95d1 100644
> > --- a/fs/xfs/scrub/dir.c
> > +++ b/fs/xfs/scrub/dir.c
> > @@ -155,6 +155,11 @@ xchk_dir_actor(
> >  	xname.type = XFS_DIR3_FT_UNKNOWN;
> >  
> >  	error = xfs_dir_lookup(sdc->sc->tp, ip, &xname, &lookup_ino, NULL);
> > +	if (error == -ENOENT) {
> > +		/* ENOENT means the hash lookup failed and the dir is corrupt */
> > +		xchk_fblock_set_corrupt(sdc->sc, XFS_DATA_FORK, offset);
> > +		return -EFSCORRUPTED;
> > +	}
> >  	if (!xchk_fblock_process_error(sdc->sc, XFS_DATA_FORK, offset,
> >  			&error))
> >  		goto out;
> 
> So why is this error handling open coded rather than doing something
> like this to use the generic, pre-existing corruption handling:
> 
> 	if (error == -ENOENT) {
> 		/* ENOENT means the hash lookup failed and the dir is corrupt */
> 		error = -EFSCORRUPTED;
> 	}
> 	if (!xchk_fblock_process_error(sdc->sc, XFS_DATA_FORK, offset,
> 			&error))
> 		goto out;

No particular reason, I just figured that I might as well cut to setting
OFLAG_CORRUPT and bailing out.  I'll change it.

--D

> Cheers,
> 
> Dave.
> -- 
> Dave Chinner
> david@fromorbit.co

Re: [PATCH 2/3] xfs: mark extended attr corrupt when lookup-by-hash fails

[Darrick J. Wong]

On Wed, Mar 04, 2020 at 09:26:43AM +1100, Dave Chinner wrote:
> On Fri, Feb 28, 2020 at 05:49:15PM -0800, Darrick J. Wong wrote:
> > From: Darrick J. Wong <darrick.wong@oracle.com>
> > 
> > In xchk_xattr_listent, we attempt to validate the extended attribute
> > hash structures by performing a attr lookup by (hashed) name.  If the
> > lookup returns ENODATA, that means that the hash information is corrupt.
> > The _process_error functions don't catch this, so we have to add that
> > explicitly.
> > 
> > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> > ---
> >  fs/xfs/scrub/attr.c |    5 +++++
> >  1 file changed, 5 insertions(+)
> > 
> > 
> > diff --git a/fs/xfs/scrub/attr.c b/fs/xfs/scrub/attr.c
> > index d9f0dd444b80..54ea1efa7ddc 100644
> > --- a/fs/xfs/scrub/attr.c
> > +++ b/fs/xfs/scrub/attr.c
> > @@ -163,6 +163,11 @@ xchk_xattr_listent(
> >  	args.valuelen = valuelen;
> >  
> >  	error = xfs_attr_get_ilocked(context->dp, &args);
> > +	if (error == -ENODATA) {
> > +		/* ENODATA means the hash lookup failed and the attr is bad */
> > +		xchk_fblock_set_corrupt(sx->sc, XFS_ATTR_FORK, args.blkno);
> > +		goto fail_xref;
> > +	}
> >  	if (!xchk_fblock_process_error(sx->sc, XFS_ATTR_FORK, args.blkno,
> >  			&error))
> >  		goto fail_xref;
> 
> Same question as the first patch.

Same resolution as the first patch. :)

--D

> Cheers,
> 
> Dave.
> -- 
> Dave Chinner
> david@fromorbit.com

Re: [PATCH 3/3] xfs: scrub should mark dir corrupt if entry points to unallocated inode

[Darrick J. Wong]

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

Re: [PATCH 3/3] xfs: scrub should mark dir corrupt if entry points to unallocated inode

[Dave Chinner]

On Tue, Mar 10, 2020 at 05:48:20PM -0700, 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.
> 
> We can also remove the !hasftype code from the function, because any
> DT_ flags we encounter on those filesystems were synthesized in core,
> not read in from disk.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
>  fs/xfs/scrub/dir.c |   39 ++++++++++++++++++++++-----------------
>  1 file changed, 22 insertions(+), 17 deletions(-)

That's much neater.

Reviewed-by: Dave Chinner <dchinner@redhat.com>

-- 
Dave Chinner
david@fromorbit.com

[PATCH 3/3] xfs: scrub should mark dir corrupt if entry points to unallocated inode

[Darrick J. Wong]

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.

We can also remove the !hasftype code from the function, because any
DT_ flags we encounter on those filesystems were synthesized in core,
not read in from disk.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 fs/xfs/scrub/dir.c |   39 ++++++++++++++++++++++-----------------
 1 file changed, 22 insertions(+), 17 deletions(-)


diff --git a/fs/xfs/scrub/dir.c b/fs/xfs/scrub/dir.c
index ef7cc8e101ab..c186c83544ac 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,26 @@ 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 to force validation of the inode number (using
+	 * the inode btree) before we look up the inode record.  If this fails
+	 * validation for any reason, we will receive EINVAL, which 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)
+		error = -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);
+	}
 	xfs_irele(ip);
 out:
 	return error;
@@ -166,8 +171,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: