[PATCH] xfs_repair: fix off by one error when rebuilding high keys

[PATCH] xfs_repair: fix off by one error when rebuilding high keys

[Darrick J. Wong]

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

Fix an off-by-one error when scanning a rmap btree block for high keys
as part of rebuilding rmap btrees during phase 5.  This causes
xfs_repair to emit a corrupt filesystem, which is bad.

This can be reproduced pretty easily by exporting
TEST_XFS_REPAIR_REBUILD=1 and running generic/051 with a 1k block size.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 repair/phase5.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/repair/phase5.c b/repair/phase5.c
index 85d1f4fb..1bacfc7f 100644
--- a/repair/phase5.c
+++ b/repair/phase5.c
@@ -1500,7 +1500,7 @@ prop_rmap_highkey(
 		bt_key->rm_offset = cpu_to_be64(
 				libxfs_rmap_irec_offset_pack(&high_key));
 
-		for (i = 1; i < numrecs - 1; i++) {
+		for (i = 1; i <= numrecs; i++) {
 			bt_key = XFS_RMAP_HIGH_KEY_ADDR(bt_hdr, i);
 			key.rm_startblock = be32_to_cpu(bt_key->rm_startblock);
 			key.rm_owner = be64_to_cpu(bt_key->rm_owner);

Re: [PATCH] xfs_repair: fix off by one error when rebuilding high keys

[Eric Sandeen]

On 1/3/19 4:33 PM, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Fix an off-by-one error when scanning a rmap btree block for high keys
> as part of rebuilding rmap btrees during phase 5.  This causes
> xfs_repair to emit a corrupt filesystem, which is bad.
> 
> This can be reproduced pretty easily by exporting
> TEST_XFS_REPAIR_REBUILD=1 and running generic/051 with a 1k block size.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
>  repair/phase5.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/repair/phase5.c b/repair/phase5.c
> index 85d1f4fb..1bacfc7f 100644
> --- a/repair/phase5.c
> +++ b/repair/phase5.c
> @@ -1500,7 +1500,7 @@ prop_rmap_highkey(
>  		bt_key->rm_offset = cpu_to_be64(
>  				libxfs_rmap_irec_offset_pack(&high_key));
>  
> -		for (i = 1; i < numrecs - 1; i++) {
> +		for (i = 1; i <= numrecs; i++) {

isn't that actually off by 2? 

Why do all the XFS_RMAP_*() macros take a 1-based index and then subtract
1 to get back to 0-based?  Seems like that's the kind of oddity that leads
to bugs like this, no?  Maybe I'm not seeing the big picture.

-Eric

>  			bt_key = XFS_RMAP_HIGH_KEY_ADDR(bt_hdr, i);
>  			key.rm_startblock = be32_to_cpu(bt_key->rm_startblock);
>  			key.rm_owner = be64_to_cpu(bt_key->rm_owner);
> 

Re: [PATCH] xfs_repair: fix off by one error when rebuilding high keys

[Eric Sandeen]

On 1/3/19 4:33 PM, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Fix an off-by-one error when scanning a rmap btree block for high keys
> as part of rebuilding rmap btrees during phase 5.  This causes
> xfs_repair to emit a corrupt filesystem, which is bad.
> 
> This can be reproduced pretty easily by exporting
> TEST_XFS_REPAIR_REBUILD=1 and running generic/051 with a 1k block size.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>

Reviewed-by: Eric Sandeen <sandeen@redhat.com>

changing the weird 1-indexing does sound like a good idea tho.

> ---
>  repair/phase5.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/repair/phase5.c b/repair/phase5.c
> index 85d1f4fb..1bacfc7f 100644
> --- a/repair/phase5.c
> +++ b/repair/phase5.c
> @@ -1500,7 +1500,7 @@ prop_rmap_highkey(
>  		bt_key->rm_offset = cpu_to_be64(
>  				libxfs_rmap_irec_offset_pack(&high_key));
>  
> -		for (i = 1; i < numrecs - 1; i++) {
> +		for (i = 1; i <= numrecs; i++) {
>  			bt_key = XFS_RMAP_HIGH_KEY_ADDR(bt_hdr, i);
>  			key.rm_startblock = be32_to_cpu(bt_key->rm_startblock);
>  			key.rm_owner = be64_to_cpu(bt_key->rm_owner);
>