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

[PATCHSET 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.

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 |   22 +++++++++++++++++-----
 1 file changed, 17 insertions(+), 5 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 |   20 ++++++++++++++++----
 1 file changed, 16 insertions(+), 4 deletions(-)


diff --git a/fs/xfs/xfs_rtalloc.c b/fs/xfs/xfs_rtalloc.c
index 4e7be6b4ca8e..8920bce4fb0a 100644
--- a/fs/xfs/xfs_rtalloc.c
+++ b/fs/xfs/xfs_rtalloc.c
@@ -928,11 +928,23 @@ xfs_growfs_rt(
 	 */
 	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)))
+	if (mp->m_rtdev_targp == NULL || !mp->m_rbmip || !mp->m_rsumip)
 		return -EINVAL;
-	if ((error = xfs_sb_validate_fsb_count(sbp, nrblocks)))
+	if (in->newblocks <= sbp->sb_rblocks)
+		return -EINVAL;
+	if (xfs_sb_version_hasrealtime(&mp->m_sb) &&
+	    in->extsize != sbp->sb_rextsize)
+		return -EINVAL;
+	if (XFS_FSB_TO_B(mp, in->extsize) > XFS_MAX_RTEXTSIZE ||
+	    XFS_FSB_TO_B(mp, in->extsize) < XFS_MIN_RTEXTSIZE)
+		return -EINVAL;
+	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 |    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);

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

[Dave Chinner]

On Mon, Jul 12, 2021 at 03:07:25PM -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>
> ---
>  fs/xfs/xfs_rtalloc.c |   20 ++++++++++++++++----
>  1 file changed, 16 insertions(+), 4 deletions(-)
> 
> 
> diff --git a/fs/xfs/xfs_rtalloc.c b/fs/xfs/xfs_rtalloc.c
> index 4e7be6b4ca8e..8920bce4fb0a 100644
> --- a/fs/xfs/xfs_rtalloc.c
> +++ b/fs/xfs/xfs_rtalloc.c
> @@ -928,11 +928,23 @@ xfs_growfs_rt(
>  	 */
>  	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)))
> +	if (mp->m_rtdev_targp == NULL || !mp->m_rbmip || !mp->m_rsumip)
>  		return -EINVAL;

Shouldn't this use XFS_IS_REALTIME_MOUNT() so it always fails if
CONFIG_XFS_RT=n?

i.e. if we have to check mp->m_rbmip and mp->m_rsumip to determine
if this mount is realtime enabled, then doesn't
XFS_IS_REALTIME_MOUNT() need to be fixed?


> -	if ((error = xfs_sb_validate_fsb_count(sbp, nrblocks)))
> +	if (in->newblocks <= sbp->sb_rblocks)
> +		return -EINVAL;
> +	if (xfs_sb_version_hasrealtime(&mp->m_sb) &&
> +	    in->extsize != sbp->sb_rextsize)
> +		return -EINVAL;

xfs_sb_version_hasrealtime() checks "sbp->sb_rblocks > 0", it's not
an actual version flag check. I think this makes much more sense
being open coded rather than masquerading as a feature check....

> +	if (XFS_FSB_TO_B(mp, in->extsize) > XFS_MAX_RTEXTSIZE ||
> +	    XFS_FSB_TO_B(mp, in->extsize) < XFS_MIN_RTEXTSIZE)
> +		return -EINVAL;
> +	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;

Otherwise looks like a reasonable set of additional checks.

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

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

[Darrick J. Wong]

On Wed, Jul 14, 2021 at 10:58:50AM +1000, Dave Chinner wrote:
> On Mon, Jul 12, 2021 at 03:07:25PM -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>
> > ---
> >  fs/xfs/xfs_rtalloc.c |   20 ++++++++++++++++----
> >  1 file changed, 16 insertions(+), 4 deletions(-)
> > 
> > 
> > diff --git a/fs/xfs/xfs_rtalloc.c b/fs/xfs/xfs_rtalloc.c
> > index 4e7be6b4ca8e..8920bce4fb0a 100644
> > --- a/fs/xfs/xfs_rtalloc.c
> > +++ b/fs/xfs/xfs_rtalloc.c
> > @@ -928,11 +928,23 @@ xfs_growfs_rt(
> >  	 */
> >  	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)))
> > +	if (mp->m_rtdev_targp == NULL || !mp->m_rbmip || !mp->m_rsumip)
> >  		return -EINVAL;
> 
> Shouldn't this use XFS_IS_REALTIME_MOUNT() so it always fails if
> CONFIG_XFS_RT=n?

xfs_rtalloc.c isn't even linked into the binary if CONFIG_XFS_RT=n.

> i.e. if we have to check mp->m_rbmip and mp->m_rsumip to determine
> if this mount is realtime enabled, then doesn't
> XFS_IS_REALTIME_MOUNT() need to be fixed?

TBH I think technically we could actually drop the m_rbmip/m_rsumip
checks since the mount will fail if those files cannot be iget'd.
That said, given how poorly tested realtime is, I figured it doesn't
hurt to double-check for this infrequent operation.

> 
> > -	if ((error = xfs_sb_validate_fsb_count(sbp, nrblocks)))
> > +	if (in->newblocks <= sbp->sb_rblocks)
> > +		return -EINVAL;
> > +	if (xfs_sb_version_hasrealtime(&mp->m_sb) &&
> > +	    in->extsize != sbp->sb_rextsize)
> > +		return -EINVAL;
> 
> xfs_sb_version_hasrealtime() checks "sbp->sb_rblocks > 0", it's not
> an actual version flag check. I think this makes much more sense
> being open coded rather than masquerading as a feature check....

Ok, I'll change it back.

> 
> > +	if (XFS_FSB_TO_B(mp, in->extsize) > XFS_MAX_RTEXTSIZE ||
> > +	    XFS_FSB_TO_B(mp, in->extsize) < XFS_MIN_RTEXTSIZE)
> > +		return -EINVAL;
> > +	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;
> 
> Otherwise looks like a reasonable set of additional checks.

Cool!  Thanks for the review.

--D

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

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