[PATCH] debugfs: ignore auto and noauto options if given

[PATCH] debugfs: ignore auto and noauto options if given

[Wolfram Sang]

The 'noauto' and 'auto' options were missed when migrating to the new
mount API. As a result, users with these in their fstab mount options
are now unable to mount debugfs filesystems, as they'll receive an
"Unknown parameter" error.

This restores the old behaviour of ignoring noauto and auto if they're
given.

Fixes: a20971c18752 ("vfs: Convert debugfs to use the new mount API")
Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---

With current top-of-tree, debugfs remained empty on my boards triggering
the message "debugfs: Unknown parameter 'auto'". I applied a similar fix
which CIFS got and largely reused the commit message from 19d51588125f
("cifs: ignore auto and noauto options if given").

Given the comment in debugfs_parse_param(), I am not sure if this patch
is a complete fix or if there are more options to be ignored. This patch
makes it work for me(tm), however.

From my light research, tracefs (which was converted to new mount API
together with debugfs) doesn't need the same fixing. But I am not
super-sure about that.

Looking forward to comments.


 fs/debugfs/inode.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/fs/debugfs/inode.c b/fs/debugfs/inode.c
index dc51df0b118d..915f0b618486 100644
--- a/fs/debugfs/inode.c
+++ b/fs/debugfs/inode.c
@@ -89,12 +89,14 @@ enum {
 	Opt_uid,
 	Opt_gid,
 	Opt_mode,
+	Opt_ignore,
 };
 
 static const struct fs_parameter_spec debugfs_param_specs[] = {
 	fsparam_u32	("gid",		Opt_gid),
 	fsparam_u32oct	("mode",	Opt_mode),
 	fsparam_u32	("uid",		Opt_uid),
+	fsparam_flag_no	("auto",	Opt_ignore),
 	{}
 };
 
-- 
2.39.2

Re: [PATCH] debugfs: ignore auto and noauto options if given

[Christian Brauner]

On Wed, May 22, 2024 at 10:38:51AM +0200, Wolfram Sang wrote:
> The 'noauto' and 'auto' options were missed when migrating to the new
> mount API. As a result, users with these in their fstab mount options
> are now unable to mount debugfs filesystems, as they'll receive an
> "Unknown parameter" error.
> 
> This restores the old behaviour of ignoring noauto and auto if they're
> given.
> 
> Fixes: a20971c18752 ("vfs: Convert debugfs to use the new mount API")
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> ---
> 
> With current top-of-tree, debugfs remained empty on my boards triggering
> the message "debugfs: Unknown parameter 'auto'". I applied a similar fix
> which CIFS got and largely reused the commit message from 19d51588125f
> ("cifs: ignore auto and noauto options if given").
> 
> Given the comment in debugfs_parse_param(), I am not sure if this patch
> is a complete fix or if there are more options to be ignored. This patch
> makes it work for me(tm), however.
> 
> From my light research, tracefs (which was converted to new mount API
> together with debugfs) doesn't need the same fixing. But I am not
> super-sure about that.

Afaict, the "auto" option has either never existent or it was removed before
the new mount api conversion time ago for debugfs. In any case, the root of the
issue is that we used to ignore unknown mount options in the old mount api so
you could pass anything that you would've wanted in there:

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

So there's two ways to fix this:

(1) We continue ignoring mount options completely when they're coming
    from the new mount api.
(2) We continue ignoring mount options toto caelo.

The advantage of (1) is that we gain the liberty to report errors to
users on unknown mount options in the future but it will break on
mount(8) from util-linux that relies on the new mount api by default. So
I think what we need is (2) so something like:

