On Mon, Apr 08, 2024 at 09:00:11AM +1000, Dave Chinner wrote:
> 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".
This is moot -- I changed exchangerange to use a permanent incompat
feature so that we can guarantee to users that if the xfs_info output
says it's enabled then it's 100% ready to go. Hence I no longer care
what happens to log incompat bits and this patch no longer needs to
exist.
--D
>
> > ---
> > 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
>
