[PATCHSET v3 0/2] xfsprogs: strengthen validation of extent size hints

[PATCHSET v3 0/2] xfsprogs: strengthen validation of extent size hints

[Darrick J. Wong]

Hi all,

While playing around with realtime extent sizes and extent size hints, I
noticed that it was very possible for userspace to trip the inode
verifiers if they tried to set an extent size hint that wasn't aligned
to the rt extent size and then create realtime files.  This series
tightens the existing checks and refactors the ioctls to use the libxfs
validation functions like the verifiers, mkfs, and repair use.

For v2, we also detect invalid extent size hints on existing filesystems
and mitigate the problem by (a) not propagating the invalid hints to new
realtime files and (b) removing invalid hints when set on directories.

v3: move the extsize validation code into a single function

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=extsize-fixes

xfsprogs git tree:
https://git.kernel.org/cgit/linux/kernel/git/djwong/xfsprogs-dev.git/log/?h=extsize-fixes
---
 mkfs/xfs_mkfs.c |    7 +++--
 repair/dinode.c |   71 ++++++++++++++++++++++++++++++++++++++++---------------
 2 files changed, 55 insertions(+), 23 deletions(-)

[PATCH 1/2] xfs_repair: validate alignment of inherited rt extent hints

[Darrick J. Wong]

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

If we encounter a directory that has been configured to pass on an
extent size hint to a new realtime file and the hint isn't an integer
multiple of the rt extent size, we should turn off the hint because that
is a misconfiguration.  Old kernels didn't check for this when copying
attributes into new files and would crash in the rt allocator.

Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
 repair/dinode.c |   71 ++++++++++++++++++++++++++++++++++++++++---------------
 1 file changed, 51 insertions(+), 20 deletions(-)


diff --git a/repair/dinode.c b/repair/dinode.c
index 291c5807..09e42ad2 100644
--- a/repair/dinode.c
+++ b/repair/dinode.c
@@ -2179,6 +2179,56 @@ _("Bad %s nsec %u on inode %" PRIu64 ", "), name, be32_to_cpu(t->t_nsec), lino);
 	}
 }
 
