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

[PATCH 1/2] 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: remove the icount check, tweak the comments a little]
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 fs/xfs/libxfs/xfs_sb.c |   45 +++++++++++++++++++++++++++++++++++----------
 1 file changed, 35 insertions(+), 10 deletions(-)


diff --git a/fs/xfs/libxfs/xfs_sb.c b/fs/xfs/libxfs/xfs_sb.c
index b3ad15956366..b2c683588519 100644
--- a/fs/xfs/libxfs/xfs_sb.c
+++ b/fs/xfs/libxfs/xfs_sb.c
@@ -599,22 +599,16 @@ xfs_sb_to_disk(
 static int
 xfs_sb_verify(
 	struct xfs_buf	*bp,
+	struct xfs_sb	*sb,
 	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,
+	return xfs_mount_validate_sb(mp, sb,
 				     bp->b_maps[0].bm_bn == XFS_SB_DADDR,
 				     check_version);
 }
@@ -637,6 +631,7 @@ xfs_sb_read_verify(
 {
 	struct xfs_mount *mp = bp->b_target->bt_mount;
 	struct xfs_dsb	*dsb = XFS_BUF_TO_SBP(bp);
+	struct xfs_sb	sb;
 	int		error;
 
 	/*
@@ -657,7 +652,13 @@ xfs_sb_read_verify(
 			}
 		}
 	}
-	error = xfs_sb_verify(bp, true);
+
+	/*
+	 * 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);
+	error = xfs_sb_verify(bp, &sb, true);
 
 out_error:
 	if (error == -EFSCORRUPTED || error == -EFSBADCRC)
@@ -693,9 +694,33 @@ xfs_sb_write_verify(
 {
 	struct xfs_mount	*mp = bp->b_target->bt_mount;
 	struct xfs_buf_log_item	*bip = bp->b_log_item;
+	struct xfs_sb		sb;
 	int			error;
 
-	error = xfs_sb_verify(bp, false);
+	/*
+	 * 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);
+
+	error = xfs_sb_verify(bp, &sb, false);
+	if (error)
+		goto err;
+
+	/*
+	 * 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 (sb.sb_fdblocks > sb.sb_dblocks ||
+	    sb.sb_ifree > sb.sb_icount) {
+		xfs_notice(mp, "SB summary counter sanity check failed");
+		error = -EFSCORRUPTED;
+		goto err;
+	}
+
+err:
 	if (error) {
 		xfs_verifier_error(bp, error, __this_address);
 		return;

[PATCH 2/2] 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>
---
 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 b2c683588519..1659016875f9 100644
--- a/fs/xfs/libxfs/xfs_sb.c
+++ b/fs/xfs/libxfs/xfs_sb.c
@@ -714,6 +714,7 @@ xfs_sb_write_verify(
 	 * cases.
 	 */
 	if (sb.sb_fdblocks > sb.sb_dblocks ||
+	    !xfs_verify_icount(mp, sb.sb_icount) ||
 	    sb.sb_ifree > sb.sb_icount) {
 		xfs_notice(mp, "SB summary counter sanity check failed");
 		error = -EFSCORRUPTED;
diff --git a/fs/xfs/libxfs/xfs_types.c b/fs/xfs/libxfs/xfs_types.c
index 2e2a243cef2e..2e9c0c25ccb6 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 += first - last + 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 2/2] xfs: verify icount in superblock write

[Bill O'Donnell]

On Thu, Jul 26, 2018 at 10:35:25AM -0700, 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>
> ---

Looks good.
Thanks.
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 b2c683588519..1659016875f9 100644
> --- a/fs/xfs/libxfs/xfs_sb.c
> +++ b/fs/xfs/libxfs/xfs_sb.c
> @@ -714,6 +714,7 @@ xfs_sb_write_verify(
>  	 * cases.
>  	 */
>  	if (sb.sb_fdblocks > sb.sb_dblocks ||
> +	    !xfs_verify_icount(mp, sb.sb_icount) ||
>  	    sb.sb_ifree > sb.sb_icount) {
>  		xfs_notice(mp, "SB summary counter sanity check failed");
>  		error = -EFSCORRUPTED;
> diff --git a/fs/xfs/libxfs/xfs_types.c b/fs/xfs/libxfs/xfs_types.c
> index 2e2a243cef2e..2e9c0c25ccb6 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 += first - last + 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__ */
> 
> --
> 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/2] xfs: verify icount in superblock write

[Dave Chinner]

On Thu, Jul 26, 2018 at 10:35:25AM -0700, 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>
> ---
>  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 b2c683588519..1659016875f9 100644
> --- a/fs/xfs/libxfs/xfs_sb.c
> +++ b/fs/xfs/libxfs/xfs_sb.c
> @@ -714,6 +714,7 @@ xfs_sb_write_verify(
>  	 * cases.
>  	 */
>  	if (sb.sb_fdblocks > sb.sb_dblocks ||
> +	    !xfs_verify_icount(mp, sb.sb_icount) ||
>  	    sb.sb_ifree > sb.sb_icount) {
>  		xfs_notice(mp, "SB summary counter sanity check failed");
>  		error = -EFSCORRUPTED;
> diff --git a/fs/xfs/libxfs/xfs_types.c b/fs/xfs/libxfs/xfs_types.c
> index 2e2a243cef2e..2e9c0c25ccb6 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 += first - last + 1;
> +	}
> +	*max = nr_inos;
> +}

And the effect of the inode32 mount option on the valid icount range?

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

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

[Darrick J. Wong]

On Fri, Jul 27, 2018 at 09:20:28AM +1000, Dave Chinner wrote:
> On Thu, Jul 26, 2018 at 10:35:25AM -0700, 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>
> > ---
> >  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 b2c683588519..1659016875f9 100644
> > --- a/fs/xfs/libxfs/xfs_sb.c
> > +++ b/fs/xfs/libxfs/xfs_sb.c
> > @@ -714,6 +714,7 @@ xfs_sb_write_verify(
> >  	 * cases.
> >  	 */
> >  	if (sb.sb_fdblocks > sb.sb_dblocks ||
> > +	    !xfs_verify_icount(mp, sb.sb_icount) ||
> >  	    sb.sb_ifree > sb.sb_icount) {
> >  		xfs_notice(mp, "SB summary counter sanity check failed");
> >  		error = -EFSCORRUPTED;
> > diff --git a/fs/xfs/libxfs/xfs_types.c b/fs/xfs/libxfs/xfs_types.c
> > index 2e2a243cef2e..2e9c0c25ccb6 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 += first - last + 1;
> > +	}
> > +	*max = nr_inos;
> > +}
> 
> And the effect of the inode32 mount option on the valid icount range?

Heh, I wondered about that.  The premise of inode32 is that we will
never allocate an inode with a number exceeding 2^32, correct?  Do we
ever write anything to that fs to say "this fs must never have inode
numbers > 2^32"?  i.e. something that permanently restricts it to
32-bit inode numbers and counts?  I don't think I see any such device.

What's supposed to happen if I create a > 1TB fs, put a bunch of files
on it such that some of them end up with inode numbers exceeding 2^32,
unmount it, and then mount it again with inode32?  Do we detect this and
refuse the mount because we can't honor the inode32 constraints?

Similarly, what if I create a filesystem with more than 4 billion files
on it, then unmount and remount with inode32?  Do we actually detect
this situation and refuse to mount because we know the counter is
already larger than 2^32?  If we allow the mount today, should we start
failing superblock writes because sb_icount is greater than 2^32?

In other words, I'm not sure inode32 can have any effect on the icount
*max if we don't refuse the mount if the fs already has 64-bit inodes.

--D

> Cheers,
> 
> Dave.
> -- 
> Dave Chinner
> david@fromorbit.com
> --
> 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/2] xfs: verify icount in superblock write

[Brian Foster]

On Thu, Jul 26, 2018 at 05:07:15PM -0700, Darrick J. Wong wrote:
> On Fri, Jul 27, 2018 at 09:20:28AM +1000, Dave Chinner wrote:
> > On Thu, Jul 26, 2018 at 10:35:25AM -0700, 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>
> > > ---
> > >  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 b2c683588519..1659016875f9 100644
> > > --- a/fs/xfs/libxfs/xfs_sb.c
> > > +++ b/fs/xfs/libxfs/xfs_sb.c
> > > @@ -714,6 +714,7 @@ xfs_sb_write_verify(
> > >  	 * cases.
> > >  	 */
> > >  	if (sb.sb_fdblocks > sb.sb_dblocks ||
> > > +	    !xfs_verify_icount(mp, sb.sb_icount) ||
> > >  	    sb.sb_ifree > sb.sb_icount) {
> > >  		xfs_notice(mp, "SB summary counter sanity check failed");
> > >  		error = -EFSCORRUPTED;
> > > diff --git a/fs/xfs/libxfs/xfs_types.c b/fs/xfs/libxfs/xfs_types.c
> > > index 2e2a243cef2e..2e9c0c25ccb6 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 += first - last + 1;

Shouldn't this be last - first?

> > > +	}
> > > +	*max = nr_inos;
> > > +}
> > 
> > And the effect of the inode32 mount option on the valid icount range?
> 
> Heh, I wondered about that.  The premise of inode32 is that we will
> never allocate an inode with a number exceeding 2^32, correct?  Do we
> ever write anything to that fs to say "this fs must never have inode
> numbers > 2^32"?  i.e. something that permanently restricts it to
> 32-bit inode numbers and counts?  I don't think I see any such device.
> 
> What's supposed to happen if I create a > 1TB fs, put a bunch of files
> on it such that some of them end up with inode numbers exceeding 2^32,
> unmount it, and then mount it again with inode32?  Do we detect this and
> refuse the mount because we can't honor the inode32 constraints?
> 
> Similarly, what if I create a filesystem with more than 4 billion files
> on it, then unmount and remount with inode32?  Do we actually detect
> this situation and refuse to mount because we know the counter is
> already larger than 2^32?  If we allow the mount today, should we start
> failing superblock writes because sb_icount is greater than 2^32?
> 

I thought an inode32 mount should allow reading existing inode64 inodes
without an issue. As noted above, it just prevents the allocation of
further inodes beyond 1TB.

> In other words, I'm not sure inode32 can have any effect on the icount
> *max if we don't refuse the mount if the fs already has 64-bit inodes.
> 

This patch looks like it doesn't consider inode32. It just ensures that
the icount falls into a valid range based on the ag geometry, which
seems broad enough to cover all cases... hm?

That aside.. since these values shouldn't change often I'm wondering if
it's worth calculating the global min/max once at mount time (we'd have
to recalc on growfs) rather than in the sb verifier path... It looks
like we already have a bunch of such misc min/max counters in xfs_mount.

Brian

> --D
> 
> > Cheers,
> > 
> > Dave.
> > -- 
> > Dave Chinner
> > david@fromorbit.com
> > --
> > 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
> --
> 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/2] xfs: verify icount in superblock write

[Eric Sandeen]

On 7/27/18 3:44 AM, Brian Foster wrote:
> On Thu, Jul 26, 2018 at 05:07:15PM -0700, Darrick J. Wong wrote:
>> On Fri, Jul 27, 2018 at 09:20:28AM +1000, Dave Chinner wrote:
>>> On Thu, Jul 26, 2018 at 10:35:25AM -0700, 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>
>>>> ---
>>>>  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 b2c683588519..1659016875f9 100644
>>>> --- a/fs/xfs/libxfs/xfs_sb.c
>>>> +++ b/fs/xfs/libxfs/xfs_sb.c
>>>> @@ -714,6 +714,7 @@ xfs_sb_write_verify(
>>>>  	 * cases.
>>>>  	 */
>>>>  	if (sb.sb_fdblocks > sb.sb_dblocks ||
>>>> +	    !xfs_verify_icount(mp, sb.sb_icount) ||
>>>>  	    sb.sb_ifree > sb.sb_icount) {
>>>>  		xfs_notice(mp, "SB summary counter sanity check failed");
>>>>  		error = -EFSCORRUPTED;
>>>> diff --git a/fs/xfs/libxfs/xfs_types.c b/fs/xfs/libxfs/xfs_types.c
>>>> index 2e2a243cef2e..2e9c0c25ccb6 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 += first - last + 1;
> 
> Shouldn't this be last - first?
> 
>>>> +	}
>>>> +	*max = nr_inos;
>>>> +}
>>>
>>> And the effect of the inode32 mount option on the valid icount range?
>>
>> Heh, I wondered about that.  The premise of inode32 is that we will
>> never allocate an inode with a number exceeding 2^32, correct?  Do we
>> ever write anything to that fs to say "this fs must never have inode
>> numbers > 2^32"?  i.e. something that permanently restricts it to
>> 32-bit inode numbers and counts?  I don't think I see any such device.
>>
>> What's supposed to happen if I create a > 1TB fs, put a bunch of files
>> on it such that some of them end up with inode numbers exceeding 2^32,
>> unmount it, and then mount it again with inode32?  Do we detect this and
>> refuse the mount because we can't honor the inode32 constraints?
>>
>> Similarly, what if I create a filesystem with more than 4 billion files
>> on it, then unmount and remount with inode32?  Do we actually detect
>> this situation and refuse to mount because we know the counter is
>> already larger than 2^32?  If we allow the mount today, should we start
>> failing superblock writes because sb_icount is greater than 2^32?
>>
> 
> I thought an inode32 mount should allow reading existing inode64 inodes
> without an issue. As noted above, it just prevents the allocation of
> further inodes beyond 1TB.
> 
>> In other words, I'm not sure inode32 can have any effect on the icount
>> *max if we don't refuse the mount if the fs already has 64-bit inodes.
>>
> 
> This patch looks like it doesn't consider inode32. It just ensures that
> the icount falls into a valid range based on the ag geometry, which
> seems broad enough to cover all cases... hm?
> 
> That aside.. since these values shouldn't change often I'm wondering if
> it's worth calculating the global min/max once at mount time (we'd have
> to recalc on growfs) rather than in the sb verifier path... It looks
> like we already have a bunch of such misc min/max counters in xfs_mount.

It does seem like a fair bit of work to calculate unchanging values.  Since
this is really only an order-ofmagnitude sanity check anyway, I wonder if
this part of the verifier isn't working too hard to arrive at the
best-possible upper bound. My 64-bit divide was dumb & broken, but wouldn't
~(dblocks/inopb) get us close enough with a lot less work?

-Eric

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

[Darrick J. Wong]

On Fri, Jul 27, 2018 at 06:44:40AM -0400, Brian Foster wrote:
> On Thu, Jul 26, 2018 at 05:07:15PM -0700, Darrick J. Wong wrote:
> > On Fri, Jul 27, 2018 at 09:20:28AM +1000, Dave Chinner wrote:
> > > On Thu, Jul 26, 2018 at 10:35:25AM -0700, 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>
> > > > ---
> > > >  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 b2c683588519..1659016875f9 100644
> > > > --- a/fs/xfs/libxfs/xfs_sb.c
> > > > +++ b/fs/xfs/libxfs/xfs_sb.c
> > > > @@ -714,6 +714,7 @@ xfs_sb_write_verify(
> > > >  	 * cases.
> > > >  	 */
> > > >  	if (sb.sb_fdblocks > sb.sb_dblocks ||
> > > > +	    !xfs_verify_icount(mp, sb.sb_icount) ||
> > > >  	    sb.sb_ifree > sb.sb_icount) {
> > > >  		xfs_notice(mp, "SB summary counter sanity check failed");
> > > >  		error = -EFSCORRUPTED;
> > > > diff --git a/fs/xfs/libxfs/xfs_types.c b/fs/xfs/libxfs/xfs_types.c
> > > > index 2e2a243cef2e..2e9c0c25ccb6 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 += first - last + 1;
> 
> Shouldn't this be last - first?

Oops, yes, will fix that.

> > > > +	}
> > > > +	*max = nr_inos;
> > > > +}
> > > 
> > > And the effect of the inode32 mount option on the valid icount range?
> > 
> > Heh, I wondered about that.  The premise of inode32 is that we will
> > never allocate an inode with a number exceeding 2^32, correct?  Do we
> > ever write anything to that fs to say "this fs must never have inode
> > numbers > 2^32"?  i.e. something that permanently restricts it to
> > 32-bit inode numbers and counts?  I don't think I see any such device.
> > 
> > What's supposed to happen if I create a > 1TB fs, put a bunch of files
> > on it such that some of them end up with inode numbers exceeding 2^32,
> > unmount it, and then mount it again with inode32?  Do we detect this and
> > refuse the mount because we can't honor the inode32 constraints?
> > 
> > Similarly, what if I create a filesystem with more than 4 billion files
> > on it, then unmount and remount with inode32?  Do we actually detect
> > this situation and refuse to mount because we know the counter is
> > already larger than 2^32?  If we allow the mount today, should we start
> > failing superblock writes because sb_icount is greater than 2^32?
> > 
> 
> I thought an inode32 mount should allow reading existing inode64 inodes
> without an issue. As noted above, it just prevents the allocation of
> further inodes beyond 1TB.
> 
> > In other words, I'm not sure inode32 can have any effect on the icount
> > *max if we don't refuse the mount if the fs already has 64-bit inodes.
> > 
> 
> This patch looks like it doesn't consider inode32. It just ensures that
> the icount falls into a valid range based on the ag geometry, which
> seems broad enough to cover all cases... hm?

Correct.

> That aside.. since these values shouldn't change often I'm wondering if
> it's worth calculating the global min/max once at mount time (we'd have
> to recalc on growfs) rather than in the sb verifier path... It looks
> like we already have a bunch of such misc min/max counters in xfs_mount.