diff --git a/fs/debugfs/inode.c b/fs/debugfs/inode.c
index dc51df0b118d..713b6f76e75d 100644
--- a/fs/debugfs/inode.c
+++ b/fs/debugfs/inode.c
@@ -107,8 +107,16 @@ static int debugfs_parse_param(struct fs_context *fc, struct fs_parameter *param
        int opt;

        opt = fs_parse(fc, debugfs_param_specs, param, &result);
-       if (opt < 0)
+       if (opt < 0) {
+               /*
+                * We might like to report bad mount options here; but
+                * traditionally debugfs has ignored all mount options
+                */
+               if (opt == -ENOPARAM)
+                       return 0;
+
                return opt;
+       }

        switch (opt) {
        case Opt_uid:


Does that fix it for you?

Re: [PATCH] debugfs: ignore auto and noauto options if given

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 1343 bytes --]

Hi Christian,

> Afaict, the "auto" option has either never existent or it was removed before
> the new mount api conversion time ago for debugfs.

Frankly, I have no idea why I put this 'auto' in my fstab ages ago. But
it seems, I am not the only one[1].

[1] https://www.ibm.com/docs/en/linux-on-systems?topic=assumptions-debugfs

> diff --git a/fs/debugfs/inode.c b/fs/debugfs/inode.c
> index dc51df0b118d..713b6f76e75d 100644
> --- a/fs/debugfs/inode.c
> +++ b/fs/debugfs/inode.c
> @@ -107,8 +107,16 @@ static int debugfs_parse_param(struct fs_context *fc, struct fs_parameter *param
>         int opt;
> 
>         opt = fs_parse(fc, debugfs_param_specs, param, &result);
> -       if (opt < 0)
> +       if (opt < 0) {
> +               /*
> +                * We might like to report bad mount options here; but
> +                * traditionally debugfs has ignored all mount options
> +                */
> +               if (opt == -ENOPARAM)
> +                       return 0;
> +
>                 return opt;
> +       }
> 
>         switch (opt) {
>         case Opt_uid:
> 
> 
> Does that fix it for you?

Yes, it does, thank you.

Reported-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
Tested-by: Wolfram Sang <wsa+renesas@sang-engineering.com>

Happy hacking,

   Wolfram


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] debugfs: ignore auto and noauto options if given

[Geert Uytterhoeven]

Hi Wolfram,

On Mon, May 27, 2024 at 12:08 PM Wolfram Sang
<wsa+renesas@sang-engineering.com> wrote:
> > Afaict, the "auto" option has either never existent or it was removed before
> > the new mount api conversion time ago for debugfs.
>
> Frankly, I have no idea why I put this 'auto' in my fstab ages ago. But
> it seems, I am not the only one[1].

fstab(5):

       defaults
           use default options: rw, suid, dev, exec, auto, nouser, and async.

       noauto
           do not mount when mount -a is given (e.g., at boot time)

So I assume "auto" is still passed when using "defaults"?

However, nowadays (since +10y?), debugfs etc. tend to no longer be
put in /etc/fstab, but be mounted automatically by some initscript.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH] debugfs: ignore auto and noauto options if given

[Christian Brauner]

On Mon, May 27, 2024 at 12:06:18PM +0200, Wolfram Sang wrote:
> Hi Christian,
> 
> > Afaict, the "auto" option has either never existent or it was removed before
> > the new mount api conversion time ago for debugfs.
> 
> Frankly, I have no idea why I put this 'auto' in my fstab ages ago. But
> it seems, I am not the only one[1].
> 
> [1] https://www.ibm.com/docs/en/linux-on-systems?topic=assumptions-debugfs
> 
> > diff --git a/fs/debugfs/inode.c b/fs/debugfs/inode.c
> > index dc51df0b118d..713b6f76e75d 100644
> > --- a/fs/debugfs/inode.c
> > +++ b/fs/debugfs/inode.c
> > @@ -107,8 +107,16 @@ static int debugfs_parse_param(struct fs_context *fc, struct fs_parameter *param
> >         int opt;
> > 
> >         opt = fs_parse(fc, debugfs_param_specs, param, &result);
> > -       if (opt < 0)
> > +       if (opt < 0) {
> > +               /*
> > +                * We might like to report bad mount options here; but
> > +                * traditionally debugfs has ignored all mount options
> > +                */
> > +               if (opt == -ENOPARAM)
> > +                       return 0;
> > +
> >                 return opt;
> > +       }
> > 
> >         switch (opt) {
> >         case Opt_uid:
> > 
> > 
> > Does that fix it for you?
> 
> Yes, it does, thank you.
> 
> Reported-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> Tested-by: Wolfram Sang <wsa+renesas@sang-engineering.com>

Thanks, applied. Should be fixed by end of the week.

Re: [PATCH] debugfs: ignore auto and noauto options if given

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 313 bytes --]


> > > Does that fix it for you?
> > 
> > Yes, it does, thank you.
> > 
> > Reported-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> > Tested-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> 
> Thanks, applied. Should be fixed by end of the week.

It is in -next but not in rc2. rc3 then?


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] debugfs: ignore auto and noauto options if given

[Christian Brauner]

On Mon, Jun 03, 2024 at 09:24:50AM +0200, Wolfram Sang wrote:
> 
> > > > Does that fix it for you?
> > > 
> > > Yes, it does, thank you.
> > > 
> > > Reported-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> > > Tested-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> > 
> > Thanks, applied. Should be fixed by end of the week.
> 
> It is in -next but not in rc2. rc3 then?

Yes, it wasn't ready when I sent the fixes for -rc2 as I just put it in
that day.

Re: [PATCH] debugfs: ignore auto and noauto options if given

[Eric Sandeen]

On 6/3/24 8:31 AM, Christian Brauner wrote:
> On Mon, Jun 03, 2024 at 09:24:50AM +0200, Wolfram Sang wrote:
>>
>>>>> Does that fix it for you?
>>>>
>>>> Yes, it does, thank you.
>>>>
>>>> Reported-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
>>>> Tested-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
>>>
>>> Thanks, applied. Should be fixed by end of the week.
>>
>> It is in -next but not in rc2. rc3 then?
> 
> Yes, it wasn't ready when I sent the fixes for -rc2 as I just put it in
> that day.
> 

See my other reply, are you sure we should make this change? From a
"keep the old behavior" POV maybe so, but this looks to me like a
bug in busybox, passing fstab hint "options" like "auto" as actual mount
options being the root cause of the problem. debugfs isn't uniquely
affected by this behavior.

I'm not dead set against the change, just wanted to point this out.

Thanks,
-Eric

Re: [PATCH] debugfs: ignore auto and noauto options if given

[Christian Brauner]

On Mon, Jun 03, 2024 at 09:17:10AM -0500, Eric Sandeen wrote:
> On 6/3/24 8:31 AM, Christian Brauner wrote:
> > On Mon, Jun 03, 2024 at 09:24:50AM +0200, Wolfram Sang wrote:
> >>
> >>>>> Does that fix it for you?
> >>>>
> >>>> Yes, it does, thank you.
> >>>>
> >>>> Reported-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> >>>> Tested-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> >>>
> >>> Thanks, applied. Should be fixed by end of the week.
> >>
> >> It is in -next but not in rc2. rc3 then?
> > 
> > Yes, it wasn't ready when I sent the fixes for -rc2 as I just put it in
> > that day.
> > 
> 
> See my other reply, are you sure we should make this change? From a
> "keep the old behavior" POV maybe so, but this looks to me like a
> bug in busybox, passing fstab hint "options" like "auto" as actual mount
> options being the root cause of the problem. debugfs isn't uniquely
> affected by this behavior.
> 
> I'm not dead set against the change, just wanted to point this out.

Hm, it seems I forgot your other mail, sorry.
So the issue is that we're breaking existing userspace and it doesn't
seem like a situation where we can just ignore broken userspace. If
busybox has been doing that for a long time we might just have to
accommodate their brokenness. Thoughts?

Re: [PATCH] debugfs: ignore auto and noauto options if given

[Eric Sandeen]

On 6/3/24 9:33 AM, Christian Brauner wrote:
> On Mon, Jun 03, 2024 at 09:17:10AM -0500, Eric Sandeen wrote:
>> On 6/3/24 8:31 AM, Christian Brauner wrote:
>>> On Mon, Jun 03, 2024 at 09:24:50AM +0200, Wolfram Sang wrote:
>>>>
>>>>>>> Does that fix it for you?
>>>>>>
>>>>>> Yes, it does, thank you.
>>>>>>
>>>>>> Reported-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
>>>>>> Tested-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
>>>>>
>>>>> Thanks, applied. Should be fixed by end of the week.
>>>>
>>>> It is in -next but not in rc2. rc3 then?
>>>
>>> Yes, it wasn't ready when I sent the fixes for -rc2 as I just put it in
>>> that day.
>>>
>>
>> See my other reply, are you sure we should make this change? From a
>> "keep the old behavior" POV maybe so, but this looks to me like a
>> bug in busybox, passing fstab hint "options" like "auto" as actual mount
>> options being the root cause of the problem. debugfs isn't uniquely
>> affected by this behavior.
>>
>> I'm not dead set against the change, just wanted to point this out.
> 
> Hm, it seems I forgot your other mail, sorry.

No worries!

> So the issue is that we're breaking existing userspace and it doesn't
> seem like a situation where we can just ignore broken userspace. If
> busybox has been doing that for a long time we might just have to
> accommodate their brokenness. Thoughts?

Yep, I can totally see that POV.

It's just that surely every other strict-parsing filesystem is also
broken in this same way, so coding around the busybox bug only in debugfs
seems a little strange. (Surely we won't change every filesystem to accept
unknown options just for busybox's benefit.)

IOWS: why do we accomodate busybox brokenness only for debugfs, given that
"auto" can be used in fstab for any filesystem?

But in simplest terms - it was, in fact, debugfs that a) changed and
b) got the bug report, so I don't have strong objections to going back
to the old behavior.

