Message ID | 162528107024.36302.9037961042426880362.stgit@locust (mailing list archive) |
---|---|
State | Superseded |
Headers | show |
Series | xfsprogs: strengthen validation of extent size hints | expand |
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>
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> >
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 >
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 --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) {