<shrug> I suppose so, but otoh I doubt this function has a lot of
overhead.  I'll look into it for the next version.

--D

> Brian
> 
> > --D
> > 
> > > Cheers,
> > > 
> > > Dave.
> > > -- 
> > > Dave Chinner
> > > david@fromorbit.com
> > > --
> > > 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
> > --
> > 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
> --
> 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/2] xfs: verify icount in superblock write

[Dave Chinner]

On Fri, Jul 27, 2018 at 06:44:40AM -0400, Brian Foster wrote:
> That aside.. since these values shouldn't change often I'm wondering if
> it's worth calculating the global min/max once at mount time (we'd have

The values are needed during the initial read of the superblock
before any "for the life of the mount" calculations can be made,
and they can change across log recovery, too, if recovery replays a
growfs transaction. Hence they really need to be point in time
calculations based on the current superblock values during
superblock reads....

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

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

[Brian Foster]

On Sun, Jul 29, 2018 at 02:39:00PM +1000, Dave Chinner wrote:
> On Fri, Jul 27, 2018 at 06:44:40AM -0400, Brian Foster wrote:
> > That aside.. since these values shouldn't change often I'm wondering if
> > it's worth calculating the global min/max once at mount time (we'd have
> 
> The values are needed during the initial read of the superblock
> before any "for the life of the mount" calculations can be made,
> and they can change across log recovery, too, if recovery replays a
> growfs transaction. Hence they really need to be point in time
> calculations based on the current superblock values during
> superblock reads....
> 

