[PATCHSET v2 0/2] xfs: small fixes to realtime growfs

[PATCHSET v2 0/2] xfs: small fixes to realtime growfs

[Darrick J. Wong]

Hi all,

While auditing the GROWFSRT ioctl, I noticed that there are a few things
missing in the argument validation code that could lead to invalid fs
geometry.  Fix the validation and overflow errors.

v2: tweak some of the checks, straighten out the int overflow cleanup

If you're going to start using this mess, you probably ought to just
pull from my git trees, which are linked below.

This is an extraordinary way to destroy everything.  Enjoy!
Comments and questions are, as always, welcome.

--D

kernel git tree:
https://git.kernel.org/cgit/linux/kernel/git/djwong/xfs-linux.git/log/?h=growfsrt-fixes
---
 fs/xfs/xfs_rtalloc.c |   49 +++++++++++++++++++++++++++++++++++++------------
 1 file changed, 37 insertions(+), 12 deletions(-)

[PATCH 1/2] xfs: improve FSGROWFSRT precondition checking

[Darrick J. Wong]

From: Darrick J. Wong <djwong@kernel.org>

Improve the checking at the start of a realtime grow operation so that
we avoid accidentally set a new extent size that is too large and avoid
adding an rt volume to a filesystem with rmap or reflink because we
don't support rt rmap or reflink yet.

While we're at it, separate the checks so that we're only testing one
aspect at a time.

Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
 fs/xfs/xfs_rtalloc.c |   39 ++++++++++++++++++++++++++++++++-------
 1 file changed, 32 insertions(+), 7 deletions(-)


diff --git a/fs/xfs/xfs_rtalloc.c b/fs/xfs/xfs_rtalloc.c
index 4e7be6b4ca8e..8f6a05db4468 100644
--- a/fs/xfs/xfs_rtalloc.c
+++ b/fs/xfs/xfs_rtalloc.c
@@ -923,16 +923,41 @@ xfs_growfs_rt(
 	uint8_t		*rsum_cache;	/* old summary cache */
 
 	sbp = &mp->m_sb;
-	/*
-	 * Initial error checking.
-	 */
+
 	if (!capable(CAP_SYS_ADMIN))
 		return -EPERM;
-	if (mp->m_rtdev_targp == NULL || mp->m_rbmip == NULL ||
-	    (nrblocks = in->newblocks) <= sbp->sb_rblocks ||
-	    (sbp->sb_rblocks && (in->extsize != sbp->sb_rextsize)))
+
+	/* Needs to have been mounted with an rt device. */
+	if (!XFS_IS_REALTIME_MOUNT(mp))
 		return -EINVAL;
-	if ((error = xfs_sb_validate_fsb_count(sbp, nrblocks)))
+	/*
+	 * Mount should fail if the rt bitmap/summary files don't load, but
+	 * we'll check anyway.
+	 */
+	if (!mp->m_rbmip || !mp->m_rsumip)
+		return -EINVAL;
+
+	/* Shrink not supported. */
+	if (in->newblocks <= sbp->sb_rblocks)
+		return -EINVAL;
+
+	/* Can only change rt extent size when adding rt volume. */
+	if (sbp->sb_rblocks > 0 && in->extsize != sbp->sb_rextsize)
+		return -EINVAL;
+
+	/* Range check the extent size. */
+	if (XFS_FSB_TO_B(mp, in->extsize) > XFS_MAX_RTEXTSIZE ||
+	    XFS_FSB_TO_B(mp, in->extsize) < XFS_MIN_RTEXTSIZE)
+		return -EINVAL;
+
+	/* Unsupported realtime features. */
+	if (xfs_sb_version_hasrmapbt(&mp->m_sb) ||
+	    xfs_sb_version_hasreflink(&mp->m_sb))
+		return -EOPNOTSUPP;
+
+	nrblocks = in->newblocks;
+	error = xfs_sb_validate_fsb_count(sbp, nrblocks);
+	if (error)
 		return error;
 	/*
 	 * Read in the last block of the device, make sure it exists.

[PATCH 2/2] xfs: fix an integer overflow error in xfs_growfs_rt

[Darrick J. Wong]

From: Darrick J. Wong <djwong@kernel.org>

During a realtime grow operation, we run a single transaction for each
rt bitmap block added to the filesystem.  This means that each step has
to be careful to increase sb_rblocks appropriately.

Fix the integer overflow error in this calculation that can happen when
the extent size is very large.  Found by running growfs to add a rt
volume to a filesystem formatted with a 1g rt extent size.

Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
 fs/xfs/xfs_rtalloc.c |   10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)


diff --git a/fs/xfs/xfs_rtalloc.c b/fs/xfs/xfs_rtalloc.c
index 8f6a05db4468..699066fb9052 100644
--- a/fs/xfs/xfs_rtalloc.c
+++ b/fs/xfs/xfs_rtalloc.c
@@ -1021,7 +1021,8 @@ xfs_growfs_rt(
 		     ((sbp->sb_rextents & ((1 << mp->m_blkbit_log) - 1)) != 0);
 	     bmbno < nrbmblocks;
 	     bmbno++) {
-		xfs_trans_t	*tp;
+		struct xfs_trans	*tp;
+		xfs_rfsblock_t		nrblocks_step;
 
 		*nmp = *mp;
 		nsbp = &nmp->m_sb;
@@ -1030,10 +1031,9 @@ xfs_growfs_rt(
 		 */
 		nsbp->sb_rextsize = in->extsize;
 		nsbp->sb_rbmblocks = bmbno + 1;
-		nsbp->sb_rblocks =
-			XFS_RTMIN(nrblocks,
-				  nsbp->sb_rbmblocks * NBBY *
-				  nsbp->sb_blocksize * nsbp->sb_rextsize);
+		nrblocks_step = (bmbno + 1) * NBBY * nsbp->sb_blocksize *
+				nsbp->sb_rextsize;
+		nsbp->sb_rblocks = min(nrblocks, nrblocks_step);
 		nsbp->sb_rextents = nsbp->sb_rblocks;
 		do_div(nsbp->sb_rextents, nsbp->sb_rextsize);
 		ASSERT(nsbp->sb_rextents != 0);

Re: [PATCH 1/2] xfs: improve FSGROWFSRT precondition checking

[Dave Chinner]

On Wed, Jul 14, 2021 at 02:25:23PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> Improve the checking at the start of a realtime grow operation so that
> we avoid accidentally set a new extent size that is too large and avoid
> adding an rt volume to a filesystem with rmap or reflink because we
> don't support rt rmap or reflink yet.
> 
> While we're at it, separate the checks so that we're only testing one
> aspect at a time.
> 
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>

So improvement. Much nice.

Reviewed-by: Dave Chinner <dchinner@redhat.com>
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH 2/2] xfs: fix an integer overflow error in xfs_growfs_rt

