Relax layout requirements for MultiUseSandbox::restore - #1728
Conversation
2074b7b to
2150200
Compare
afdbb20 to
d4e7519
Compare
d4e7519 to
f0fa0c2
Compare
3518a13 to
a110461
Compare
a110461 to
8297af8
Compare
There was a problem hiding this comment.
Pull request overview
This PR relaxes MultiUseSandbox::restore compatibility rules so snapshots can be restored across different guest binaries and sandbox memory layouts, as long as the target sandbox’s registered host functions satisfy the snapshot’s requirements.
Changes:
- Remove layout-compatibility validation from snapshot restore, and drop the
SnapshotLayoutMismatcherror path. - Update restore to fully reset VM mappings during restore and clear crashdump state that would otherwise refer to the pre-restore guest.
- Add/expand tests covering cross-guest restore (WIT/Rust/C) and cross-layout restore, including persisted snapshots with non-default layouts.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/hyperlight_host/tests/wit_test.rs | Adds integration tests restoring WIT snapshots over Rust/C guests and vice-versa. |
| src/hyperlight_host/src/sandbox/snapshot/mod.rs | Removes Snapshot::validate_compatibility (layout + host funcs) helper. |
| src/hyperlight_host/src/sandbox/snapshot/file_tests.rs | Adds persisted non-default layout test; updates restore semantics comment. |
| src/hyperlight_host/src/sandbox/initialized_multi_use.rs | Implements relaxed restore (host-func-only validation) and remaps/unmaps regions during restore; updates docs/tests. |
| src/hyperlight_host/src/mem/layout.rs | Removes SandboxMemoryLayout::is_compatible_with and its tests. |
| src/hyperlight_host/src/hypervisor/hyperlight_vm/mod.rs | Adds clear_crashdump_binary_path for post-restore crashdump correctness. |
| src/hyperlight_host/src/error.rs | Removes HyperlightError::SnapshotLayoutMismatch variant. |
| CHANGELOG.md | Notes the new restore flexibility. |
Suppressed comments (1)
src/hyperlight_host/src/sandbox/initialized_multi_use.rs:469
restorenow unmaps all VM regions returned byself.vm.get_mapped_regions()before applying the snapshot. The rustdoc forrestoredoes not mention that mappings created viamap_region/map_file_cowwill be removed, which is a user-visible behavior change and can surprise callers relying on those mappings persisting across restores.
/// signature). Extras on the sandbox are allowed. The registry
/// itself is left unchanged. A mismatch returns
/// [`SnapshotHostFunctionMismatch`](crate::HyperlightError::SnapshotHostFunctionMismatch)
/// carrying the missing names and signature differences.
///
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
8297af8 to
5edde4f
Compare
5edde4f to
2c77eff
Compare
1313477 to
d0c4cf0
Compare
d0c4cf0 to
900796a
Compare
900796a to
715d187
Compare
The merge-base changed after approval.
715d187 to
09d6fda
Compare
09d6fda to
e618cc9
Compare
| for region in ¤t_regions { | ||
| self.vm | ||
| .unmap_region(region) | ||
| .map_err(HyperlightVmError::UnmapRegion)?; |
There was a problem hiding this comment.
Does this also need to make the sandbox unrecoverable if it fails? Or is it intended that this just re-poisoning the sandbox and letting the user retry is fine? I would worry a little that an error here is likely to be about the health of the system as a whole, and I feel like this encourages a retry loop while sandbox.status().is_poisoned() { sandbox.restore(...); } which might end up spinning.
There was a problem hiding this comment.
My idea was that it shouldn't make it Unrecoverable because it can be retried without risk and doesn't leave sandbox in a bad state.
And yeah I worry that most unexpected hypervisor error are likely about the system as a whole... Unsure if there's something better we can do here though
|
|
||
| self.mem_mgr | ||
| .request_libc_rng_reseed(rand::random::<u32>())?; | ||
| self.pt_root_finder = None; |
There was a problem hiding this comment.
Until we properly add the root finder to the snapshot, I think it is defensible to leave it in here and say "the user should write a root finder which works with any guest that they load", since (a) most users probably only use 1 or 2 different "base" guests and (b) the root finder has the opportunity to inspect the guest state and dispatch on what guest it thinks it is.
Signed-off-by: Ludvig Liljenberg <4257730+ludfjig@users.noreply.github.com>
Signed-off-by: Ludvig Liljenberg <4257730+ludfjig@users.noreply.github.com>
Signed-off-by: Ludvig Liljenberg <4257730+ludfjig@users.noreply.github.com>
Signed-off-by: Ludvig Liljenberg <4257730+ludfjig@users.noreply.github.com>
5e74093 to
a537f79
Compare
This makes it possible to restore snapshots from different guest binaries and memory layouts into a sandbox, provided the registered host functions are compatible.
closes #1592