[PATCH] debugfs show actual source in /proc/mounts

[PATCH] debugfs show actual source in /proc/mounts

[Marc Aurèle La France]

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@tuyoix.net>
Fixes: a20971c18752 ("vfs: Convert debugfs to use the new mount API")
Cc: stable@vger.kernel.org # 6.10.x: 9f111059e725: fs_parse: add uid & gid option option parsing helpers
Cc: stable@vger.kernel.org # 6.10.x: 49abee5991e1: debugfs: Convert to new uid/gid option parsing helpers

diff -NRapruz -X /etc/diff.excludes linux-6.11.0-rc2/fs/debugfs/inode.c devel-6.11.0-rc2/fs/debugfs/inode.c
--- linux-6.11.0-rc2/fs/debugfs/inode.c	2024-08-04 14:50:53.000000000 -0600
+++ devel-6.11.0-rc2/fs/debugfs/inode.c	2024-08-05 17:12:45.414338128 -0600
@@ -89,12 +89,14 @@ enum {
 	Opt_uid,
 	Opt_gid,
 	Opt_mode,
+	Opt_source,
 };

 static const struct fs_parameter_spec debugfs_param_specs[] = {
 	fsparam_gid	("gid",		Opt_gid),
 	fsparam_u32oct	("mode",	Opt_mode),
 	fsparam_uid	("uid",		Opt_uid),
+	fsparam_string	("source",	Opt_source),
 	{}
 };

@@ -126,6 +128,12 @@ static int debugfs_parse_param(struct fs_context *fc, struct fs_parameter *param)
 	case Opt_mode:
 		opts->mode = result.uint_32 & S_IALLUGO;
 		break;
+	case Opt_source:
+		if (fc->source)
+			return invalfc(fc, "Multiple sources specified");
+		fc->source = param->string;
+		param->string = NULL;
+		break;
 	/*
 	 * We might like to report bad mount options here;
 	 * but traditionally debugfs has ignored all mount options

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

[Greg Kroah-Hartman]

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@tuyoix.net>
> Fixes: a20971c18752 ("vfs: Convert debugfs to use the new mount API")
> Cc: stable@vger.kernel.org # 6.10.x: 9f111059e725: fs_parse: add uid & gid option option parsing helpers
> Cc: stable@vger.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.

greg k-h

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

[Eric Sandeen]

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@tuyoix.net>
>> Fixes: a20971c18752 ("vfs: Convert debugfs to use the new mount API")
>> Cc: stable@vger.kernel.org # 6.10.x: 9f111059e725: fs_parse: add uid & gid option option parsing helpers
>> Cc: stable@vger.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?

Thanks,
-Eric


> greg k-h
> 

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

[Greg Kroah-Hartman]

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@tuyoix.net>
> >> Fixes: a20971c18752 ("vfs: Convert debugfs to use the new mount API")
> >> Cc: stable@vger.kernel.org # 6.10.x: 9f111059e725: fs_parse: add uid & gid option option parsing helpers
> >> Cc: stable@vger.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...)

thanks,

greg k-h

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

[Eric Sandeen]

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@tuyoix.net>
>>>> Fixes: a20971c18752 ("vfs: Convert debugfs to use the new mount API")
>>>> Cc: stable@vger.kernel.org # 6.10.x: 9f111059e725: fs_parse: add uid & gid option option parsing helpers
>>>> Cc: stable@vger.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 

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

[Greg Kroah-Hartman]

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@tuyoix.net>
> >>>> Fixes: a20971c18752 ("vfs: Convert debugfs to use the new mount API")
> >>>> Cc: stable@vger.kernel.org # 6.10.x: 9f111059e725: fs_parse: add uid & gid option option parsing helpers
> >>>> Cc: stable@vger.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

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

[Marc Aurèle La France]

On Tue, 2024-Aug-13, 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@tuyoix.net>
>>> Fixes: a20971c18752 ("vfs: Convert debugfs to use the new mount API")
>>> Cc: stable@vger.kernel.org # 6.10.x: 9f111059e725: fs_parse: add uid & gid option option parsing helpers
>>> Cc: stable@vger.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?

Meaning what?  I'd say you're in a better position to explain why debugfs
shouldn't follow other fs's in this regard.

Marc.

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

[Marc Aurèle La France]

After commit 0c07c273a5fe ("debugfs: continue to ignore unknown mount
options"), 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@tuyoix.net>
Fixes: 0c07c273a5fe ("debugfs: continue to ignore unknown mount options")
Cc: stable@vger.kernel.org # 6.10.x: 9f111059e725: fs_parse: add uid & gid option option parsing helpers
Cc: stable@vger.kernel.org # 6.10.x: 49abee5991e1: debugfs: Convert to new uid/gid option parsing helpers

diff -NRapruz -X /etc/diff.excludes linux-6.11.0-rc2/fs/debugfs/inode.c devel-6.11.0-rc2/fs/debugfs/inode.c
--- linux-6.11.0-rc5/fs/debugfs/inode.c
+++ devel-6.11.0-rc5/fs/debugfs/inode.c
@@ -89,12 +89,14 @@ enum {
 	Opt_uid,
 	Opt_gid,
 	Opt_mode,
+	Opt_source,
 };

 static const struct fs_parameter_spec debugfs_param_specs[] = {
 	fsparam_gid	("gid",		Opt_gid),
 	fsparam_u32oct	("mode",	Opt_mode),
 	fsparam_uid	("uid",		Opt_uid),
+	fsparam_string	("source",	Opt_source),
 	{}
 };

@@ -126,6 +128,12 @@ static int debugfs_parse_param(struct fs_context *fc, struct fs_parameter *param)
 	case Opt_mode:
 		opts->mode = result.uint_32 & S_IALLUGO;
 		break;
+	case Opt_source:
+		if (fc->source)
+			return invalfc(fc, "Multiple sources specified");
+		fc->source = param->string;
+		param->string = NULL;
+		break;
 	/*
 	 * We might like to report bad mount options here;
 	 * but traditionally debugfs has ignored all mount options

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

[Eric Sandeen]

On 8/29/24 10:44 PM, Marc Aurèle La France wrote:
> After commit 0c07c273a5fe ("debugfs: continue to ignore unknown mount
> options"), 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@tuyoix.net>
> Fixes: 0c07c273a5fe ("debugfs: continue to ignore unknown mount options")
> Cc: stable@vger.kernel.org # 6.10.x: 9f111059e725: fs_parse: add uid & gid option option parsing helpers
> Cc: stable@vger.kernel.org # 6.10.x: 49abee5991e1: debugfs: Convert to new uid/gid option parsing helpers
> 
> diff -NRapruz -X /etc/diff.excludes linux-6.11.0-rc2/fs/debugfs/inode.c devel-6.11.0-rc2/fs/debugfs/inode.c
> --- linux-6.11.0-rc5/fs/debugfs/inode.c
> +++ devel-6.11.0-rc5/fs/debugfs/inode.c
> @@ -89,12 +89,14 @@ enum {
>  	Opt_uid,
>  	Opt_gid,
>  	Opt_mode,
> +	Opt_source,
>  };
> 
>  static const struct fs_parameter_spec debugfs_param_specs[] = {
>  	fsparam_gid	("gid",		Opt_gid),
>  	fsparam_u32oct	("mode",	Opt_mode),
>  	fsparam_uid	("uid",		Opt_uid),
> +	fsparam_string	("source",	Opt_source),
>  	{}
>  };
> 
> @@ -126,6 +128,12 @@ static int debugfs_parse_param(struct fs_context *fc, struct fs_parameter *param)
>  	case Opt_mode:
>  		opts->mode = result.uint_32 & S_IALLUGO;
>  		break;

Sorry for missing your earlier question, I was thinking that perhaps a
comment along the lines of this would be helpful:

	/*
	 * Because debugfs accepts all mount options and indicates
	 * success even for unknown options, we must process "source"
	 * ourselves here; the vfs won't do it for us.
	 */
