diff mbox series

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

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

Commit Message

Darrick J. Wong July 3, 2021, 2:57 a.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.

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

Comments

Christoph Hellwig July 4, 2021, 8:51 a.m. UTC | #1 
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>
Carlos Maiolino July 8, 2021, 7:11 a.m. UTC | #2 
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>
>
Darrick J. Wong July 8, 2021, 10:31 p.m. UTC | #3 
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
>
Carlos Maiolino July 9, 2021, 7:41 a.m. UTC | #4 
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
> > 
>
diff mbox series

Patch

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) {