On Fri, May 14, 2021 at 08:38:35AM -0400, Brian Foster wrote:
> On Wed, May 12, 2021 at 06:01:58PM -0700, Darrick J. Wong wrote:
> > From: Darrick J. Wong <djwong@xxxxxxxxxx>
> >
> > The RTINHERIT bit can be set on a directory so that newly created
> > regular files will have the REALTIME bit set to store their data on the
> > realtime volume. If an extent size hint (and EXTSZINHERIT) are set on
> > the directory, the hint will also be copied into the new file.
> >
> > As pointed out in previous patches, for realtime files we require the
> > extent size hint be an integer multiple of the realtime extent, but we
> > don't perform the same validation on a directory with both RTINHERIT and
> > EXTSZINHERIT set, even though the only use-case of that combination is
> > to propagate extent size hints into new realtime files. This leads to
> > inode corruption errors when the bad values are propagated.
> >
> > Strengthen the validation routine to avoid this situation and fix the
> > open-coded unit conversion while we're at it. Note that this is
> > technically a breaking change to the ondisk format, but the risk should
> > be minimal because (a) most vendors disable realtime, (b) letting
> > unaligned hints propagate to new files would immediately crash the
> > filesystem, and (c) xfs_repair flags such filesystems as corrupt, so
> > anyone with such a configuration is broken already anyway.
> >
> > Signed-off-by: Darrick J. Wong <djwong@xxxxxxxxxx>
> > ---
>
> Ok, so this looks more like a proper fix, but does this turn an existing
> directory with (rtinherit && extszinherit) and a badly aligned extsz
> hint into a read validation error?
Hmm, you're right. This fix needs to be more targeted in its nature.
For non-rt filesystems, the rtinherit bit being set on a directory is
benign because we won't set the realtime bit on new files, so there's no
need to introduce a new verifier error that will fail existing
filesystems.
We /do/ need to trap the misconfiguration for filesystems with an rt
volume because those filesystems will fail if the propagation happens.
I think the solution here is to change the verifier check here to
prevent the spread of bad extent size hints:
if (rt_flag || (xfs_sb_version_hasrealtime(&mp->m_sb) &&
rtinherit_flag && inherit_flag))
blocksize_bytes = XFS_FSB_TO_B(mp, mp->m_sb.sb_rextsize);
else
blocksize_bytes = mp->m_sb.sb_blocksize;
...and add a check to xfs_ioctl_setattr_check_extsize to prevent
sysadmins from misconfiguring directories in the first place.
--D
> Brian
>
> > fs/xfs/libxfs/xfs_inode_buf.c | 7 ++++---
> > 1 file changed, 4 insertions(+), 3 deletions(-)
> >
> >
> > diff --git a/fs/xfs/libxfs/xfs_inode_buf.c b/fs/xfs/libxfs/xfs_inode_buf.c
> > index 5c9a7440d9e4..25261dd73290 100644
> > --- a/fs/xfs/libxfs/xfs_inode_buf.c
> > +++ b/fs/xfs/libxfs/xfs_inode_buf.c
> > @@ -569,19 +569,20 @@ xfs_inode_validate_extsize(
> > uint16_t mode,
> > uint16_t flags)
> > {
> > - bool rt_flag;
> > + bool rt_flag, rtinherit_flag;
> > bool hint_flag;
> > bool inherit_flag;
> > uint32_t extsize_bytes;
> > uint32_t blocksize_bytes;
> >
> > rt_flag = (flags & XFS_DIFLAG_REALTIME);
> > + rtinherit_flag = (flags & XFS_DIFLAG_RTINHERIT);
> > hint_flag = (flags & XFS_DIFLAG_EXTSIZE);
> > inherit_flag = (flags & XFS_DIFLAG_EXTSZINHERIT);
> > extsize_bytes = XFS_FSB_TO_B(mp, extsize);
> >
> > - if (rt_flag)
> > - blocksize_bytes = mp->m_sb.sb_rextsize << mp->m_sb.sb_blocklog;
> > + if (rt_flag || (rtinherit_flag && inherit_flag))
> > + blocksize_bytes = XFS_FSB_TO_B(mp, mp->m_sb.sb_rextsize);
> > else
> > blocksize_bytes = mp->m_sb.sb_blocksize;
> >
> >
>
