[5/7] xfs_repair: fix handling of data blocks colliding with existing metadata

Commit Message

Darrick J. Wong Sept. 7, 2020, 5:52 p.m. UTC

From: Darrick J. Wong <[e-mail]>

Prior to commit a406779bc8d8, any blocks in a data fork extent that
collided with existing blocks would cause the entire data fork extent to
be rejected.  Unfortunately, the patch to add data block sharing support
suppressed checking for any collision, including metadata.  What we
really wanted to do here during a check_dups==1 scan is to is check for
specific collisions and without updating the block mapping data.

So, move the check_dups test after the for-switch construction.  This
re-enables detecting collisions between data fork blocks and a
previously scanned chunk of metadata, and improves the specificity of
the error message that results.

This was found by fuzzing recs[2].free=zeroes in xfs/364, though this
patch alone does not solve all the problems that scenario presents.

Fixes: a406779bc8d8 ("xfs_repair: handle multiple owners of data blocks")
Signed-off-by: Darrick J. Wong <[e-mail]>
---
 repair/dinode.c |   33 +++++++++++----------------------
 1 file changed, 11 insertions(+), 22 deletions(-)

Comments

Christoph Hellwig Sept. 8, 2020, 2:52 p.m. UTC | #1

On Mon, Sep 07, 2020 at 10:52:28AM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <[e-mail]>
> 
> Prior to commit a406779bc8d8, any blocks in a data fork extent that
> collided with existing blocks would cause the entire data fork extent to
> be rejected.  Unfortunately, the patch to add data block sharing support
> suppressed checking for any collision, including metadata.  What we
> really wanted to do here during a check_dups==1 scan is to is check for
> specific collisions and without updating the block mapping data.
> 
> So, move the check_dups test after the for-switch construction.  This
> re-enables detecting collisions between data fork blocks and a
> previously scanned chunk of metadata, and improves the specificity of
> the error message that results.
> 
> This was found by fuzzing recs[2].free=zeroes in xfs/364, though this
> patch alone does not solve all the problems that scenario presents.
> 
> Fixes: a406779bc8d8 ("xfs_repair: handle multiple owners of data blocks")
> Signed-off-by: Darrick J. Wong <[e-mail]>

Looks good,

Reviewed-by: Christoph Hellwig <[e-mail]>

Patch

diff --git a/repair/dinode.c b/repair/dinode.c
index 1fe68bd41117..7577b50ffb2b 100644
--- a/repair/dinode.c
+++ b/repair/dinode.c
@@ -476,28 +476,6 @@  _("Fatal error: inode %" PRIu64 " - blkmap_set_ext(): %s\n"
 			locked_agno = agno;
 		}
 
-		if (check_dups) {
-			/*
-			 * if we're just checking the bmap for dups,
-			 * return if we find one, otherwise, continue
-			 * checking each entry without setting the
-			 * block bitmap
-			 */
-			if (!(type == XR_INO_DATA &&
-			    xfs_sb_version_hasreflink(&mp->m_sb)) &&
-			    search_dup_extent(agno, agbno, ebno)) {
-				do_warn(
-_("%s fork in ino %" PRIu64 " claims dup extent, "
-  "off - %" PRIu64 ", start - %" PRIu64 ", cnt %" PRIu64 "\n"),
-					forkname, ino, irec.br_startoff,
-					irec.br_startblock,
-					irec.br_blockcount);
-				goto done;
-			}
-			*tot += irec.br_blockcount;
-			continue;
-		}
-
 		for (b = irec.br_startblock;
 		     agbno < ebno;
 		     b += blen, agbno += blen) {
@@ -554,6 +532,17 @@  _("illegal state %d in block map %" PRIu64 "\n"),
 			}
 		}
 
+		if (check_dups) {
+			/*
+			 * If we're just checking the bmap for dups and we
+			 * didn't find any non-reflink collisions, update our
+			 * inode's block count and move on to the next extent.
+			 * We're not yet updating the block usage information.
+			 */
+			*tot += irec.br_blockcount;
+			continue;
+		}
+
 		/*
 		 * Update the internal extent map only after we've checked
 		 * every block in this extent.  The first time we reject this