linux-kernel - Re: [PATCH] debugfs show actual source in /proc/mounts

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <2024081445-grimacing-unfair-d31e@gregkh>
Date: Wed, 14 Aug 2024 17:26:10 +0200
From: Greg Kroah-Hartman <gregkh@...uxfoundation.org>
To: Eric Sandeen <sandeen@...hat.com>
Cc: Marc Aurèle La France <tsi@...oix.net>,
	"Rafael J. Wysocki" <rafael@...nel.org>,
	Christian Brauner <brauner@...nel.org>,
	David Howells <dhowells@...hat.com>, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] debugfs show actual source in /proc/mounts

On Wed, Aug 14, 2024 at 10:20:09AM -0500, Eric Sandeen wrote:
> On 8/13/24 11:50 PM, Greg Kroah-Hartman wrote:
> > On Tue, Aug 13, 2024 at 02:18:07PM -0500, Eric Sandeen wrote:
> >> On 8/13/24 4:54 AM, Greg Kroah-Hartman wrote:
> >>> On Sat, Aug 10, 2024 at 01:25:27PM -0600, Marc Aurèle La France wrote:
> >>>> After its conversion to the new mount API, debugfs displays "none" in
> >>>> /proc/mounts instead of the actual source.  Fix this by recognising its
> >>>> "source" mount option.
> >>>>
> >>>> Signed-off-by: Marc Aurèle La France <tsi@...oix.net>
> >>>> Fixes: a20971c18752 ("vfs: Convert debugfs to use the new mount API")
> >>>> Cc: stable@...r.kernel.org # 6.10.x: 9f111059e725: fs_parse: add uid & gid option option parsing helpers
> >>>> Cc: stable@...r.kernel.org # 6.10.x: 49abee5991e1: debugfs: Convert to new uid/gid option parsing helpers
> >>>
> >>> As this came from a fs tree, I'll let the vfs maintainer take it if they
> >>> think it is ok as I know nothing about the fs_parse stuff at the moment,
> >>> sorry.
> >>
> >> Hm, I guess this is OK, though it seems a little unexpected for debugfs
> >> to have to parse the trivial internal "source" option.
> >>
> >> This actually worked OK until
> >>
> >> 0c07c273a5fe debugfs: continue to ignore unknown mount options
> >>
> >> but after that commit, debugfs claims to parse "source" successfully even
> >> though it has not. So really, it Fixes: that commit, not the original
> >> conversion.
> >>
> >> I'm not sure of a better approach offhand, but maybe a comment about why
> >> Opt_source exists in debugfs would help future readers?
> > 
> > Why is debugfs unique here?  Why does it need to do something that
> > nothing else needs to do (like sysfs or tracefs or anything else...)
> 
> It's kind of a long sad story.
> 
> Originally, debugfs took no mount options. And because it had no mount option
> handling, it had nowhere to reject anything, and so any/all random options
> were silently accepted and ignored.
> 
> Then mode/uid/gid mount options were added to it in 2012 with:
> 
> d6e486868cde debugfs: add mode, uid and gid options
> 
> and it continued to ignore unknown options:
> 
> +               /*
> +                * We might like to report bad mount options here;
> +                * but traditionally debugfs has ignored all mount options
> +                */
> 
> A decade+ later, I forward-ported dhowells' mount API conversion with
> 
> a20971c18752 vfs: Convert debugfs to use the new mount API
> 
> after some discussion and agreement that we really should be rejecting
> unknown options, and all seemed well until ...
> 
> https://lore.kernel.org/linux-kernel/20240527100618.np2wqiw5mz7as3vk@ninjato/T/
> 
> it was reported that busybox was (incorrectly) passing in "auto" and "noauto"
> from fstab during mount, and thus failing. So, 
> 
> 0c07c273a5fe debugfs: continue to ignore unknown mount options
> 
> got merged to re-allow/re-ignore all unknown options in the spirit of "don't
> break userspace". All unknown options were re-ignored including, as it turns
> out, "source" which then led to the current reported problem.
> 
> As for why it's different from tracefs, that's a good question. Tracefs was
> cut-n-pasted from debugfs, and had the "ignore unknown options" behavior from
> day one, which in retrospect was probably a mistake. And now its behavior
> does differ from debugfs, but nobody has reported the busybox problem against
> it, I guess. This is an inconsistency.
> 
> sysfs has no ->parse_param at all, so vfs_parse_fs_param() falls into the:
> 
>         /* If the filesystem doesn't take any arguments, give it the
>          * default handling of source.
>          */
>         ret = vfs_parse_fs_param_source(fc, param);
> 
> case which handles the internal "source" argument just fine.
> 

Thanks for the history work here, makes sense in a sad way :(

No objection from me for this change now.

greg k-h