| Message ID | 20240522083851.37668-1-wsa+renesas@sang-engineering.com (mailing list archive) |
|---|---|
| State | Under Review |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
| Series | debugfs: ignore auto and noauto options if given | expand |
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?
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
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
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.
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? >
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
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
> > > 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?
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.
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
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?
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
> 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...
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.
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), {} };
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(+)