Received-SPF: pass (google.com: domain of linux-kernel+bounces-188755-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Date: Fri, 24 May 2024 15:55:10 +0200
From: Christian Brauner <brauner@kernel.org>
To: Wolfram Sang <wsa+renesas@sang-engineering.com>, 
	Eric Sandeen <sandeen@redhat.com>
Cc: linux-renesas-soc@vger.kernel.org, linux-fsdevel@vger.kernel.org, 
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>, "Rafael J. Wysocki" <rafael@kernel.org>, 
	David Howells <dhowells@redhat.com>, linux-kernel@vger.kernel.org
Subject: Re: [PATCH] debugfs: ignore auto and noauto options if given
Message-ID: <20240524-glasfaser-gerede-fdff887f8ae2@brauner>
References: <20240522083851.37668-1-wsa+renesas@sang-engineering.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <20240522083851.37668-1-wsa+renesas@sang-engineering.com>

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?