-Eric

Re: [PATCH] debugfs: ignore auto and noauto options if given

[Christian Brauner]

On Mon, Jun 03, 2024 at 10:13:43AM -0500, Eric Sandeen wrote:
> On 6/3/24 9:33 AM, Christian Brauner wrote:
> > On Mon, Jun 03, 2024 at 09:17:10AM -0500, Eric Sandeen wrote:
> >> On 6/3/24 8:31 AM, Christian Brauner wrote:
> >>> On Mon, Jun 03, 2024 at 09:24:50AM +0200, Wolfram Sang wrote:
> >>>>
> >>>>>>> Does that fix it for you?
> >>>>>>
> >>>>>> Yes, it does, thank you.
> >>>>>>
> >>>>>> Reported-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> >>>>>> Tested-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> >>>>>
> >>>>> Thanks, applied. Should be fixed by end of the week.
> >>>>
> >>>> It is in -next but not in rc2. rc3 then?
> >>>
> >>> Yes, it wasn't ready when I sent the fixes for -rc2 as I just put it in
> >>> that day.
> >>>
> >>
> >> See my other reply, are you sure we should make this change? From a
> >> "keep the old behavior" POV maybe so, but this looks to me like a
> >> bug in busybox, passing fstab hint "options" like "auto" as actual mount
> >> options being the root cause of the problem. debugfs isn't uniquely
> >> affected by this behavior.
> >>
> >> I'm not dead set against the change, just wanted to point this out.
> > 
> > Hm, it seems I forgot your other mail, sorry.
> 
> No worries!
> 
> > So the issue is that we're breaking existing userspace and it doesn't
> > seem like a situation where we can just ignore broken userspace. If
> > busybox has been doing that for a long time we might just have to
> > accommodate their brokenness. Thoughts?
> 
> Yep, I can totally see that POV.
> 
> It's just that surely every other strict-parsing filesystem is also
> broken in this same way, so coding around the busybox bug only in debugfs
> seems a little strange. (Surely we won't change every filesystem to accept
> unknown options just for busybox's benefit.)
> 
> IOWS: why do we accomodate busybox brokenness only for debugfs, given that
> "auto" can be used in fstab for any filesystem?

I suspect that not that most filesystems aren't mounted from fstab which
is why we've never saw reports.

Re: [PATCH] debugfs: ignore auto and noauto options if given

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 411 bytes --]


