[1/7] xfs_repair: replace verify_inum with libxfs inode validators
diff mbox series

Message ID 158086360402.2079685.8627541630086580270.stgit@magnolia
State Accepted
Headers show
Series
  • xfs_repair: do not trash valid root dirs
Related show

Commit Message

Darrick J. Wong Feb. 5, 2020, 12:46 a.m. UTC
From: Darrick J. Wong <darrick.wong@oracle.com>

Repair uses the verify_inum function to validate inode numbers that it
finds in the superblock and in directories.  libxfs now has validator
functions to cover that kind of thing, so remove verify_inum().  As a
side bonus, this means that we will flag directories that point to the
quota/realtime metadata inodes.

This fixes a regression found by fuzzing u3.sfdir3.hdr.parent.i4 to
lastbit (aka making a directory's .. point to the user quota inode) in
xfs/384.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 libxfs/libxfs_api_defs.h |    1 +
 repair/dino_chunks.c     |    2 +-
 repair/dinode.c          |   29 -----------------------------
 repair/dinode.h          |    4 ----
 repair/dir2.c            |    7 +++----
 repair/phase4.c          |   12 ++++++------
 repair/phase6.c          |    8 ++++----
 7 files changed, 15 insertions(+), 48 deletions(-)

Comments

Darrick J. Wong Feb. 5, 2020, 12:50 a.m. UTC | #1
On Tue, Feb 04, 2020 at 04:46:44PM -0800, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Repair uses the verify_inum function to validate inode numbers that it
> finds in the superblock and in directories.  libxfs now has validator
> functions to cover that kind of thing, so remove verify_inum().  As a
> side bonus, this means that we will flag directories that point to the
> quota/realtime metadata inodes.
> 
> This fixes a regression found by fuzzing u3.sfdir3.hdr.parent.i4 to
> lastbit (aka making a directory's .. point to the user quota inode) in
> xfs/384.

Whoops, this was supposed to be in the previous series, not this one.

--D

> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
>  libxfs/libxfs_api_defs.h |    1 +
>  repair/dino_chunks.c     |    2 +-
>  repair/dinode.c          |   29 -----------------------------
>  repair/dinode.h          |    4 ----
>  repair/dir2.c            |    7 +++----
>  repair/phase4.c          |   12 ++++++------
>  repair/phase6.c          |    8 ++++----
>  7 files changed, 15 insertions(+), 48 deletions(-)
> 
> 
> diff --git a/libxfs/libxfs_api_defs.h b/libxfs/libxfs_api_defs.h
> index 6e09685b..9daf2635 100644
> --- a/libxfs/libxfs_api_defs.h
> +++ b/libxfs/libxfs_api_defs.h
> @@ -176,6 +176,7 @@
>  #define xfs_trans_roll			libxfs_trans_roll
>  
>  #define xfs_verify_cksum		libxfs_verify_cksum
> +#define xfs_verify_dir_ino		libxfs_verify_dir_ino
>  #define xfs_verify_ino			libxfs_verify_ino
>  #define xfs_verify_rtbno		libxfs_verify_rtbno
>  #define xfs_zero_extent			libxfs_zero_extent
> diff --git a/repair/dino_chunks.c b/repair/dino_chunks.c
> index 00b67468..dbf3d37a 100644
> --- a/repair/dino_chunks.c
> +++ b/repair/dino_chunks.c
> @@ -65,7 +65,7 @@ check_aginode_block(xfs_mount_t	*mp,
>   * inode chunk.  returns number of new inodes if things are good
>   * and 0 if bad.  start is the start of the discovered inode chunk.
>   * routine assumes that ino is a legal inode number
> - * (verified by verify_inum()).  If the inode chunk turns out
> + * (verified by libxfs_verify_ino()).  If the inode chunk turns out
>   * to be good, this routine will put the inode chunk into
>   * the good inode chunk tree if required.
>   *
> diff --git a/repair/dinode.c b/repair/dinode.c
> index 8af2cb25..0d9c96be 100644
> --- a/repair/dinode.c
> +++ b/repair/dinode.c
> @@ -171,35 +171,6 @@ verify_ag_bno(xfs_sb_t *sbp,
>  	return 1;
>  }
>  
> -/*
> - * returns 0 if inode number is valid, 1 if bogus
> - */
> -int
> -verify_inum(xfs_mount_t		*mp,
> -		xfs_ino_t	ino)
> -{
> -	xfs_agnumber_t	agno;
> -	xfs_agino_t	agino;
> -	xfs_agblock_t	agbno;
> -	xfs_sb_t	*sbp = &mp->m_sb;;
> -
> -	/* range check ag #, ag block.  range-checking offset is pointless */
> -
> -	agno = XFS_INO_TO_AGNO(mp, ino);
> -	agino = XFS_INO_TO_AGINO(mp, ino);
> -	agbno = XFS_AGINO_TO_AGBNO(mp, agino);
> -	if (agbno == 0)
> -		return 1;
> -
> -	if (ino == 0 || ino == NULLFSINO)
> -		return(1);
> -
> -	if (ino != XFS_AGINO_TO_INO(mp, agno, agino))
> -		return(1);
> -
> -	return verify_ag_bno(sbp, agno, agbno);
> -}
> -
>  /*
>   * have a separate routine to ensure that we don't accidentally
>   * lose illegally set bits in the agino by turning it into an FSINO
> diff --git a/repair/dinode.h b/repair/dinode.h
> index aa177465..98238357 100644
> --- a/repair/dinode.h
> +++ b/repair/dinode.h
> @@ -77,10 +77,6 @@ verify_uncertain_dinode(xfs_mount_t *mp,
>  		xfs_agnumber_t agno,
>  		xfs_agino_t ino);
>  
> -int
> -verify_inum(xfs_mount_t		*mp,
> -		xfs_ino_t	ino);
> -
>  int
>  verify_aginum(xfs_mount_t	*mp,
>  		xfs_agnumber_t	agno,
> diff --git a/repair/dir2.c b/repair/dir2.c
> index e43a9732..723aee1f 100644
> --- a/repair/dir2.c
> +++ b/repair/dir2.c
> @@ -215,7 +215,7 @@ process_sf_dir2(
>  		if (lino == ino) {
>  			junkit = 1;
>  			junkreason = _("current");
> -		} else if (verify_inum(mp, lino)) {
> +		} else if (!libxfs_verify_dir_ino(mp, lino)) {
>  			junkit = 1;
>  			junkreason = _("invalid");
>  		} else if (lino == mp->m_sb.sb_rbmino)  {
> @@ -486,8 +486,7 @@ _("corrected entry offsets in directory %" PRIu64 "\n"),
>  	 * If the validation fails for the root inode we fix it in
>  	 * the next else case.
>  	 */
> -	if (verify_inum(mp, *parent) && ino != mp->m_sb.sb_rootino)  {
> -
> +	if (!libxfs_verify_dir_ino(mp, *parent) && ino != mp->m_sb.sb_rootino) {
>  		do_warn(
>  _("bogus .. inode number (%" PRIu64 ") in directory inode %" PRIu64 ", "),
>  				*parent, ino);
> @@ -674,7 +673,7 @@ process_dir2_data(
>  			 * (or did it ourselves) during phase 3.
>  			 */
>  			clearino = 0;
> -		} else if (verify_inum(mp, ent_ino)) {
> +		} else if (!libxfs_verify_dir_ino(mp, ent_ino)) {
>  			/*
>  			 * Bad inode number.  Clear the inode number and the
>  			 * entry will get removed later.  We don't trash the
> diff --git a/repair/phase4.c b/repair/phase4.c
> index e1ba778f..8197db06 100644
> --- a/repair/phase4.c
> +++ b/repair/phase4.c
> @@ -36,7 +36,7 @@ quotino_check(xfs_mount_t *mp)
>  	ino_tree_node_t *irec;
>  
>  	if (mp->m_sb.sb_uquotino != NULLFSINO && mp->m_sb.sb_uquotino != 0)  {
> -		if (verify_inum(mp, mp->m_sb.sb_uquotino))
> +		if (!libxfs_verify_ino(mp, mp->m_sb.sb_uquotino))
>  			irec = NULL;
>  		else
>  			irec = find_inode_rec(mp,
> @@ -52,7 +52,7 @@ quotino_check(xfs_mount_t *mp)
>  	}
>  
>  	if (mp->m_sb.sb_gquotino != NULLFSINO && mp->m_sb.sb_gquotino != 0)  {
> -		if (verify_inum(mp, mp->m_sb.sb_gquotino))
> +		if (!libxfs_verify_ino(mp, mp->m_sb.sb_gquotino))
>  			irec = NULL;
>  		else
>  			irec = find_inode_rec(mp,
> @@ -68,7 +68,7 @@ quotino_check(xfs_mount_t *mp)
>  	}
>  
>  	if (mp->m_sb.sb_pquotino != NULLFSINO && mp->m_sb.sb_pquotino != 0)  {
> -		if (verify_inum(mp, mp->m_sb.sb_pquotino))
> +		if (!libxfs_verify_ino(mp, mp->m_sb.sb_pquotino))
>  			irec = NULL;
>  		else
>  			irec = find_inode_rec(mp,
> @@ -112,9 +112,9 @@ quota_sb_check(xfs_mount_t *mp)
>  	    (mp->m_sb.sb_pquotino == NULLFSINO || mp->m_sb.sb_pquotino == 0))  {
>  		lost_quotas = 1;
>  		fs_quotas = 0;
> -	} else if (!verify_inum(mp, mp->m_sb.sb_uquotino) &&
> -			!verify_inum(mp, mp->m_sb.sb_gquotino) &&
> -			!verify_inum(mp, mp->m_sb.sb_pquotino)) {
> +	} else if (libxfs_verify_ino(mp, mp->m_sb.sb_uquotino) &&
> +		   libxfs_verify_ino(mp, mp->m_sb.sb_gquotino) &&
> +		   libxfs_verify_ino(mp, mp->m_sb.sb_pquotino)) {
>  		fs_quotas = 1;
>  	}
>  }
> diff --git a/repair/phase6.c b/repair/phase6.c
> index 0874b649..70135694 100644
> --- a/repair/phase6.c
> +++ b/repair/phase6.c
> @@ -1814,7 +1814,7 @@ longform_dir2_entry_check_data(
>  			}
>  			continue;
>  		}
> -		ASSERT(no_modify || !verify_inum(mp, inum));
> +		ASSERT(no_modify || libxfs_verify_dir_ino(mp, inum));
>  		/*
>  		 * special case the . entry.  we know there's only one
>  		 * '.' and only '.' points to itself because bogus entries
> @@ -1845,7 +1845,7 @@ longform_dir2_entry_check_data(
>  		/*
>  		 * skip entries with bogus inumbers if we're in no modify mode
>  		 */
> -		if (no_modify && verify_inum(mp, inum))
> +		if (no_modify && !libxfs_verify_dir_ino(mp, inum))
>  			continue;
>  
>  		/* validate ftype field if supported */
> @@ -2634,14 +2634,14 @@ shortform_dir2_entry_check(xfs_mount_t	*mp,
>  		fname[sfep->namelen] = '\0';
>  
>  		ASSERT(no_modify || (lino != NULLFSINO && lino != 0));
> -		ASSERT(no_modify || !verify_inum(mp, lino));
> +		ASSERT(no_modify || libxfs_verify_dir_ino(mp, lino));
>  
>  		/*
>  		 * Also skip entries with bogus inode numbers if we're
>  		 * in no modify mode.
>  		 */
>  
> -		if (no_modify && verify_inum(mp, lino))  {
> +		if (no_modify && !libxfs_verify_dir_ino(mp, lino))  {
>  			next_sfep = libxfs_dir2_sf_nextentry(mp, sfp, sfep);
>  			continue;
>  		}
>
Christoph Hellwig Feb. 17, 2020, 1:50 p.m. UTC | #2
On Tue, Feb 04, 2020 at 04:46:44PM -0800, Darrick J. Wong wrote:
> This fixes a regression found by fuzzing u3.sfdir3.hdr.parent.i4 to
> lastbit (aka making a directory's .. point to the user quota inode) in
> xfs/384.

Is that a bug or a regression?  If the latter, what commit caused the
regression?

Otherwise looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>
Darrick J. Wong Feb. 19, 2020, 4:32 a.m. UTC | #3
On Mon, Feb 17, 2020 at 05:50:01AM -0800, Christoph Hellwig wrote:
> On Tue, Feb 04, 2020 at 04:46:44PM -0800, Darrick J. Wong wrote:
> > This fixes a regression found by fuzzing u3.sfdir3.hdr.parent.i4 to
> > lastbit (aka making a directory's .. point to the user quota inode) in
> > xfs/384.
> 
> Is that a bug or a regression?  If the latter, what commit caused the
> regression?

Eh, it's a bug found by a fuzzer fstest, so I guess this should be
reworded somehwat:

"This fixes a bug found by fuzzing..."

--D

> Otherwise looks good:
> 
> Reviewed-by: Christoph Hellwig <hch@lst.de>
Eric Sandeen Feb. 26, 2020, 4:55 p.m. UTC | #4
On 2/4/20 4:46 PM, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Repair uses the verify_inum function to validate inode numbers that it
> finds in the superblock and in directories.  libxfs now has validator
> functions to cover that kind of thing, so remove verify_inum().  As a
> side bonus, this means that we will flag directories that point to the
> quota/realtime metadata inodes.
> 
> This fixes a regression found by fuzzing u3.sfdir3.hdr.parent.i4 to
> lastbit (aka making a directory's .. point to the user quota inode) in
> xfs/384.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>

(I wanted to be sure 0 and NULLFSINO were still properly rejected; they are.)

I'll do my best to remember to edit the changelog, and

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

Patch
diff mbox series

diff --git a/libxfs/libxfs_api_defs.h b/libxfs/libxfs_api_defs.h
index 6e09685b..9daf2635 100644
--- a/libxfs/libxfs_api_defs.h
+++ b/libxfs/libxfs_api_defs.h
@@ -176,6 +176,7 @@ 
 #define xfs_trans_roll			libxfs_trans_roll
 
 #define xfs_verify_cksum		libxfs_verify_cksum
+#define xfs_verify_dir_ino		libxfs_verify_dir_ino
 #define xfs_verify_ino			libxfs_verify_ino
 #define xfs_verify_rtbno		libxfs_verify_rtbno
 #define xfs_zero_extent			libxfs_zero_extent
diff --git a/repair/dino_chunks.c b/repair/dino_chunks.c
index 00b67468..dbf3d37a 100644
--- a/repair/dino_chunks.c
+++ b/repair/dino_chunks.c
@@ -65,7 +65,7 @@  check_aginode_block(xfs_mount_t	*mp,
  * inode chunk.  returns number of new inodes if things are good
  * and 0 if bad.  start is the start of the discovered inode chunk.
  * routine assumes that ino is a legal inode number
- * (verified by verify_inum()).  If the inode chunk turns out
+ * (verified by libxfs_verify_ino()).  If the inode chunk turns out
  * to be good, this routine will put the inode chunk into
  * the good inode chunk tree if required.
  *
diff --git a/repair/dinode.c b/repair/dinode.c
index 8af2cb25..0d9c96be 100644
--- a/repair/dinode.c
+++ b/repair/dinode.c
@@ -171,35 +171,6 @@  verify_ag_bno(xfs_sb_t *sbp,
 	return 1;
 }
 
-/*
- * returns 0 if inode number is valid, 1 if bogus
- */
-int
-verify_inum(xfs_mount_t		*mp,
-		xfs_ino_t	ino)
-{
-	xfs_agnumber_t	agno;
-	xfs_agino_t	agino;
-	xfs_agblock_t	agbno;
-	xfs_sb_t	*sbp = &mp->m_sb;;
-
-	/* range check ag #, ag block.  range-checking offset is pointless */
-
-	agno = XFS_INO_TO_AGNO(mp, ino);
-	agino = XFS_INO_TO_AGINO(mp, ino);
-	agbno = XFS_AGINO_TO_AGBNO(mp, agino);
-	if (agbno == 0)
-		return 1;
-
-	if (ino == 0 || ino == NULLFSINO)
-		return(1);
-
-	if (ino != XFS_AGINO_TO_INO(mp, agno, agino))
-		return(1);
-
-	return verify_ag_bno(sbp, agno, agbno);
-}
-
 /*
  * have a separate routine to ensure that we don't accidentally
  * lose illegally set bits in the agino by turning it into an FSINO
diff --git a/repair/dinode.h b/repair/dinode.h
index aa177465..98238357 100644
--- a/repair/dinode.h
+++ b/repair/dinode.h
@@ -77,10 +77,6 @@  verify_uncertain_dinode(xfs_mount_t *mp,
 		xfs_agnumber_t agno,
 		xfs_agino_t ino);
 
-int
-verify_inum(xfs_mount_t		*mp,
-		xfs_ino_t	ino);
-
 int
 verify_aginum(xfs_mount_t	*mp,
 		xfs_agnumber_t	agno,
diff --git a/repair/dir2.c b/repair/dir2.c
index e43a9732..723aee1f 100644
--- a/repair/dir2.c
+++ b/repair/dir2.c
@@ -215,7 +215,7 @@  process_sf_dir2(
 		if (lino == ino) {
 			junkit = 1;
 			junkreason = _("current");
-		} else if (verify_inum(mp, lino)) {
+		} else if (!libxfs_verify_dir_ino(mp, lino)) {
 			junkit = 1;
 			junkreason = _("invalid");
 		} else if (lino == mp->m_sb.sb_rbmino)  {
@@ -486,8 +486,7 @@  _("corrected entry offsets in directory %" PRIu64 "\n"),
 	 * If the validation fails for the root inode we fix it in
 	 * the next else case.
 	 */
-	if (verify_inum(mp, *parent) && ino != mp->m_sb.sb_rootino)  {
-
+	if (!libxfs_verify_dir_ino(mp, *parent) && ino != mp->m_sb.sb_rootino) {
 		do_warn(
 _("bogus .. inode number (%" PRIu64 ") in directory inode %" PRIu64 ", "),
 				*parent, ino);
@@ -674,7 +673,7 @@  process_dir2_data(
 			 * (or did it ourselves) during phase 3.
 			 */
 			clearino = 0;
-		} else if (verify_inum(mp, ent_ino)) {
+		} else if (!libxfs_verify_dir_ino(mp, ent_ino)) {
 			/*
 			 * Bad inode number.  Clear the inode number and the
 			 * entry will get removed later.  We don't trash the
diff --git a/repair/phase4.c b/repair/phase4.c
index e1ba778f..8197db06 100644
--- a/repair/phase4.c
+++ b/repair/phase4.c
@@ -36,7 +36,7 @@  quotino_check(xfs_mount_t *mp)
 	ino_tree_node_t *irec;
 
 	if (mp->m_sb.sb_uquotino != NULLFSINO && mp->m_sb.sb_uquotino != 0)  {
-		if (verify_inum(mp, mp->m_sb.sb_uquotino))
+		if (!libxfs_verify_ino(mp, mp->m_sb.sb_uquotino))
 			irec = NULL;
 		else
 			irec = find_inode_rec(mp,
@@ -52,7 +52,7 @@  quotino_check(xfs_mount_t *mp)
 	}
 
 	if (mp->m_sb.sb_gquotino != NULLFSINO && mp->m_sb.sb_gquotino != 0)  {
-		if (verify_inum(mp, mp->m_sb.sb_gquotino))
+		if (!libxfs_verify_ino(mp, mp->m_sb.sb_gquotino))
 			irec = NULL;
 		else
 			irec = find_inode_rec(mp,
@@ -68,7 +68,7 @@  quotino_check(xfs_mount_t *mp)
 	}
 
 	if (mp->m_sb.sb_pquotino != NULLFSINO && mp->m_sb.sb_pquotino != 0)  {
-		if (verify_inum(mp, mp->m_sb.sb_pquotino))
+		if (!libxfs_verify_ino(mp, mp->m_sb.sb_pquotino))
 			irec = NULL;
 		else
 			irec = find_inode_rec(mp,
@@ -112,9 +112,9 @@  quota_sb_check(xfs_mount_t *mp)
 	    (mp->m_sb.sb_pquotino == NULLFSINO || mp->m_sb.sb_pquotino == 0))  {
 		lost_quotas = 1;
 		fs_quotas = 0;
-	} else if (!verify_inum(mp, mp->m_sb.sb_uquotino) &&
-			!verify_inum(mp, mp->m_sb.sb_gquotino) &&
-			!verify_inum(mp, mp->m_sb.sb_pquotino)) {
+	} else if (libxfs_verify_ino(mp, mp->m_sb.sb_uquotino) &&
+		   libxfs_verify_ino(mp, mp->m_sb.sb_gquotino) &&
+		   libxfs_verify_ino(mp, mp->m_sb.sb_pquotino)) {
 		fs_quotas = 1;
 	}
 }
diff --git a/repair/phase6.c b/repair/phase6.c
index 0874b649..70135694 100644
--- a/repair/phase6.c
+++ b/repair/phase6.c
@@ -1814,7 +1814,7 @@  longform_dir2_entry_check_data(
 			}
 			continue;
 		}
-		ASSERT(no_modify || !verify_inum(mp, inum));
+		ASSERT(no_modify || libxfs_verify_dir_ino(mp, inum));
 		/*
 		 * special case the . entry.  we know there's only one
 		 * '.' and only '.' points to itself because bogus entries
@@ -1845,7 +1845,7 @@  longform_dir2_entry_check_data(
 		/*
 		 * skip entries with bogus inumbers if we're in no modify mode
 		 */
-		if (no_modify && verify_inum(mp, inum))
+		if (no_modify && !libxfs_verify_dir_ino(mp, inum))
 			continue;
 
 		/* validate ftype field if supported */
@@ -2634,14 +2634,14 @@  shortform_dir2_entry_check(xfs_mount_t	*mp,
 		fname[sfep->namelen] = '\0';
 
 		ASSERT(no_modify || (lino != NULLFSINO && lino != 0));
-		ASSERT(no_modify || !verify_inum(mp, lino));
+		ASSERT(no_modify || libxfs_verify_dir_ino(mp, lino));
 
 		/*
 		 * Also skip entries with bogus inode numbers if we're
 		 * in no modify mode.
 		 */
 
-		if (no_modify && verify_inum(mp, lino))  {
+		if (no_modify && !libxfs_verify_dir_ino(mp, lino))  {
 			next_sfep = libxfs_dir2_sf_nextentry(mp, sfp, sfep);
 			continue;
 		}