[PATCH 0/3] xfs-4.19: superblock verifier cleanups

[PATCH 0/3] xfs-4.19: superblock verifier cleanups

[Darrick J. Wong]

Hi all,

This series refactors the superblock verification routines into three
predicates: one to handle checks that are only done at read time,
another for write time checks, and a third for checks common to both.
We then add some sanity checks for the summary counters to the write
verifier (a reworked version of Bill O'Donnell's earlier patch), and end
by addding a verifier for inode counts and adding that into the
superblock write checks.

Comments and questions are, as always, welcome.

--D

[PATCH 1/3] xfs: refactor superblock verifiers

[Darrick J. Wong]

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

Split the superblock verifier into the common checks, the read-time
checks, and the write-time check functions.  No functional changes, but
we're setting up to add more write-only checks.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 fs/xfs/libxfs/xfs_sb.c |  205 ++++++++++++++++++++++++++----------------------
 1 file changed, 112 insertions(+), 93 deletions(-)


diff --git a/fs/xfs/libxfs/xfs_sb.c b/fs/xfs/libxfs/xfs_sb.c
index b3ad15956366..516bef7b0f50 100644
--- a/fs/xfs/libxfs/xfs_sb.c
+++ b/fs/xfs/libxfs/xfs_sb.c
@@ -96,80 +96,96 @@ xfs_perag_put(
 	trace_xfs_perag_put(pag->pag_mount, pag->pag_agno, ref, _RET_IP_);
 }
 
-/*
- * Check the validity of the SB found.
- */
+/* Check all the superblock fields we care about when reading one in. */
 STATIC int
