On 8/14/25 4:05 AM, Aleksa Sarai wrote:
> On 2025-08-05, Eric Sandeen <sandeen@xxxxxxxxxx> 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
>>
>>
>
