On Tue, May 25, 2021 at 12:49:02PM +0200, Carlos Maiolino wrote:
> Hi Darrick.
>
> On Mon, May 24, 2021 at 11:15:31PM -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.
> >
> > Because there may be existing filesystems with such a configuration, we
> > cannot simply amend the inode verifier to trip on these directories and
> > call it a day because that will cause previously "working" filesystems
> > to start throwing errors abruptly. Note that it's valid to have
> > directories with rtinherit set even if there is no realtime volume, in
> > which case the problem does not manifest because rtinherit is ignored if
> > there's no realtime device; and it's possible that someone set the flag,
> > crashed, repaired the filesystem (which clears the hint on the realtime
> > file) and continued.
> >
> > Therefore, mitigate this issue in several ways: First, if we try to
> > write out an inode with both rtinherit/extszinherit set and an unaligned
> > extent size hint, turn off the hint to correct the error. Second, if
> > someone tries to misconfigure a directory via the fssetxattr ioctl, fail
> > the ioctl. Third, reverify both extent size hint values when we
> > propagate heritable inode attributes from parent to child, to prevent
> > misconfigurations from spreading.
> >
> > Signed-off-by: Darrick J. Wong <djwong@xxxxxxxxxx>
> > ---
> > v2: disable incorrect hints at runtime instead of whacking filesystems
> > with verifier errors
> > v3: revise the comment in the verifier to describe the source of the
> > problem, the observable symptoms, and how the solution fits the
> > historical context
>
> IMHO the patch is fine, I have just one comment I'd like to address though:
>
> > + /*
> > + * 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're logging a
> > + * directory that is misconfigured in this way, clear the hint.
> > + */
> > + if ((ip->i_diflags & XFS_DIFLAG_RTINHERIT) &&
> > + (ip->i_diflags & XFS_DIFLAG_EXTSZINHERIT) &&
> > + (ip->i_extsize % ip->i_mount->m_sb.sb_rextsize) > 0) {
> > + ip->i_diflags &= ~(XFS_DIFLAG_EXTSIZE |
> > + XFS_DIFLAG_EXTSZINHERIT);
> > + ip->i_extsize = 0;
> > + flags |= XFS_ILOG_CORE;
> > + }
> > +
> ...
> > + * that we don't let broken hints propagate.
> > + */
> > + failaddr = xfs_inode_validate_extsize(ip->i_mount, ip->i_extsize,
> > + VFS_I(ip)->i_mode, ip->i_diflags);
> > + if (failaddr) {
> > + ip->i_diflags &= ~(XFS_DIFLAG_EXTSIZE |
> > + XFS_DIFLAG_EXTSZINHERIT);
> > + ip->i_extsize = 0;
> > + }
> > }
>
> ...
> > + /* Don't let invalid cowextsize hints propagate. */
> > + failaddr = xfs_inode_validate_cowextsize(ip->i_mount, ip->i_cowextsize,
> > + VFS_I(ip)->i_mode, ip->i_diflags, ip->i_diflags2);
> > + if (failaddr) {
> > + ip->i_diflags2 &= ~XFS_DIFLAG2_COWEXTSIZE;
> > + ip->i_cowextsize = 0;
> > + }
> > }
>
> In all cases above, wouldn't be interesting to at least log the fact we are
> resetting the extent size? At least in debug mode? This may let users clueless
> on why the extent size has been reset, or at least give us some debug data when
> required?
Ok, I'll add an xfs_info message to log the fact that we are changing
inode attributes. Thanks for the review!
--D
>
>
> The patch itself looks fine, with or without logging the extsize reset, you can
> add:
>
> Reviewed-by: Carlos Maiolino <cmaiolino@xxxxxxxxxx>
>
> Cheers
>
> >
> > /*
> > diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
> > index 6407921aca96..1fe4c1fc0aea 100644
> > --- a/fs/xfs/xfs_ioctl.c
> > +++ b/fs/xfs/xfs_ioctl.c
> > @@ -1291,6 +1291,21 @@ xfs_ioctl_setattr_check_extsize(
> >
> > new_diflags = xfs_flags2diflags(ip, fa->fsx_xflags);
> >
> > + /*
> > + * 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. Don't let sysadmins
> > + * misconfigure directories.
> > + */
> > + if ((new_diflags & XFS_DIFLAG_RTINHERIT) &&
> > + (new_diflags & XFS_DIFLAG_EXTSZINHERIT)) {
> > + unsigned int rtextsize_bytes;
> > +
> > + rtextsize_bytes = XFS_FSB_TO_B(mp, mp->m_sb.sb_rextsize);
> > + if (fa->fsx_extsize % rtextsize_bytes)
> > + return -EINVAL;
> > + }
> > +
> > failaddr = xfs_inode_validate_extsize(ip->i_mount,
> > XFS_B_TO_FSB(mp, fa->fsx_extsize),
> > VFS_I(ip)->i_mode, new_diflags);
> >
>
> --
> Carlos
>
