fs/debugfs/inode.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-)
Mount options (uid, gid, mode) are silently ignored when debugfs is
mounted. This is a regression introduced during the conversion to the
new mount API.
When the mount API conversion was done, the line that sets
sb->s_fs_info to the parsed options was removed. This causes
debugfs_apply_options() to operate on a NULL pointer.
As an example, with the bug the "mode" mount option is ignored:
$ mount -o mode=0666 -t debugfs debugfs /tmp/debugfs_test
$ mount | grep debugfs_test
debugfs on /tmp/debugfs_test type debugfs (rw,relatime)
$ ls -ld /tmp/debugfs_test
drwx------ 25 root root 0 Aug 4 14:16 /tmp/debugfs_test
With the fix applied, it works as expected:
$ mount -o mode=0666 -t debugfs debugfs /tmp/debugfs_test
$ mount | grep debugfs_test
debugfs on /tmp/debugfs_test type debugfs (rw,relatime,mode=666)
$ ls -ld /tmp/debugfs_test
drw-rw-rw- 37 root root 0 Aug 2 17:28 /tmp/debugfs_test
Fix this by restoring the missing sb->s_fs_info assignment in
debugfs_fill_super() and by calling debugfs_reconfigure() in
debugfs_get_tree() to apply options when reusing an existing
superblock.
Fixes: a20971c18752 ("vfs: Convert debugfs to use the new mount API")
Closes: https://bugzilla.kernel.org/show_bug.cgi?id=220406
Signed-off-by: Charalampos Mitrodimas <charmitro@posteo.net>
---
fs/debugfs/inode.c | 8 +++++++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/fs/debugfs/inode.c b/fs/debugfs/inode.c
index a0357b0cf362d8ac47ff810e162402d6a8ae2cb9..ffe6402c77126b2a23beaa85160dfe578f59599c 100644
--- a/fs/debugfs/inode.c
+++ b/fs/debugfs/inode.c
@@ -275,6 +275,7 @@ static int debugfs_fill_super(struct super_block *sb, struct fs_context *fc)
set_default_d_op(sb, &debugfs_dops);
sb->s_d_flags |= DCACHE_DONTCACHE;
+ sb->s_fs_info = fc->s_fs_info;
debugfs_apply_options(sb);
return 0;
@@ -282,10 +283,15 @@ static int debugfs_fill_super(struct super_block *sb, struct fs_context *fc)
static int debugfs_get_tree(struct fs_context *fc)
{
+ int err;
+
if (!(debugfs_allow & DEBUGFS_ALLOW_API))
return -EPERM;
- return get_tree_single(fc, debugfs_fill_super);
+ err = get_tree_single(fc, debugfs_fill_super);
+ if (!err)
+ err = debugfs_reconfigure(fc);
+ return err;
}
static void debugfs_free_fc(struct fs_context *fc)
---
base-commit: 3c4a063b1f8ab71352df1421d9668521acb63cd9
change-id: 20250804-debugfs-mount-opts-2a68d7741f05
Best regards,
--
Charalampos Mitrodimas <charmitro@posteo.net>On Mon, Aug 04, 2025 at 02:30:04PM +0000, Charalampos Mitrodimas wrote:
> Mount options (uid, gid, mode) are silently ignored when debugfs is
> mounted. This is a regression introduced during the conversion to the
> new mount API.
>
> When the mount API conversion was done, the line that sets
> sb->s_fs_info to the parsed options was removed. This causes
> debugfs_apply_options() to operate on a NULL pointer.
>
> As an example, with the bug the "mode" mount option is ignored:
>
> $ mount -o mode=0666 -t debugfs debugfs /tmp/debugfs_test
> $ mount | grep debugfs_test
> debugfs on /tmp/debugfs_test type debugfs (rw,relatime)
> $ ls -ld /tmp/debugfs_test
> drwx------ 25 root root 0 Aug 4 14:16 /tmp/debugfs_test
>
> With the fix applied, it works as expected:
>
> $ mount -o mode=0666 -t debugfs debugfs /tmp/debugfs_test
> $ mount | grep debugfs_test
> debugfs on /tmp/debugfs_test type debugfs (rw,relatime,mode=666)
> $ ls -ld /tmp/debugfs_test
> drw-rw-rw- 37 root root 0 Aug 2 17:28 /tmp/debugfs_test
>
> Fix this by restoring the missing sb->s_fs_info assignment in
> debugfs_fill_super() and by calling debugfs_reconfigure() in
> debugfs_get_tree() to apply options when reusing an existing
> superblock.
>
> Fixes: a20971c18752 ("vfs: Convert debugfs to use the new mount API")
> Closes: https://bugzilla.kernel.org/show_bug.cgi?id=220406
> Signed-off-by: Charalampos Mitrodimas <charmitro@posteo.net>
> ---
> fs/debugfs/inode.c | 8 +++++++-
> 1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/fs/debugfs/inode.c b/fs/debugfs/inode.c
> index a0357b0cf362d8ac47ff810e162402d6a8ae2cb9..ffe6402c77126b2a23beaa85160dfe578f59599c 100644
> --- a/fs/debugfs/inode.c
> +++ b/fs/debugfs/inode.c
> @@ -275,6 +275,7 @@ static int debugfs_fill_super(struct super_block *sb, struct fs_context *fc)
> set_default_d_op(sb, &debugfs_dops);
> sb->s_d_flags |= DCACHE_DONTCACHE;
>
> + sb->s_fs_info = fc->s_fs_info;
> debugfs_apply_options(sb);
>
> return 0;
> @@ -282,10 +283,15 @@ static int debugfs_fill_super(struct super_block *sb, struct fs_context *fc)
>
> static int debugfs_get_tree(struct fs_context *fc)
> {
> + int err;
> +
> if (!(debugfs_allow & DEBUGFS_ALLOW_API))
> return -EPERM;
>
> - return get_tree_single(fc, debugfs_fill_super);
> + err = get_tree_single(fc, debugfs_fill_super);
> + if (!err)
> + err = debugfs_reconfigure(fc);
> + return err;
> }
>
> static void debugfs_free_fc(struct fs_context *fc)
>
> ---
> base-commit: 3c4a063b1f8ab71352df1421d9668521acb63cd9
> change-id: 20250804-debugfs-mount-opts-2a68d7741f05
>
> Best regards,
> --
> Charalampos Mitrodimas <charmitro@posteo.net>
>
Hi,
This is the friendly patch-bot of Greg Kroah-Hartman. You have sent him
a patch that has triggered this response. He used to manually respond
to these common problems, but in order to save his sanity (he kept
writing the same thing over and over, yet to different people), I was
created. Hopefully you will not take offence and will fix the problem
in your patch and resubmit it so that it can be accepted into the Linux
kernel tree.
You are receiving this message because of the following common error(s)
as indicated below:
- You have marked a patch with a "Fixes:" tag for a commit that is in an
older released kernel, yet you do not have a cc: stable line in the
signed-off-by area at all, which means that the patch will not be
applied to any older kernel releases. To properly fix this, please
follow the documented rules in the
Documentation/process/stable-kernel-rules.rst file for how to resolve
this.
If you wish to discuss this problem further, or you have questions about
how to resolve this issue, please feel free to respond to this email and
Greg will reply once he has dug out from the pending patches received
from other developers.
thanks,
greg k-h's patch email bot
On 8/4/25 9:30 AM, Charalampos Mitrodimas wrote: > Mount options (uid, gid, mode) are silently ignored when debugfs is > mounted. This is a regression introduced during the conversion to the > new mount API. > > When the mount API conversion was done, the line that sets > sb->s_fs_info to the parsed options was removed. This causes > debugfs_apply_options() to operate on a NULL pointer. > > As an example, with the bug the "mode" mount option is ignored: > > $ mount -o mode=0666 -t debugfs debugfs /tmp/debugfs_test > $ mount | grep debugfs_test > debugfs on /tmp/debugfs_test type debugfs (rw,relatime) > $ ls -ld /tmp/debugfs_test > drwx------ 25 root root 0 Aug 4 14:16 /tmp/debugfs_test Argh. So, this looks a lot like the issue that got fixed for tracefs in: e4d32142d1de tracing: Fix tracefs mount options Let me look at this; tracefs & debugfs are quite similar, so perhaps keeping the fix consistent would make sense as well but I'll dig into it a bit more. Thanks, -Eric
On 8/4/25 12:22 PM, Eric Sandeen wrote: > On 8/4/25 9:30 AM, Charalampos Mitrodimas wrote: >> Mount options (uid, gid, mode) are silently ignored when debugfs is >> mounted. This is a regression introduced during the conversion to the >> new mount API. >> >> When the mount API conversion was done, the line that sets >> sb->s_fs_info to the parsed options was removed. This causes >> debugfs_apply_options() to operate on a NULL pointer. >> >> As an example, with the bug the "mode" mount option is ignored: >> >> $ mount -o mode=0666 -t debugfs debugfs /tmp/debugfs_test >> $ mount | grep debugfs_test >> debugfs on /tmp/debugfs_test type debugfs (rw,relatime) >> $ ls -ld /tmp/debugfs_test >> drwx------ 25 root root 0 Aug 4 14:16 /tmp/debugfs_test > > Argh. So, this looks a lot like the issue that got fixed for tracefs in: > > e4d32142d1de tracing: Fix tracefs mount options > > Let me look at this; tracefs & debugfs are quite similar, so perhaps > keeping the fix consistent would make sense as well but I'll dig > into it a bit more. So, yes - a fix following the pattern of e4d32142d1de does seem to resolve this issue. However, I think we might be playing whack-a-mole here (fixing one fs at a time, when the problem is systemic) among filesystems that use get_tree_single() and have configurable options. For example, pstore: # umount /sys/fs/pstore # mount -t pstore -o kmsg_bytes=65536 none /sys/fs/pstore # mount | grep pstore none on /sys/fs/pstore type pstore (rw,relatime,seclabel) # mount -o remount,kmsg_bytes=65536 /sys/fs/pstore # mount | grep pstore none on /sys/fs/pstore type pstore (rw,relatime,seclabel,kmsg_bytes=65536) # I think gadgetfs most likely has the same problem but I'm not yet sure how to test that. I have no real objection to merging your patch, though I like the consistency of following e4d32142d1de a bit more. But I think we should find a graceful solution so that any filesystem using get_tree_single can avoid this pitfall, if possible. -Eric
Eric Sandeen <sandeen@redhat.com> writes: > On 8/4/25 12:22 PM, Eric Sandeen wrote: >> On 8/4/25 9:30 AM, Charalampos Mitrodimas wrote: >>> Mount options (uid, gid, mode) are silently ignored when debugfs is >>> mounted. This is a regression introduced during the conversion to the >>> new mount API. >>> >>> When the mount API conversion was done, the line that sets >>> sb->s_fs_info to the parsed options was removed. This causes >>> debugfs_apply_options() to operate on a NULL pointer. >>> >>> As an example, with the bug the "mode" mount option is ignored: >>> >>> $ mount -o mode=0666 -t debugfs debugfs /tmp/debugfs_test >>> $ mount | grep debugfs_test >>> debugfs on /tmp/debugfs_test type debugfs (rw,relatime) >>> $ ls -ld /tmp/debugfs_test >>> drwx------ 25 root root 0 Aug 4 14:16 /tmp/debugfs_test >> >> Argh. So, this looks a lot like the issue that got fixed for tracefs in: >> >> e4d32142d1de tracing: Fix tracefs mount options >> >> Let me look at this; tracefs & debugfs are quite similar, so perhaps >> keeping the fix consistent would make sense as well but I'll dig >> into it a bit more. > > So, yes - a fix following the pattern of e4d32142d1de does seem to resolve > this issue. > > However, I think we might be playing whack-a-mole here (fixing one fs at a time, > when the problem is systemic) among filesystems that use get_tree_single() > and have configurable options. For example, pstore: > > # umount /sys/fs/pstore > > # mount -t pstore -o kmsg_bytes=65536 none /sys/fs/pstore > # mount | grep pstore > none on /sys/fs/pstore type pstore (rw,relatime,seclabel) > > # mount -o remount,kmsg_bytes=65536 /sys/fs/pstore > # mount | grep pstore > none on /sys/fs/pstore type pstore (rw,relatime,seclabel,kmsg_bytes=65536) > # > > I think gadgetfs most likely has the same problem but I'm not yet sure > how to test that. > > I have no real objection to merging your patch, though I like the > consistency of following e4d32142d1de a bit more. But I think we should > find a graceful solution so that any filesystem using get_tree_single > can avoid this pitfall, if possible. Hi, thanks for the review, and yes you're right. Maybe a potential systemic fix would be to make get_tree_single() always call fc->ops->reconfigure() after vfs_get_super() when reusing an existing superblock, fixing all affected filesystems at once. > > -Eric
On 8/5/25 12:22 PM, Charalampos Mitrodimas wrote: > Eric Sandeen <sandeen@redhat.com> writes: > >> On 8/4/25 12:22 PM, Eric Sandeen wrote: >>> On 8/4/25 9:30 AM, Charalampos Mitrodimas wrote: >>>> Mount options (uid, gid, mode) are silently ignored when debugfs is >>>> mounted. This is a regression introduced during the conversion to the >>>> new mount API. >>>> >>>> When the mount API conversion was done, the line that sets >>>> sb->s_fs_info to the parsed options was removed. This causes >>>> debugfs_apply_options() to operate on a NULL pointer. >>>> >>>> As an example, with the bug the "mode" mount option is ignored: >>>> >>>> $ mount -o mode=0666 -t debugfs debugfs /tmp/debugfs_test >>>> $ mount | grep debugfs_test >>>> debugfs on /tmp/debugfs_test type debugfs (rw,relatime) >>>> $ ls -ld /tmp/debugfs_test >>>> drwx------ 25 root root 0 Aug 4 14:16 /tmp/debugfs_test >>> >>> Argh. So, this looks a lot like the issue that got fixed for tracefs in: >>> >>> e4d32142d1de tracing: Fix tracefs mount options >>> >>> Let me look at this; tracefs & debugfs are quite similar, so perhaps >>> keeping the fix consistent would make sense as well but I'll dig >>> into it a bit more. >> >> So, yes - a fix following the pattern of e4d32142d1de does seem to resolve >> this issue. >> >> However, I think we might be playing whack-a-mole here (fixing one fs at a time, >> when the problem is systemic) among filesystems that use get_tree_single() >> and have configurable options. For example, pstore: >> >> # umount /sys/fs/pstore >> >> # mount -t pstore -o kmsg_bytes=65536 none /sys/fs/pstore >> # mount | grep pstore >> none on /sys/fs/pstore type pstore (rw,relatime,seclabel) >> >> # mount -o remount,kmsg_bytes=65536 /sys/fs/pstore >> # mount | grep pstore >> none on /sys/fs/pstore type pstore (rw,relatime,seclabel,kmsg_bytes=65536) >> # >> >> I think gadgetfs most likely has the same problem but I'm not yet sure >> how to test that. >> >> I have no real objection to merging your patch, though I like the >> consistency of following e4d32142d1de a bit more. But I think we should >> find a graceful solution so that any filesystem using get_tree_single >> can avoid this pitfall, if possible. > > Hi, thanks for the review, and yes you're right. > > Maybe a potential systemic fix would be to make get_tree_single() always > call fc->ops->reconfigure() after vfs_get_super() when reusing an > existing superblock, fixing all affected filesystems at once. Yep, I'm looking into that. mount_single used to do this, and IIRC we discussed it before but for some reason opted not to. It seems a bit trickier than I first expected, but I might just be dense. ;) -Eric
On Wed, Aug 06, 2025 at 11:33:11AM -0500, Eric Sandeen wrote: > On 8/5/25 12:22 PM, Charalampos Mitrodimas wrote: > > Eric Sandeen <sandeen@redhat.com> writes: > > > >> On 8/4/25 12:22 PM, Eric Sandeen wrote: > >>> On 8/4/25 9:30 AM, Charalampos Mitrodimas wrote: > >>>> Mount options (uid, gid, mode) are silently ignored when debugfs is > >>>> mounted. This is a regression introduced during the conversion to the > >>>> new mount API. > >>>> > >>>> When the mount API conversion was done, the line that sets > >>>> sb->s_fs_info to the parsed options was removed. This causes > >>>> debugfs_apply_options() to operate on a NULL pointer. > >>>> > >>>> As an example, with the bug the "mode" mount option is ignored: > >>>> > >>>> $ mount -o mode=0666 -t debugfs debugfs /tmp/debugfs_test > >>>> $ mount | grep debugfs_test > >>>> debugfs on /tmp/debugfs_test type debugfs (rw,relatime) > >>>> $ ls -ld /tmp/debugfs_test > >>>> drwx------ 25 root root 0 Aug 4 14:16 /tmp/debugfs_test > >>> > >>> Argh. So, this looks a lot like the issue that got fixed for tracefs in: > >>> > >>> e4d32142d1de tracing: Fix tracefs mount options > >>> > >>> Let me look at this; tracefs & debugfs are quite similar, so perhaps > >>> keeping the fix consistent would make sense as well but I'll dig > >>> into it a bit more. > >> > >> So, yes - a fix following the pattern of e4d32142d1de does seem to resolve > >> this issue. > >> > >> However, I think we might be playing whack-a-mole here (fixing one fs at a time, > >> when the problem is systemic) among filesystems that use get_tree_single() > >> and have configurable options. For example, pstore: > >> > >> # umount /sys/fs/pstore > >> > >> # mount -t pstore -o kmsg_bytes=65536 none /sys/fs/pstore > >> # mount | grep pstore > >> none on /sys/fs/pstore type pstore (rw,relatime,seclabel) > >> > >> # mount -o remount,kmsg_bytes=65536 /sys/fs/pstore > >> # mount | grep pstore > >> none on /sys/fs/pstore type pstore (rw,relatime,seclabel,kmsg_bytes=65536) > >> # > >> > >> I think gadgetfs most likely has the same problem but I'm not yet sure > >> how to test that. > >> > >> I have no real objection to merging your patch, though I like the > >> consistency of following e4d32142d1de a bit more. But I think we should > >> find a graceful solution so that any filesystem using get_tree_single > >> can avoid this pitfall, if possible. > > > > Hi, thanks for the review, and yes you're right. > > > > Maybe a potential systemic fix would be to make get_tree_single() always > > call fc->ops->reconfigure() after vfs_get_super() when reusing an > > existing superblock, fixing all affected filesystems at once. > > Yep, I'm looking into that. mount_single used to do this, and IIRC we discussed > it before but for some reason opted not to. It seems a bit trickier than I first > expected, but I might just be dense. ;) If we can make it work generically, we should. I too don't remember what the reasons were for not doing it that way.
On 8/8/25 9:13 AM, Christian Brauner wrote: > On Wed, Aug 06, 2025 at 11:33:11AM -0500, Eric Sandeen wrote: >> On 8/5/25 12:22 PM, Charalampos Mitrodimas wrote: ... >>> Hi, thanks for the review, and yes you're right. >>> >>> Maybe a potential systemic fix would be to make get_tree_single() always >>> call fc->ops->reconfigure() after vfs_get_super() when reusing an >>> existing superblock, fixing all affected filesystems at once. >> >> Yep, I'm looking into that. mount_single used to do this, and IIRC we discussed >> it before but for some reason opted not to. It seems a bit trickier than I first >> expected, but I might just be dense. ;) > > If we can make it work generically, we should. I too don't remember what > the reasons were for not doing it that way. Sorry for the long delay here. Talked to dhowells about this and his POV (which is convincing, I think) is that even though mount_single used to call do_remount_sb for an extant single sb, this was probably Bad(tm). Bad, IIUC, because it's not a given that options are safe to be changed in this way, and that policy really should be up to each individual filesystem. So while we still need to audit and fix any get_tree_single() filesystems that changed behavior with the new mount api, may as well fix up debugfs for now since the bug was reported. Charalampos - Your patch oopses on boot for me - I think that when you added sb->s_fs_info = fc->s_fs_info; in debugfs_fill_super, you're actually NULLing out the one in the sb, because sget_fc has already transferred fc->s_fs_info to sb->s_fs_info, and NULLed fc->s_fs_info prior to this. Then when we get to _debugfs_apply_options, *fsi = sb->s_fs_info; is also NULL so using it there oopses. If you want to send a V2 with fixed up stable cc: I'd suggest following the pattern of what was done for tracefs in e4d32142d1de, which I think works OK and would at least lend some consistency, as the code is similar. If not, let me know and I'll work on an update. Thanks, -Eric
Eric Sandeen <sandeen@sandeen.net> writes: > On 8/8/25 9:13 AM, Christian Brauner wrote: >> On Wed, Aug 06, 2025 at 11:33:11AM -0500, Eric Sandeen wrote: >>> On 8/5/25 12:22 PM, Charalampos Mitrodimas wrote: > > ... > >>>> Hi, thanks for the review, and yes you're right. >>>> >>>> Maybe a potential systemic fix would be to make get_tree_single() always >>>> call fc->ops->reconfigure() after vfs_get_super() when reusing an >>>> existing superblock, fixing all affected filesystems at once. >>> >>> Yep, I'm looking into that. mount_single used to do this, and IIRC we discussed >>> it before but for some reason opted not to. It seems a bit trickier than I first >>> expected, but I might just be dense. ;) >> >> If we can make it work generically, we should. I too don't remember what >> the reasons were for not doing it that way. > > Sorry for the long delay here. Talked to dhowells about this and his > POV (which is convincing, I think) is that even though mount_single used to > call do_remount_sb for an extant single sb, this was probably Bad(tm). > Bad, IIUC, because it's not a given that options are safe to be changed > in this way, and that policy really should be up to each individual > filesystem. > > So while we still need to audit and fix any get_tree_single() > filesystems that changed behavior with the new mount api, may as well > fix up debugfs for now since the bug was reported. What if we add a new flag (.fs_flags), say FS_SINGLE_RECONF, to file_system_type that makes get_tree_single() automatically call reconfigure() when reusing an existing superblock? Filesystems could then just opt-in by adding it to .fs_flags. > > Charalampos - > > Your patch oopses on boot for me - I think that when you added > > sb->s_fs_info = fc->s_fs_info; Yes, did take notice of this yesterday when I revisited it. > > in debugfs_fill_super, you're actually NULLing out the one in the sb, > because sget_fc has already transferred fc->s_fs_info to sb->s_fs_info, > and NULLed fc->s_fs_info prior to this. Then when we get to > _debugfs_apply_options, *fsi = sb->s_fs_info; is also NULL so using it > there oopses. > > If you want to send a V2 with fixed up stable cc: I'd suggest following the > pattern of what was done for tracefs in e4d32142d1de, which I think works > OK and would at least lend some consistency, as the code is similar. > > If not, let me know and I'll work on an update. As a matter of fact, I have a v2 exactly like this ready to sent. Doing so in a bit. > > Thanks, > -Eric Thanks! C. Mitrodimas
On 2025-08-05, Eric Sandeen <sandeen@redhat.com> wrote:
> On 8/4/25 12:22 PM, Eric Sandeen wrote:
> > On 8/4/25 9:30 AM, Charalampos Mitrodimas wrote:
> >> Mount options (uid, gid, mode) are silently ignored when debugfs is
> >> mounted. This is a regression introduced during the conversion to the
> >> new mount API.
> >>
> >> When the mount API conversion was done, the line that sets
> >> sb->s_fs_info to the parsed options was removed. This causes
> >> debugfs_apply_options() to operate on a NULL pointer.
> >>
> >> As an example, with the bug the "mode" mount option is ignored:
> >>
> >> $ mount -o mode=0666 -t debugfs debugfs /tmp/debugfs_test
> >> $ mount | grep debugfs_test
> >> debugfs on /tmp/debugfs_test type debugfs (rw,relatime)
> >> $ ls -ld /tmp/debugfs_test
> >> drwx------ 25 root root 0 Aug 4 14:16 /tmp/debugfs_test
> >
> > Argh. So, this looks a lot like the issue that got fixed for tracefs in:
> >
> > e4d32142d1de tracing: Fix tracefs mount options
> >
> > Let me look at this; tracefs & debugfs are quite similar, so perhaps
> > keeping the fix consistent would make sense as well but I'll dig
> > into it a bit more.
>
> So, yes - a fix following the pattern of e4d32142d1de does seem to resolve
> this issue.
>
> However, I think we might be playing whack-a-mole here (fixing one fs at a time,
> when the problem is systemic) among filesystems that use get_tree_single()
> and have configurable options. For example, pstore:
>
> # umount /sys/fs/pstore
>
> # mount -t pstore -o kmsg_bytes=65536 none /sys/fs/pstore
> # mount | grep pstore
> none on /sys/fs/pstore type pstore (rw,relatime,seclabel)
>
> # mount -o remount,kmsg_bytes=65536 /sys/fs/pstore
> # mount | grep pstore
> none on /sys/fs/pstore type pstore (rw,relatime,seclabel,kmsg_bytes=65536)
> #
Isn't this just a standard consequence of the classic "ignore mount
flags if we are reusing a superblock" behaviour? Not doing this can lead
to us silently clearing security-related flags ("acl" is the common
example used) and was the main reason for FSCONFIG_CMD_CREATE_EXCL.
Maybe for some filesystems (like debugfs), it makes sense to permit a
mount operation to silently reconfigure existing mounts, but this should
be an opt-in knob per-filesystem.
Also, if we plan to do this then you almost certainly want to have
fs_context track which set of parameters were set and then only
reconfigure those parameters *which were set*. At the moment,
fs_context_for_reconfigure() works around this by having the current
sb_flags and other configuration be loaded via init_fs_context(), but if
you do an auto-reconfigure with an fs_context created for mounting then
you won't inherit _any_ of the old mount options. This could lead to a
situation like:
% mount -t pstore -o ro /sys/fs/pstore
% mount -t pstore -o kmsg_bytes=65536 /tmp
% # /sys/fs/pstore is now rw.
Which is really not ideal, as it would make it incredibly fragile for
anyone to try to mount these filesystems without breaking other mounts
on the system.
If fs_context tracked which parameters were configured and only applied
the set ones, at least you would avoid unintentionally unsetting
parameters of the original mount.
FWIW, cgroupv1 has a warning when this situation happens (see the
pr_warn() in cgroup1_root_to_use()). I always wondered why this wasn't
done on the VFS level, as a warning is probably enough to alert admins
about this behaviour without resorting to implicitly changing the mount
options of existing mounts.
> I think gadgetfs most likely has the same problem but I'm not yet sure
> how to test that.
>
> I have no real objection to merging your patch, though I like the
> consistency of following e4d32142d1de a bit more. But I think we should
> find a graceful solution so that any filesystem using get_tree_single
> can avoid this pitfall, if possible.
>
> -Eric
>
>
--
Aleksa Sarai
Senior Software Engineer (Containers)
SUSE Linux GmbH
https://www.cyphar.com/
On 8/14/25 4:05 AM, Aleksa Sarai wrote:
> On 2025-08-05, Eric Sandeen <sandeen@redhat.com> wrote:
>> On 8/4/25 12:22 PM, Eric Sandeen wrote:
>>> On 8/4/25 9:30 AM, Charalampos Mitrodimas wrote:
>>>> Mount options (uid, gid, mode) are silently ignored when debugfs is
>>>> mounted. This is a regression introduced during the conversion to the
>>>> new mount API.
>>>>
>>>> When the mount API conversion was done, the line that sets
>>>> sb->s_fs_info to the parsed options was removed. This causes
>>>> debugfs_apply_options() to operate on a NULL pointer.
>>>>
>>>> As an example, with the bug the "mode" mount option is ignored:
>>>>
>>>> $ mount -o mode=0666 -t debugfs debugfs /tmp/debugfs_test
>>>> $ mount | grep debugfs_test
>>>> debugfs on /tmp/debugfs_test type debugfs (rw,relatime)
>>>> $ ls -ld /tmp/debugfs_test
>>>> drwx------ 25 root root 0 Aug 4 14:16 /tmp/debugfs_test
>>>
>>> Argh. So, this looks a lot like the issue that got fixed for tracefs in:
>>>
>>> e4d32142d1de tracing: Fix tracefs mount options
>>>
>>> Let me look at this; tracefs & debugfs are quite similar, so perhaps
>>> keeping the fix consistent would make sense as well but I'll dig
>>> into it a bit more.
>>
>> So, yes - a fix following the pattern of e4d32142d1de does seem to resolve
>> this issue.
>>
>> However, I think we might be playing whack-a-mole here (fixing one fs at a time,
>> when the problem is systemic) among filesystems that use get_tree_single()
>> and have configurable options. For example, pstore:
>>
>> # umount /sys/fs/pstore
>>
>> # mount -t pstore -o kmsg_bytes=65536 none /sys/fs/pstore
>> # mount | grep pstore
>> none on /sys/fs/pstore type pstore (rw,relatime,seclabel)
>>
>> # mount -o remount,kmsg_bytes=65536 /sys/fs/pstore
>> # mount | grep pstore
>> none on /sys/fs/pstore type pstore (rw,relatime,seclabel,kmsg_bytes=65536)
>> #
>
> Isn't this just a standard consequence of the classic "ignore mount
> flags if we are reusing a superblock" behaviour? Not doing this can lead
> to us silently clearing security-related flags ("acl" is the common
> example used) and was the main reason for FSCONFIG_CMD_CREATE_EXCL.
Perhaps, but I think it is a change in behavior since before the mount
API change. On Centos Stream 8 (sorry, that was the handy VM I had around) ;)
<fresh boot>
# mount | grep pstore
pstore on /sys/fs/pstore type pstore (rw,nosuid,nodev,noexec,relatime,seclabel)
# umount /sys/fs/pstore
# mount -t pstore -o kmsg_bytes=65536 none /sys/fs/pstore
# mount | grep pstore
none on /sys/fs/pstore type pstore (rw,relatime,seclabel,kmsg_bytes=65536)
(kmsg_bytes was accepted on older kernel, vs not in prior example on new kernel)
# mount -o remount,kmsg_bytes=65536 /sys/fs/pstore
# mount | grep pstore
none on /sys/fs/pstore type pstore (rw,relatime,seclabel,kmsg_bytes=65536)
remount behaves as expected...
-Eric
> Maybe for some filesystems (like debugfs), it makes sense to permit a
> mount operation to silently reconfigure existing mounts, but this should
> be an opt-in knob per-filesystem.
>
> Also, if we plan to do this then you almost certainly want to have
> fs_context track which set of parameters were set and then only
> reconfigure those parameters *which were set*. At the moment,
> fs_context_for_reconfigure() works around this by having the current
> sb_flags and other configuration be loaded via init_fs_context(), but if
> you do an auto-reconfigure with an fs_context created for mounting then
> you won't inherit _any_ of the old mount options. This could lead to a
> situation like:
>
> % mount -t pstore -o ro /sys/fs/pstore
> % mount -t pstore -o kmsg_bytes=65536 /tmp
> % # /sys/fs/pstore is now rw.
>
> Which is really not ideal, as it would make it incredibly fragile for
> anyone to try to mount these filesystems without breaking other mounts
> on the system.
>
> If fs_context tracked which parameters were configured and only applied
> the set ones, at least you would avoid unintentionally unsetting
> parameters of the original mount.
>
> FWIW, cgroupv1 has a warning when this situation happens (see the
> pr_warn() in cgroup1_root_to_use()). I always wondered why this wasn't
> done on the VFS level, as a warning is probably enough to alert admins
> about this behaviour without resorting to implicitly changing the mount
> options of existing mounts.
>
>> I think gadgetfs most likely has the same problem but I'm not yet sure
>> how to test that.
>>
>> I have no real objection to merging your patch, though I like the
>> consistency of following e4d32142d1de a bit more. But I think we should
>> find a graceful solution so that any filesystem using get_tree_single
>> can avoid this pitfall, if possible.
>>
>> -Eric
>>
>>
>
On 2025-08-14, Eric Sandeen <sandeen@sandeen.net> wrote:
> On 8/14/25 4:05 AM, Aleksa Sarai wrote:
> > On 2025-08-05, Eric Sandeen <sandeen@redhat.com> wrote:
> >> On 8/4/25 12:22 PM, Eric Sandeen wrote:
> >>> On 8/4/25 9:30 AM, Charalampos Mitrodimas wrote:
> >>>> Mount options (uid, gid, mode) are silently ignored when debugfs is
> >>>> mounted. This is a regression introduced during the conversion to the
> >>>> new mount API.
> >>>>
> >>>> When the mount API conversion was done, the line that sets
> >>>> sb->s_fs_info to the parsed options was removed. This causes
> >>>> debugfs_apply_options() to operate on a NULL pointer.
> >>>>
> >>>> As an example, with the bug the "mode" mount option is ignored:
> >>>>
> >>>> $ mount -o mode=0666 -t debugfs debugfs /tmp/debugfs_test
> >>>> $ mount | grep debugfs_test
> >>>> debugfs on /tmp/debugfs_test type debugfs (rw,relatime)
> >>>> $ ls -ld /tmp/debugfs_test
> >>>> drwx------ 25 root root 0 Aug 4 14:16 /tmp/debugfs_test
> >>>
> >>> Argh. So, this looks a lot like the issue that got fixed for tracefs in:
> >>>
> >>> e4d32142d1de tracing: Fix tracefs mount options
> >>>
> >>> Let me look at this; tracefs & debugfs are quite similar, so perhaps
> >>> keeping the fix consistent would make sense as well but I'll dig
> >>> into it a bit more.
> >>
> >> So, yes - a fix following the pattern of e4d32142d1de does seem to resolve
> >> this issue.
> >>
> >> However, I think we might be playing whack-a-mole here (fixing one fs at a time,
> >> when the problem is systemic) among filesystems that use get_tree_single()
> >> and have configurable options. For example, pstore:
> >>
> >> # umount /sys/fs/pstore
> >>
> >> # mount -t pstore -o kmsg_bytes=65536 none /sys/fs/pstore
> >> # mount | grep pstore
> >> none on /sys/fs/pstore type pstore (rw,relatime,seclabel)
> >>
> >> # mount -o remount,kmsg_bytes=65536 /sys/fs/pstore
> >> # mount | grep pstore
> >> none on /sys/fs/pstore type pstore (rw,relatime,seclabel,kmsg_bytes=65536)
> >> #
> >
> > Isn't this just a standard consequence of the classic "ignore mount
> > flags if we are reusing a superblock" behaviour? Not doing this can lead
> > to us silently clearing security-related flags ("acl" is the common
> > example used) and was the main reason for FSCONFIG_CMD_CREATE_EXCL.
>
> Perhaps, but I think it is a change in behavior since before the mount
> API change. On Centos Stream 8 (sorry, that was the handy VM I had around) ;)
>
> <fresh boot>
>
> # mount | grep pstore
> pstore on /sys/fs/pstore type pstore (rw,nosuid,nodev,noexec,relatime,seclabel)
>
> # umount /sys/fs/pstore
> # mount -t pstore -o kmsg_bytes=65536 none /sys/fs/pstore
> # mount | grep pstore
> none on /sys/fs/pstore type pstore (rw,relatime,seclabel,kmsg_bytes=65536)
>
> (kmsg_bytes was accepted on older kernel, vs not in prior example on new kernel)
>
> # mount -o remount,kmsg_bytes=65536 /sys/fs/pstore
> # mount | grep pstore
> none on /sys/fs/pstore type pstore (rw,relatime,seclabel,kmsg_bytes=65536)
>
> remount behaves as expected...
Yes, you're quite right.
I was mostly thinking about the later discussion about making this
generic. Though I think even for filesystems that want this feature (or
have to have it because of back-compat) we should still emit some kind
of warning (or at least an informational message if it is opt-in?). This
superblock reuse behaviour is basically undocumented (though I am
including it in the new fsconfig(2) man pages, finally) and so it seems
prudent to actually provide this information to userspace.
For what it's worth, David Howells actually did implement a way to port
things from mount_single() and maintain the reconf-on-mount behaviour
back in commit 43ce4c1feadb ("vfs: Add a single-or-reconfig keying to
vfs_get_super()") and probably intended to port filesystems to it but
this never happened and it was removed in commit e062abaec65b ("super:
remove get_tree_single_reconf()"). Maybe he changed his mind? @David?
It's also seems a little fruity (from a userspace perspective) to me
that s_flags won't be reconfigured by this AFAICS -- this is made worse
by the fact that fsconfig(2) makes the configuration interface for
fc->sb_flags into the same one as fc->s_fs_info and so userspace doesn't
immediately see which flags are in which set. If we did make this a
generic opt-in per-filesystem maybe we would want to at least make that
bit consistent (but for bdev-backed supers we would need to keep the
current SB_RDONLY checks, obviously).
> -Eric
>
> > Maybe for some filesystems (like debugfs), it makes sense to permit a
> > mount operation to silently reconfigure existing mounts, but this should
> > be an opt-in knob per-filesystem.
> >
> > Also, if we plan to do this then you almost certainly want to have
> > fs_context track which set of parameters were set and then only
> > reconfigure those parameters *which were set*. At the moment,
> > fs_context_for_reconfigure() works around this by having the current
> > sb_flags and other configuration be loaded via init_fs_context(), but if
> > you do an auto-reconfigure with an fs_context created for mounting then
> > you won't inherit _any_ of the old mount options. This could lead to a
> > situation like:
> >
> > % mount -t pstore -o ro /sys/fs/pstore
> > % mount -t pstore -o kmsg_bytes=65536 /tmp
> > % # /sys/fs/pstore is now rw.
> >
> > Which is really not ideal, as it would make it incredibly fragile for
> > anyone to try to mount these filesystems without breaking other mounts
> > on the system.
> >
> > If fs_context tracked which parameters were configured and only applied
> > the set ones, at least you would avoid unintentionally unsetting
> > parameters of the original mount.
> >
> > FWIW, cgroupv1 has a warning when this situation happens (see the
> > pr_warn() in cgroup1_root_to_use()). I always wondered why this wasn't
> > done on the VFS level, as a warning is probably enough to alert admins
> > about this behaviour without resorting to implicitly changing the mount
> > options of existing mounts.
> >
> >> I think gadgetfs most likely has the same problem but I'm not yet sure
> >> how to test that.
> >>
> >> I have no real objection to merging your patch, though I like the
> >> consistency of following e4d32142d1de a bit more. But I think we should
> >> find a graceful solution so that any filesystem using get_tree_single
> >> can avoid this pitfall, if possible.
> >>
> >> -Eric
> >>
> >>
> >
>
--
Aleksa Sarai
Senior Software Engineer (Containers)
SUSE Linux GmbH
https://www.cyphar.com/
On 2025-08-14, Aleksa Sarai <cyphar@cyphar.com> wrote:
> On 2025-08-05, Eric Sandeen <sandeen@redhat.com> wrote:
> > On 8/4/25 12:22 PM, Eric Sandeen wrote:
> > > On 8/4/25 9:30 AM, Charalampos Mitrodimas wrote:
> > >> Mount options (uid, gid, mode) are silently ignored when debugfs is
> > >> mounted. This is a regression introduced during the conversion to the
> > >> new mount API.
> > >>
> > >> When the mount API conversion was done, the line that sets
> > >> sb->s_fs_info to the parsed options was removed. This causes
> > >> debugfs_apply_options() to operate on a NULL pointer.
> > >>
> > >> As an example, with the bug the "mode" mount option is ignored:
> > >>
> > >> $ mount -o mode=0666 -t debugfs debugfs /tmp/debugfs_test
> > >> $ mount | grep debugfs_test
> > >> debugfs on /tmp/debugfs_test type debugfs (rw,relatime)
> > >> $ ls -ld /tmp/debugfs_test
> > >> drwx------ 25 root root 0 Aug 4 14:16 /tmp/debugfs_test
> > >
> > > Argh. So, this looks a lot like the issue that got fixed for tracefs in:
> > >
> > > e4d32142d1de tracing: Fix tracefs mount options
> > >
> > > Let me look at this; tracefs & debugfs are quite similar, so perhaps
> > > keeping the fix consistent would make sense as well but I'll dig
> > > into it a bit more.
> >
> > So, yes - a fix following the pattern of e4d32142d1de does seem to resolve
> > this issue.
> >
> > However, I think we might be playing whack-a-mole here (fixing one fs at a time,
> > when the problem is systemic) among filesystems that use get_tree_single()
> > and have configurable options. For example, pstore:
> >
> > # umount /sys/fs/pstore
> >
> > # mount -t pstore -o kmsg_bytes=65536 none /sys/fs/pstore
> > # mount | grep pstore
> > none on /sys/fs/pstore type pstore (rw,relatime,seclabel)
> >
> > # mount -o remount,kmsg_bytes=65536 /sys/fs/pstore
> > # mount | grep pstore
> > none on /sys/fs/pstore type pstore (rw,relatime,seclabel,kmsg_bytes=65536)
> > #
>
> Isn't this just a standard consequence of the classic "ignore mount
> flags if we are reusing a superblock" behaviour? Not doing this can lead
> to us silently clearing security-related flags ("acl" is the common
> example used) and was the main reason for FSCONFIG_CMD_CREATE_EXCL.
>
> Maybe for some filesystems (like debugfs), it makes sense to permit a
> mount operation to silently reconfigure existing mounts, but this should
> be an opt-in knob per-filesystem.
>
> Also, if we plan to do this then you almost certainly want to have
> fs_context track which set of parameters were set and then only
> reconfigure those parameters *which were set*. At the moment,
> fs_context_for_reconfigure() works around this by having the current
> sb_flags and other configuration be loaded via init_fs_context(), but if
> you do an auto-reconfigure with an fs_context created for mounting then
> you won't inherit _any_ of the old mount options. This could lead to a
> situation like:
>
> % mount -t pstore -o ro /sys/fs/pstore
> % mount -t pstore -o kmsg_bytes=65536 /tmp
> % # /sys/fs/pstore is now rw.
>
> Which is really not ideal, as it would make it incredibly fragile for
> anyone to try to mount these filesystems without breaking other mounts
> on the system.
>
> If fs_context tracked which parameters were configured and only applied
> the set ones, at least you would avoid unintentionally unsetting
> parameters of the original mount.
My mistake, fs_context does this already with fc->sb_flags_mask. That
leaves each filesystem to handle this properly for fc->s_fs_info, and
the ones I've checked _do_ handle this properly -- false alarm! (I
missed this on the first pass-through.)
I guess then that this is more of a question of what users expect. I
agree with what David Howells was quoted as saying, which is that
silently doing this is really suboptimal. I still feel that logging a
warning is more preferable -- if the VFS can be told whether
fc->s_fs_info diverges from sb->s_fs_info, then we can log a warning (or
alternatively, it could be done by each filesystem and VFS does it for
the generic s_flags).
This is arguably more practical than FSCONFIG_CMD_CREATE_EXCL for most
users because it could give you feedback on what parameters were
problematic, and if there was no warning then you don't need to worry
about the mount sharing a superblock. FSCONFIG_CMD_CREATE_EXCL would
then only be needed for truly paranoid programs. AFAICS, mount(8) does
now forward warning messages from fclog, so this would mean admins would
be able to see the warning immediately from their mount(8) call. Older
mount(2)-based users would see it in dmesg.
I can take a look writing a patch for this?
> FWIW, cgroupv1 has a warning when this situation happens (see the
> pr_warn() in cgroup1_root_to_use()). I always wondered why this wasn't
> done on the VFS level, as a warning is probably enough to alert admins
> about this behaviour without resorting to implicitly changing the mount
> options of existing mounts.
>
> > I think gadgetfs most likely has the same problem but I'm not yet sure
> > how to test that.
> >
> > I have no real objection to merging your patch, though I like the
> > consistency of following e4d32142d1de a bit more. But I think we should
> > find a graceful solution so that any filesystem using get_tree_single
> > can avoid this pitfall, if possible.
> >
> > -Eric
--
Aleksa Sarai
Senior Software Engineer (Containers)
SUSE Linux GmbH
https://www.cyphar.com/
© 2016 - 2025 Red Hat, Inc.