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

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

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 |   28 +++++++++++++++++++++++++++-
 2 files changed, 31 insertions(+), 4 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.

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


diff --git a/repair/dinode.c b/repair/dinode.c
index 291c5807..1275c90b 100644
--- a/repair/dinode.c
+++ b/repair/dinode.c
@@ -2178,6 +2178,31 @@ _("Bad %s nsec %u on inode %" PRIu64 ", "), name, be32_to_cpu(t->t_nsec), lino);
 		*dirty = 1;
 	}
 }
+/*
+ * Inode verifiers on older kernels don't check that the extent size hint is an
+ * integer multiple of the rt extent size on a directory with both rtinherit
+ * and extszinherit flags set.  If we encounter a directory that is
+ * misconfigured in this way, or a regular file that inherited a bad hint from
+ * a directory, clear the hint.
+ */
+static bool
+zap_bad_rt_extsize_hint(
+	struct xfs_mount	*mp,
+	struct xfs_dinode	*dino)
+{
+	uint16_t		diflags = be16_to_cpu(dino->di_flags);
+
+	if (!(diflags & XFS_DIFLAG_REALTIME) &&
+	    !(diflags & XFS_DIFLAG_RTINHERIT))
+		return false;
+
+	if (!(diflags & XFS_DIFLAG_EXTSIZE) &&
+	    !(diflags & XFS_DIFLAG_EXTSZINHERIT))
+		return false;
+
+	return (be32_to_cpu(dino->di_extsize) % mp->m_sb.sb_rextsize) != 0;
+}
+
 
 /*
  * returns 0 if the inode is ok, 1 if the inode is corrupt
@@ -2694,7 +2719,8 @@ _("bad (negative) size %" PRId64 " on inode %" PRIu64 "\n"),
 	 * only regular files with REALTIME or EXTSIZE flags set can have
 	 * extsize set, or directories with EXTSZINHERIT.
 	 */
-	if (libxfs_inode_validate_extsize(mp,
+	if (zap_bad_rt_extsize_hint(mp, dino) ||
+	    libxfs_inode_validate_extsize(mp,
 			be32_to_cpu(dino->di_extsize),
 			be16_to_cpu(dino->di_mode),
 			be16_to_cpu(dino->di_flags)) != NULL) {

[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;

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

[Christoph Hellwig]

On Fri, Jul 02, 2021 at 07:57:50PM -0700, 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.
> 
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> ---
>  repair/dinode.c |   28 +++++++++++++++++++++++++++-
>  1 file changed, 27 insertions(+), 1 deletion(-)
> 
> 
> diff --git a/repair/dinode.c b/repair/dinode.c
> index 291c5807..1275c90b 100644
> --- a/repair/dinode.c
> +++ b/repair/dinode.c
> @@ -2178,6 +2178,31 @@ _("Bad %s nsec %u on inode %" PRIu64 ", "), name, be32_to_cpu(t->t_nsec), lino);
>  		*dirty = 1;
>  	}
>  }
> +/*
> + * Inode verifiers on older kernels don't check that the extent size hint is an
> + * integer multiple of the rt extent size on a directory with both rtinherit
> + * and extszinherit flags set.  If we encounter a directory that is
> + * misconfigured in this way, or a regular file that inherited a bad hint from
> + * a directory, clear the hint.
> + */
> +static bool
> +zap_bad_rt_extsize_hint(

The name suggests this function does the zapping itself, while it
actually leaves that to the caller.

Oterwise looks good:

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

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>

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

[Carlos Maiolino]

On Sun, Jul 04, 2021 at 09:51:43AM +0100, Christoph Hellwig wrote:
> On Fri, Jul 02, 2021 at 07:57:50PM -0700, 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.
> > 
> > Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> > ---
> >  repair/dinode.c |   28 +++++++++++++++++++++++++++-
> >  1 file changed, 27 insertions(+), 1 deletion(-)
> > 
> > 
> > diff --git a/repair/dinode.c b/repair/dinode.c
> > index 291c5807..1275c90b 100644
> > --- a/repair/dinode.c
> > +++ b/repair/dinode.c
> > @@ -2178,6 +2178,31 @@ _("Bad %s nsec %u on inode %" PRIu64 ", "), name, be32_to_cpu(t->t_nsec), lino);
> >  		*dirty = 1;
> >  	}
> >  }
> > +/*
> > + * Inode verifiers on older kernels don't check that the extent size hint is an
> > + * integer multiple of the rt extent size on a directory with both rtinherit
> > + * and extszinherit flags set.  If we encounter a directory that is
> > + * misconfigured in this way, or a regular file that inherited a bad hint from
> > + * a directory, clear the hint.
> > + */
> > +static bool
> > +zap_bad_rt_extsize_hint(
> 
> The name suggests this function does the zapping itself, while it
> actually leaves that to the caller.

+1 here, I also led me to believe this was actually zeroing the extsize, but the
patch's logic is fine.

Maybe something like

{is,has}_bad_rt_extsize_hint()?

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

-- 
Carlos

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 1/2] xfs_repair: validate alignment of inherited rt extent hints

