On Thu, Jul 08, 2021 at 09:11:52PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@xxxxxxxxxx>
>
> While auditing the realtime growfs code, I realized that the GROWFSRT
> ioctl (and by extension xfs_growfs) has always allowed sysadmins to
> change the realtime extent size when adding a realtime section to the
> filesystem. Since we also have always allowed sysadmins to set
> RTINHERIT and EXTSZINHERIT on directories even if there is no realtime
> device, this invalidates the premise laid out in the comments added in
> commit 603f000b15f2.
>
> In other words, this is not a case of inadequate metadata validation.
> This is a case of nearly forgotten (and apparently untested) but
> supported functionality. Update the comments to reflect what we've
> learned, and remove the log message about correcting the misalignment.
>
> Fixes: 603f000b15f2 ("xfs: validate extsz hints against rt extent size when rtinherit is set")
> Signed-off-by: Darrick J. Wong <djwong@xxxxxxxxxx>
Reviewed-by: Carlos Maiolino <cmaiolino@xxxxxxxxxx>
> ---
> fs/xfs/libxfs/xfs_inode_buf.c | 28 ++++++++++++++++------------
> fs/xfs/libxfs/xfs_trans_inode.c | 10 ++++------
> fs/xfs/xfs_ioctl.c | 8 ++++----
> 3 files changed, 24 insertions(+), 22 deletions(-)
>
> diff --git a/fs/xfs/libxfs/xfs_inode_buf.c b/fs/xfs/libxfs/xfs_inode_buf.c
> index 04ce361688f7..84ea2e0af9f0 100644
> --- a/fs/xfs/libxfs/xfs_inode_buf.c
> +++ b/fs/xfs/libxfs/xfs_inode_buf.c
> @@ -592,23 +592,27 @@ xfs_inode_validate_extsize(
> /*
> * This comment describes a historic gap in this verifier function.
> *
> - * On older kernels, the extent size hint verifier doesn't check that
> - * the extent size hint is an integer multiple of the realtime extent
> - * size on a directory with both RTINHERIT and EXTSZINHERIT flags set.
> - * The verifier has always enforced the alignment rule for regular
> - * files with the REALTIME flag set.
> + * For a directory with both RTINHERIT and EXTSZINHERIT flags set, this
> + * function has never checked that the extent size hint is an integer
> + * multiple of the realtime extent size. Since we allow users to set
> + * this combination on non-rt filesystems /and/ to change the rt
> + * extent size when adding a rt device to a filesystem, the net effect
> + * is that users can configure a filesystem anticipating one rt
> + * geometry and change their minds later. Directories do not use the
> + * extent size hint, so this is harmless for them.
> *
> * If a directory with a misaligned extent size hint is allowed to
> * propagate that hint into a new regular realtime file, the result
> * is that the inode cluster buffer verifier will trigger a corruption
> - * shutdown the next time it is run.
> + * shutdown the next time it is run, because the verifier has always
> + * enforced the alignment rule for regular files.
> *
> - * Unfortunately, there could be filesystems with these misconfigured
> - * directories in the wild, so we cannot add a check to this verifier
> - * at this time because that will result a new source of directory
> - * corruption errors when reading an existing filesystem. Instead, we
> - * permit the misconfiguration to pass through the verifiers so that
> - * callers of this function can correct and mitigate externally.
> + * Because we allow administrators to set a new rt extent size when
> + * adding a rt section, we cannot add a check to this verifier because
> + * that will result a new source of directory corruption errors when
> + * reading an existing filesystem. Instead, we rely on callers to
> + * decide when alignment checks are appropriate, and fix things up as
> + * needed.
> */
>
> if (rt_flag)
> diff --git a/fs/xfs/libxfs/xfs_trans_inode.c b/fs/xfs/libxfs/xfs_trans_inode.c
> index 8d595a5c4abd..16f723ebe8dd 100644
> --- a/fs/xfs/libxfs/xfs_trans_inode.c
> +++ b/fs/xfs/libxfs/xfs_trans_inode.c
> @@ -143,16 +143,14 @@ xfs_trans_log_inode(
> }
>
> /*
> - * 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.
> + * Inode verifiers do not 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) {
> - xfs_info_once(ip->i_mount,
> - "Correcting misaligned extent size hint in inode 0x%llx.", ip->i_ino);
> ip->i_diflags &= ~(XFS_DIFLAG_EXTSIZE |
> XFS_DIFLAG_EXTSZINHERIT);
> ip->i_extsize = 0;
> diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
> index 0f6794333b01..cfc2e099d558 100644
> --- a/fs/xfs/xfs_ioctl.c
> +++ b/fs/xfs/xfs_ioctl.c
> @@ -1292,10 +1292,10 @@ 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.
> + * Inode verifiers do not 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)) {
>
--
Carlos
