Message-ID: <1dcf4661-97c2-4727-b4c5-f05785196dcb@sandeen.net>
Date: Thu, 14 Aug 2025 11:46:08 -0500
From: Eric Sandeen <sandeen@...deen.net>
To: Aleksa Sarai <cyphar@...har.com>, Eric Sandeen <sandeen@...hat.com>
Cc: Charalampos Mitrodimas <charmitro@...teo.net>,
 Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
 "Rafael J. Wysocki" <rafael@...nel.org>, Danilo Krummrich <dakr@...nel.org>,
 Christian Brauner <brauner@...nel.org>, David Howells <dhowells@...hat.com>,
 linux-kernel@...r.kernel.org, linux-fsdevel <linux-fsdevel@...r.kernel.org>
Subject: Re: [PATCH] debugfs: fix mount options not being applied

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

Powered by blists - more mailing lists