Re: [PATCH] debugfs: fix mount options not being applied

Re: [PATCH] debugfs: fix mount options not being applied

[Eric Sandeen]

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

Re: [PATCH] debugfs: fix mount options not being applied

[Charalampos Mitrodimas]

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

Re: [PATCH] debugfs: fix mount options not being applied

[Eric Sandeen]

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

Re: [PATCH] debugfs: fix mount options not being applied

[Christian Brauner]

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.

Re: [PATCH] debugfs: fix mount options not being applied

[Eric Sandeen]

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 

Re: [PATCH] debugfs: fix mount options not being applied

[Charalampos Mitrodimas]

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

Re: [PATCH] debugfs: fix mount options not being applied

[Aleksa Sarai]

[-- Attachment #1: Type: text/plain, Size: 4081 bytes --]

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/

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 265 bytes --]

Re: [PATCH] debugfs: fix mount options not being applied

[Aleksa Sarai]

[-- Attachment #1: Type: text/plain, Size: 5591 bytes --]

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/

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 265 bytes --]

Re: [PATCH] debugfs: fix mount options not being applied

[Eric Sandeen]

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
>>
>>
> 

Re: [PATCH] debugfs: fix mount options not being applied

[Aleksa Sarai]

[-- Attachment #1: Type: text/plain, Size: 6739 bytes --]

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/

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 265 bytes --]