diff mbox series

[4/4] xfs: directory scrub should check the null bestfree entries too

Message ID 160494587794.772802.11043398495774645870.stgit@magnolia (mailing list archive)
State Accepted, archived
Headers show
Series xfs: fix various scrub problems | expand

Commit Message

Darrick J. Wong Nov. 9, 2020, 6:17 p.m. UTC
From: Darrick J. Wong <darrick.wong@oracle.com>

Teach the directory scrubber to check all the bestfree entries,
including the null ones.  We want to be able to detect the case where
the entry is null but there actually /is/ a directory data block.

Found by fuzzing lbests[0] = ones in xfs/391.

Fixes: df481968f33b ("xfs: scrub directory freespace")
Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 fs/xfs/scrub/dir.c |   27 ++++++++++++++++++++-------
 1 file changed, 20 insertions(+), 7 deletions(-)

Comments

Chandan Babu R Nov. 13, 2020, 9:08 a.m. UTC | #1
On Monday 9 November 2020 11:47:58 PM IST Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Teach the directory scrubber to check all the bestfree entries,
> including the null ones.  We want to be able to detect the case where
> the entry is null but there actually /is/ a directory data block.
> 
> Found by fuzzing lbests[0] = ones in xfs/391.
>

Looks good to me.

Reviewed-by: Chandan Babu R <chandanrlinux@gmail.com>

> Fixes: df481968f33b ("xfs: scrub directory freespace")
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
>  fs/xfs/scrub/dir.c |   27 ++++++++++++++++++++-------
>  1 file changed, 20 insertions(+), 7 deletions(-)
> 
> 
> diff --git a/fs/xfs/scrub/dir.c b/fs/xfs/scrub/dir.c
> index 7c432997edad..b045e95c2ea7 100644
> --- a/fs/xfs/scrub/dir.c
> +++ b/fs/xfs/scrub/dir.c
> @@ -558,14 +558,27 @@ xchk_directory_leaf1_bestfree(
>  	/* Check all the bestfree entries. */
>  	for (i = 0; i < bestcount; i++, bestp++) {
>  		best = be16_to_cpu(*bestp);
> +		error = xfs_dir3_data_read(sc->tp, sc->ip,
> +				xfs_dir2_db_to_da(args->geo, i),
> +				XFS_DABUF_MAP_HOLE_OK,
> +				&dbp);
> +		if (!xchk_fblock_process_error(sc, XFS_DATA_FORK, lblk,
> +				&error))
> +			break;
> +
> +		if (!dbp) {
> +			if (best != NULLDATAOFF) {
> +				xchk_fblock_set_corrupt(sc, XFS_DATA_FORK,
> +						lblk);
> +				break;
> +			}
> +			continue;
> +		}
> +
>  		if (best == NULLDATAOFF)
> -			continue;
> -		error = xfs_dir3_data_read(sc->tp, sc->ip,
> -				i * args->geo->fsbcount, 0, &dbp);
> -		if (!xchk_fblock_process_error(sc, XFS_DATA_FORK, lblk,
> -				&error))
> -			break;
> -		xchk_directory_check_freesp(sc, lblk, dbp, best);
> +			xchk_fblock_set_corrupt(sc, XFS_DATA_FORK, lblk);
> +		else
> +			xchk_directory_check_freesp(sc, lblk, dbp, best);
>  		xfs_trans_brelse(sc->tp, dbp);
>  		if (sc->sm->sm_flags & XFS_SCRUB_OFLAG_CORRUPT)
>  			break;
> 
>
Christoph Hellwig Nov. 14, 2020, 10:40 a.m. UTC | #2
On Mon, Nov 09, 2020 at 10:17:58AM -0800, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Teach the directory scrubber to check all the bestfree entries,
> including the null ones.  We want to be able to detect the case where
> the entry is null but there actually /is/ a directory data block.
> 
> Found by fuzzing lbests[0] = ones in xfs/391.
> 
> Fixes: df481968f33b ("xfs: scrub directory freespace")
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>
diff mbox series

Patch

diff --git a/fs/xfs/scrub/dir.c b/fs/xfs/scrub/dir.c
index 7c432997edad..b045e95c2ea7 100644
--- a/fs/xfs/scrub/dir.c
+++ b/fs/xfs/scrub/dir.c
@@ -558,14 +558,27 @@  xchk_directory_leaf1_bestfree(
 	/* Check all the bestfree entries. */
 	for (i = 0; i < bestcount; i++, bestp++) {
 		best = be16_to_cpu(*bestp);
+		error = xfs_dir3_data_read(sc->tp, sc->ip,
+				xfs_dir2_db_to_da(args->geo, i),
+				XFS_DABUF_MAP_HOLE_OK,
+				&dbp);
+		if (!xchk_fblock_process_error(sc, XFS_DATA_FORK, lblk,
+				&error))
+			break;
+
+		if (!dbp) {
+			if (best != NULLDATAOFF) {
+				xchk_fblock_set_corrupt(sc, XFS_DATA_FORK,
+						lblk);
+				break;
+			}
+			continue;
+		}
+
 		if (best == NULLDATAOFF)
-			continue;
-		error = xfs_dir3_data_read(sc->tp, sc->ip,
-				i * args->geo->fsbcount, 0, &dbp);
-		if (!xchk_fblock_process_error(sc, XFS_DATA_FORK, lblk,
-				&error))
-			break;
-		xchk_directory_check_freesp(sc, lblk, dbp, best);
+			xchk_fblock_set_corrupt(sc, XFS_DATA_FORK, lblk);
+		else
+			xchk_directory_check_freesp(sc, lblk, dbp, best);
 		xfs_trans_brelse(sc->tp, dbp);
 		if (sc->sm->sm_flags & XFS_SCRUB_OFLAG_CORRUPT)
 			break;