diff mbox series

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

Message ID 162750694801.44422.7638736426191840754.stgit@magnolia (mailing list archive)
State Accepted
Headers show
Series xfsprogs: strengthen validation of extent size hints | expand

Commit Message

Darrick J. Wong July 28, 2021, 9:15 p.m. UTC 
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(-)

Comments

Eric Sandeen July 28, 2021, 10:11 p.m. UTC | #1 
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
>
diff mbox series

Patch

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