linux-kernel - Re: [PATCH V3 4/4] 9p: convert to the new mount API

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] [day] [month] [year] [list]

Message-ID: <aO1NLCI3kIdgWcvh@codewreck.org>
Date: Tue, 14 Oct 2025 04:04:12 +0900
From: Dominique Martinet <asmadeus@...ewreck.org>
To: Eric Sandeen <sandeen@...deen.net>
Cc: Eric Sandeen <sandeen@...hat.com>, 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

Eric Sandeen wrote on Mon, Oct 13, 2025 at 01:46:42PM -0500:
> > ... 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.

No harm done :)

And thanks for the explanation, the vfs parsing being done only if the
fs-specific parsing failed was far from obvious for me!

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

(I assume the former was 3a987b88a425)

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

Yes, I copied that from nfs and it looks like debugfs does the same.

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

Not much better given it does the work twice but we could return -EINVAL
in the case Opt_source statement to optimize for code size...
I'm not sure that's quite clearer though, I'll stick to "doing what
everyone else" does for now and we/someone can revisit this later.

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

Happy to roll it in directly, I'll reply again when I find time to
finish testing and push to next.
-- 
Dominique Martinet | Asmadeus