+static void
+validate_extsize(
+	struct xfs_mount	*mp,
+	struct xfs_dinode	*dino,
+	xfs_ino_t		lino,
+	int			*dirty)
+{
+	uint16_t		flags = be16_to_cpu(dino->di_flags);
+	unsigned int		value = be32_to_cpu(dino->di_extsize);
+	bool			misaligned = false;
+	bool			bad;
+
+	/*
+	 * XFS allows a sysadmin to change the rt extent size when adding a rt
+	 * section to a filesystem after formatting.  If there are any
+	 * directories with extszinherit and rtinherit set, the hint could
+	 * become misaligned with the new rextsize.  The verifier doesn't check
+	 * this, because we allow rtinherit directories even without an rt
+	 * device.
+	 */
+	if ((flags & XFS_DIFLAG_EXTSZINHERIT) &&
+	    (flags & XFS_DIFLAG_RTINHERIT) &&
+	    value % mp->m_sb.sb_rextsize > 0)
+		misaligned = true;
+
+	/*
+	 * Complain if the verifier fails.
+	 *
+	 * Old kernels didn't check the alignment of extsize hints when copying
+	 * them to new regular realtime files.  The inode verifier now checks
+	 * the alignment (because misaligned hints cause misbehavior in the rt
+	 * allocator), so we have to complain and fix them.
+	 */
+	bad = libxfs_inode_validate_extsize(mp, value,
+			be16_to_cpu(dino->di_mode), flags) != NULL;
+	if (bad || misaligned) {
+		do_warn(
+_("Bad extent size %u on inode %" PRIu64 ", "),
+				value, lino);
+		if (!no_modify)  {
+			do_warn(_("resetting to zero\n"));
+			dino->di_extsize = 0;
+			dino->di_flags &= ~cpu_to_be16(XFS_DIFLAG_EXTSIZE |
+						       XFS_DIFLAG_EXTSZINHERIT);
+			*dirty = 1;
+		} else
+			do_warn(_("would reset to zero\n"));
+	}
+}
+
 /*
  * returns 0 if the inode is ok, 1 if the inode is corrupt
  * check_dups can be set to 1 *only* when called by the
@@ -2690,26 +2740,7 @@ _("bad (negative) size %" PRId64 " on inode %" PRIu64 "\n"),
 	if (process_check_sb_inodes(mp, dino, lino, &type, dirty) != 0)
 		goto clear_bad_out;
 
-	/*
-	 * only regular files with REALTIME or EXTSIZE flags set can have
-	 * extsize set, or directories with EXTSZINHERIT.
-	 */
-	if (libxfs_inode_validate_extsize(mp,
-			be32_to_cpu(dino->di_extsize),
-			be16_to_cpu(dino->di_mode),
-			be16_to_cpu(dino->di_flags)) != NULL) {
-		do_warn(
-_("Bad extent size %u on inode %" PRIu64 ", "),
-				be32_to_cpu(dino->di_extsize), lino);
-		if (!no_modify)  {
-			do_warn(_("resetting to zero\n"));
-			dino->di_extsize = 0;
-			dino->di_flags &= ~cpu_to_be16(XFS_DIFLAG_EXTSIZE |
-						       XFS_DIFLAG_EXTSZINHERIT);
-			*dirty = 1;
-		} else
-			do_warn(_("would reset to zero\n"));
-	}
+	validate_extsize(mp, dino, lino, dirty);
 
 	/*
 	 * Only (regular files and directories) with COWEXTSIZE flags

[PATCH 2/2] mkfs: validate rt extent size hint when rtinherit is set

[Darrick J. Wong]

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

Extent size hints exist to nudge the behavior of the file data block
allocator towards trying to make aligned allocations.  Therefore, it
doesn't make sense to allow a hint that isn't a multiple of the
fundamental allocation unit for a given file.

This means that if the sysadmin is formatting with rtinherit set on the
root dir, validate_extsize_hint needs to check the hint value on a
simulated realtime file to make sure that it's correct.  Unfortunately,
the gate check here was for a nonzero rt extent size, which is wrong
since we never format with rtextsize==0.  This leads to absurd failures
such as:

# mkfs.xfs -f /dev/sdf -r extsize=7b -d rtinherit=0,extszinherit=13
illegal extent size hint 13, must be less than 649088 and a multiple of 7.

Signed-off-by: Darrick J. Wong <djwong@kernel.org>
Reviewed-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Carlos Maiolino <cmaiolino@redhat.com>
---
 mkfs/xfs_mkfs.c |    7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)


diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
index f84a42f9..9c14c04e 100644
--- a/mkfs/xfs_mkfs.c
+++ b/mkfs/xfs_mkfs.c
@@ -2384,10 +2384,11 @@ _("illegal extent size hint %lld, must be less than %u.\n"),
 	}
 
 	/*
-	 * Now we do it again with a realtime file so that we know the hint and
-	 * flag that get passed on to realtime files will be correct.
+	 * If the value is to be passed on to realtime files, revalidate with
+	 * a realtime file so that we know the hint and flag that get passed on
+	 * to realtime files will be correct.
 	 */
-	if (mp->m_sb.sb_rextsize == 0)
+	if (!(cli->fsx.fsx_xflags & FS_XFLAG_RTINHERIT))
 		return;
 
 	flags = XFS_DIFLAG_REALTIME;

Re: [PATCH 1/2] xfs_repair: validate alignment of inherited rt extent hints

[Eric Sandeen]

On 7/28/21 2:15 PM, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> If we encounter a directory that has been configured to pass on an
> extent size hint to a new realtime file and the hint isn't an integer
> multiple of the rt extent size, we should turn off the hint because that
> is a misconfiguration.  Old kernels didn't check for this when copying
> attributes into new files and would crash in the rt allocator.
> 
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>

This looks reasonable to me, it's 90% refactoring and 10% new test.

My only concern is that a failed validation of the extent size /hint/ still
says ""Bad extent size %u on inode" but I'm not sure that's terribly important
to change ... so unless you want to -

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

