Re: VFS regression with 9pfs ("Lookup would have caused loop")

Re: VFS regression with 9pfs ("Lookup would have caused loop")

[Will Deacon]

On Mon, Sep 23, 2024 at 11:05:08AM +0100, Will Deacon wrote:
> Hi Eric,
> 
> I'm trying to use kvmtool to run a simple guest under an Android host
> but, for v6.9+ guest kernels, 'init' reliably fails to run from a 9pfs
> mount because VFS emits this error:
> 
>   | VFS: Lookup of 'com.android.runtime' in 9p 9p would have caused loop
> 
> The host directory being shared is a little odd, as it has symlinks out
> to other mount points. In the guest, /apex is a symlink to /host/apex.
> On the host, /apex/com.android.runtime is a mounted loopback device:
> 
> /dev/block/loop13 on /apex/com.android.runtime type ext4 (ro,dirsync,seclabel,nodev,noatime)
> 
> This used to work prior to 724a08450f74 ("fs/9p: simplify iget to remove
> unnecessary paths") and it looks like Oleg ran into something similar
> before:
> 
>   https://lore.kernel.org/all/20240408141436.GA17022@redhat.com/
> 
> although he worked around it by driving QEMU with different options.
> 
> I can confirm that reverting the following commits gets mainline guests
> working again for me:
> 
> 	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"
> 
> Do you have any better ideas? I'm happy to test anything you might have,
> since this is 100% reproducible on my setup.

Adding the regression tracker as I've not heard anything back on this :(

#regzbot introduced: 724a08450f74

Will

Re: VFS regression with 9pfs ("Lookup would have caused loop")

[Thorsten Leemhuis]

Hi Will! Top-posting for once, to make this easily accessible to everyone.

Thx for bringing this to my attention. I had hoped that Eric might reply
and waited a bit, but that did not happen. I kind of expected that, as
he seems to be  somewhat afk, as the last mail from him on lore is from
mid-September; and in the weeks before that he did not post much either.
Hmmm. :-/

CCed Christian and Al, maybe they might be able to help directly or
indirectly somehow. If not, we likely need to get Linus involved to
decide if we want to at least temporarily revert the changes you mentioned.

Ciao, Thorsten

On 09.10.24 17:34, Will Deacon wrote:
> On Mon, Sep 23, 2024 at 11:05:08AM +0100, Will Deacon wrote:
>>
>> I'm trying to use kvmtool to run a simple guest under an Android host
>> but, for v6.9+ guest kernels, 'init' reliably fails to run from a 9pfs
>> mount because VFS emits this error:
>>
>>   | VFS: Lookup of 'com.android.runtime' in 9p 9p would have caused loop
>>
>> The host directory being shared is a little odd, as it has symlinks out
>> to other mount points. In the guest, /apex is a symlink to /host/apex.
>> On the host, /apex/com.android.runtime is a mounted loopback device:
>>
>> /dev/block/loop13 on /apex/com.android.runtime type ext4 (ro,dirsync,seclabel,nodev,noatime)
>>
>> This used to work prior to 724a08450f74 ("fs/9p: simplify iget to remove
>> unnecessary paths") and it looks like Oleg ran into something similar
>> before:
>>
>>   https://lore.kernel.org/all/20240408141436.GA17022@redhat.com/
>>
>> although he worked around it by driving QEMU with different options.
>>
>> I can confirm that reverting the following commits gets mainline guests
>> working again for me:
>>
>> 	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"
>>
>> Do you have any better ideas? I'm happy to test anything you might have,
>> since this is 100% reproducible on my setup.
> 
> Adding the regression tracker as I've not heard anything back on this :(
> #regzbot introduced: 724a08450f74

Re: VFS regression with 9pfs ("Lookup would have caused loop")

[asmadeus]

Thorsten Leemhuis wrote on Tue, Oct 15, 2024 at 08:07:10PM +0200:
> Thx for bringing this to my attention. I had hoped that Eric might reply
> and waited a bit, but that did not happen. I kind of expected that, as
> he seems to be  somewhat afk, as the last mail from him on lore is from
> mid-September; and in the weeks before that he did not post much either.
> Hmmm. :-/

Right, I had hoped he'd find time to look further into this and kept my
head in the ground, but it looks like we'll have to handle this somehow...

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)

> CCed Christian and Al, maybe they might be able to help directly or
> indirectly somehow. If not, we likely need to get Linus involved to
> decide if we want to at least temporarily revert the changes you mentioned.

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

So the question really just is do we have or can we build a workable, so
the question is can we resonable do any better, or do we just want to
live wth the old behaviour.
(Note that as far as I understand the old code isn't 100% "loop" proof
either anyway, a open(O_CREAT)/mkdir/mknod could happen to get identical
inode numbers as well, it's just less likely so folks haven't been
hitting it)


Thanks,
-- 
Dominique

Re: VFS regression with 9pfs ("Lookup would have caused loop")

[Will Deacon]

Hi Dominique,

On Wed, Oct 16, 2024 at 06:39:28PM +0900, asmadeus@codewreck.org wrote:
> Thorsten Leemhuis wrote on Tue, Oct 15, 2024 at 08:07:10PM +0200:
> > Thx for bringing this to my attention. I had hoped that Eric might reply
> > and waited a bit, but that did not happen. I kind of expected that, as
> > he seems to be  somewhat afk, as the last mail from him on lore is from
> > mid-September; and in the weeks before that he did not post much either.
> > Hmmm. :-/
> 
> Right, I had hoped he'd find time to look further into this and kept my
> head in the ground, but it looks like we'll have to handle this somehow...
> 
> 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.

> > CCed Christian and Al, maybe they might be able to help directly or
> > indirectly somehow. If not, we likely need to get Linus involved to
> > decide if we want to at least temporarily revert the changes you mentioned.
> 
> 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.

> So the question really just is do we have or can we build a workable, so
> the question is can we resonable do any better, or do we just want to
> live wth the old behaviour.
> (Note that as far as I understand the old code isn't 100% "loop" proof
> either anyway, a open(O_CREAT)/mkdir/mknod could happen to get identical
> inode numbers as well, it's just less likely so folks haven't been
> hitting it)

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.

Cheers,

Will

Re: VFS regression with 9pfs ("Lookup would have caused loop")

[Dominique Martinet]

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

Re: VFS regression with 9pfs ("Lookup would have caused loop")

[Linux regression tracking (Thorsten Leemhuis)]

On 23.10.24 01:00, Dominique Martinet wrote:
> Will Deacon wrote on Tue, Oct 22, 2024 at 04:01:49PM +0100:
>
> [...]

Dominique, thx for taking the time to take care of this.

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

Will might be the better person to ask, but since you asked me: I think
I've see a "megarevert" recently from the corner of my eye, but that
made me go "huh, that is unusual". My perception might wrong and in some
situations they might be a good idea. Not sure if this is one to to the
UAF. But that would only be relevant during a bisection or for everyone
stupid enough to backport only some of the reverts (if I understood you
right). I guess a proper patch description and a common Fixes: tag for
all four should prevent that.

Ciao, THorsten

Re: VFS regression with 9pfs ("Lookup would have caused loop")

[Will Deacon]

On Wed, Oct 23, 2024 at 08:00:04AM +0900, Dominique Martinet wrote:
> 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)