[Dave Chinner]

On Wed, Jul 14, 2021 at 02:25:28PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> During a realtime grow operation, we run a single transaction for each
> rt bitmap block added to the filesystem.  This means that each step has
> to be careful to increase sb_rblocks appropriately.
> 
> Fix the integer overflow error in this calculation that can happen when
> the extent size is very large.  Found by running growfs to add a rt
> volume to a filesystem formatted with a 1g rt extent size.
> 
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> ---
>  fs/xfs/xfs_rtalloc.c |   10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)

Much easier to read now.

Reviewed-by: Dave Chinner <dchinner@redhat.com>

-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH 1/2] xfs: improve FSGROWFSRT precondition checking

[Christoph Hellwig]

On Wed, Jul 14, 2021 at 02:25:23PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> Improve the checking at the start of a realtime grow operation so that
> we avoid accidentally set a new extent size that is too large and avoid
> adding an rt volume to a filesystem with rmap or reflink because we
> don't support rt rmap or reflink yet.
> 
> While we're at it, separate the checks so that we're only testing one
> aspect at a time.

Looks good,

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

Re: [PATCH 2/2] xfs: fix an integer overflow error in xfs_growfs_rt

[Christoph Hellwig]

Looks good,

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

Re: [PATCH 2/2] xfs: fix an integer overflow error in xfs_growfs_rt

[Darrick J. Wong]

