[PATCH 0/4] xfs: fix various scrub problems

[PATCH 0/4] xfs: fix various scrub problems

[Darrick J. Wong]

Hi all,

While continuing my assessment of the online fsck code, I observed
various other minor problems in the online scrub code.  The most serious
of the bugs involves the extent reference count checker, which
incorrectly counts the number of rmaps it loads into the shared-extent
record stack, and results in scrub being unable to detect off-by-one
errors in the reference count.

The three patches after that tighten the checking around the mininum
number of records in a btree block; flags for rmap records; and
validating that a null entry in the leaf bestfree block actually
correspond to a hole in the directory data segment.

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-5.10
---
 fs/xfs/scrub/bmap.c     |    8 ++++----
 fs/xfs/scrub/btree.c    |   45 +++++++++++++++++++++++++++------------------
 fs/xfs/scrub/dir.c      |   27 ++++++++++++++++++++-------
 fs/xfs/scrub/refcount.c |    8 +++-----
 4 files changed, 54 insertions(+), 34 deletions(-)

[PATCH 1/4] xfs: fix brainos in the refcount scrubber's rmap fragment processor

[Darrick J. Wong]

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

Fix some serious WTF in the reference count scrubber's rmap fragment
processing.  The code comment says that this loop is supposed to move
all fragment records starting at or before bno onto the worklist, but
there's no obvious reason why nr (the number of items added) should
increment starting from 1, and breaking the loop when we've added the
target number seems dubious since we could have more rmap fragments that
should have been added to the worklist.

This seems to manifest in xfs/411 when adding one to the refcount field.

Fixes: dbde19da9637 ("xfs: cross-reference the rmapbt data with the refcountbt")
Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 fs/xfs/scrub/refcount.c |    8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)


