[PATCH 0/3] xfs: fix serious bugs in rmap key comparison

[PATCH 0/3] xfs: fix serious bugs in rmap key comparison

[Darrick J. Wong]

Hi all,

Last week I found some time to spend auditing the effectiveness of the
online repair prototype code, and discovered a serious bug in the rmap
code.  Each btree type provides four comparison predicates: one for
comparing a key against the current record; one for comparing two
arbitrary record keys, and one each for checking if a btree block's
records or keys are in the correct order.

Unfortunately, I encoded a major thinko into those last three functions.
The XFS_RMAP_OFF macro masks off the three namespace bits before we
perform a comparison, which means that key comparisons do not notice
differences between the unwritten, bmbt, or attr fork status.  On a
consistent filesystem this is not an issue because there can only ever
be overlapping rmap records for written inode data fork extents, which
is why we've not yet seen any problems in the field.

Fortunately, the last two functions are used by debugging asserts and
online scrub to check the contents of a btree block, so the severity of
the flaw there is not high.

Unfortunately, the flaw in _diff_two_keys is more severe, because it is
used for query_range requests.  Ranged queries are used by the regular
rmap handling code when reflink is enabled; and it is used in the rmap
btree validation routines of both xfs_scrub and xfs_repair.  As I
mentioned above, the flaw should not manifest on a *consistent*
filesystem, but for fuzzed (or corrupt) filesystems, this seriously
impacts our ability to detect problems.

The first two patches in this series fix two places where we pass the
wrong flags arguments to the rmap query functions (which didn't
previously cause lookup failures due to the broken code) and the third
patch fixes the comparison functions.

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=rmap-fixes-5.10
---
 fs/xfs/libxfs/xfs_rmap.c       |    2 +-
 fs/xfs/libxfs/xfs_rmap_btree.c |   16 ++++++++--------
 fs/xfs/scrub/bmap.c            |    2 ++
 3 files changed, 11 insertions(+), 9 deletions(-)

[PATCH 1/3] xfs: fix flags argument to rmap lookup when converting shared file rmaps

[Darrick J. Wong]

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

Pass the same oldext argument (which contains the existing rmapping's
unwritten state) to xfs_rmap_lookup_le_range at the start of
xfs_rmap_convert_shared.  At this point in the code, flags is zero,
which means that we perform lookups using the wrong key.

Fixes: 3f165b334e51 ("xfs: convert unwritten status of reverse mappings for shared files")
Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 fs/xfs/libxfs/xfs_rmap.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)