Heh, no worries. It has a funny leading space at the start which is
weirdly jarring when you read it!

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

Nice, thanks for digging into this. I'm now pretty grateful I ran into
the vfs error rather than whacky filesystem behaviours.

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

Agreed, and I'm happy to test any subsequent patches now that I've got
my Android environment up and running again.

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

I'll leave it up to you, but I'd personally do four separate reverts
because it makes the whole process entirely mechanical. I can't recall
seeing a megarevert before.

Will

Re: VFS regression with 9pfs ("Lookup would have caused loop")

[Christian Brauner]

On Tue, Oct 15, 2024 at 08:07:10PM +0200, Thorsten Leemhuis wrote:
> Hi Will! Top-posting for once, to make this easily accessible to everyone.
> 
> Thx for bringing this to my attention. I had hoped that Eric might reply
> and waited a bit, but that did not happen. I kind of expected that, as
> he seems to be  somewhat afk, as the last mail from him on lore is from
> mid-September; and in the weeks before that he did not post much either.
> Hmmm. :-/
> 
> CCed Christian and Al, maybe they might be able to help directly or
> indirectly somehow. If not, we likely need to get Linus involved to
> decide if we want to at least temporarily revert the changes you mentioned.

Sorry, I'm just getting to this thread now as I'm still out with a
fscking case of the flu.
/me reads...

> 
> Ciao, Thorsten
> 
> On 09.10.24 17:34, Will Deacon wrote:
> > On Mon, Sep 23, 2024 at 11:05:08AM +0100, Will Deacon wrote:
> >>
> >> I'm trying to use kvmtool to run a simple guest under an Android host
> >> but, for v6.9+ guest kernels, 'init' reliably fails to run from a 9pfs
> >> mount because VFS emits this error:
> >>
> >>   | VFS: Lookup of 'com.android.runtime' in 9p 9p would have caused loop
> >>
> >> The host directory being shared is a little odd, as it has symlinks out
> >> to other mount points. In the guest, /apex is a symlink to /host/apex.
> >> On the host, /apex/com.android.runtime is a mounted loopback device:
> >>
> >> /dev/block/loop13 on /apex/com.android.runtime type ext4 (ro,dirsync,seclabel,nodev,noatime)
> >>
> >> This used to work prior to 724a08450f74 ("fs/9p: simplify iget to remove
> >> unnecessary paths") and it looks like Oleg ran into something similar
> >> before:
> >>
> >>   https://lore.kernel.org/all/20240408141436.GA17022@redhat.com/
> >>
> >> although he worked around it by driving QEMU with different options.
> >>
> >> I can confirm that reverting the following commits gets mainline guests
> >> working again for me:
> >>
> >> 	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"
> >>
> >> Do you have any better ideas? I'm happy to test anything you might have,
> >> since this is 100% reproducible on my setup.
> > 
> > Adding the regression tracker as I've not heard anything back on this :(
> > #regzbot introduced: 724a08450f74

Re: VFS regression with 9pfs ("Lookup would have caused loop")

[Christian Brauner]

On Fri, Oct 25, 2024 at 05:25:47PM +0200, Christian Brauner wrote:
> On Tue, Oct 15, 2024 at 08:07:10PM +0200, Thorsten Leemhuis wrote:
> > Hi Will! Top-posting for once, to make this easily accessible to everyone.
> > 
> > Thx for bringing this to my attention. I had hoped that Eric might reply
> > and waited a bit, but that did not happen. I kind of expected that, as
> > he seems to be  somewhat afk, as the last mail from him on lore is from
> > mid-September; and in the weeks before that he did not post much either.
> > Hmmm. :-/
> > 
> > CCed Christian and Al, maybe they might be able to help directly or
> > indirectly somehow. If not, we likely need to get Linus involved to
> > decide if we want to at least temporarily revert the changes you mentioned.
> 
> Sorry, I'm just getting to this thread now as I'm still out with a
> fscking case of the flu.
> /me reads...

Ok, so this seems handled already. Great.