[Darrick J. Wong]

On Thu, Jul 08, 2021 at 09:11:16AM +0200, Carlos Maiolino wrote:
> On Sun, Jul 04, 2021 at 09:51:43AM +0100, Christoph Hellwig wrote:
> > On Fri, Jul 02, 2021 at 07:57:50PM -0700, 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.
> > > 
> > > Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> > > ---
> > >  repair/dinode.c |   28 +++++++++++++++++++++++++++-
> > >  1 file changed, 27 insertions(+), 1 deletion(-)
> > > 
> > > 
> > > diff --git a/repair/dinode.c b/repair/dinode.c
> > > index 291c5807..1275c90b 100644
> > > --- a/repair/dinode.c
> > > +++ b/repair/dinode.c
> > > @@ -2178,6 +2178,31 @@ _("Bad %s nsec %u on inode %" PRIu64 ", "), name, be32_to_cpu(t->t_nsec), lino);
> > >  		*dirty = 1;
> > >  	}
> > >  }
> > > +/*
> > > + * Inode verifiers on older kernels don't check that the extent size hint is an
> > > + * integer multiple of the rt extent size on a directory with both rtinherit
> > > + * and extszinherit flags set.  If we encounter a directory that is
> > > + * misconfigured in this way, or a regular file that inherited a bad hint from
> > > + * a directory, clear the hint.
> > > + */
> > > +static bool
> > > +zap_bad_rt_extsize_hint(
> > 
> > The name suggests this function does the zapping itself, while it
> > actually leaves that to the caller.
> 
> +1 here, I also led me to believe this was actually zeroing the extsize, but the
> patch's logic is fine.
> 
> Maybe something like
> 
> {is,has}_bad_rt_extsize_hint()?

Renamed to is_misaligned_extsize_hint().

--D

> 
> > 
> > Oterwise looks good:
> > 
> > Reviewed-by: Christoph Hellwig <hch@lst.de>
> > 
> 
> -- 
> Carlos
> 

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

[Carlos Maiolino]

On Thu, Jul 08, 2021 at 03:31:57PM -0700, Darrick J. Wong wrote:
> On Thu, Jul 08, 2021 at 09:11:16AM +0200, Carlos Maiolino wrote:
> > On Sun, Jul 04, 2021 at 09:51:43AM +0100, Christoph Hellwig wrote:
> > > On Fri, Jul 02, 2021 at 07:57:50PM -0700, 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.
> > > > 
> > > > Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> > > > ---
> > > >  repair/dinode.c |   28 +++++++++++++++++++++++++++-
> > > >  1 file changed, 27 insertions(+), 1 deletion(-)
> > > > 
> > > > 
> > > > diff --git a/repair/dinode.c b/repair/dinode.c
> > > > index 291c5807..1275c90b 100644
> > > > --- a/repair/dinode.c
> > > > +++ b/repair/dinode.c
> > > > @@ -2178,6 +2178,31 @@ _("Bad %s nsec %u on inode %" PRIu64 ", "), name, be32_to_cpu(t->t_nsec), lino);
> > > >  		*dirty = 1;
> > > >  	}
> > > >  }
> > > > +/*
> > > > + * Inode verifiers on older kernels don't check that the extent size hint is an
> > > > + * integer multiple of the rt extent size on a directory with both rtinherit
> > > > + * and extszinherit flags set.  If we encounter a directory that is
> > > > + * misconfigured in this way, or a regular file that inherited a bad hint from
> > > > + * a directory, clear the hint.
> > > > + */
> > > > +static bool
> > > > +zap_bad_rt_extsize_hint(
> > > 
> > > The name suggests this function does the zapping itself, while it
> > > actually leaves that to the caller.
> > 
> > +1 here, I also led me to believe this was actually zeroing the extsize, but the
> > patch's logic is fine.
> > 
> > Maybe something like
> > 
> > {is,has}_bad_rt_extsize_hint()?
> 
> Renamed to is_misaligned_extsize_hint().

Great, thanks Darrick!

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

> 
> --D
> 
> > 
> > > 
> > > Oterwise looks good:
> > > 
> > > Reviewed-by: Christoph Hellwig <hch@lst.de>
> > > 
> > 
> > -- 
> > Carlos
> > 
> 

-- 
Carlos

[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;