> See my other reply, are you sure we should make this change? From a
> "keep the old behavior" POV maybe so, but this looks to me like a
> bug in busybox, passing fstab hint "options" like "auto" as actual mount
> options being the root cause of the problem. debugfs isn't uniquely
> affected by this behavior.

For the record, I plan to fix busybox. However, there are still a lot of
old versions around...


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] debugfs: ignore auto and noauto options if given

[Eric Sandeen]

On 5/24/24 8:55 AM, Christian Brauner wrote:
> On Wed, May 22, 2024 at 10:38:51AM +0200, Wolfram Sang wrote:
>> The 'noauto' and 'auto' options were missed when migrating to the new
>> mount API. As a result, users with these in their fstab mount options
>> are now unable to mount debugfs filesystems, as they'll receive an
>> "Unknown parameter" error.
>>
>> This restores the old behaviour of ignoring noauto and auto if they're
>> given.
>>
>> Fixes: a20971c18752 ("vfs: Convert debugfs to use the new mount API")
>> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
>> ---
>>
>> With current top-of-tree, debugfs remained empty on my boards triggering
>> the message "debugfs: Unknown parameter 'auto'". I applied a similar fix
>> which CIFS got and largely reused the commit message from 19d51588125f
>> ("cifs: ignore auto and noauto options if given").
>>
>> Given the comment in debugfs_parse_param(), I am not sure if this patch
>> is a complete fix or if there are more options to be ignored. This patch
>> makes it work for me(tm), however.
>>
>> From my light research, tracefs (which was converted to new mount API
>> together with debugfs) doesn't need the same fixing. But I am not
>> super-sure about that.
> 
> Afaict, the "auto" option has either never existent or it was removed before
> the new mount api conversion time ago for debugfs. In any case, the root of the
> issue is that we used to ignore unknown mount options in the old mount api so
> you could pass anything that you would've wanted in there:
> 
> /*
>  * We might like to report bad mount options here;
>  * but traditionally debugfs has ignored all mount options
>  */
> 
> So there's two ways to fix this:
> 
> (1) We continue ignoring mount options completely when they're coming
>     from the new mount api.
> (2) We continue ignoring mount options toto caelo.
> 
> The advantage of (1) is that we gain the liberty to report errors to
> users on unknown mount options in the future but it will break on
> mount(8) from util-linux that relies on the new mount api by default. So
> I think what we need is (2) so something like:

