Message-ID: <ZxgudMCSgbDOEjpD@codewreck.org>
Date: Wed, 23 Oct 2024 08:00:04 +0900
From: Dominique Martinet <asmadeus@...ewreck.org>
To: Will Deacon <will@...nel.org>, ericvh@...nel.org
Cc: Thorsten Leemhuis <regressions@...mhuis.info>, lucho@...kov.net,
	Christian Brauner <brauner@...nel.org>,
	Alexander Viro <viro@...iv.linux.org.uk>, oss@...debyte.com,
	v9fs@...ts.linux.dev, linux-kernel@...r.kernel.org, oleg@...hat.com,
	keirf@...gle.com, regressions@...ts.linux.dev
Subject: Re: VFS regression with 9pfs ("Lookup would have caused loop")

Will Deacon wrote on Tue, Oct 22, 2024 at 04:01:49PM +0100:
> > One note though he did sent a patch that seems related and wasn't sent
> > for merge:
> > https://lore.kernel.org/all/CAFkjPTn7JAbmKYASaeBNVpumOncPaReiPbc4Ph6ik3nNf8UTNg@mail.gmail.com/T/#u
> >
> > Will, perhaps you can try it? I'm pretty sure the setup to reproduce
> > this is easy enough that I'll be able to reproduce in less than an hour
> > (export two tmpfs [sequential inode number fs] wthin the same 9p mount
> > in qemu without 'multidevs=remap'), but I don't even have that time
> > right now.
> > 
> > (I didn't even read the patch properly and it might not help at all,
> > sorry in this case)
> 
> I think this patch landed upsteam as d05dcfdf5e16 (" fs/9p: mitigate
> inode collisions") and so I can confirm that it doesn't help with the
> issue.

Ugh, yes, sorry I'm blind it's there alright (you even listed it in your
first post in the list of commits to revert)

> > I'm not sure this really needs to get Linus involved - it's breaking a
> > server that used to work even if qemu has been printing a warning about
> > these duplicate qid.path for a while, and the server really is the
> > better place to remap these inodes as we have no idea of the underlying
> > device id as far as I know...
> 
> FWIW, I'm not using QEMU at all. This is with kvmtool which, for better
> or worse, prints no such diagnostic and used to be reliable enough with
> whatever magic the kernel had prior to v6.9.

Yes, that problem isn't specific to qemu, anything that re-exports
multiple filesystems using underlying inode numbers directly has this
risk (e.g. even diod/nfs-ganesha tcp mounts ought to reproduce if they
don't mangle inodes); I agree we need to handle servers throwing junk at
us.

It's easy enough to reproduce with qemu if remapping is disabled as
expected (I don't hit the VFS warning in d_splice_alias() but the
behaviour is definitely wrong):
----
# host side
cd /tmp/linux-test
mkdir m1 m2
mount -t tmpfs tmpfs m1
mount -t tmpfs tmpfs m2
mkdir m1/dir m2/dir
echo foo > m1/dir/foo
echo bar > m2/dir/bar

# guest side
# started with -virtfs local,path=/tmp/linux-test,mount_tag=tmp,security_model=mapped-file
mount -t 9p -o trans=virtio,debug=1 tmp /mnt/t

ls /mnt/t/m1/dir
# foo
ls /mnt/t/m2/dir
# bar (works ok if directry isn't open)

# cd to keep first dir's inode alive
cd /mnt/t/m1/dir
ls /mnt/t/m2/dir
# foo (should be bar)
----

This can also be observed with files if mounting with cache=fscache or
similar (but interestingly even keeping the file open will properly
disociate both files in default cache=none mode); either way we won't be
able to properly work with hard links if we assume the server isn't
reliable; I guess that if someone wants that we'd really need to
implement some capability negotiation at mount time so the server can
tell us what is supported and unless that is done assume the server
cannot reliably assign inodes...

And if we go that way then we just shouldn't ever look at the inode
number?... Which seems to pretty much be what the old code was doing in
the "new" path of v9fs_qid_iget(); the compare function would just
always say the inodes are different and get a new one...
The mitigation Eric implemented is similar enough (re-added a 'new'
parameter, and fail if I_NEW isn't set when new was requested), but it
looks like v9fs_fid_iget_dotl() isn't called at all when accessing the
other overlapping directory, so that wasn't effective here as some
higher level of caching caught the inode first.


I've also confirmed reverting the 4 commits listed by Will do fix both
behaviors (along with a fscache warning when hitting the duplicate inode
file, but that's expected):
        724a08450f74 "fs/9p: simplify iget to remove unnecessary paths"
        11763a8598f8 "fs/9p: fix uaf in in v9fs_stat2inode_dotl"
        10211b4a23cf "fs/9p: remove redundant pointer v9ses"
        d05dcfdf5e16 " fs/9p: mitigate inode collisions"


> I'm happy to test patches if there's anything available, but otherwise
> the reverts at least get us back to the old behaviour if nobody has time
> to come up with something better.

I think that's the sane thing to do, let's first go back to something
that works and we can try again if/when someone has time - I've
certainly just spent more time here than I have, and starting over is just
a matter of reapplying the patches in a local branch so it's not like
there's anything to lose (afaiu this was intended as code cleanup to
make the code more maintainable rather than fixing something specific)



Thorsten, is there a preferred way reverts should be done?
In this case it'd probably make sense to squash the 4 reverts in a
single megarevert? At the very least getting anywhere in the middle with
the uaf isn't something one would want... And they all made it in 6.9
together so there's no benefit in splitting them for backport either.

Once that's decided I'll prepare something to send Linus in a few days,
I don't think there's much point in sitting on this either...


Thanks,
-- 
Dominique Martinet | Asmadeus

Powered by blists - more mailing lists