On Wed, Jul 14, 2021 at 11:12:15AM +1000, Dave Chinner wrote:
> On Mon, Jul 12, 2021 at 03:07:31PM -0700, Darrick J. Wong wrote:
> > From: Darrick J. Wong <djwong@kernel.org>
> > 
> > During a realtime grow operation, we run a single transaction for each
> > rt bitmap block added to the filesystem.  This means that each step has
> > to be careful to increase sb_rblocks appropriately.
> > 
> > Fix the integer overflow error in this calculation that can happen when
> > the extent size is very large.  Found by running growfs to add a rt
> > volume to a filesystem formatted with a 1g rt extent size.
> > 
> > Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> > ---
> >  fs/xfs/xfs_rtalloc.c |    2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > 
> > diff --git a/fs/xfs/xfs_rtalloc.c b/fs/xfs/xfs_rtalloc.c
> > index 8920bce4fb0a..a47d43c30283 100644
> > --- a/fs/xfs/xfs_rtalloc.c
> > +++ b/fs/xfs/xfs_rtalloc.c
> > @@ -1019,7 +1019,7 @@ xfs_growfs_rt(
> >  		nsbp->sb_rbmblocks = bmbno + 1;
> >  		nsbp->sb_rblocks =
> >  			XFS_RTMIN(nrblocks,
> > -				  nsbp->sb_rbmblocks * NBBY *
> > +				  (xfs_rfsblock_t)nsbp->sb_rbmblocks * NBBY *
> >  				  nsbp->sb_blocksize * nsbp->sb_rextsize);
> >  		nsbp->sb_rextents = nsbp->sb_rblocks;
> >  		do_div(nsbp->sb_rextents, nsbp->sb_rextsize);
> 
> Oh, that's just nasty code.  This needs a comment explaining that the
> cast is to avoid an overflow, otherwise someone will come along
> later and remove the "unnecessary" cast.
> 
> Alternatively, because we do "nsbp->sb_rbmblocks = bmbno + 1;" a
> couple of lines above, this could be done differently without the
> need for a cast. Make bmbno a xfs_rfsblock_t, and simply write the
> code as:
> 
> 		nsbp->sb_rblocks = min(nrblocks,
> 					(bmbno + 1) * NBBY *
> 					nsbp->sb_blocksize * nsbp->sb_rextsize);
> 		nsbp->sb_rbmblocks = bmbno + 1;

I like that, it'll get changed in the next revision.

> Notes for future cleanup:
> 
> #define XFS_RTMIN(a,b) ((a) < (b) ? (a) : (b))
> 
> Needs to die.

Heh, yes.

--D

> 
> Cheers,
> 
> Dave.
> -- 
> Dave Chinner
> david@fromorbit.com

Re: [PATCH 2/2] xfs: fix an integer overflow error in xfs_growfs_rt

[Dave Chinner]

On Mon, Jul 12, 2021 at 03:07:31PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> During a realtime grow operation, we run a single transaction for each
> rt bitmap block added to the filesystem.  This means that each step has
> to be careful to increase sb_rblocks appropriately.
> 
> Fix the integer overflow error in this calculation that can happen when
> the extent size is very large.  Found by running growfs to add a rt
> volume to a filesystem formatted with a 1g rt extent size.
> 
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> ---
>  fs/xfs/xfs_rtalloc.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> 
> diff --git a/fs/xfs/xfs_rtalloc.c b/fs/xfs/xfs_rtalloc.c
> index 8920bce4fb0a..a47d43c30283 100644
> --- a/fs/xfs/xfs_rtalloc.c
> +++ b/fs/xfs/xfs_rtalloc.c
> @@ -1019,7 +1019,7 @@ xfs_growfs_rt(
>  		nsbp->sb_rbmblocks = bmbno + 1;
>  		nsbp->sb_rblocks =
>  			XFS_RTMIN(nrblocks,
> -				  nsbp->sb_rbmblocks * NBBY *
> +				  (xfs_rfsblock_t)nsbp->sb_rbmblocks * NBBY *
>  				  nsbp->sb_blocksize * nsbp->sb_rextsize);
>  		nsbp->sb_rextents = nsbp->sb_rblocks;
>  		do_div(nsbp->sb_rextents, nsbp->sb_rextsize);

Oh, that's just nasty code.  This needs a comment explaining that the
cast is to avoid an overflow, otherwise someone will come along
later and remove the "unnecessary" cast.

Alternatively, because we do "nsbp->sb_rbmblocks = bmbno + 1;" a
couple of lines above, this could be done differently without the
need for a cast. Make bmbno a xfs_rfsblock_t, and simply write the
code as:

		nsbp->sb_rblocks = min(nrblocks,
					(bmbno + 1) * NBBY *
					nsbp->sb_blocksize * nsbp->sb_rextsize);
		nsbp->sb_rbmblocks = bmbno + 1;

Notes for future cleanup:

#define XFS_RTMIN(a,b) ((a) < (b) ? (a) : (b))

Needs to die.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

[PATCH 2/2] xfs: fix an integer overflow error in xfs_growfs_rt

[Darrick J. Wong]

From: Darrick J. Wong <djwong@kernel.org>

During a realtime grow operation, we run a single transaction for each
rt bitmap block added to the filesystem.  This means that each step has
to be careful to increase sb_rblocks appropriately.

Fix the integer overflow error in this calculation that can happen when
the extent size is very large.  Found by running growfs to add a rt
volume to a filesystem formatted with a 1g rt extent size.

Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
 fs/xfs/xfs_rtalloc.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)


diff --git a/fs/xfs/xfs_rtalloc.c b/fs/xfs/xfs_rtalloc.c
index 8920bce4fb0a..a47d43c30283 100644
--- a/fs/xfs/xfs_rtalloc.c
+++ b/fs/xfs/xfs_rtalloc.c
@@ -1019,7 +1019,7 @@ xfs_growfs_rt(
 		nsbp->sb_rbmblocks = bmbno + 1;
 		nsbp->sb_rblocks =
 			XFS_RTMIN(nrblocks,
-				  nsbp->sb_rbmblocks * NBBY *
+				  (xfs_rfsblock_t)nsbp->sb_rbmblocks * NBBY *
 				  nsbp->sb_blocksize * nsbp->sb_rextsize);
 		nsbp->sb_rextents = nsbp->sb_rblocks;
 		do_div(nsbp->sb_rextents, nsbp->sb_rextsize);