Possible race on the hypervisor page table (ptable in src/mm.c)?

[Jeehoon Kang]

Hi all,

I think there may be a race condition on the hypervisor page table (`ptable` in src/mm.c).

In api.c, `api_vm_configure()` calls `mm_identity_map()`, which modifies the hypervisor page table.

It seems multiple threads may call `api_vm_configure()` concurrently, thereby modifying the hypervisor page table at the same time.

But the hypervisor page table doesn't seem like supporting concurrency, so I guess there may be a race condition.

What do you think of it?

[Andrew Scull]

Thanks for raising the concern. In this case we intend the page table to be protected by the lock the on the VM struct which is being locked earlier on in `api_vm_configure()`. These relationships aren't particularly obvious but in general the items in the VM struct are protected its lock and the vCPU struct is protected by its lock too. The locking is fairly coarse grained at the moment to try and keep simplicity before concerns of performance leak in.

Let me know if that makes sense or you notice that it isn't being respected anywhere!

--
You received this message because you are subscribed to the Google Groups "Hafnium" group.
To unsubscribe from this group and stop receiving emails from it, send an email to hafnium-discu...@googlegroups.com.
To post to this group, send email to hafnium...@googlegroups.com.
To view this discussion on the web, visit https://groups.google.com/d/msgid/hafnium-discuss/c8468511-9ab4-4f81-994a-8e46a740778b%40googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[Jeehoon Kang]

Dear Andrew,

Thanks for prompt reply.

I still think there's a race, because multiple VMs may call `api_vm_configure()` at the same time, thereby concurrently accessing the hypervisor page table via `mm_identity_map().  So in my opinion, even though each VM is protected by its own lock, the hypervisor page table is not protected by VM or VCPU locks.

Thanks,

Jeehoon

On Tuesday, May 21, 2019 at 6:33:07 PM UTC+9, Andrew Scull wrote:

Thanks for raising the concern. In this case we intend the page table to be protected by the lock the on the VM struct which is being locked earlier on in `api_vm_configure()`. These relationships aren't particularly obvious but in general the items in the VM struct are protected its lock and the vCPU struct is protected by its lock too. The locking is fairly coarse grained at the moment to try and keep simplicity before concerns of performance leak in.

Let me know if that makes sense or you notice that it isn't being respected anywhere!

On Tue, 21 May 2019 at 04:55, Jeehoon Kang <jeeho...@cp.kaist.ac.kr> wrote:

Hi all,

I think there may be a race condition on the hypervisor page table (`ptable` in src/mm.c).

In api.c, `api_vm_configure()` calls `mm_identity_map()`, which modifies the hypervisor page table.

It seems multiple threads may call `api_vm_configure()` concurrently, thereby modifying the hypervisor page table at the same time.

But the hypervisor page table doesn't seem like supporting concurrency, so I guess there may be a race condition.

What do you think of it?

--
You received this message because you are subscribed to the Google Groups "Hafnium" group.

To unsubscribe from this group and stop receiving emails from it, send an email to hafnium...@googlegroups.com.

[Andrew Scull]

Right, sorry, I was thinking of the wrong thing. This is problematic and there are a few other places that could fall fowl. I'm going to create a bug internally so we can address this problem fully.

To unsubscribe from this group and stop receiving emails from it, send an email to hafnium-discu...@googlegroups.com.

To post to this group, send email to hafnium...@googlegroups.com.

To view this discussion on the web, visit https://groups.google.com/d/msgid/hafnium-discuss/9d9a62f5-9d8f-45c7-b459-ad01a8153fc6%40googlegroups.com.

[Jeehoon Kang]

Dear Andrew,

Thank you again for your prompt reply.

I wonder how do you expect the fix will look like.  Maybe we can turn page table to a nonblocking data structure in a future, but I guess a simple and immediate fix will be just protecting the hypervisor page table with a spinlock, right?

(I'm asking this because I want to do the same thing to my Rust port.)

Thanks,

Jeehoon

On Tuesday, May 21, 2019 at 9:05:04 PM UTC+9, Andrew Scull wrote:

Right, sorry, I was thinking of the wrong thing. This is problematic and there are a few other places that could fall fowl. I'm going to create a bug internally so we can address this problem fully.

[Andrew Scull]

Yes, I'm expecting it will be a spinlock for the hypervisor page table. It seems we'll want to be able to lock the table for multiple calls to map atomically so we might end up with a `struct vm_locked` style thing to emulate the more strongly typed approach of a language like rust. We don't expect Hypervisor page table updates to occur very frequently so performance here isn't a number one concern at the moment. I won't be able to look at this properly very soon though.

To unsubscribe from this group and stop receiving emails from it, send an email to hafnium-discu...@googlegroups.com.

To post to this group, send email to hafnium...@googlegroups.com.

To view this discussion on the web, visit https://groups.google.com/d/msgid/hafnium-discuss/f2513d04-d35b-4cc4-92e1-43d9c2652391%40googlegroups.com.