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?
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
