underhill_mem: add RCU implementation to synchronize bitmap changes #1383

jstarks · 2025-05-19T23:25:01Z

When guest memory page protections are changed (e.g., pages are transitioned between shared and private), we need to flush concurrent accesses to those pages by the paravisor before updating the page state in hardware. Otherwise, faults or cross-VTL data leaks may occur.

Add this synchronization as cheaply as we can: add a simple RCU (Read-Copy-Update) mechanism that allows threads accessing guest memory to cheaply synchronize with threads mutating the page access bitmaps. Use the membarrier() syscall on Linux to allow readers to operate without memory barriers, shifting the expensive to the (infrequent) bitmap update paths.

Only enable this mechanism in OpenHCL, since other environments do not rely on bitmap-based guest memory access controls.

When guest memory page protections are changed (e.g., pages are transitioned between shared and private), we need to flush concurrent accesses to those pages by the paravisor before updating the page state in hardware. Otherwise, faults or cross-VTL data leaks may occur. Add this synchronization as cheaply as we can: add a simple RCU (Read-Copy-Update) mechanism that allows threads accessing guest memory to cheaply synchronize with threads mutating the page access bitmaps. Use the membarrier() syscall on Linux to allow readers to operate without memory barriers, shifting the expensive to the (infrequent) bitmap update paths. Only enable this mechanism in OpenHCL, since other environments do not rely on bitmap-based guest memory access controls.

vm/vmcore/guestmem/fuzz/Cargo.toml

smalis-msft · 2025-05-20T15:13:45Z

vm/vmcore/guestmem/src/lib.rs

@@ -328,7 +327,12 @@ pub unsafe trait GuestMemoryAccess: 'static + Send + Sync {
    /// fails, then the associated `*_fallback` routine is called to handle the
    /// error.
    ///
-    /// TODO: add a synchronization scheme.
+    /// Bitmap checks are performed under the [`minircu::global()`] RCU domain,


This feels really easy to forget. Is there some way we could do this automatically with a drop guard or something?

No. The bitmap is controlled completely by the implementation of GuestMemoryAccess.

And you don't have to flush after every change, just changes that reduce access. And you probably want to try to batch things. So I don't think there's much we can do here.

What would be the implications of a caller reducing access and forgetting to sync?

There might be callers accessing memory using the old permissions for a short period. Which is probably never OK.

All the more reason we should try to make this harder to forget then. Is there really no way we could create some sort of write guard or drop guard or something around the relevent methods? Could we have a way to err on the side of oversyncing with some way to opt-out when it's not needed instead?

I think to do that, we'd probably have to move all the bitmap manipulation code into this crate. We'd still want to let the implementor control the allocation of the bitmap, though, since we might want it to come from non-kernel-managed memory, etc. And we'd have to create some guards for batched updates, yes. The guard could track if you actually restricted a bitmap and only flush the RCU in that case (and provide some kind of "I know what I'm doing" override).

I'd be open to such a change, but I think it's beyond the scope of what I want to do here.

Worth filing an issue on yourself to track?

We similarly have to remember to call run before trying to read the bitmap, right? Since it's quiesced on run_vp.

vm/vmcore/guestmem/src/lib.rs

smalis-msft · 2025-05-20T15:15:56Z

vm/vmcore/guestmem/src/lib.rs

                    }
                }
            }
-        }
+        };
+        // If the `rcu` feature is enabled, run the function in an RCU critical


Suggested change

// If the `rcu` feature is enabled, run the function in an RCU critical

// If the `bitmap` feature is enabled, run the function in an RCU critical

support/minircu/src/lib.rs

smalis-msft · 2025-05-20T15:24:19Z

support/minircu/src/lib.rs

+}
+
+/// A pointer representing a valid reference to a value.
+struct TlsRef<T>(*const T);


There should probably be some sort of bound on this type, as otherwise it could accidentally point to anything and the SAFETY comments might not hold.

I don't see a way to do that.

The only type this is used to point to currently is an AtomicU64 right? Can we just make a wrapper type for that field and have TlsRef not be generic?

I don't think this is any more or less safe as a concrete type.

I guess, it makes me uneasy that there's absolutely nothing backing up or asserting the safety comment though.

jstarks requested review from a team as code owners May 19, 2025 23:25

jstarks requested a review from sluck-msft May 19, 2025 23:25

jstarks added 3 commits May 19, 2025 23:47

clarify

f8839f8

fmt

fed9f73

tests

bb4d5dc