diff --git a/fs/xfs/libxfs/xfs_rmap.c b/fs/xfs/libxfs/xfs_rmap.c
index 340c83f76c80..2668ebe02865 100644
--- a/fs/xfs/libxfs/xfs_rmap.c
+++ b/fs/xfs/libxfs/xfs_rmap.c
@@ -1514,7 +1514,7 @@ xfs_rmap_convert_shared(
 	 * record for our insertion point. This will also give us the record for
 	 * start block contiguity tests.
 	 */
-	error = xfs_rmap_lookup_le_range(cur, bno, owner, offset, flags,
+	error = xfs_rmap_lookup_le_range(cur, bno, owner, offset, oldext,
 			&PREV, &i);
 	if (error)
 		goto done;

Re: [PATCH 1/3] xfs: fix flags argument to rmap lookup when converting shared file rmaps

[Christoph Hellwig]

On Mon, Nov 09, 2020 at 10:17:15AM -0800, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Pass the same oldext argument (which contains the existing rmapping's
> unwritten state) to xfs_rmap_lookup_le_range at the start of
> xfs_rmap_convert_shared.  At this point in the code, flags is zero,
> which means that we perform lookups using the wrong key.

Looks good,

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

[PATCH 2/3] xfs: set the unwritten bit in rmap lookup flags in xchk_bmap_get_rmapextents

[Darrick J. Wong]

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

When the bmbt scrubber is looking up rmap extents, we need to set the
extent flags from the bmbt record fully.  This will matter once we fix
the rmap btree comparison functions to check those flags correctly.

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


diff --git a/fs/xfs/scrub/bmap.c b/fs/xfs/scrub/bmap.c
index 955302e7cdde..412e2ec55e38 100644
--- a/fs/xfs/scrub/bmap.c
+++ b/fs/xfs/scrub/bmap.c
@@ -113,6 +113,8 @@ xchk_bmap_get_rmap(
 
 	if (info->whichfork == XFS_ATTR_FORK)
 		rflags |= XFS_RMAP_ATTR_FORK;
+	if (irec->br_state == XFS_EXT_UNWRITTEN)
+		rflags |= XFS_RMAP_UNWRITTEN;
 
 	/*
 	 * CoW staging extents are owned (on disk) by the refcountbt, so

Re: [PATCH 2/3] xfs: set the unwritten bit in rmap lookup flags in xchk_bmap_get_rmapextents

[Christoph Hellwig]

On Mon, Nov 09, 2020 at 10:17:22AM -0800, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> When the bmbt scrubber is looking up rmap extents, we need to set the
> extent flags from the bmbt record fully.  This will matter once we fix
> the rmap btree comparison functions to check those flags correctly.
> 
> 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 3/3] xfs: fix rmap key and record comparison functions

[Darrick J. Wong]

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

Keys for extent interval records in the reverse mapping btree are
supposed to be computed as follows:

(physical block, owner, fork, is_btree, is_unwritten, offset)

This provides users the ability to look up a reverse mapping from a bmbt
record -- start with the physical block; then if there are multiple
records for the same block, move on to the owner; then the inode fork
type; and so on to the file offset.

However, the key comparison functions incorrectly remove the
fork/btree/unwritten information that's encoded in the on-disk offset.
This means that lookup comparisons are only done with:

(physical block, owner, offset)

This means that queries can return incorrect results.  On consistent
filesystems this hasn't been an issue because blocks are never shared
between forks or with bmbt blocks; and are never unwritten.  However,
this bug means that online repair cannot always detect corruption in the
key information in internal rmapbt nodes.

Found by fuzzing keys[1].attrfork = ones on xfs/371.

Fixes: 4b8ed67794fe ("xfs: add rmap btree operations")
Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 fs/xfs/libxfs/xfs_rmap_btree.c |   16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)


diff --git a/fs/xfs/libxfs/xfs_rmap_btree.c b/fs/xfs/libxfs/xfs_rmap_btree.c
index beb81c84a937..577a66381327 100644
--- a/fs/xfs/libxfs/xfs_rmap_btree.c
+++ b/fs/xfs/libxfs/xfs_rmap_btree.c
@@ -243,8 +243,8 @@ xfs_rmapbt_key_diff(
 	else if (y > x)
 		return -1;
 
-	x = XFS_RMAP_OFF(be64_to_cpu(kp->rm_offset));
-	y = rec->rm_offset;
+	x = be64_to_cpu(kp->rm_offset);
+	y = xfs_rmap_irec_offset_pack(rec);
 	if (x > y)
 		return 1;
 	else if (y > x)
@@ -275,8 +275,8 @@ xfs_rmapbt_diff_two_keys(
 	else if (y > x)
 		return -1;
 
-	x = XFS_RMAP_OFF(be64_to_cpu(kp1->rm_offset));
-	y = XFS_RMAP_OFF(be64_to_cpu(kp2->rm_offset));
+	x = be64_to_cpu(kp1->rm_offset);
+	y = be64_to_cpu(kp2->rm_offset);
 	if (x > y)
 		return 1;
 	else if (y > x)
@@ -390,8 +390,8 @@ xfs_rmapbt_keys_inorder(
 		return 1;
 	else if (a > b)
 		return 0;
-	a = XFS_RMAP_OFF(be64_to_cpu(k1->rmap.rm_offset));
-	b = XFS_RMAP_OFF(be64_to_cpu(k2->rmap.rm_offset));
+	a = be64_to_cpu(k1->rmap.rm_offset);
+	b = be64_to_cpu(k2->rmap.rm_offset);
 	if (a <= b)
 		return 1;
 	return 0;
@@ -420,8 +420,8 @@ xfs_rmapbt_recs_inorder(
 		return 1;
 	else if (a > b)
 		return 0;
-	a = XFS_RMAP_OFF(be64_to_cpu(r1->rmap.rm_offset));
-	b = XFS_RMAP_OFF(be64_to_cpu(r2->rmap.rm_offset));
+	a = be64_to_cpu(r1->rmap.rm_offset);
+	b = be64_to_cpu(r2->rmap.rm_offset);
 	if (a <= b)
 		return 1;
 	return 0;

Re: [PATCH 3/3] xfs: fix rmap key and record comparison functions

[Christoph Hellwig]

On Mon, Nov 09, 2020 at 10:17:28AM -0800, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Keys for extent interval records in the reverse mapping btree are
> supposed to be computed as follows:
> 
> (physical block, owner, fork, is_btree, is_unwritten, offset)
> 
> This provides users the ability to look up a reverse mapping from a bmbt
> record -- start with the physical block; then if there are multiple
> records for the same block, move on to the owner; then the inode fork
> type; and so on to the file offset.
> 
> However, the key comparison functions incorrectly remove the

s/remove/removes/ ?

Otherwise looks good:

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