diff --git a/fs/xfs/scrub/refcount.c b/fs/xfs/scrub/refcount.c
index beaeb6fa3119..dd672e6bbc75 100644
--- a/fs/xfs/scrub/refcount.c
+++ b/fs/xfs/scrub/refcount.c
@@ -170,7 +170,6 @@ xchk_refcountbt_process_rmap_fragments(
 	 */
 	INIT_LIST_HEAD(&worklist);
 	rbno = NULLAGBLOCK;
-	nr = 1;
 
 	/* Make sure the fragments actually /are/ in agbno order. */
 	bno = 0;
@@ -184,15 +183,14 @@ xchk_refcountbt_process_rmap_fragments(
 	 * Find all the rmaps that start at or before the refc extent,
 	 * and put them on the worklist.
 	 */
+	nr = 0;
 	list_for_each_entry_safe(frag, n, &refchk->fragments, list) {
-		if (frag->rm.rm_startblock > refchk->bno)
-			goto done;
+		if (frag->rm.rm_startblock > refchk->bno || nr > target_nr)
+			break;
 		bno = frag->rm.rm_startblock + frag->rm.rm_blockcount;
 		if (bno < rbno)
 			rbno = bno;
 		list_move_tail(&frag->list, &worklist);
-		if (nr == target_nr)
-			break;
 		nr++;
 	}
 

Re: [PATCH 1/4] xfs: fix brainos in the refcount scrubber's rmap fragment processor

[Christoph Hellwig]

On Mon, Nov 09, 2020 at 10:17:39AM -0800, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Fix some serious WTF in the reference count scrubber's rmap fragment
> processing.  The code comment says that this loop is supposed to move
> all fragment records starting at or before bno onto the worklist, but
> there's no obvious reason why nr (the number of items added) should
> increment starting from 1, and breaking the loop when we've added the
> target number seems dubious since we could have more rmap fragments that
> should have been added to the worklist.
> 
> This seems to manifest in xfs/411 when adding one to the refcount field.
> 
> Fixes: dbde19da9637 ("xfs: cross-reference the rmapbt data with the refcountbt")

Looks good,

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

Re: [PATCH 1/4] xfs: fix brainos in the refcount scrubber's rmap fragment processor

[Chandan Babu R]

On Monday 9 November 2020 11:47:39 PM IST Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Fix some serious WTF in the reference count scrubber's rmap fragment
> processing.  The code comment says that this loop is supposed to move
> all fragment records starting at or before bno onto the worklist, but
> there's no obvious reason why nr (the number of items added) should
> increment starting from 1, and breaking the loop when we've added the
> target number seems dubious since we could have more rmap fragments that
> should have been added to the worklist.
> 
> This seems to manifest in xfs/411 when adding one to the refcount field.
> 
> Fixes: dbde19da9637 ("xfs: cross-reference the rmapbt data with the refcountbt")
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
>  fs/xfs/scrub/refcount.c |    8 +++-----
>  1 file changed, 3 insertions(+), 5 deletions(-)
> 
> 
> diff --git a/fs/xfs/scrub/refcount.c b/fs/xfs/scrub/refcount.c
> index beaeb6fa3119..dd672e6bbc75 100644
> --- a/fs/xfs/scrub/refcount.c
> +++ b/fs/xfs/scrub/refcount.c
> @@ -170,7 +170,6 @@ xchk_refcountbt_process_rmap_fragments(
>  	 */
>  	INIT_LIST_HEAD(&worklist);
>  	rbno = NULLAGBLOCK;
> -	nr = 1;
>  
>  	/* Make sure the fragments actually /are/ in agbno order. */
>  	bno = 0;
> @@ -184,15 +183,14 @@ xchk_refcountbt_process_rmap_fragments(
>  	 * Find all the rmaps that start at or before the refc extent,
>  	 * and put them on the worklist.
>  	 */
> +	nr = 0;
>  	list_for_each_entry_safe(frag, n, &refchk->fragments, list) {
> -		if (frag->rm.rm_startblock > refchk->bno)
> -			goto done;
> +		if (frag->rm.rm_startblock > refchk->bno || nr > target_nr)
> +			break;

In the case of fuzzed refcnt value of 1, The condition "nr > target_nr" causes
"nr != target_nr" condition (appearing after the loop) to evaluate to true
(since atleast two rmap entries would be present for the refcount extent)
which in turn causes xchk_refcountbt_xref_rmap() to flag the data structure as
corrupt. Please let me know if my understanding of the code flow is correct?

>  		bno = frag->rm.rm_startblock + frag->rm.rm_blockcount;
>  		if (bno < rbno)
>  			rbno = bno;
>  		list_move_tail(&frag->list, &worklist);
> -		if (nr == target_nr)
> -			break;
>  		nr++;
>  	}
>  
> 
> 


-- 
chandan

Re: [PATCH 1/4] xfs: fix brainos in the refcount scrubber's rmap fragment processor

[Darrick J. Wong]

On Thu, Nov 12, 2020 at 06:21:52PM +0530, Chandan Babu R wrote:
> On Monday 9 November 2020 11:47:39 PM IST Darrick J. Wong wrote:
> > From: Darrick J. Wong <darrick.wong@oracle.com>
> > 
> > Fix some serious WTF in the reference count scrubber's rmap fragment
> > processing.  The code comment says that this loop is supposed to move
> > all fragment records starting at or before bno onto the worklist, but
> > there's no obvious reason why nr (the number of items added) should
> > increment starting from 1, and breaking the loop when we've added the
> > target number seems dubious since we could have more rmap fragments that
> > should have been added to the worklist.
> > 
> > This seems to manifest in xfs/411 when adding one to the refcount field.
> > 
> > Fixes: dbde19da9637 ("xfs: cross-reference the rmapbt data with the refcountbt")
> > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> > ---
> >  fs/xfs/scrub/refcount.c |    8 +++-----
> >  1 file changed, 3 insertions(+), 5 deletions(-)
> > 
> > 
> > diff --git a/fs/xfs/scrub/refcount.c b/fs/xfs/scrub/refcount.c
> > index beaeb6fa3119..dd672e6bbc75 100644
> > --- a/fs/xfs/scrub/refcount.c
> > +++ b/fs/xfs/scrub/refcount.c
> > @@ -170,7 +170,6 @@ xchk_refcountbt_process_rmap_fragments(
> >  	 */
> >  	INIT_LIST_HEAD(&worklist);
> >  	rbno = NULLAGBLOCK;
> > -	nr = 1;
> >  
> >  	/* Make sure the fragments actually /are/ in agbno order. */
> >  	bno = 0;
> > @@ -184,15 +183,14 @@ xchk_refcountbt_process_rmap_fragments(
> >  	 * Find all the rmaps that start at or before the refc extent,
> >  	 * and put them on the worklist.
> >  	 */
> > +	nr = 0;
> >  	list_for_each_entry_safe(frag, n, &refchk->fragments, list) {
> > -		if (frag->rm.rm_startblock > refchk->bno)
> > -			goto done;
> > +		if (frag->rm.rm_startblock > refchk->bno || nr > target_nr)
> > +			break;
> 
> In the case of fuzzed refcnt value of 1, The condition "nr > target_nr" causes
> "nr != target_nr" condition (appearing after the loop) to evaluate to true
> (since atleast two rmap entries would be present for the refcount extent)
> which in turn causes xchk_refcountbt_xref_rmap() to flag the data structure as
> corrupt. Please let me know if my understanding of the code flow is correct?

Right.

--D

> >  		bno = frag->rm.rm_startblock + frag->rm.rm_blockcount;
> >  		if (bno < rbno)
> >  			rbno = bno;
> >  		list_move_tail(&frag->list, &worklist);
> > -		if (nr == target_nr)
> > -			break;
> >  		nr++;
> >  	}
> >  
> > 
> > 
> 
> 
> -- 
> chandan
> 
> 
> 

Re: [PATCH 1/4] xfs: fix brainos in the refcount scrubber's rmap fragment processor

[Chandan Babu R]

On Thursday 12 November 2020 9:35:26 PM IST Darrick J. Wong wrote:
> On Thu, Nov 12, 2020 at 06:21:52PM +0530, Chandan Babu R wrote:
> > On Monday 9 November 2020 11:47:39 PM IST Darrick J. Wong wrote:
> > > From: Darrick J. Wong <darrick.wong@oracle.com>
> > > 
> > > Fix some serious WTF in the reference count scrubber's rmap fragment
> > > processing.  The code comment says that this loop is supposed to move
> > > all fragment records starting at or before bno onto the worklist, but
> > > there's no obvious reason why nr (the number of items added) should
> > > increment starting from 1, and breaking the loop when we've added the
> > > target number seems dubious since we could have more rmap fragments that
> > > should have been added to the worklist.
> > > 
> > > This seems to manifest in xfs/411 when adding one to the refcount field.
> > > 
> > > Fixes: dbde19da9637 ("xfs: cross-reference the rmapbt data with the refcountbt")
> > > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> > > ---
> > >  fs/xfs/scrub/refcount.c |    8 +++-----
> > >  1 file changed, 3 insertions(+), 5 deletions(-)
> > > 
> > > 
> > > diff --git a/fs/xfs/scrub/refcount.c b/fs/xfs/scrub/refcount.c
> > > index beaeb6fa3119..dd672e6bbc75 100644
> > > --- a/fs/xfs/scrub/refcount.c
> > > +++ b/fs/xfs/scrub/refcount.c
> > > @@ -170,7 +170,6 @@ xchk_refcountbt_process_rmap_fragments(
> > >  	 */
> > >  	INIT_LIST_HEAD(&worklist);
> > >  	rbno = NULLAGBLOCK;
> > > -	nr = 1;
> > >  
> > >  	/* Make sure the fragments actually /are/ in agbno order. */
> > >  	bno = 0;
> > > @@ -184,15 +183,14 @@ xchk_refcountbt_process_rmap_fragments(
> > >  	 * Find all the rmaps that start at or before the refc extent,
> > >  	 * and put them on the worklist.
> > >  	 */
> > > +	nr = 0;
> > >  	list_for_each_entry_safe(frag, n, &refchk->fragments, list) {
> > > -		if (frag->rm.rm_startblock > refchk->bno)
> > > -			goto done;
> > > +		if (frag->rm.rm_startblock > refchk->bno || nr > target_nr)
> > > +			break;
> > 
> > In the case of fuzzed refcnt value of 1, The condition "nr > target_nr" causes
> > "nr != target_nr" condition (appearing after the loop) to evaluate to true
> > (since atleast two rmap entries would be present for the refcount extent)
> > which in turn causes xchk_refcountbt_xref_rmap() to flag the data structure as
> > corrupt. Please let me know if my understanding of the code flow is correct?
> 
> Right.
>
Ok. In that case the code change in this patch is handling the erroneous
scenario correctly.

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

> --D
> 
> > >  		bno = frag->rm.rm_startblock + frag->rm.rm_blockcount;
> > >  		if (bno < rbno)
> > >  			rbno = bno;
> > >  		list_move_tail(&frag->list, &worklist);
> > > -		if (nr == target_nr)
> > > -			break;
> > >  		nr++;
> > >  	}
> > >  
> > > 
> > > 
> > 
> > 
> 


-- 
chandan

[PATCH 2/4] xfs: fix the minrecs logic when dealing with inode root child blocks

[Darrick J. Wong]

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

The comment and logic in xchk_btree_check_minrecs for dealing with
inode-rooted btrees isn't quite correct.  While the direct children of
the inode root are allowed to have fewer records than what would
normally be allowed for a regular ondisk btree block, this is only true
if there is only one child block and the number of records don't fit in
the inode root.

Fixes: 08a3a692ef58 ("xfs: btree scrub should check minrecs")
Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 fs/xfs/scrub/btree.c |   45 +++++++++++++++++++++++++++------------------
 1 file changed, 27 insertions(+), 18 deletions(-)


diff --git a/fs/xfs/scrub/btree.c b/fs/xfs/scrub/btree.c
index f52a7b8256f9..debf392e0515 100644
--- a/fs/xfs/scrub/btree.c
+++ b/fs/xfs/scrub/btree.c
@@ -452,32 +452,41 @@ xchk_btree_check_minrecs(
 	int			level,
 	struct xfs_btree_block	*block)
 {
-	unsigned int		numrecs;
-	int			ok_level;
-
-	numrecs = be16_to_cpu(block->bb_numrecs);
+	struct xfs_btree_cur	*cur = bs->cur;
+	unsigned int		root_level = cur->bc_nlevels - 1;
+	unsigned int		numrecs = be16_to_cpu(block->bb_numrecs);
 
 	/* More records than minrecs means the block is ok. */
-	if (numrecs >= bs->cur->bc_ops->get_minrecs(bs->cur, level))
+	if (numrecs >= cur->bc_ops->get_minrecs(cur, level))
 		return;
 
 	/*
-	 * Certain btree blocks /can/ have fewer than minrecs records.  Any
-	 * level greater than or equal to the level of the highest dedicated
-	 * btree block are allowed to violate this constraint.
-	 *
-	 * For a btree rooted in a block, the btree root can have fewer than
-	 * minrecs records.  If the btree is rooted in an inode and does not
-	 * store records in the root, the direct children of the root and the
-	 * root itself can have fewer than minrecs records.
+	 * For btrees rooted in the inode, it's possible that the root block
+	 * contents spilled into a regular ondisk block because there wasn't
+	 * enough space in the inode root.  The number of records in that
+	 * child block might be less than the standard minrecs, but that's ok
+	 * provided that there's only one direct child of the root.
 	 */
-	ok_level = bs->cur->bc_nlevels - 1;
-	if (bs->cur->bc_flags & XFS_BTREE_ROOT_IN_INODE)
-		ok_level--;
-	if (level >= ok_level)
+	if ((cur->bc_flags & XFS_BTREE_ROOT_IN_INODE) &&
+	    level == cur->bc_nlevels - 2) {
+		struct xfs_btree_block	*root_block;
+		struct xfs_buf		*root_bp;
+		int			root_maxrecs;
+
+		root_block = xfs_btree_get_block(cur, root_level, &root_bp);
+		root_maxrecs = cur->bc_ops->get_dmaxrecs(cur, root_level);
+		if (be16_to_cpu(root_block->bb_numrecs) != 1 ||
+		    numrecs <= root_maxrecs)
+			xchk_btree_set_corrupt(bs->sc, cur, level);
 		return;
+	}
 
-	xchk_btree_set_corrupt(bs->sc, bs->cur, level);
+	/*
+	 * Otherwise, only the root level is allowed to have fewer than minrecs
+	 * records or keyptrs.
+	 */
+	if (level < root_level)
+		xchk_btree_set_corrupt(bs->sc, cur, level);
 }
 
 /*

Re: [PATCH 2/4] xfs: fix the minrecs logic when dealing with inode root child blocks

[Chandan Babu R]

On Monday 9 November 2020 11:47:45 PM IST Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> The comment and logic in xchk_btree_check_minrecs for dealing with
> inode-rooted btrees isn't quite correct.  While the direct children of
> the inode root are allowed to have fewer records than what would
> normally be allowed for a regular ondisk btree block, this is only true
> if there is only one child block and the number of records don't fit in
> the inode root.
>

The code changes are consistent with rules provided in the comments.

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

> Fixes: 08a3a692ef58 ("xfs: btree scrub should check minrecs")
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
>  fs/xfs/scrub/btree.c |   45 +++++++++++++++++++++++++++------------------
>  1 file changed, 27 insertions(+), 18 deletions(-)
> 
> 
> diff --git a/fs/xfs/scrub/btree.c b/fs/xfs/scrub/btree.c
> index f52a7b8256f9..debf392e0515 100644
> --- a/fs/xfs/scrub/btree.c
> +++ b/fs/xfs/scrub/btree.c
> @@ -452,32 +452,41 @@ xchk_btree_check_minrecs(
>  	int			level,
>  	struct xfs_btree_block	*block)
>  {
> -	unsigned int		numrecs;
> -	int			ok_level;
> -
> -	numrecs = be16_to_cpu(block->bb_numrecs);
> +	struct xfs_btree_cur	*cur = bs->cur;
> +	unsigned int		root_level = cur->bc_nlevels - 1;
> +	unsigned int		numrecs = be16_to_cpu(block->bb_numrecs);
>  
>  	/* More records than minrecs means the block is ok. */
> -	if (numrecs >= bs->cur->bc_ops->get_minrecs(bs->cur, level))
> +	if (numrecs >= cur->bc_ops->get_minrecs(cur, level))
>  		return;
>  
>  	/*
> -	 * Certain btree blocks /can/ have fewer than minrecs records.  Any
> -	 * level greater than or equal to the level of the highest dedicated
> -	 * btree block are allowed to violate this constraint.
> -	 *
> -	 * For a btree rooted in a block, the btree root can have fewer than
> -	 * minrecs records.  If the btree is rooted in an inode and does not
> -	 * store records in the root, the direct children of the root and the
> -	 * root itself can have fewer than minrecs records.
> +	 * For btrees rooted in the inode, it's possible that the root block
> +	 * contents spilled into a regular ondisk block because there wasn't
> +	 * enough space in the inode root.  The number of records in that
> +	 * child block might be less than the standard minrecs, but that's ok
> +	 * provided that there's only one direct child of the root.
>  	 */
> -	ok_level = bs->cur->bc_nlevels - 1;
> -	if (bs->cur->bc_flags & XFS_BTREE_ROOT_IN_INODE)
> -		ok_level--;
> -	if (level >= ok_level)
> +	if ((cur->bc_flags & XFS_BTREE_ROOT_IN_INODE) &&
> +	    level == cur->bc_nlevels - 2) {
> +		struct xfs_btree_block	*root_block;
> +		struct xfs_buf		*root_bp;
> +		int			root_maxrecs;
> +
> +		root_block = xfs_btree_get_block(cur, root_level, &root_bp);
> +		root_maxrecs = cur->bc_ops->get_dmaxrecs(cur, root_level);
> +		if (be16_to_cpu(root_block->bb_numrecs) != 1 ||
> +		    numrecs <= root_maxrecs)
> +			xchk_btree_set_corrupt(bs->sc, cur, level);
>  		return;
> +	}
>  
> -	xchk_btree_set_corrupt(bs->sc, bs->cur, level);
> +	/*
> +	 * Otherwise, only the root level is allowed to have fewer than minrecs
> +	 * records or keyptrs.
> +	 */
> +	if (level < root_level)
> +		xchk_btree_set_corrupt(bs->sc, cur, level);
>  }
>  
>  /*
> 
> 


-- 
chandan

Re: [PATCH 2/4] xfs: fix the minrecs logic when dealing with inode root child blocks

[Christoph Hellwig]

On Mon, Nov 09, 2020 at 10:17:45AM -0800, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> The comment and logic in xchk_btree_check_minrecs for dealing with
> inode-rooted btrees isn't quite correct.  While the direct children of
> the inode root are allowed to have fewer records than what would
> normally be allowed for a regular ondisk btree block, this is only true
> if there is only one child block and the number of records don't fit in
> the inode root.
> 
> Fixes: 08a3a692ef58 ("xfs: btree scrub should check minrecs")
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>

Looks good,

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

[PATCH 3/4] xfs: strengthen rmap record flags checking

[Darrick J. Wong]

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

We always know the correct state of the rmap record flags (attr, bmbt,
unwritten) so check them by direct comparison.

Fixes: d852657ccfc0 ("xfs: cross-reference reverse-mapping btree")
Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 fs/xfs/scrub/bmap.c |    8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)


diff --git a/fs/xfs/scrub/bmap.c b/fs/xfs/scrub/bmap.c
index 412e2ec55e38..fed56d213a3f 100644
--- a/fs/xfs/scrub/bmap.c
+++ b/fs/xfs/scrub/bmap.c
@@ -218,13 +218,13 @@ xchk_bmap_xref_rmap(
 	 * which doesn't track unwritten state.
 	 */
 	if (owner != XFS_RMAP_OWN_COW &&
-	    irec->br_state == XFS_EXT_UNWRITTEN &&
-	    !(rmap.rm_flags & XFS_RMAP_UNWRITTEN))
+	    !!(irec->br_state == XFS_EXT_UNWRITTEN) !=
+	    !!(rmap.rm_flags & XFS_RMAP_UNWRITTEN))
 		xchk_fblock_xref_set_corrupt(info->sc, info->whichfork,
 				irec->br_startoff);
 
-	if (info->whichfork == XFS_ATTR_FORK &&
-	    !(rmap.rm_flags & XFS_RMAP_ATTR_FORK))
+	if (!!(info->whichfork == XFS_ATTR_FORK) !=
+	    !!(rmap.rm_flags & XFS_RMAP_ATTR_FORK))
 		xchk_fblock_xref_set_corrupt(info->sc, info->whichfork,
 				irec->br_startoff);
 	if (rmap.rm_flags & XFS_RMAP_BMBT_BLOCK)

Re: [PATCH 3/4] xfs: strengthen rmap record flags checking

[Chandan Babu R]

On Monday 9 November 2020 11:47:51 PM IST Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> We always know the correct state of the rmap record flags (attr, bmbt,
> unwritten) so check them by direct comparison.
>

The statement "operand1 == operand2" returns a 1 or 0 as its value. So the
"!!"  operation on the resulting value is probably not required. But still,
the changes are logically correct.

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

> Fixes: d852657ccfc0 ("xfs: cross-reference reverse-mapping btree")
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
>  fs/xfs/scrub/bmap.c |    8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> 
> diff --git a/fs/xfs/scrub/bmap.c b/fs/xfs/scrub/bmap.c
> index 412e2ec55e38..fed56d213a3f 100644
> --- a/fs/xfs/scrub/bmap.c
> +++ b/fs/xfs/scrub/bmap.c
> @@ -218,13 +218,13 @@ xchk_bmap_xref_rmap(
>  	 * which doesn't track unwritten state.
>  	 */
>  	if (owner != XFS_RMAP_OWN_COW &&
> -	    irec->br_state == XFS_EXT_UNWRITTEN &&
> -	    !(rmap.rm_flags & XFS_RMAP_UNWRITTEN))
> +	    !!(irec->br_state == XFS_EXT_UNWRITTEN) !=
> +	    !!(rmap.rm_flags & XFS_RMAP_UNWRITTEN))
>  		xchk_fblock_xref_set_corrupt(info->sc, info->whichfork,
>  				irec->br_startoff);
>  
> -	if (info->whichfork == XFS_ATTR_FORK &&
> -	    !(rmap.rm_flags & XFS_RMAP_ATTR_FORK))
> +	if (!!(info->whichfork == XFS_ATTR_FORK) !=
> +	    !!(rmap.rm_flags & XFS_RMAP_ATTR_FORK))
>  		xchk_fblock_xref_set_corrupt(info->sc, info->whichfork,
>  				irec->br_startoff);
>  	if (rmap.rm_flags & XFS_RMAP_BMBT_BLOCK)
> 
> 


-- 
chandan

Re: [PATCH 3/4] xfs: strengthen rmap record flags checking

[Christoph Hellwig]

On Mon, Nov 09, 2020 at 10:17:51AM -0800, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> We always know the correct state of the rmap record flags (attr, bmbt,
> unwritten) so check them by direct comparison.
> 
> Fixes: d852657ccfc0 ("xfs: cross-reference reverse-mapping btree")
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>

Looks good,

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

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

[Darrick J. Wong]

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(-)


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;

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

[Chandan Babu R]

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


-- 
chandan

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

[Christoph Hellwig]

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>