Message-ID: <2025-08-14.1755177392-elderly-somber-portal-duress-1AbFcr@cyphar.com>
Date: Thu, 14 Aug 2025 23:47:24 +1000
From: Aleksa Sarai <cyphar@...har.com>
To: 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 2025-08-14, Aleksa Sarai <cyphar@...har.com> 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.
> 
> 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/

Download attachment "signature.asc" of type "application/pgp-signature" (266 bytes)

Powered by blists - more mailing lists