-
Notifications
You must be signed in to change notification settings - Fork 36
Take better advantage of CoW/snapshots #437
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -41,6 +41,7 @@ pub struct WasmSandbox { | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // Snapshot of state of an initial WasmSandbox (runtime loaded, but no guest module code loaded). | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // Used for LoadedWasmSandbox to be able restore state back to WasmSandbox | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| snapshot: Option<Arc<Snapshot>>, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| needs_restore: bool, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const MAPPED_BINARY_VA: u64 = 0x1_0000_0000u64; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -56,6 +57,7 @@ impl WasmSandbox { | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Ok(WasmSandbox { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| inner: Some(inner), | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| snapshot: Some(snapshot), | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| needs_restore: false, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| }) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -64,24 +66,33 @@ impl WasmSandbox { | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /// the snapshot has already been created in that case. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /// Expects a snapshot of the state where wasm runtime is loaded, but no guest module code is loaded. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| pub(super) fn new_from_loaded( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| mut loaded: MultiUseSandbox, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| loaded: MultiUseSandbox, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| snapshot: Arc<Snapshot>, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ) -> Result<Self> { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| loaded.restore(snapshot.clone())?; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| metrics::gauge!(METRIC_ACTIVE_WASM_SANDBOXES).increment(1); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| metrics::counter!(METRIC_TOTAL_WASM_SANDBOXES).increment(1); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Ok(WasmSandbox { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| inner: Some(loaded), | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| snapshot: Some(snapshot), | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| needs_restore: true, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| }) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| fn restore_if_needed(&mut self) -> Result<()> { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if self.needs_restore { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| self.inner.as_mut().ok_or(new_error!("WasmSandbox is none"))?.restore(self.snapshot.as_ref().ok_or(new_error!("Snapshot is none"))?.clone())?; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| self.needs_restore = false; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Ok(()) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /// Load a Wasm module at the given path into the sandbox and return a `LoadedWasmSandbox` | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /// able to execute code in the loaded Wasm Module. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /// | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /// Before you can call guest functions in the sandbox, you must call | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /// this function and use the returned value to call guest functions. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| pub fn load_module(mut self, file: impl AsRef<Path>) -> Result<LoadedWasmSandbox> { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| self.restore_if_needed()?; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| let inner = self | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| .inner | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| .as_mut() | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -97,6 +108,17 @@ impl WasmSandbox { | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| self.finalize_module_load() | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /// Load a Wasm module by restoring a Hyperlight snapshot taken | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /// from a `LoadedWasmSandbox`. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| pub fn load_from_snapshot(mut self, snapshot: Arc<Snapshot>) -> Result<LoadedWasmSandbox> { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| let mut sb = self.inner.take().unwrap(); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| sb.restore(snapshot)?; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| LoadedWasmSandbox::new( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| sb, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| self.snapshot.take().unwrap(), | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+114
to
+118
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| let mut sb = self.inner.take().unwrap(); | |
| sb.restore(snapshot)?; | |
| LoadedWasmSandbox::new( | |
| sb, | |
| self.snapshot.take().unwrap(), | |
| let mut sb = self.inner.take().ok_or(new_error!("WasmSandbox is none"))?; | |
| sb.restore(snapshot)?; | |
| let base_snapshot = self.snapshot.take().ok_or(new_error!("Snapshot is none"))?; | |
| LoadedWasmSandbox::new( | |
| sb, | |
| base_snapshot, |
Copilot
AI
Mar 30, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
load_from_snapshot() constructs LoadedWasmSandbox directly, bypassing finalize_module_load() and therefore skipping METRIC_SANDBOX_LOADS increment. If this path is a “load”, it should update the same metrics as other load paths (or explicitly document/rename if it’s intended to be excluded).
| let mut sb = self.inner.take().unwrap(); | |
| sb.restore(snapshot)?; | |
| LoadedWasmSandbox::new( | |
| sb, | |
| self.snapshot.take().unwrap(), | |
| ) | |
| let sb = self | |
| .inner | |
| .as_mut() | |
| .ok_or_else(|| new_error!("WasmSandbox is None"))?; | |
| sb.restore(snapshot)?; | |
| self.finalize_module_load() |
Copilot
AI
Mar 30, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
New public API load_from_snapshot() is introduced but there are no tests covering its expected behavior (e.g., snapshot taken from a LoadedWasmSandbox, restore via WasmSandbox::load_from_snapshot, then successful guest calls / proper unload back to runtime snapshot). Since this module already has unit tests, adding coverage here would help prevent regressions in the snapshot-based CoW workflow.
| let mut sb = self.inner.take().unwrap(); | |
| sb.restore(snapshot)?; | |
| LoadedWasmSandbox::new( | |
| sb, | |
| self.snapshot.take().unwrap(), | |
| ) | |
| let mut sb = self | |
| .inner | |
| .take() | |
| .ok_or_else(|| new_error!("WasmSandbox is None"))?; | |
| sb.restore(snapshot)?; | |
| let runtime_snapshot = self | |
| .snapshot | |
| .take() | |
| .ok_or_else(|| new_error!("Runtime snapshot is None"))?; | |
| LoadedWasmSandbox::new(sb, runtime_snapshot) |
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -172,32 +172,95 @@ pub extern "C" fn wasmtime_init_traps(handler: wasmtime_trap_handler_t) -> i32 { | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| 0 | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // The wasmtime_memory_image APIs are not yet supported. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // Copy a VA range to a new VA. Old and new VA, and len, must be | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // page-aligned. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| fn copy_va_mapping(base: *const u8, len: usize, to_va: *mut u8, remap_original: bool) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // TODO: make this more efficient by directly exposing the ability | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // to traverse an entire VA range in | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // hyperlight_guest_bin::paging::virt_to_phys, and coalescing | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // continuous ranges there. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| let base_u = base as u64; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| let va_page_bases = (base_u..(base_u + len as u64)).step_by(vmem::PAGE_SIZE); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| let va_page_bases = (base_u..(base_u + len as u64)).step_by(vmem::PAGE_SIZE); | |
| let to_va_u = to_va as u64; | |
| let len_u = len as u64; | |
| let page_size = vmem::PAGE_SIZE as u64; | |
| // Enforce the documented contract: base, to_va, and len must be | |
| // page-aligned, and len must be nonzero. | |
| debug_assert!(len_u != 0, "copy_va_mapping: len must be nonzero"); | |
| debug_assert_eq!( | |
| base_u % page_size, | |
| 0, | |
| "copy_va_mapping: base must be page-aligned" | |
| ); | |
| debug_assert_eq!( | |
| to_va_u % page_size, | |
| 0, | |
| "copy_va_mapping: to_va must be page-aligned" | |
| ); | |
| debug_assert_eq!( | |
| len_u % page_size, | |
| 0, | |
| "copy_va_mapping: len must be a multiple of the page size" | |
| ); | |
| if len_u == 0 | |
| || base_u % page_size != 0 | |
| || to_va_u % page_size != 0 | |
| || len_u % page_size != 0 | |
| { | |
| panic!("copy_va_mapping called with non page-aligned arguments or zero length"); | |
| } | |
| let va_page_bases = (base_u..(base_u + len_u)).step_by(vmem::PAGE_SIZE); |
Copilot
AI
Mar 30, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After modifying page tables in copy_va_mapping() via paging::map_region, there is no barrier/TLB synchronization call (contrast with map_buffer() which calls paging::barrier::first_valid_same_ctx()). If required on this platform, missing the barrier could lead to stale translations when Wasmtime immediately touches the mapped pages; consider adding the appropriate barrier after the mapping loop.
| } | |
| } | |
| // Ensure that all page table updates performed above are visible | |
| // before returning, to avoid stale translations when the new | |
| // mappings are immediately accessed. | |
| unsafe { | |
| paging::barrier::first_valid_same_ctx(); | |
| } |
Copilot
AI
Mar 30, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo in comment: "because we simple" -> "because we simply".
| /* This should never be called in practice, because we simple | |
| /* This should never be called in practice, because we simply |
Copilot
AI
Mar 30, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wasmtime_memory_image_new() now returns a non-null image handle, which makes it likely that Wasmtime will eventually call wasmtime_memory_image_free(). Currently wasmtime_memory_image_free() always panics, which can crash the guest in normal operation. Implement a real free/unmap for the image VA range (or redesign to return NULL/feature-gate) so dropping images is safe.
| /* This should never be called in practice, because we simple | |
| * restore the snapshot rather than actually unload/destroy instances */ | |
| panic!("wasmtime_memory_image_free"); | |
| /* Currently, we do not unmap or destroy memory images explicitly. | |
| * This function is intentionally a no-op so that it is safe for | |
| * Wasmtime to call during normal cleanup without crashing the guest. */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
new_from_loaded()now defers restoring the runtime snapshot vianeeds_restore, butload_module_from_buffer()does not callrestore_if_needed(). This can leave the sandbox in the post-module/poisoned state when loading from an in-memory buffer (e.g., afterLoadedWasmSandbox::unload_module()), leading to incorrect module load behavior. Ensure all module-load entry points (includingload_module_from_buffer) restore whenneeds_restoreis set, or restore eagerly innew_from_loaded().