> +	case Opt_source:
> +		if (fc->source)
> +			return invalfc(fc, "Multiple sources specified");
> +		fc->source = param->string;
> +		param->string = NULL;
> +		break;

but I suppose it's not a big deal unless others think it is.

Reviewed-by: Eric Sandeen <sandeen@redhat.com>

>  	/*
>  	 * We might like to report bad mount options here;
>  	 * but traditionally debugfs has ignored all mount options
> 

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

[Marc Aurèle La France]

On Fri, 2024-Aug-30, Eric Sandeen wrote:
> On 8/29/24 10:44 PM, Marc Aurèle La France wrote:
>> After commit 0c07c273a5fe ("debugfs: continue to ignore unknown mount
>> options"), 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@tuyoix.net>
>> Fixes: 0c07c273a5fe ("debugfs: continue to ignore unknown mount options")
>> Cc: stable@vger.kernel.org # 6.10.x: 9f111059e725: fs_parse: add uid & gid option option parsing helpers
>> Cc: stable@vger.kernel.org # 6.10.x: 49abee5991e1: debugfs: Convert to new uid/gid option parsing helpers

>> diff -NRapruz -X /etc/diff.excludes linux-6.11.0-rc2/fs/debugfs/inode.c devel-6.11.0-rc2/fs/debugfs/inode.c
>> --- linux-6.11.0-rc5/fs/debugfs/inode.c
>> +++ devel-6.11.0-rc5/fs/debugfs/inode.c
>> @@ -89,12 +89,14 @@ enum {
>>  	Opt_uid,
>>  	Opt_gid,
>>  	Opt_mode,
>> +	Opt_source,
>>  };
>>
>>  static const struct fs_parameter_spec debugfs_param_specs[] = {
>>  	fsparam_gid	("gid",		Opt_gid),
>>  	fsparam_u32oct	("mode",	Opt_mode),
>>  	fsparam_uid	("uid",		Opt_uid),
>> +	fsparam_string	("source",	Opt_source),
>>  	{}
>>  };
>>
>> @@ -126,6 +128,12 @@ static int debugfs_parse_param(struct fs_context *fc, struct fs_parameter *param)
>>  	case Opt_mode:
>>  		opts->mode = result.uint_32 & S_IALLUGO;
>>  		break;

> Sorry for missing your earlier question, I was thinking that perhaps a
> comment along the lines of this would be helpful:

> 	/*
> 	 * Because debugfs accepts all mount options and indicates
> 	 * success even for unknown options, we must process "source"
> 	 * ourselves here; the vfs won't do it for us.
> 	 */

I disagree.  It is the new mount API that treats the source as a mount
"option", so to speak, despite what `man mount` says.  Therefore the
consequences of that change should be documented in the API, not here nor
in any other fs.  Besides, there's already too much echo in the comments
around this code.

>> +	case Opt_source:
>> +		if (fc->source)
>> +			return invalfc(fc, "Multiple sources specified");
>> +		fc->source = param->string;
>> +		param->string = NULL;
>> +		break;

> but I suppose it's not a big deal unless others think it is.

> Reviewed-by: Eric Sandeen <sandeen@redhat.com>

Thanks.

Marc.