Received-SPF: pass (google.com: domain of linux-kernel+bounces-194672-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Message-ID: <39a2d0a7-20f3-4a51-b2e0-1ade3eab14c5@sandeen.net>
Date: Wed, 29 May 2024 15:43:45 -0500
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH] debugfs: ignore auto and noauto options if given
To: Christian Brauner <brauner@kernel.org>,
 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
References: <20240522083851.37668-1-wsa+renesas@sang-engineering.com>
 <20240524-glasfaser-gerede-fdff887f8ae2@brauner>
Content-Language: en-US
From: Eric Sandeen <sandeen@sandeen.net>
In-Reply-To: <20240524-glasfaser-gerede-fdff887f8ae2@brauner>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit

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?
>