Re: [PATCH] xfs: read only mounts with fsopen mount API are busted

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




On 16/1/24 12:33, Dave Chinner wrote:
From: Dave Chinner <dchinner@xxxxxxxxxx>

Recently xfs/513 started failing on my test machines testing "-o
ro,norecovery" mount options. This was being emitted in dmesg:

[ 9906.932724] XFS (pmem0): no-recovery mounts must be read-only.

Turns out, readonly mounts with the fsopen()/fsconfig() mount API
have been busted since day zero. It's only taken 5 years for debian
unstable to start using this "new" mount API, and shortly after this
I noticed xfs/513 had started to fail as per above.

The syscall trace is:

fsopen("xfs", FSOPEN_CLOEXEC)           = 3
mount_setattr(-1, NULL, 0, NULL, 0)     = -1 EINVAL (Invalid argument)
.....
fsconfig(3, FSCONFIG_SET_STRING, "source", "/dev/pmem0", 0) = 0
fsconfig(3, FSCONFIG_SET_FLAG, "ro", NULL, 0) = 0
fsconfig(3, FSCONFIG_SET_FLAG, "norecovery", NULL, 0) = 0
fsconfig(3, FSCONFIG_CMD_CREATE, NULL, NULL, 0) = -1 EINVAL (Invalid argument)
close(3)                                = 0

Showing that the actual mount instantiation (FSCONFIG_CMD_CREATE) is
what threw out the error.

During mount instantiation, we call xfs_fs_validate_params() which
does:

         /* No recovery flag requires a read-only mount */
         if (xfs_has_norecovery(mp) && !xfs_is_readonly(mp)) {
                 xfs_warn(mp, "no-recovery mounts must be read-only.");
                 return -EINVAL;
         }

and xfs_is_readonly() checks internal mount flags for read only
state. This state is set in xfs_init_fs_context() from the
context superblock flag state:

         /*
          * Copy binary VFS mount flags we are interested in.
          */
         if (fc->sb_flags & SB_RDONLY)
                 set_bit(XFS_OPSTATE_READONLY, &mp->m_opstate);

With the old mount API, all of the VFS specific superblock flags
had already been parsed and set before xfs_init_fs_context() is
called, so this all works fine.

However, in the brave new fsopen/fsconfig world,
xfs_init_fs_context() is called from fsopen() context, before any
VFS superblock have been set or parsed. Hence if we use fsopen(),
the internal XFS readonly state is *never set*. Hence anything that
depends on xfs_is_readonly() actually returning true for read only
mounts is broken if fsopen() has been used to mount the filesystem.

Fix this by moving this internal state initialisation to
xfs_fs_fill_super() before we attempt to validate the parameters
that have been set prior to the FSCONFIG_CMD_CREATE call being made.

Wow, good catch, and equally good analysis, Dave.


Ian


Signed-off-by: Dave Chinner <dchinner@xxxxxxxxxx>
Fixes: 73e5fff98b64 ("xfs: switch to use the new mount-api")
cc: stable@xxxxxxxxxxxxxxx
---
  fs/xfs/xfs_super.c | 27 +++++++++++++++++----------
  1 file changed, 17 insertions(+), 10 deletions(-)

diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
index 96cb00e94551..0506632b5cf2 100644
--- a/fs/xfs/xfs_super.c
+++ b/fs/xfs/xfs_super.c
@@ -1508,6 +1508,18 @@ xfs_fs_fill_super(
mp->m_super = sb; + /*
+	 * Copy VFS mount flags from the context now that all parameter parsing
+	 * is guaranteed to have been completed by either the old mount API or
+	 * the newer fsopen/fsconfig API.
+	 */
+	if (fc->sb_flags & SB_RDONLY)
+		set_bit(XFS_OPSTATE_READONLY, &mp->m_opstate);
+	if (fc->sb_flags & SB_DIRSYNC)
+		mp->m_features |= XFS_FEAT_DIRSYNC;
+	if (fc->sb_flags & SB_SYNCHRONOUS)
+		mp->m_features |= XFS_FEAT_WSYNC;
+
  	error = xfs_fs_validate_params(mp);
  	if (error)
  		return error;
@@ -1977,6 +1989,11 @@ static const struct fs_context_operations xfs_context_ops = {
  	.free        = xfs_fs_free,
  };
+/*
+ * WARNING: do not initialise any parameters in this function that depend on
+ * mount option parsing having already been performed as this can be called from
+ * fsopen() before any parameters have been set.
+ */
  static int xfs_init_fs_context(
  	struct fs_context	*fc)
  {
@@ -2008,16 +2025,6 @@ static int xfs_init_fs_context(
  	mp->m_logbsize = -1;
  	mp->m_allocsize_log = 16; /* 64k */
- /*
-	 * Copy binary VFS mount flags we are interested in.
-	 */
-	if (fc->sb_flags & SB_RDONLY)
-		set_bit(XFS_OPSTATE_READONLY, &mp->m_opstate);
-	if (fc->sb_flags & SB_DIRSYNC)
-		mp->m_features |= XFS_FEAT_DIRSYNC;
-	if (fc->sb_flags & SB_SYNCHRONOUS)
-		mp->m_features |= XFS_FEAT_WSYNC;
-
  	fc->s_fs_info = mp;
  	fc->ops = &xfs_context_ops;




[Index of Archives]     [XFS Filesystem Development (older mail)]     [Linux Filesystem Development]     [Linux Audio Users]     [Yosemite Trails]     [Linux Kernel]     [Linux RAID]     [Linux SCSI]


  Powered by Linux