I'm not so worried about storing the calculated total as much as
simplifying the implementation. All but the last iteration in the
current loop calculate a fixed value, so this can trivially be
simplified to something like:

	xfs_agino_range(mp, --agcount, first, last);
	max = last - first + 1;
	if (agcount) {
		xfs_agino_range(mp, agcount - 1, first, last);
		max += (last - first + 1) * agcount;
	}

... which (if it mattered) could probably simplify even further if we
factored out the additional portion of the calculation that is fixed:

	xfs_agino_range(mp, --agcount, first, last);
	max = last - first + 1;
	if (agcount)
		max += agcount * mp->m_aginodes;

I.e., with an ->m_aginodes field in xfs_mount that is analogous to
sb_agblocks.

Brian

> Cheers,
> 
> Dave.
> -- 
> Dave Chinner
> david@fromorbit.com
> --
> 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 1/2] libxfs: add more bounds checking to sb sanity checks

[Bill O'Donnell]

On Thu, Jul 26, 2018 at 10:35:19AM -0700, 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: remove the icount check, tweak the comments a little]
> Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---

Reviewed-by:
Bill O'Donnell <billodo@redhat.com>

>  fs/xfs/libxfs/xfs_sb.c |   45 +++++++++++++++++++++++++++++++++++----------
>  1 file changed, 35 insertions(+), 10 deletions(-)
> 
> 
> diff --git a/fs/xfs/libxfs/xfs_sb.c b/fs/xfs/libxfs/xfs_sb.c
> index b3ad15956366..b2c683588519 100644
> --- a/fs/xfs/libxfs/xfs_sb.c
> +++ b/fs/xfs/libxfs/xfs_sb.c
> @@ -599,22 +599,16 @@ xfs_sb_to_disk(
>  static int
>  xfs_sb_verify(
>  	struct xfs_buf	*bp,
> +	struct xfs_sb	*sb,
>  	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,
> +	return xfs_mount_validate_sb(mp, sb,
>  				     bp->b_maps[0].bm_bn == XFS_SB_DADDR,
>  				     check_version);
>  }
> @@ -637,6 +631,7 @@ xfs_sb_read_verify(
>  {
>  	struct xfs_mount *mp = bp->b_target->bt_mount;
>  	struct xfs_dsb	*dsb = XFS_BUF_TO_SBP(bp);
> +	struct xfs_sb	sb;
>  	int		error;
>  
>  	/*
> @@ -657,7 +652,13 @@ xfs_sb_read_verify(
>  			}
>  		}
>  	}
> -	error = xfs_sb_verify(bp, true);
> +
> +	/*
> +	 * 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);
> +	error = xfs_sb_verify(bp, &sb, true);
>  
>  out_error:
>  	if (error == -EFSCORRUPTED || error == -EFSBADCRC)
> @@ -693,9 +694,33 @@ xfs_sb_write_verify(
>  {
>  	struct xfs_mount	*mp = bp->b_target->bt_mount;
>  	struct xfs_buf_log_item	*bip = bp->b_log_item;
> +	struct xfs_sb		sb;
>  	int			error;
>  
> -	error = xfs_sb_verify(bp, false);
> +	/*
> +	 * 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);
> +
> +	error = xfs_sb_verify(bp, &sb, false);
> +	if (error)
> +		goto err;
> +
> +	/*
> +	 * 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 (sb.sb_fdblocks > sb.sb_dblocks ||
> +	    sb.sb_ifree > sb.sb_icount) {
> +		xfs_notice(mp, "SB summary counter sanity check failed");
> +		error = -EFSCORRUPTED;
> +		goto err;
> +	}
> +
> +err:
>  	if (error) {
>  		xfs_verifier_error(bp, error, __this_address);
>  		return;
> 

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

[Dave Chinner]

On Thu, Jul 26, 2018 at 10:35:19AM -0700, 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: remove the icount check, tweak the comments a little]
> Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
>  fs/xfs/libxfs/xfs_sb.c |   45 +++++++++++++++++++++++++++++++++++----------
>  1 file changed, 35 insertions(+), 10 deletions(-)
> 
> 
> diff --git a/fs/xfs/libxfs/xfs_sb.c b/fs/xfs/libxfs/xfs_sb.c
> index b3ad15956366..b2c683588519 100644
> --- a/fs/xfs/libxfs/xfs_sb.c
> +++ b/fs/xfs/libxfs/xfs_sb.c
> @@ -599,22 +599,16 @@ xfs_sb_to_disk(
>  static int
>  xfs_sb_verify(
>  	struct xfs_buf	*bp,
> +	struct xfs_sb	*sb,
>  	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,
> +	return xfs_mount_validate_sb(mp, sb,
>  				     bp->b_maps[0].bm_bn == XFS_SB_DADDR,
>  				     check_version);

Why do we need to keep this wrapper function? The in-progress check
is a primary superblock read verifier check (only useful at mount
time), as is the "check_version" flag used to validate the V5
feature mask during mount.

i.e. xfs_sb_verify() needs to go away, and the read-side only checks
need to be moved from xfs_mount_validate_sb() into
xfs_sb_read_verify() too. At which point, xfs_mount_validate_sb()
has nothing to do with mount and can be renamed to
xfs_sb_verify_common()....

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

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

[Eric Sandeen]

On 7/26/18 4:28 PM, Dave Chinner wrote:
> On Thu, Jul 26, 2018 at 10:35:19AM -0700, 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: remove the icount check, tweak the comments a little]
>> Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
>> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
>> ---
>>  fs/xfs/libxfs/xfs_sb.c |   45 +++++++++++++++++++++++++++++++++++----------
>>  1 file changed, 35 insertions(+), 10 deletions(-)
>>
>>
>> diff --git a/fs/xfs/libxfs/xfs_sb.c b/fs/xfs/libxfs/xfs_sb.c
>> index b3ad15956366..b2c683588519 100644
>> --- a/fs/xfs/libxfs/xfs_sb.c
>> +++ b/fs/xfs/libxfs/xfs_sb.c
>> @@ -599,22 +599,16 @@ xfs_sb_to_disk(
>>  static int
>>  xfs_sb_verify(
>>  	struct xfs_buf	*bp,
>> +	struct xfs_sb	*sb,
>>  	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,
>> +	return xfs_mount_validate_sb(mp, sb,
>>  				     bp->b_maps[0].bm_bn == XFS_SB_DADDR,
>>  				     check_version);
> 
> Why do we need to keep this wrapper function? The in-progress check
> is a primary superblock read verifier check (only useful at mount
> time), as is the "check_version" flag used to validate the V5
> feature mask during mount.
> 
> i.e. xfs_sb_verify() needs to go away, and the read-side only checks
> need to be moved from xfs_mount_validate_sb() into
> xfs_sb_read_verify() too. At which point, xfs_mount_validate_sb()
> has nothing to do with mount and can be renamed to
> xfs_sb_verify_common()....

Yeah, I had thought about that too, but Darrick mentioned other pending
rearrangements and figured maybe it could come as a followup patch.

-Eric