Argh, sorry I missed this thread until now.

FWIW, I think the "ignore unknown mount options" was a weird old artifact;
unknown options were only ignored originally because there were none at all,
hence no parser to reject anything.

Still, it seems odd to me that "auto/noauto" were ever passed to the kernel,
I thought those were just a hint to userspace mount tools, no?

And why wouldn't every other filesystem with rejection of unknown options
fail in the same way?

And indeed, if I add this line to my fstab on a fedora rawhide box with the
latest upstream kernel that has the new mount API debugfs patch present

debugfs /debugfs-test debugfs auto 0 0 

and strace "mount -a" or "mount /debugfs-test" I do not see any "auto" options
being passed to the kernel, and both commands succeed happily. Same if I change
"auto" to "noauto"

@Wolfram, what did your actual fstab line look like? I wonder what is actually
trying to pass auto as a mount option, and why...

-Eric

> diff --git a/fs/debugfs/inode.c b/fs/debugfs/inode.c
> index dc51df0b118d..713b6f76e75d 100644
> --- a/fs/debugfs/inode.c
> +++ b/fs/debugfs/inode.c
> @@ -107,8 +107,16 @@ static int debugfs_parse_param(struct fs_context *fc, struct fs_parameter *param
>         int opt;
> 
>         opt = fs_parse(fc, debugfs_param_specs, param, &result);
> -       if (opt < 0)
> +       if (opt < 0) {
> +               /*
> +                * We might like to report bad mount options here; but
> +                * traditionally debugfs has ignored all mount options
> +                */
> +               if (opt == -ENOPARAM)
> +                       return 0;
> +
>                 return opt;
> +       }
> 
>         switch (opt) {
>         case Opt_uid:
> 
> 
> Does that fix it for you?
> 

Re: [PATCH] debugfs: ignore auto and noauto options if given

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 430 bytes --]

Hi Eric,

thanks for replying!

> @Wolfram, what did your actual fstab line look like? I wonder what is actually
> trying to pass auto as a mount option, and why...

My fstab entry looks like this:

debugfs /sys/kernel/debug       debugfs auto            0       0

This happened on an embedded system using busybox 1.33.0. strace is
currently not installed but I can try adding it if that is needed.

Happy hacking,

   Wolfram


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] debugfs: ignore auto and noauto options if given

[Eric Sandeen]

On 5/29/24 5:05 PM, Wolfram Sang wrote:
> Hi Eric,
> 
> thanks for replying!
> 
>> @Wolfram, what did your actual fstab line look like? I wonder what is actually
>> trying to pass auto as a mount option, and why...
> 
> My fstab entry looks like this:
> 
> debugfs /sys/kernel/debug       debugfs auto            0       0
> 
> This happened on an embedded system using busybox 1.33.0. strace is
> currently not installed but I can try adding it if that is needed.
> 
> Happy hacking,
> 
>    Wolfram
> 

Welp, that looks like it:

# strace busybox mount /debugfs-test
...
stat("debugfs", 0x7fffc05d3d60)         = -1 ENOENT (No such file or directory)
mount("debugfs", "/debugfs-test", "debugfs", MS_SILENT, "auto") = -1 EINVAL (Invalid argument)
write(2, "mount: mounting debugfs on /debu"..., 66mount: mounting debugfs on /debugfs-test failed: Invalid argument
) = 66

This does not appear to be unique to debugfs:

# grep tmp /etc/fstab 
/dev/loop0	/tmp/xfs	xfs	auto 0 0
# strace busybox mount /tmp/xfs
...
mount("/dev/loop0", "/tmp/xfs", "xfs", MS_SILENT, "auto") = -1 EINVAL (Invalid argument)
write(2, "mount: mounting /dev/loop0 on /t"..., 64mount: mounting /dev/loop0 on /tmp/xfs failed: Invalid argument
) = 64

# dmesg | grep auto | tail -n 1
[ 1931.471667] xfs: Unknown parameter 'auto'

This looks to me like a busybox flaw, not a debugfs bug (change in unknown
argument behavior notwithstanding...)

-Eric