On Tue, Mar 26, 2024 at 06:51:00PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@xxxxxxxxxx>
>
> Only allow the addition of new log incompat features to the primary
> superblock if the sysadmin provides explicit consent via a mount option
> or if the process has administrative privileges. This should prevent
> surprises when trying to recover dirty logs on old kernels.
>
> Signed-off-by: Darrick J. Wong <djwong@xxxxxxxxxx>
> Reviewed-by: Christoph Hellwig <hch@xxxxxx>
As I said originally when this was proposed, the logic needs to
default to allow log incompat features to be added rather than
disallow.
Essentially, having the default as "not allowed" means in future
every single XFS mount on every single system is going to have to
add this mount option to allow new log format features to used.
This "default = disallow" means our regression test systems will not
be exercising features based on this code without explicitly
expanding every independent test configuration matrix by another
dimension. This essentially means there will be almost no test
coverage for these dynamic features..
So, yeah, I think this needs to default to "allow", not "disallow".
> ---
> Documentation/admin-guide/xfs.rst | 7 +++++++
> fs/xfs/xfs_mount.c | 26 ++++++++++++++++++++++++++
> fs/xfs/xfs_mount.h | 3 +++
> fs/xfs/xfs_super.c | 12 +++++++++++-
> 4 files changed, 47 insertions(+), 1 deletion(-)
>
>
> diff --git a/Documentation/admin-guide/xfs.rst b/Documentation/admin-guide/xfs.rst
> index b67772cf36d6d..52acd95b2b754 100644
> --- a/Documentation/admin-guide/xfs.rst
> +++ b/Documentation/admin-guide/xfs.rst
> @@ -21,6 +21,13 @@ Mount Options
>
> When mounting an XFS filesystem, the following options are accepted.
>
> + add_log_feat/noadd_log_feat
> + Permit unprivileged userspace to use functionality that requires
> + the addition of log incompat feature bits to the superblock.
> + The feature bits will be cleared during a clean unmount.
> + Old kernels cannot recover dirty logs if they do not recognize
> + all log incompat feature bits.
> +
> allocsize=size
> Sets the buffered I/O end-of-file preallocation size when
> doing delayed allocation writeout (default size is 64KiB).
> diff --git a/fs/xfs/xfs_mount.c b/fs/xfs/xfs_mount.c
> index d37ba10f5fa33..a0b271758f910 100644
> --- a/fs/xfs/xfs_mount.c
> +++ b/fs/xfs/xfs_mount.c
> @@ -1281,6 +1281,27 @@ xfs_force_summary_recalc(
> xfs_fs_mark_sick(mp, XFS_SICK_FS_COUNTERS);
> }
>
> +/*
> + * Allow the log feature upgrade only if the sysadmin permits it via mount
> + * option; or the caller is the administrator. If the @want_audit parameter
> + * is true, then a denial due to insufficient privileges will be logged.
> + */
> +bool
> +xfs_can_add_incompat_log_features(
> + struct xfs_mount *mp,
> + bool want_audit)
> +{
> + /* Always allowed if the mount option is set */
> + if (mp->m_features & XFS_FEAT_ADD_LOG_FEAT)
> + return true;
Please define a __XFS_HAS_FEAT() macro for this feature bit and
use xfs_has_log_features_enabled() wrapper for it.
-Dave.
--
Dave Chinner
david@xxxxxxxxxxxxx