-xfs_mount_validate_sb(
-	xfs_mount_t	*mp,
-	xfs_sb_t	*sbp,
-	bool		check_inprogress,
-	bool		check_version)
+xfs_validate_sb_read(
+	struct xfs_mount	*mp,
+	struct xfs_sb		*sbp)
 {
-	uint32_t	agcount = 0;
-	uint32_t	rem;
-
-	if (sbp->sb_magicnum != XFS_SB_MAGIC) {
-		xfs_warn(mp, "bad magic number");
-		return -EWRONGFS;
-	}
-
-
-	if (!xfs_sb_good_version(sbp)) {
-		xfs_warn(mp, "bad version");
-		return -EWRONGFS;
-	}
+	if (!xfs_sb_version_hascrc(sbp))
+		return 0;
 
 	/*
-	 * Version 5 superblock feature mask validation. Reject combinations the
-	 * kernel cannot support up front before checking anything else. For
-	 * write validation, we don't need to check feature masks.
+	 * Version 5 superblock feature mask validation. Reject combinations
+	 * the kernel cannot support up front before checking anything else.
+	 * For write validation, we don't need to check feature masks.
 	 */
-	if (check_version && XFS_SB_VERSION_NUM(sbp) == XFS_SB_VERSION_5) {
-		if (xfs_sb_has_compat_feature(sbp,
-					XFS_SB_FEAT_COMPAT_UNKNOWN)) {
-			xfs_warn(mp,
+	if (xfs_sb_has_compat_feature(sbp, XFS_SB_FEAT_COMPAT_UNKNOWN)) {
+		xfs_warn(mp,
 "Superblock has unknown compatible features (0x%x) enabled.",
-				(sbp->sb_features_compat &
-						XFS_SB_FEAT_COMPAT_UNKNOWN));
-			xfs_warn(mp,
+			(sbp->sb_features_compat & XFS_SB_FEAT_COMPAT_UNKNOWN));
+		xfs_warn(mp,
 "Using a more recent kernel is recommended.");
-		}
+	}
 
-		if (xfs_sb_has_ro_compat_feature(sbp,
-					XFS_SB_FEAT_RO_COMPAT_UNKNOWN)) {
-			xfs_alert(mp,
+	if (xfs_sb_has_ro_compat_feature(sbp,
+				XFS_SB_FEAT_RO_COMPAT_UNKNOWN)) {
+		xfs_alert(mp,
 "Superblock has unknown read-only compatible features (0x%x) enabled.",
-				(sbp->sb_features_ro_compat &
-						XFS_SB_FEAT_RO_COMPAT_UNKNOWN));
-			if (!(mp->m_flags & XFS_MOUNT_RDONLY)) {
-				xfs_warn(mp,
+			(sbp->sb_features_ro_compat &
+					XFS_SB_FEAT_RO_COMPAT_UNKNOWN));
+		if (!(mp->m_flags & XFS_MOUNT_RDONLY)) {
+			xfs_warn(mp,
 "Attempted to mount read-only compatible filesystem read-write.");
-				xfs_warn(mp,
+			xfs_warn(mp,
 "Filesystem can only be safely mounted read only.");
 
-				return -EINVAL;
-			}
+			return -EINVAL;
 		}
-		if (xfs_sb_has_incompat_feature(sbp,
-					XFS_SB_FEAT_INCOMPAT_UNKNOWN)) {
-			xfs_warn(mp,
+	}
+	if (xfs_sb_has_incompat_feature(sbp,
+				XFS_SB_FEAT_INCOMPAT_UNKNOWN)) {
+		xfs_warn(mp,
 "Superblock has unknown incompatible features (0x%x) enabled.",
-				(sbp->sb_features_incompat &
-						XFS_SB_FEAT_INCOMPAT_UNKNOWN));
-			xfs_warn(mp,
+			(sbp->sb_features_incompat &
+					XFS_SB_FEAT_INCOMPAT_UNKNOWN));
+		xfs_warn(mp,
 "Filesystem can not be safely mounted by this kernel.");
-			return -EINVAL;
-		}
-	} else if (xfs_sb_version_hascrc(sbp)) {
-		/*
-		 * We can't read verify the sb LSN because the read verifier is
-		 * called before the log is allocated and processed. We know the
-		 * log is set up before write verifier (!check_version) calls,
-		 * so just check it here.
-		 */
-		if (!xfs_log_check_lsn(mp, sbp->sb_lsn))
-			return -EFSCORRUPTED;
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+/* Check all the superblock fields we care about when writing one out. */
+STATIC int
+xfs_validate_sb_write(
+	struct xfs_mount	*mp,
+	struct xfs_sb		*sbp)
+{
+	if (!xfs_sb_version_hascrc(sbp))
+		return 0;
+
+	/*
+	 * We can't read verify the sb LSN because the read verifier is called
+	 * before the log is allocated and processed. We know the log is set up
+	 * before write verifier (!check_version) calls, so just check it here.
+	 */
+	if (!xfs_log_check_lsn(mp, sbp->sb_lsn))
+		return -EFSCORRUPTED;
+
+	return 0;
+}
+
+/* Check the validity of the SB. */
+STATIC int
+xfs_validate_sb_common(
+	struct xfs_mount	*mp,
+	struct xfs_buf		*bp,
+	struct xfs_sb		*sbp)
+{
+	uint32_t		agcount = 0;
+	uint32_t		rem;
+
+	if (sbp->sb_magicnum != XFS_SB_MAGIC) {
+		xfs_warn(mp, "bad magic number");
+		return -EWRONGFS;
+	}
+
+
+	if (!xfs_sb_good_version(sbp)) {
+		xfs_warn(mp, "bad version");
+		return -EWRONGFS;
 	}
 
 	if (xfs_sb_version_has_pquotino(sbp)) {
@@ -321,7 +337,12 @@ xfs_mount_validate_sb(
 		return -EFBIG;
 	}
 
-	if (check_inprogress && sbp->sb_inprogress) {
+	/*
+	 * Don't touch the filesystem if a user tool thinks it owns the primary
+	 * superblock.  mkfs doesn't clear the flag from secondary supers, so
+	 * we don't check them at all.
+	 */
+	if (XFS_BUF_ADDR(bp) == XFS_SB_DADDR && sbp->sb_inprogress) {
 		xfs_warn(mp, "Offline file system operation in progress!");
 		return -EFSCORRUPTED;
 	}
@@ -596,29 +617,6 @@ xfs_sb_to_disk(
 	}
 }
 
-static int
-xfs_sb_verify(
-	struct xfs_buf	*bp,
-	bool		check_version)
-{
-	struct xfs_mount *mp = bp->b_target->bt_mount;
-	struct xfs_sb	sb;
-
-	/*
-	 * Use call variant which doesn't convert quota flags from disk 
-	 * format, because xfs_mount_validate_sb checks the on-disk flags.
-	 */
-	__xfs_sb_from_disk(&sb, XFS_BUF_TO_SBP(bp), false);
-
-	/*
-	 * Only check the in progress field for the primary superblock as
-	 * mkfs.xfs doesn't clear it from secondary superblocks.
-	 */
-	return xfs_mount_validate_sb(mp, &sb,
-				     bp->b_maps[0].bm_bn == XFS_SB_DADDR,
-				     check_version);
-}
-
 /*
  * If the superblock has the CRC feature bit set or the CRC field is non-null,
  * check that the CRC is valid.  We check the CRC field is non-null because a
@@ -633,11 +631,12 @@ xfs_sb_verify(
  */
 static void
 xfs_sb_read_verify(
-	struct xfs_buf	*bp)
+	struct xfs_buf		*bp)
 {
-	struct xfs_mount *mp = bp->b_target->bt_mount;
-	struct xfs_dsb	*dsb = XFS_BUF_TO_SBP(bp);
-	int		error;
+	struct xfs_sb		sb;
+	struct xfs_mount	*mp = bp->b_target->bt_mount;
+	struct xfs_dsb		*dsb = XFS_BUF_TO_SBP(bp);
+	int			error;
 
 	/*
 	 * open code the version check to avoid needing to convert the entire
@@ -657,7 +656,16 @@ xfs_sb_read_verify(
 			}
 		}
 	}
-	error = xfs_sb_verify(bp, true);
+
+	/*
+	 * Check all the superblock fields.  Don't byteswap the xquota flags
+	 * because _verify_common checks the on-disk values.
+	 */
+	__xfs_sb_from_disk(&sb, XFS_BUF_TO_SBP(bp), false);
+	error = xfs_validate_sb_common(mp, bp, &sb);
+	if (error)
+		goto out_error;
+	error = xfs_validate_sb_read(mp, &sb);
 
 out_error:
 	if (error == -EFSCORRUPTED || error == -EFSBADCRC)
@@ -691,15 +699,22 @@ static void
 xfs_sb_write_verify(
 	struct xfs_buf		*bp)
 {
+	struct xfs_sb		sb;
 	struct xfs_mount	*mp = bp->b_target->bt_mount;
 	struct xfs_buf_log_item	*bip = bp->b_log_item;
 	int			error;
 
-	error = xfs_sb_verify(bp, false);
-	if (error) {
-		xfs_verifier_error(bp, error, __this_address);
-		return;
-	}
+	/*
+	 * Check all the superblock fields.  Don't byteswap the xquota flags
+	 * because _verify_common checks the on-disk values.
+	 */
+	__xfs_sb_from_disk(&sb, XFS_BUF_TO_SBP(bp), false);
+	error = xfs_validate_sb_common(mp, bp, &sb);
+	if (error)
+		goto out_error;
+	error = xfs_validate_sb_write(mp, &sb);
+	if (error)
+		goto out_error;
 
 	if (!xfs_sb_version_hascrc(&mp->m_sb))
 		return;
@@ -708,6 +723,10 @@ xfs_sb_write_verify(
 		XFS_BUF_TO_SBP(bp)->sb_lsn = cpu_to_be64(bip->bli_item.li_lsn);
 
 	xfs_buf_update_cksum(bp, XFS_SB_CRC_OFF);
+	return;
+
+out_error:
+	xfs_verifier_error(bp, error, __this_address);
 }
 
 const struct xfs_buf_ops xfs_sb_buf_ops = {

[PATCH 2/3] libxfs: add more bounds checking to sb sanity checks

[Darrick J. Wong]

From: Bill O'Donnell <billodo@redhat.com>

Current sb verifier doesn't check bounds on sb_fdblocks and sb_ifree.
Add sanity checks for these parameters.

Signed-off-by: Bill O'Donnell <billodo@redhat.com>
[darrick: port to refactored sb validation predicates]
Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 fs/xfs/libxfs/xfs_sb.c |   12 ++++++++++++
 1 file changed, 12 insertions(+)


diff --git a/fs/xfs/libxfs/xfs_sb.c b/fs/xfs/libxfs/xfs_sb.c
index 516bef7b0f50..64bc471d57e6 100644
--- a/fs/xfs/libxfs/xfs_sb.c
+++ b/fs/xfs/libxfs/xfs_sb.c
@@ -153,6 +153,18 @@ xfs_validate_sb_write(
 	struct xfs_mount	*mp,
 	struct xfs_sb		*sbp)
 {
+	/*
+	 * Carry out additional sb sanity checks exclusively for writes.
+	 * We don't do these checks for reads, since faulty parameters could
+	 * be fixed in the log, and we shouldn't prohibit mounting for those
+	 * cases.
+	 */
+	if (sbp->sb_fdblocks > sbp->sb_dblocks ||
+	    sbp->sb_ifree > sbp->sb_icount) {
+		xfs_warn(mp, "SB summary counter sanity check failed");
+		return -EFSCORRUPTED;
+	}
+
 	if (!xfs_sb_version_hascrc(sbp))
 		return 0;
 

[PATCH 3/3] xfs: verify icount in superblock write

[Darrick J. Wong]

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

Add a helper predicate to check the inode count for sanity, then use it
in the superblock write verifier to inspect sb_icount.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
Reviewed-by: Bill O'Donnell <billodo@redhat.com>
---
 fs/xfs/libxfs/xfs_sb.c    |    1 +
 fs/xfs/libxfs/xfs_types.c |   34 ++++++++++++++++++++++++++++++++++
 fs/xfs/libxfs/xfs_types.h |    1 +
 3 files changed, 36 insertions(+)


diff --git a/fs/xfs/libxfs/xfs_sb.c b/fs/xfs/libxfs/xfs_sb.c
index 64bc471d57e6..e5972121e82d 100644
--- a/fs/xfs/libxfs/xfs_sb.c
+++ b/fs/xfs/libxfs/xfs_sb.c
@@ -160,6 +160,7 @@ xfs_validate_sb_write(
 	 * cases.
 	 */
 	if (sbp->sb_fdblocks > sbp->sb_dblocks ||
+	    !xfs_verify_icount(mp, sbp->sb_icount) ||
 	    sbp->sb_ifree > sbp->sb_icount) {
 		xfs_warn(mp, "SB summary counter sanity check failed");
 		return -EFSCORRUPTED;
diff --git a/fs/xfs/libxfs/xfs_types.c b/fs/xfs/libxfs/xfs_types.c
index 2e2a243cef2e..57f4fd028898 100644
--- a/fs/xfs/libxfs/xfs_types.c
+++ b/fs/xfs/libxfs/xfs_types.c
@@ -171,3 +171,37 @@ xfs_verify_rtbno(
 {
 	return rtbno < mp->m_sb.sb_rblocks;
 }
+
+/* Calculate the range of valid icount values. */
+static void
+xfs_icount_range(
+	struct xfs_mount	*mp,
+	unsigned long long	*min,
+	unsigned long long	*max)
+{
+	unsigned long long	nr_inos = 0;
+	xfs_agnumber_t		agno;
+
+	/* root, rtbitmap, rtsum all live in the first chunk */
+	*min = XFS_INODES_PER_CHUNK;
+
+	for (agno = 0; agno < mp->m_sb.sb_agcount; agno++) {
+		xfs_agino_t	first, last;
+
+		xfs_agino_range(mp, agno, &first, &last);
+		nr_inos += last - first + 1;
+	}
+	*max = nr_inos;
+}
+
+/* Sanity-checking of inode counts. */
+bool
+xfs_verify_icount(
+	struct xfs_mount	*mp,
+	unsigned long long	icount)
+{
+	unsigned long long	min, max;
+
+	xfs_icount_range(mp, &min, &max);
+	return icount >= min && icount < max;
+}
diff --git a/fs/xfs/libxfs/xfs_types.h b/fs/xfs/libxfs/xfs_types.h
index 4055d62f690c..b9e6c89284c3 100644
--- a/fs/xfs/libxfs/xfs_types.h
+++ b/fs/xfs/libxfs/xfs_types.h
@@ -165,5 +165,6 @@ bool xfs_verify_ino(struct xfs_mount *mp, xfs_ino_t ino);
 bool xfs_internal_inum(struct xfs_mount *mp, xfs_ino_t ino);
 bool xfs_verify_dir_ino(struct xfs_mount *mp, xfs_ino_t ino);
 bool xfs_verify_rtbno(struct xfs_mount *mp, xfs_rtblock_t rtbno);
+bool xfs_verify_icount(struct xfs_mount *mp, unsigned long long icount);
 
 #endif	/* __XFS_TYPES_H__ */

Re: [PATCH 1/3] xfs: refactor superblock verifiers

[Eric Sandeen]

On 7/30/18 12:30 AM, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Split the superblock verifier into the common checks, the read-time
> checks, and the write-time check functions.  No functional changes, but
> we're setting up to add more write-only checks.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>

Couple nitpicks below,change them on the way in if you like.

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

> ---
>  fs/xfs/libxfs/xfs_sb.c |  205 ++++++++++++++++++++++++++----------------------
>  1 file changed, 112 insertions(+), 93 deletions(-)
> 
> 
> diff --git a/fs/xfs/libxfs/xfs_sb.c b/fs/xfs/libxfs/xfs_sb.c
> index b3ad15956366..516bef7b0f50 100644
> --- a/fs/xfs/libxfs/xfs_sb.c
> +++ b/fs/xfs/libxfs/xfs_sb.c
> @@ -96,80 +96,96 @@ xfs_perag_put(
>  	trace_xfs_perag_put(pag->pag_mount, pag->pag_agno, ref, _RET_IP_);
>  }
>  
> -/*
> - * Check the validity of the SB found.
> - */
> +/* Check all the superblock fields we care about when reading one in. */
>  STATIC int
> -xfs_mount_validate_sb(
> -	xfs_mount_t	*mp,
> -	xfs_sb_t	*sbp,
> -	bool		check_inprogress,
> -	bool		check_version)
> +xfs_validate_sb_read(
> +	struct xfs_mount	*mp,
> +	struct xfs_sb		*sbp)
>  {
> -	uint32_t	agcount = 0;
> -	uint32_t	rem;
> -
> -	if (sbp->sb_magicnum != XFS_SB_MAGIC) {
> -		xfs_warn(mp, "bad magic number");
> -		return -EWRONGFS;
> -	}
> -
> -
> -	if (!xfs_sb_good_version(sbp)) {
> -		xfs_warn(mp, "bad version");
> -		return -EWRONGFS;
> -	}
> +	if (!xfs_sb_version_hascrc(sbp))
> +		return 0;

Can we make this

	if (!(XFS_SB_VERSION_NUM(sbp) == XFS_SB_VERSION_5)) ?

nitpicky but it always bugs me to see "do we have metadata crcs" as a standin
for other available features (like feature flags).  Yes, they go together,
but bleah.

(and the original code tested superblock version, right?)

>  	/*
> -	 * Version 5 superblock feature mask validation. Reject combinations the
> -	 * kernel cannot support up front before checking anything else. For
> -	 * write validation, we don't need to check feature masks.
> +	 * Version 5 superblock feature mask validation. Reject combinations
> +	 * the kernel cannot support up front before checking anything else.
> +	 * For write validation, we don't need to check feature masks.

Huh, I wonder why not.  But ok, keeping the same behavior.

>  	 */
> -	if (check_version && XFS_SB_VERSION_NUM(sbp) == XFS_SB_VERSION_5) {
> -		if (xfs_sb_has_compat_feature(sbp,
> -					XFS_SB_FEAT_COMPAT_UNKNOWN)) {
> -			xfs_warn(mp,
> +	if (xfs_sb_has_compat_feature(sbp, XFS_SB_FEAT_COMPAT_UNKNOWN)) {
> +		xfs_warn(mp,
>  "Superblock has unknown compatible features (0x%x) enabled.",
> -				(sbp->sb_features_compat &
> -						XFS_SB_FEAT_COMPAT_UNKNOWN));
> -			xfs_warn(mp,
> +			(sbp->sb_features_compat & XFS_SB_FEAT_COMPAT_UNKNOWN));
> +		xfs_warn(mp,
>  "Using a more recent kernel is recommended.");
> -		}
> +	}
>  
> -		if (xfs_sb_has_ro_compat_feature(sbp,
> -					XFS_SB_FEAT_RO_COMPAT_UNKNOWN)) {
> -			xfs_alert(mp,
> +	if (xfs_sb_has_ro_compat_feature(sbp,
> +				XFS_SB_FEAT_RO_COMPAT_UNKNOWN)) {
> +		xfs_alert(mp,
>  "Superblock has unknown read-only compatible features (0x%x) enabled.",
> -				(sbp->sb_features_ro_compat &
> -						XFS_SB_FEAT_RO_COMPAT_UNKNOWN));
> -			if (!(mp->m_flags & XFS_MOUNT_RDONLY)) {
> -				xfs_warn(mp,
> +			(sbp->sb_features_ro_compat &
> +					XFS_SB_FEAT_RO_COMPAT_UNKNOWN));
> +		if (!(mp->m_flags & XFS_MOUNT_RDONLY)) {
> +			xfs_warn(mp,
>  "Attempted to mount read-only compatible filesystem read-write.");
> -				xfs_warn(mp,
> +			xfs_warn(mp,
>  "Filesystem can only be safely mounted read only.");
>  
> -				return -EINVAL;
> -			}
> +			return -EINVAL;
>  		}
> -		if (xfs_sb_has_incompat_feature(sbp,
> -					XFS_SB_FEAT_INCOMPAT_UNKNOWN)) {
> -			xfs_warn(mp,
> +	}
> +	if (xfs_sb_has_incompat_feature(sbp,
> +				XFS_SB_FEAT_INCOMPAT_UNKNOWN)) {
> +		xfs_warn(mp,
>  "Superblock has unknown incompatible features (0x%x) enabled.",
> -				(sbp->sb_features_incompat &
> -						XFS_SB_FEAT_INCOMPAT_UNKNOWN));
> -			xfs_warn(mp,
> +			(sbp->sb_features_incompat &
> +					XFS_SB_FEAT_INCOMPAT_UNKNOWN));
> +		xfs_warn(mp,
>  "Filesystem can not be safely mounted by this kernel.");
> -			return -EINVAL;
> -		}
> -	} else if (xfs_sb_version_hascrc(sbp)) {
> -		/*
> -		 * We can't read verify the sb LSN because the read verifier is
> -		 * called before the log is allocated and processed. We know the
> -		 * log is set up before write verifier (!check_version) calls,
> -		 * so just check it here.
> -		 */
> -		if (!xfs_log_check_lsn(mp, sbp->sb_lsn))
> -			return -EFSCORRUPTED;
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +
> +/* Check all the superblock fields we care about when writing one out. */
> +STATIC int
> +xfs_validate_sb_write(
> +	struct xfs_mount	*mp,
> +	struct xfs_sb		*sbp)
> +{
> +	if (!xfs_sb_version_hascrc(sbp))
> +		return 0;
> +
> +	/*
> +	 * We can't read verify the sb LSN because the read verifier is called
> +	 * before the log is allocated and processed. We know the log is set up
> +	 * before write verifier (!check_version) calls, so just check it here.
> +	 */
> +	if (!xfs_log_check_lsn(mp, sbp->sb_lsn))
> +		return -EFSCORRUPTED;
> +
> +	return 0;
> +}
> +
> +/* Check the validity of the SB. */
> +STATIC int
> +xfs_validate_sb_common(
> +	struct xfs_mount	*mp,
> +	struct xfs_buf		*bp,
> +	struct xfs_sb		*sbp)
> +{
> +	uint32_t		agcount = 0;
> +	uint32_t		rem;
> +
> +	if (sbp->sb_magicnum != XFS_SB_MAGIC) {
> +		xfs_warn(mp, "bad magic number");
> +		return -EWRONGFS;
> +	}
> +
> +

just one newline pls ;)  (even if it was there before)

> +	if (!xfs_sb_good_version(sbp)) {
> +		xfs_warn(mp, "bad version");
> +		return -EWRONGFS;
>  	}
>  
>  	if (xfs_sb_version_has_pquotino(sbp)) {

Re: [PATCH 2/3] libxfs: add more bounds checking to sb sanity checks

[Eric Sandeen]

On 7/30/18 12:30 AM, Darrick J. Wong wrote:
> From: Bill O'Donnell <billodo@redhat.com>
> 
> Current sb verifier doesn't check bounds on sb_fdblocks and sb_ifree.
> Add sanity checks for these parameters.
> 
> Signed-off-by: Bill O'Donnell <billodo@redhat.com>
> [darrick: port to refactored sb validation predicates]
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>

comment nitpicks below, but otherwise

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

> ---
>  fs/xfs/libxfs/xfs_sb.c |   12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> 
> diff --git a/fs/xfs/libxfs/xfs_sb.c b/fs/xfs/libxfs/xfs_sb.c
> index 516bef7b0f50..64bc471d57e6 100644
> --- a/fs/xfs/libxfs/xfs_sb.c
> +++ b/fs/xfs/libxfs/xfs_sb.c
> @@ -153,6 +153,18 @@ xfs_validate_sb_write(
>  	struct xfs_mount	*mp,
>  	struct xfs_sb		*sbp)
>  {
> +	/*
> +	 * Carry out additional sb sanity checks exclusively for writes.

We're in xfs_validate_sb_write so that's obvious, can drop this line.

> +	 * We don't do these checks for reads, since faulty parameters could
> +	 * be fixed in the log, and we shouldn't prohibit mounting for those
> +	 * cases.
> +	 */

Hm, it's not really a log reaplay issue, right?  These summary counters
get reinitialized at mount, so failing to mount before we overwrite them
anyway makes no sense.

/*
 * These summary counters get re-initialized after they are read
 * during mount, so this is a write-only check.
 */

?  And yeah, modulo lazycount... but whatevs.

-Eric

> +	if (sbp->sb_fdblocks > sbp->sb_dblocks ||
> +	    sbp->sb_ifree > sbp->sb_icount) {
> +		xfs_warn(mp, "SB summary counter sanity check failed");
> +		return -EFSCORRUPTED;
> +	}
> +
>  	if (!xfs_sb_version_hascrc(sbp))
>  		return 0;

Re: [PATCH 3/3] xfs: verify icount in superblock write

[Eric Sandeen]

On 7/30/18 12:30 AM, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Add a helper predicate to check the inode count for sanity, then use it
> in the superblock write verifier to inspect sb_icount.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> Reviewed-by: Bill O'Donnell <billodo@redhat.com>
> ---
>  fs/xfs/libxfs/xfs_sb.c    |    1 +
>  fs/xfs/libxfs/xfs_types.c |   34 ++++++++++++++++++++++++++++++++++
>  fs/xfs/libxfs/xfs_types.h |    1 +
>  3 files changed, 36 insertions(+)
> 
> 
> diff --git a/fs/xfs/libxfs/xfs_sb.c b/fs/xfs/libxfs/xfs_sb.c
> index 64bc471d57e6..e5972121e82d 100644
> --- a/fs/xfs/libxfs/xfs_sb.c
> +++ b/fs/xfs/libxfs/xfs_sb.c
> @@ -160,6 +160,7 @@ xfs_validate_sb_write(
>  	 * cases.
>  	 */
>  	if (sbp->sb_fdblocks > sbp->sb_dblocks ||
> +	    !xfs_verify_icount(mp, sbp->sb_icount) ||
>  	    sbp->sb_ifree > sbp->sb_icount) {
>  		xfs_warn(mp, "SB summary counter sanity check failed");
>  		return -EFSCORRUPTED;
> diff --git a/fs/xfs/libxfs/xfs_types.c b/fs/xfs/libxfs/xfs_types.c
> index 2e2a243cef2e..57f4fd028898 100644
> --- a/fs/xfs/libxfs/xfs_types.c
> +++ b/fs/xfs/libxfs/xfs_types.c
> @@ -171,3 +171,37 @@ xfs_verify_rtbno(
>  {
>  	return rtbno < mp->m_sb.sb_rblocks;
>  }
> +
> +/* Calculate the range of valid icount values. */
> +static void
> +xfs_icount_range(
> +	struct xfs_mount	*mp,
> +	unsigned long long	*min,
> +	unsigned long long	*max)
> +{
> +	unsigned long long	nr_inos = 0;
> +	xfs_agnumber_t		agno;
> +
> +	/* root, rtbitmap, rtsum all live in the first chunk */
> +	*min = XFS_INODES_PER_CHUNK;
> +
> +	for (agno = 0; agno < mp->m_sb.sb_agcount; agno++) {
> +		xfs_agino_t	first, last;
> +
> +		xfs_agino_range(mp, agno, &first, &last);
> +		nr_inos += last - first + 1;
> +	}
> +	*max = nr_inos;

I still think this is more work than we need to do for a verifier,
TBH.

In practice how far off is this from do_div(dblocks, inodes_per_block) ?

Oh well.   I guess computationally it's pretty cheap...

> +}
> +
> +/* Sanity-checking of inode counts. */
> +bool
> +xfs_verify_icount(
> +	struct xfs_mount	*mp,
> +	unsigned long long	icount)
> +{
> +	unsigned long long	min, max;
> +
> +	xfs_icount_range(mp, &min, &max);
> +	return icount >= min && icount < max;

Since you've calculated it down to the very last inode, why is
"= max" out of range?

> +}
> diff --git a/fs/xfs/libxfs/xfs_types.h b/fs/xfs/libxfs/xfs_types.h
> index 4055d62f690c..b9e6c89284c3 100644
> --- a/fs/xfs/libxfs/xfs_types.h
> +++ b/fs/xfs/libxfs/xfs_types.h
> @@ -165,5 +165,6 @@ bool xfs_verify_ino(struct xfs_mount *mp, xfs_ino_t ino);
>  bool xfs_internal_inum(struct xfs_mount *mp, xfs_ino_t ino);
>  bool xfs_verify_dir_ino(struct xfs_mount *mp, xfs_ino_t ino);
>  bool xfs_verify_rtbno(struct xfs_mount *mp, xfs_rtblock_t rtbno);
> +bool xfs_verify_icount(struct xfs_mount *mp, unsigned long long icount);
>  
>  #endif	/* __XFS_TYPES_H__ */

Re: [PATCH 1/3] xfs: refactor superblock verifiers

[Darrick J. Wong]

On Mon, Jul 30, 2018 at 06:06:51PM -0500, Eric Sandeen wrote:
> On 7/30/18 12:30 AM, Darrick J. Wong wrote:
> > From: Darrick J. Wong <darrick.wong@oracle.com>
> > 
> > Split the superblock verifier into the common checks, the read-time
> > checks, and the write-time check functions.  No functional changes, but
> > we're setting up to add more write-only checks.
> > 
> > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Couple nitpicks below,change them on the way in if you like.
> 
> Reviewed-by: Eric Sandeen <sandeen@redhat.com>
> 
> > ---
> >  fs/xfs/libxfs/xfs_sb.c |  205 ++++++++++++++++++++++++++----------------------
> >  1 file changed, 112 insertions(+), 93 deletions(-)
> > 
> > 
> > diff --git a/fs/xfs/libxfs/xfs_sb.c b/fs/xfs/libxfs/xfs_sb.c
> > index b3ad15956366..516bef7b0f50 100644
> > --- a/fs/xfs/libxfs/xfs_sb.c
> > +++ b/fs/xfs/libxfs/xfs_sb.c
> > @@ -96,80 +96,96 @@ xfs_perag_put(
> >  	trace_xfs_perag_put(pag->pag_mount, pag->pag_agno, ref, _RET_IP_);
> >  }
> >  
> > -/*
> > - * Check the validity of the SB found.
> > - */
> > +/* Check all the superblock fields we care about when reading one in. */
> >  STATIC int
> > -xfs_mount_validate_sb(
> > -	xfs_mount_t	*mp,
> > -	xfs_sb_t	*sbp,
> > -	bool		check_inprogress,
> > -	bool		check_version)
> > +xfs_validate_sb_read(
> > +	struct xfs_mount	*mp,
> > +	struct xfs_sb		*sbp)
> >  {
> > -	uint32_t	agcount = 0;
> > -	uint32_t	rem;
> > -
> > -	if (sbp->sb_magicnum != XFS_SB_MAGIC) {
> > -		xfs_warn(mp, "bad magic number");
> > -		return -EWRONGFS;
> > -	}
> > -
> > -
> > -	if (!xfs_sb_good_version(sbp)) {
> > -		xfs_warn(mp, "bad version");
> > -		return -EWRONGFS;
> > -	}
> > +	if (!xfs_sb_version_hascrc(sbp))
> > +		return 0;
> 
> Can we make this
> 
> 	if (!(XFS_SB_VERSION_NUM(sbp) == XFS_SB_VERSION_5)) ?
> 
> nitpicky but it always bugs me to see "do we have metadata crcs" as a standin
> for other available features (like feature flags).  Yes, they go together,
> but bleah.
> 
> (and the original code tested superblock version, right?)

One of them did, the other one used the helper.  It was weird.

> >  	/*
> > -	 * Version 5 superblock feature mask validation. Reject combinations the
> > -	 * kernel cannot support up front before checking anything else. For
> > -	 * write validation, we don't need to check feature masks.
> > +	 * Version 5 superblock feature mask validation. Reject combinations
> > +	 * the kernel cannot support up front before checking anything else.
> > +	 * For write validation, we don't need to check feature masks.
> 
> Huh, I wonder why not.  But ok, keeping the same behavior.
> 
> >  	 */
> > -	if (check_version && XFS_SB_VERSION_NUM(sbp) == XFS_SB_VERSION_5) {
> > -		if (xfs_sb_has_compat_feature(sbp,
> > -					XFS_SB_FEAT_COMPAT_UNKNOWN)) {
> > -			xfs_warn(mp,
> > +	if (xfs_sb_has_compat_feature(sbp, XFS_SB_FEAT_COMPAT_UNKNOWN)) {
> > +		xfs_warn(mp,
> >  "Superblock has unknown compatible features (0x%x) enabled.",
> > -				(sbp->sb_features_compat &
> > -						XFS_SB_FEAT_COMPAT_UNKNOWN));
> > -			xfs_warn(mp,
> > +			(sbp->sb_features_compat & XFS_SB_FEAT_COMPAT_UNKNOWN));
> > +		xfs_warn(mp,
> >  "Using a more recent kernel is recommended.");
> > -		}
> > +	}
> >  
> > -		if (xfs_sb_has_ro_compat_feature(sbp,
> > -					XFS_SB_FEAT_RO_COMPAT_UNKNOWN)) {
> > -			xfs_alert(mp,
> > +	if (xfs_sb_has_ro_compat_feature(sbp,
> > +				XFS_SB_FEAT_RO_COMPAT_UNKNOWN)) {
> > +		xfs_alert(mp,
> >  "Superblock has unknown read-only compatible features (0x%x) enabled.",
> > -				(sbp->sb_features_ro_compat &
> > -						XFS_SB_FEAT_RO_COMPAT_UNKNOWN));
> > -			if (!(mp->m_flags & XFS_MOUNT_RDONLY)) {
> > -				xfs_warn(mp,
> > +			(sbp->sb_features_ro_compat &
> > +					XFS_SB_FEAT_RO_COMPAT_UNKNOWN));
> > +		if (!(mp->m_flags & XFS_MOUNT_RDONLY)) {
> > +			xfs_warn(mp,
> >  "Attempted to mount read-only compatible filesystem read-write.");
> > -				xfs_warn(mp,
> > +			xfs_warn(mp,
> >  "Filesystem can only be safely mounted read only.");
> >  
> > -				return -EINVAL;
> > -			}
> > +			return -EINVAL;
> >  		}
> > -		if (xfs_sb_has_incompat_feature(sbp,
> > -					XFS_SB_FEAT_INCOMPAT_UNKNOWN)) {
> > -			xfs_warn(mp,
> > +	}
> > +	if (xfs_sb_has_incompat_feature(sbp,
> > +				XFS_SB_FEAT_INCOMPAT_UNKNOWN)) {
> > +		xfs_warn(mp,
> >  "Superblock has unknown incompatible features (0x%x) enabled.",
> > -				(sbp->sb_features_incompat &
> > -						XFS_SB_FEAT_INCOMPAT_UNKNOWN));
> > -			xfs_warn(mp,
> > +			(sbp->sb_features_incompat &
> > +					XFS_SB_FEAT_INCOMPAT_UNKNOWN));
> > +		xfs_warn(mp,
> >  "Filesystem can not be safely mounted by this kernel.");
> > -			return -EINVAL;
> > -		}
> > -	} else if (xfs_sb_version_hascrc(sbp)) {
> > -		/*
> > -		 * We can't read verify the sb LSN because the read verifier is
> > -		 * called before the log is allocated and processed. We know the
> > -		 * log is set up before write verifier (!check_version) calls,
> > -		 * so just check it here.
> > -		 */
> > -		if (!xfs_log_check_lsn(mp, sbp->sb_lsn))
> > -			return -EFSCORRUPTED;
> > +		return -EINVAL;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +/* Check all the superblock fields we care about when writing one out. */
> > +STATIC int
> > +xfs_validate_sb_write(
> > +	struct xfs_mount	*mp,
> > +	struct xfs_sb		*sbp)
> > +{
> > +	if (!xfs_sb_version_hascrc(sbp))
> > +		return 0;
> > +
> > +	/*
> > +	 * We can't read verify the sb LSN because the read verifier is called
> > +	 * before the log is allocated and processed. We know the log is set up
> > +	 * before write verifier (!check_version) calls, so just check it here.
> > +	 */
> > +	if (!xfs_log_check_lsn(mp, sbp->sb_lsn))
> > +		return -EFSCORRUPTED;
> > +
> > +	return 0;
> > +}
> > +
> > +/* Check the validity of the SB. */
> > +STATIC int
> > +xfs_validate_sb_common(
> > +	struct xfs_mount	*mp,
> > +	struct xfs_buf		*bp,
> > +	struct xfs_sb		*sbp)
> > +{
> > +	uint32_t		agcount = 0;
> > +	uint32_t		rem;
> > +
> > +	if (sbp->sb_magicnum != XFS_SB_MAGIC) {
> > +		xfs_warn(mp, "bad magic number");
> > +		return -EWRONGFS;
> > +	}
> > +
> > +
> 
> just one newline pls ;)  (even if it was there before)

Ok, I'll fix those things on the way in.

--D

> > +	if (!xfs_sb_good_version(sbp)) {
> > +		xfs_warn(mp, "bad version");
> > +		return -EWRONGFS;
> >  	}
> >  
> >  	if (xfs_sb_version_has_pquotino(sbp)) {
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/3] libxfs: add more bounds checking to sb sanity checks

[Darrick J. Wong]

On Mon, Jul 30, 2018 at 06:16:40PM -0500, Eric Sandeen wrote:
> On 7/30/18 12:30 AM, Darrick J. Wong wrote:
> > From: Bill O'Donnell <billodo@redhat.com>
> > 
> > Current sb verifier doesn't check bounds on sb_fdblocks and sb_ifree.
> > Add sanity checks for these parameters.
> > 
> > Signed-off-by: Bill O'Donnell <billodo@redhat.com>
> > [darrick: port to refactored sb validation predicates]
> > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> 
> comment nitpicks below, but otherwise
> 
> Reviewed-by: Eric Sandeen <sandeen@redhat.com>
> 
> > ---
> >  fs/xfs/libxfs/xfs_sb.c |   12 ++++++++++++
> >  1 file changed, 12 insertions(+)
> > 
> > 
> > diff --git a/fs/xfs/libxfs/xfs_sb.c b/fs/xfs/libxfs/xfs_sb.c
> > index 516bef7b0f50..64bc471d57e6 100644
> > --- a/fs/xfs/libxfs/xfs_sb.c
> > +++ b/fs/xfs/libxfs/xfs_sb.c
> > @@ -153,6 +153,18 @@ xfs_validate_sb_write(
> >  	struct xfs_mount	*mp,
> >  	struct xfs_sb		*sbp)
> >  {
> > +	/*
> > +	 * Carry out additional sb sanity checks exclusively for writes.
> 
> We're in xfs_validate_sb_write so that's obvious, can drop this line.
> 
> > +	 * We don't do these checks for reads, since faulty parameters could
> > +	 * be fixed in the log, and we shouldn't prohibit mounting for those
> > +	 * cases.
> > +	 */
> 
> Hm, it's not really a log reaplay issue, right?  These summary counters
> get reinitialized at mount, so failing to mount before we overwrite them
> anyway makes no sense.

Well, we don't reinitialize them if ( (!lazysbcount) or (clean log) )
and (non-crazy values)...

> /*
>  * These summary counters get re-initialized after they are read
>  * during mount, so this is a write-only check.

They're not always re-initialized -- only if we had a dirty lazysbcont
fs or the values were crazy.

/*
 * Carry out additional sb summary counter sanity checks when we write
 * the superblock.  We skip this in the read validator because there
 * could be newer superblocks in the log and if the values are garbage
 * even after replay we'll recalculate them at the end of log mount.
 */

--D

>  */
> 
> ?  And yeah, modulo lazycount... but whatevs.
> 
> -Eric
> 
> > +	if (sbp->sb_fdblocks > sbp->sb_dblocks ||
> > +	    sbp->sb_ifree > sbp->sb_icount) {
> > +		xfs_warn(mp, "SB summary counter sanity check failed");
> > +		return -EFSCORRUPTED;
> > +	}
> > +
> >  	if (!xfs_sb_version_hascrc(sbp))
> >  		return 0;
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/3] libxfs: add more bounds checking to sb sanity checks

[Eric Sandeen]

On 7/30/18 6:38 PM, Darrick J. Wong wrote:
> On Mon, Jul 30, 2018 at 06:16:40PM -0500, Eric Sandeen wrote:
>> On 7/30/18 12:30 AM, Darrick J. Wong wrote:
>>> From: Bill O'Donnell <billodo@redhat.com>
>>>
>>> Current sb verifier doesn't check bounds on sb_fdblocks and sb_ifree.
>>> Add sanity checks for these parameters.
>>>
>>> Signed-off-by: Bill O'Donnell <billodo@redhat.com>
>>> [darrick: port to refactored sb validation predicates]
>>> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
>>
>> comment nitpicks below, but otherwise
>>
>> Reviewed-by: Eric Sandeen <sandeen@redhat.com>
>>
>>> ---
>>>  fs/xfs/libxfs/xfs_sb.c |   12 ++++++++++++
>>>  1 file changed, 12 insertions(+)
>>>
>>>
>>> diff --git a/fs/xfs/libxfs/xfs_sb.c b/fs/xfs/libxfs/xfs_sb.c
>>> index 516bef7b0f50..64bc471d57e6 100644
>>> --- a/fs/xfs/libxfs/xfs_sb.c
>>> +++ b/fs/xfs/libxfs/xfs_sb.c
>>> @@ -153,6 +153,18 @@ xfs_validate_sb_write(
>>>  	struct xfs_mount	*mp,
>>>  	struct xfs_sb		*sbp)
>>>  {
>>> +	/*
>>> +	 * Carry out additional sb sanity checks exclusively for writes.
>>
>> We're in xfs_validate_sb_write so that's obvious, can drop this line.
>>
>>> +	 * We don't do these checks for reads, since faulty parameters could
>>> +	 * be fixed in the log, and we shouldn't prohibit mounting for those
>>> +	 * cases.
>>> +	 */
>>
>> Hm, it's not really a log reaplay issue, right?  These summary counters
>> get reinitialized at mount, so failing to mount before we overwrite them
>> anyway makes no sense.
> 
> Well, we don't reinitialize them if ( (!lazysbcount) or (clean log) )
> and (non-crazy values)...
> 
>> /*
>>  * These summary counters get re-initialized after they are read
>>  * during mount, so this is a write-only check.
> 
> They're not always re-initialized -- only if we had a dirty lazysbcont
> fs or the values were crazy.
> 
> /*
>  * Carry out additional sb summary counter sanity checks when we write
>  * the superblock.  We skip this in the read validator because there
>  * could be newer superblocks in the log and if the values are garbage
>  * even after replay we'll recalculate them at the end of log mount.
>  */

Oh, ok sure.  Given my ongoing confusion an explicit/complete comment is
probably good.  For me, if for nobody else.  ;)

Thanks,
-Eric

Re: [PATCH 3/3] xfs: verify icount in superblock write

[Darrick J. Wong]

On Mon, Jul 30, 2018 at 06:26:29PM -0500, Eric Sandeen wrote:
> On 7/30/18 12:30 AM, Darrick J. Wong wrote:
> > From: Darrick J. Wong <darrick.wong@oracle.com>
> > 
> > Add a helper predicate to check the inode count for sanity, then use it
> > in the superblock write verifier to inspect sb_icount.
> > 
> > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> > Reviewed-by: Bill O'Donnell <billodo@redhat.com>
> > ---
> >  fs/xfs/libxfs/xfs_sb.c    |    1 +
> >  fs/xfs/libxfs/xfs_types.c |   34 ++++++++++++++++++++++++++++++++++
> >  fs/xfs/libxfs/xfs_types.h |    1 +
> >  3 files changed, 36 insertions(+)
> > 
> > 
> > diff --git a/fs/xfs/libxfs/xfs_sb.c b/fs/xfs/libxfs/xfs_sb.c
> > index 64bc471d57e6..e5972121e82d 100644
> > --- a/fs/xfs/libxfs/xfs_sb.c
> > +++ b/fs/xfs/libxfs/xfs_sb.c
> > @@ -160,6 +160,7 @@ xfs_validate_sb_write(
> >  	 * cases.
> >  	 */
> >  	if (sbp->sb_fdblocks > sbp->sb_dblocks ||
> > +	    !xfs_verify_icount(mp, sbp->sb_icount) ||
> >  	    sbp->sb_ifree > sbp->sb_icount) {
> >  		xfs_warn(mp, "SB summary counter sanity check failed");
> >  		return -EFSCORRUPTED;
> > diff --git a/fs/xfs/libxfs/xfs_types.c b/fs/xfs/libxfs/xfs_types.c
> > index 2e2a243cef2e..57f4fd028898 100644
> > --- a/fs/xfs/libxfs/xfs_types.c
> > +++ b/fs/xfs/libxfs/xfs_types.c
> > @@ -171,3 +171,37 @@ xfs_verify_rtbno(
> >  {
> >  	return rtbno < mp->m_sb.sb_rblocks;
> >  }
> > +
> > +/* Calculate the range of valid icount values. */
> > +static void
> > +xfs_icount_range(
> > +	struct xfs_mount	*mp,
> > +	unsigned long long	*min,
> > +	unsigned long long	*max)
> > +{
> > +	unsigned long long	nr_inos = 0;
> > +	xfs_agnumber_t		agno;
> > +
> > +	/* root, rtbitmap, rtsum all live in the first chunk */
> > +	*min = XFS_INODES_PER_CHUNK;
> > +
> > +	for (agno = 0; agno < mp->m_sb.sb_agcount; agno++) {
> > +		xfs_agino_t	first, last;
> > +
> > +		xfs_agino_range(mp, agno, &first, &last);
> > +		nr_inos += last - first + 1;
> > +	}
> > +	*max = nr_inos;
> 
> I still think this is more work than we need to do for a verifier,
> TBH.
> 
> In practice how far off is this from do_div(dblocks, inodes_per_block) ?
> 
> Oh well.   I guess computationally it's pretty cheap...
> 
> > +}
> > +
> > +/* Sanity-checking of inode counts. */
> > +bool
> > +xfs_verify_icount(
> > +	struct xfs_mount	*mp,
> > +	unsigned long long	icount)
> > +{
> > +	unsigned long long	min, max;
> > +
> > +	xfs_icount_range(mp, &min, &max);
> > +	return icount >= min && icount < max;
> 
> Since you've calculated it down to the very last inode, why is
> "= max" out of range?

I think that's a bug, will fix. :/

--D

> > +}
> > diff --git a/fs/xfs/libxfs/xfs_types.h b/fs/xfs/libxfs/xfs_types.h
> > index 4055d62f690c..b9e6c89284c3 100644
> > --- a/fs/xfs/libxfs/xfs_types.h
> > +++ b/fs/xfs/libxfs/xfs_types.h
> > @@ -165,5 +165,6 @@ bool xfs_verify_ino(struct xfs_mount *mp, xfs_ino_t ino);
> >  bool xfs_internal_inum(struct xfs_mount *mp, xfs_ino_t ino);
> >  bool xfs_verify_dir_ino(struct xfs_mount *mp, xfs_ino_t ino);
> >  bool xfs_verify_rtbno(struct xfs_mount *mp, xfs_rtblock_t rtbno);
> > +bool xfs_verify_icount(struct xfs_mount *mp, unsigned long long icount);
> >  
> >  #endif	/* __XFS_TYPES_H__ */
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html