* [PATCH] xfs: read only mounts with fsopen mount API are busted
@ 2023-08-23 22:02 Dave Chinner
2023-08-23 22:18 ` Darrick J. Wong
0 siblings, 1 reply; 6+ messages in thread
From: Dave Chinner @ 2023-08-23 22:02 UTC (permalink / raw)
To: linux-xfs
From: Dave Chinner <dchinner@redhat.com>
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.
Signed-off-by: Dave Chinner <dchinner@redhat.com>
---
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 09638e8fb4ee..8ca01510628b 100644
--- a/fs/xfs/xfs_super.c
+++ b/fs/xfs/xfs_super.c
@@ -1509,6 +1509,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)
goto out_free_names;
@@ -1988,6 +2000,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)
{
@@ -2019,16 +2036,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;
--
2.40.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] xfs: read only mounts with fsopen mount API are busted
2023-08-23 22:02 [PATCH] xfs: read only mounts with fsopen mount API are busted Dave Chinner
@ 2023-08-23 22:18 ` Darrick J. Wong
2023-08-23 22:46 ` Dave Chinner
0 siblings, 1 reply; 6+ messages in thread
From: Darrick J. Wong @ 2023-08-23 22:18 UTC (permalink / raw)
To: Dave Chinner; +Cc: linux-xfs
On Thu, Aug 24, 2023 at 08:02:25AM +1000, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
>
> 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.
>
> Signed-off-by: Dave Chinner <dchinner@redhat.com>
Huh. Wow. I would have expected to find /anything/ in fstests that
exercises the new mount api, but:
lax:~/cdev/work/fstests(0)> git grep -E '(fsconfig|fspick|fsopen)'
lax:~/cdev/work/fstests(1)>
What other weird things are lurking here?
Anyhow, I guess that's a side effect of xfs_mount mirroring some of the
vfs state flags, so....
Reviewed-by: Darrick J. Wong <djwong@kernel.org>
--D
> ---
> 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 09638e8fb4ee..8ca01510628b 100644
> --- a/fs/xfs/xfs_super.c
> +++ b/fs/xfs/xfs_super.c
> @@ -1509,6 +1509,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)
> goto out_free_names;
> @@ -1988,6 +2000,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)
> {
> @@ -2019,16 +2036,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;
>
> --
> 2.40.1
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] xfs: read only mounts with fsopen mount API are busted
2023-08-23 22:18 ` Darrick J. Wong
@ 2023-08-23 22:46 ` Dave Chinner
0 siblings, 0 replies; 6+ messages in thread
From: Dave Chinner @ 2023-08-23 22:46 UTC (permalink / raw)
To: Darrick J. Wong; +Cc: linux-xfs
On Wed, Aug 23, 2023 at 03:18:08PM -0700, Darrick J. Wong wrote:
> On Thu, Aug 24, 2023 at 08:02:25AM +1000, Dave Chinner wrote:
> > From: Dave Chinner <dchinner@redhat.com>
> >
> > 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.
> >
> > Signed-off-by: Dave Chinner <dchinner@redhat.com>
>
> Huh. Wow. I would have expected to find /anything/ in fstests that
> exercises the new mount api, but:
>
> lax:~/cdev/work/fstests(0)> git grep -E '(fsconfig|fspick|fsopen)'
> lax:~/cdev/work/fstests(1)>
>
> What other weird things are lurking here?
For everyone not on #xfs on oftc, here's a summary of the last day
or so since I discovered the above problem.
New xfs/270 failures with the fsconfig/fsopen mount API tell us that
unknown RO-compat bit mounts have also been completely broken since
~2018.
xfs/270 has been shutting down with superblock corruption after
mounting with an unknown RO-compat bit since 2018, but the attempt
to remount rw detects the unknown RO-compat bit before the code
checks for a shutdown situation, and hence the test passes even on a
shut down filesystem. Hence the test passes, and nobody has noticed
that the RO feature compat functionality is completely broken....
Which then lead us to the fact we are using RO-compat bits for
reflink and rmapbt, which introduce new log items and so actually
have log incompat feature bit requirements. RO-compat bits
don't cover recovery of unknown log features, just prevent new
modifications of the filesystem post-mount. So allowing log recovery
when RO-compat feature bits are set is also broken because we
screwed up reflink/rmapbt feature bit definitions, and that's
effective zero-day bugs for those features.
And, well, we modelled the XFS RO-compat functionality on the ext4
feature bit behaviour, so.....
> Anyhow, I guess that's a side effect of xfs_mount mirroring some of the
> vfs state flags, so....
Yup. But I don't see a way to easily avoid that...
> Reviewed-by: Darrick J. Wong <djwong@kernel.org>
Thanks!
-Dave.
--
Dave Chinner
david@fromorbit.com
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] xfs: read only mounts with fsopen mount API are busted
2024-01-16 4:33 Dave Chinner
2024-01-16 5:21 ` Christoph Hellwig
@ 2024-01-17 2:33 ` Ian Kent
1 sibling, 0 replies; 6+ messages in thread
From: Ian Kent @ 2024-01-17 2:33 UTC (permalink / raw)
To: Dave Chinner, linux-xfs; +Cc: chandan.babu
On 16/1/24 12:33, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
>
> 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@redhat.com>
> Fixes: 73e5fff98b64 ("xfs: switch to use the new mount-api")
> cc: stable@vger.kernel.org
> ---
> 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;
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] xfs: read only mounts with fsopen mount API are busted
2024-01-16 4:33 Dave Chinner
@ 2024-01-16 5:21 ` Christoph Hellwig
2024-01-17 2:33 ` Ian Kent
1 sibling, 0 replies; 6+ messages in thread
From: Christoph Hellwig @ 2024-01-16 5:21 UTC (permalink / raw)
To: Dave Chinner; +Cc: linux-xfs, chandan.babu
Looks good:
Reviewed-by: Christoph Hellwig <hch@lst.de>
>
> +/*
> + * 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(
... while you're at it, can you switch this to the normal XFS format:
static int
xfs_init_fs_context(
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH] xfs: read only mounts with fsopen mount API are busted
@ 2024-01-16 4:33 Dave Chinner
2024-01-16 5:21 ` Christoph Hellwig
2024-01-17 2:33 ` Ian Kent
0 siblings, 2 replies; 6+ messages in thread
From: Dave Chinner @ 2024-01-16 4:33 UTC (permalink / raw)
To: linux-xfs; +Cc: chandan.babu
From: Dave Chinner <dchinner@redhat.com>
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.
Signed-off-by: Dave Chinner <dchinner@redhat.com>
Fixes: 73e5fff98b64 ("xfs: switch to use the new mount-api")
cc: stable@vger.kernel.org
---
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;
--
2.43.0
^ permalink raw reply related [flat|nested] 6+ messages in thread
end of thread, other threads:[~2024-01-17 2:33 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-08-23 22:02 [PATCH] xfs: read only mounts with fsopen mount API are busted Dave Chinner
2023-08-23 22:18 ` Darrick J. Wong
2023-08-23 22:46 ` Dave Chinner
2024-01-16 4:33 Dave Chinner
2024-01-16 5:21 ` Christoph Hellwig
2024-01-17 2:33 ` Ian Kent
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.