> ---
>  repair/dinode.c |   71 ++++++++++++++++++++++++++++++++++++++++---------------
>  1 file changed, 51 insertions(+), 20 deletions(-)
> 
> 
> diff --git a/repair/dinode.c b/repair/dinode.c
> index 291c5807..09e42ad2 100644
> --- a/repair/dinode.c
> +++ b/repair/dinode.c
> @@ -2179,6 +2179,56 @@ _("Bad %s nsec %u on inode %" PRIu64 ", "), name, be32_to_cpu(t->t_nsec), lino);
>  	}
>  }
>  
> +static void
> +validate_extsize(
> +	struct xfs_mount	*mp,
> +	struct xfs_dinode	*dino,
> +	xfs_ino_t		lino,
> +	int			*dirty)
> +{
> +	uint16_t		flags = be16_to_cpu(dino->di_flags);
> +	unsigned int		value = be32_to_cpu(dino->di_extsize);
> +	bool			misaligned = false;
> +	bool			bad;
> +
> +	/*
> +	 * XFS allows a sysadmin to change the rt extent size when adding a rt
> +	 * section to a filesystem after formatting.  If there are any
> +	 * directories with extszinherit and rtinherit set, the hint could
> +	 * become misaligned with the new rextsize.  The verifier doesn't check
> +	 * this, because we allow rtinherit directories even without an rt
> +	 * device.
> +	 */
> +	if ((flags & XFS_DIFLAG_EXTSZINHERIT) &&
> +	    (flags & XFS_DIFLAG_RTINHERIT) &&
> +	    value % mp->m_sb.sb_rextsize > 0)
> +		misaligned = true;
> +
> +	/*
> +	 * Complain if the verifier fails.
> +	 *
> +	 * Old kernels didn't check the alignment of extsize hints when copying
> +	 * them to new regular realtime files.  The inode verifier now checks
> +	 * the alignment (because misaligned hints cause misbehavior in the rt
> +	 * allocator), so we have to complain and fix them.
> +	 */
> +	bad = libxfs_inode_validate_extsize(mp, value,
> +			be16_to_cpu(dino->di_mode), flags) != NULL;
> +	if (bad || misaligned) {
> +		do_warn(
> +_("Bad extent size %u on inode %" PRIu64 ", "),
> +				value, lino);
> +		if (!no_modify)  {
> +			do_warn(_("resetting to zero\n"));
> +			dino->di_extsize = 0;
> +			dino->di_flags &= ~cpu_to_be16(XFS_DIFLAG_EXTSIZE |
> +						       XFS_DIFLAG_EXTSZINHERIT);
> +			*dirty = 1;
> +		} else
> +			do_warn(_("would reset to zero\n"));
> +	}
> +}
> +
>  /*
>   * returns 0 if the inode is ok, 1 if the inode is corrupt
>   * check_dups can be set to 1 *only* when called by the
> @@ -2690,26 +2740,7 @@ _("bad (negative) size %" PRId64 " on inode %" PRIu64 "\n"),
>  	if (process_check_sb_inodes(mp, dino, lino, &type, dirty) != 0)
>  		goto clear_bad_out;
>  
> -	/*
> -	 * only regular files with REALTIME or EXTSIZE flags set can have
> -	 * extsize set, or directories with EXTSZINHERIT.
> -	 */
> -	if (libxfs_inode_validate_extsize(mp,
> -			be32_to_cpu(dino->di_extsize),
> -			be16_to_cpu(dino->di_mode),
> -			be16_to_cpu(dino->di_flags)) != NULL) {
> -		do_warn(
> -_("Bad extent size %u on inode %" PRIu64 ", "),
> -				be32_to_cpu(dino->di_extsize), lino);
> -		if (!no_modify)  {
> -			do_warn(_("resetting to zero\n"));
> -			dino->di_extsize = 0;
> -			dino->di_flags &= ~cpu_to_be16(XFS_DIFLAG_EXTSIZE |
> -						       XFS_DIFLAG_EXTSZINHERIT);
> -			*dirty = 1;
> -		} else
> -			do_warn(_("would reset to zero\n"));
> -	}
> +	validate_extsize(mp, dino, lino, dirty);
>  
>  	/*
>  	 * Only (regular files and directories) with COWEXTSIZE flags
> 

Re: [PATCH 2/2] mkfs: validate rt extent size hint when rtinherit is set

[Carlos Maiolino]

On Fri, Jul 02, 2021 at 07:57:55PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> Extent size hints exist to nudge the behavior of the file data block
> allocator towards trying to make aligned allocations.  Therefore, it
> doesn't make sense to allow a hint that isn't a multiple of the
> fundamental allocation unit for a given file.
> 
> This means that if the sysadmin is formatting with rtinherit set on the
> root dir, validate_extsize_hint needs to check the hint value on a
> simulated realtime file to make sure that it's correct.  Unfortunately,
> the gate check here was for a nonzero rt extent size, which is wrong
> since we never format with rtextsize==0.  This leads to absurd failures
> such as:

Reviewed-by: Carlos Maiolino <cmaiolino@redhat.com>

> 
> # mkfs.xfs -f /dev/sdf -r extsize=7b -d rtinherit=0,extszinherit=13
> illegal extent size hint 13, must be less than 649088 and a multiple of 7.
> 
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> ---
>  mkfs/xfs_mkfs.c |    7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> 
> diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
> index f84a42f9..9c14c04e 100644
> --- a/mkfs/xfs_mkfs.c
> +++ b/mkfs/xfs_mkfs.c
> @@ -2384,10 +2384,11 @@ _("illegal extent size hint %lld, must be less than %u.\n"),
>  	}
>  
>  	/*
> -	 * Now we do it again with a realtime file so that we know the hint and
> -	 * flag that get passed on to realtime files will be correct.
> +	 * If the value is to be passed on to realtime files, revalidate with
> +	 * a realtime file so that we know the hint and flag that get passed on
> +	 * to realtime files will be correct.
>  	 */
> -	if (mp->m_sb.sb_rextsize == 0)
> +	if (!(cli->fsx.fsx_xflags & FS_XFLAG_RTINHERIT))
>  		return;
>  
>  	flags = XFS_DIFLAG_REALTIME;
> 

