lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <bc86b13e-1252-4bf0-86f9-77da37f5e37a@sandeen.net>
Date: Mon, 13 Oct 2025 13:46:42 -0500
From: Eric Sandeen <sandeen@...deen.net>
To: Dominique Martinet <asmadeus@...ewreck.org>,
 Eric Sandeen <sandeen@...hat.com>
Cc: v9fs@...ts.linux.dev, linux-fsdevel@...r.kernel.org,
 linux-kernel@...r.kernel.org, ericvh@...nel.org, lucho@...kov.net,
 linux_oss@...debyte.com, eadavis@...com
Subject: Re: [PATCH V3 4/4] 9p: convert to the new mount API

On 10/13/25 5:26 AM, Dominique Martinet wrote:
> Hi Eric,
> 
> Thanks for this V3!
> 
> I find it much cleaner, hopefully will be easier to debug :)

Good news and bad news, I see.

> ... Which turned out to be needed right away, trying with qemu's 9p
> export "mount -t 9p -o trans=virtio tmp /mnt" apparently calls
> p9_virtio_create() with fc->source == NULL, instead of the expected
> "tmp" string
> (FWIW I tried '-o trans=tcp 127.0.0.1' and I got the same problem in
> p9_fd_create_tcp(), might be easier to test with diod if that's what you
> used)

I swear I tested this, but you are right, and it fails for me too.

Oh ... I know what this is :(

Introducing the "ignore unknown mount options" change in V4 caused it to
also ignore the unknown "source" option and report success; this made the
vfs think "source" was already handled in vfs_parse_fs_param() and
therefore it does not call vfs_parse_fs_param_source(). This has bitten
me before and it's a bit confusing.

I'm not sure how I missed this in my V4 testing, I'm very sorry.

> Looking at other filesystems (e.g. fs/nfs/fs_context.c but others are
> the same) it looks like they all define a fsparam_string "source" option
> explicitly?...

Not all of them; filesystems that reject unknown options have "source"
handled for them in the VFS, but for filesystems like debugfs that
ignore unknown parameters it had to handle it explicitly. (Other
filesystems may do so for other reasons I suppose).

See also a20971c18752 which fixed a20971c18752, though the bug had
slightly less of an impact.

> Something like this looks like it works to do (+ probably make the error
> more verbose? nothing in dmesg hints at why mount returns EINVAL...)
> -----
> diff --git a/fs/9p/v9fs.c b/fs/9p/v9fs.c
> index 6c07635f5776..999d54a0c7d9 100644
> --- a/fs/9p/v9fs.c
> +++ b/fs/9p/v9fs.c
> @@ -34,6 +34,8 @@ struct kmem_cache *v9fs_inode_cache;
>   */
>  
>  enum {
> +	/* Mount-point source */
> +	Opt_source,
>  	/* Options that take integer arguments */
>  	Opt_debug, Opt_dfltuid, Opt_dfltgid, Opt_afid,
>  	/* String options */
> @@ -82,6 +84,7 @@ static const struct constant_table p9_cache_mode[] = {
>   * the client, and all the transports.
>   */
>  const struct fs_parameter_spec v9fs_param_spec[] = {
> +	fsparam_string  ("source",      Opt_source),
>  	fsparam_u32hex	("debug",	Opt_debug),
>  	fsparam_uid	("dfltuid",	Opt_dfltuid),
>  	fsparam_gid	("dfltgid",	Opt_dfltgid),
> @@ -210,6 +213,14 @@ int v9fs_parse_param(struct fs_context *fc, struct fs_parameter *param)
>  	}
>  
>  	switch (opt) {
> +	case Opt_source:
> +                if (fc->source) {
> +			pr_info("p9: multiple sources not supported\n");
> +			return -EINVAL;
> +		}
> +		fc->source = param->string;
> +		param->string = NULL;

Yep, this looks correct, I think. It essentially "steals" the string from
the param and sets it in fc->source since the VFS won't do it for us.

I can't help but feel like there's maybe a better treewide fix for this
to make it all a bit less opaque, but for now this is what other
filesystems do, and so I think this is the right fix for my series at
this point.

Would you like me to send an updated patch with this change, or will you
just fix it on your end?

Thanks,
-Eric

> +		break;
>  	case Opt_debug:
>  		session_opts->debug = result.uint_32;
>  #ifdef CONFIG_NET_9P_DEBUG
> -----
> 
> I'll try to find some time to test a mix of actual mount options later
> this week
> 
> Cheers,


Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