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: <48c5e8bb-2f65-41ee-b9ce-f31b2997b751@redhat.com>
Date: Wed, 14 Aug 2024 10:20:09 -0500
From: Eric Sandeen <sandeen@...hat.com>
To: Greg Kroah-Hartman <gregkh@...uxfoundation.org>
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 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.

-Eric