-- 
Carlos

Re: [PATCH 2/2] mkfs: validate rt extent size hint when rtinherit is set

[Christoph Hellwig]

Looks good,

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

[PATCH 2/2] mkfs: validate rt extent size hint when rtinherit is set

[Darrick J. Wong]

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

Extent size hints exist to nudge the behavior of the file data block
allocator towards trying to make aligned allocations.  Therefore, it
doesn't make sense to allow a hint that isn't a multiple of the
fundamental allocation unit for a given file.

This means that if the sysadmin is formatting with rtinherit set on the
root dir, validate_extsize_hint needs to check the hint value on a
simulated realtime file to make sure that it's correct.  Unfortunately,
the gate check here was for a nonzero rt extent size, which is wrong
since we never format with rtextsize==0.  This leads to absurd failures
such as:

# mkfs.xfs -f /dev/sdf -r extsize=7b -d rtinherit=0,extszinherit=13
illegal extent size hint 13, must be less than 649088 and a multiple of 7.

Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
 mkfs/xfs_mkfs.c |    7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)


diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
index f84a42f9..9c14c04e 100644
--- a/mkfs/xfs_mkfs.c
+++ b/mkfs/xfs_mkfs.c
@@ -2384,10 +2384,11 @@ _("illegal extent size hint %lld, must be less than %u.\n"),
 	}
 
 	/*
-	 * Now we do it again with a realtime file so that we know the hint and
-	 * flag that get passed on to realtime files will be correct.
+	 * If the value is to be passed on to realtime files, revalidate with
+	 * a realtime file so that we know the hint and flag that get passed on
+	 * to realtime files will be correct.
 	 */
-	if (mp->m_sb.sb_rextsize == 0)
+	if (!(cli->fsx.fsx_xflags & FS_XFLAG_RTINHERIT))
 		return;
 
 	flags = XFS_DIFLAG_REALTIME;