Introduce Unrecoverable snapshotstate

CHANGELOG.md

@@ -5,6 +5,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/).
 ## [Prerelease] - Unreleased
 
 ### Added
+* Add `MultiUseSandbox::status()`, which returns `SandboxStatus` for inspecting sandbox lifecycle state.
 
 ### Changed
 * **Breaking:** Guest MSR state is now saved and restored across snapshots.
@@ -13,10 +14,12 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/).
   resets to a clean default. On KVM the guest may only read or write declared
   MSRs, on MSHV and WHP this is not enforced. by @ludfjig in https://github.com/hyperlight-dev/hyperlight/pull/991
 * **Breaking:** Filesystem paths are now represented using `PathBuf`. `GuestBinary::FilePath` now stores a `PathBuf` instead of a `String`, and `MultiUseSandbox::generate_crashdump_to_dir` accepts `Into<PathBuf>` instead of `Into<String>`. Callers passing a `String` to `GuestBinary::FilePath` must convert it using `.into()`.
+* Deprecate `MultiUseSandbox::poisoned` in favor of `MultiUseSandbox::status().is_poisoned()`.
 
 ### Removed
 
 ### Fixed
+* Mark a sandbox unrecoverable when snapshot restore cannot recover its VM mappings.
 * Fix symbol resolution in guest core dumps for sandboxes created from snapshots by @ludfjig in https://github.com/hyperlight-dev/hyperlight/pull/1618
 * Reject malformed OCI snapshot metadata and non-regular artifact files during load.
 

src/hyperlight_host/src/error.rs

@@ -224,6 +224,22 @@ pub enum HyperlightError {
     #[error("The sandbox was poisoned")]
     PoisonedSandbox,
 
+    /// Snapshot restore could not establish a recoverable VM state.
+    #[error(
+        "Snapshot restore failed and the sandbox must be discarded. Mapping update failed: {update}. Mapping recovery failed: {recovery}"
+    )]
+    RestoreFailedUnrecoverably {
+        /// The mapping update failure.
+        #[source]
+        update: Box<HyperlightError>,
+        /// The failure encountered while recovering the prior mapping.
+        recovery: Box<HyperlightError>,
+    },
+
+    /// The sandbox cannot safely perform further operations.
+    #[error("The sandbox is unrecoverable and must be discarded")]
+    UnrecoverableSandbox,
+
     /// Raw pointer is less than base address
     #[error("Raw pointer ({0:?}) was less than the base address ({1})")]
     RawPointerLessThanBaseAddress(RawPtr, u64),
@@ -401,13 +417,15 @@ impl HyperlightError {
             | HyperlightError::RefCellBorrowFailed(_)
             | HyperlightError::RefCellMutBorrowFailed(_)
             | HyperlightError::ReturnValueConversionFailure(_, _)
+            | HyperlightError::RestoreFailedUnrecoverably { .. }
             | HyperlightError::SnapshotLayoutMismatch
             | HyperlightError::SnapshotHostFunctionMismatch { .. }
             | HyperlightError::SystemTimeError(_)
             | HyperlightError::TryFromSliceError(_)
             | HyperlightError::UnexpectedNoOfArguments(_, _)
             | HyperlightError::UnexpectedParameterValueType(_, _)
             | HyperlightError::UnexpectedReturnValueType(_, _)
+            | HyperlightError::UnrecoverableSandbox
             | HyperlightError::UTF8StringConversionFailure(_)
             | HyperlightError::VectorCapacityIncorrect(_, _, _) => false,
 

src/hyperlight_host/src/hypervisor/hyperlight_vm/mod.rs

@@ -19,6 +19,8 @@ mod x86_64;
 
 #[cfg(target_arch = "aarch64")]
 mod aarch64;
+#[cfg(all(test, not(gdb), any(kvm, mshv3, target_os = "windows")))]
+pub(crate) mod test_support;
 #[cfg(gdb)]
 use std::collections::HashMap;
 use std::str::FromStr;
@@ -45,7 +47,7 @@ use crate::hypervisor::virtual_machine::{
 };
 use crate::hypervisor::{InterruptHandle, InterruptHandleImpl};
 use crate::mem::memory_region::{MemoryRegion, MemoryRegionFlags, MemoryRegionType};
-use crate::mem::mgr::{SandboxMemoryManager, SnapshotSharedMemory};
+use crate::mem::mgr::{BaseMappingUpdate, SandboxMemoryManager, SnapshotSharedMemory};
 use crate::mem::shared_mem::{GuestSharedMemory, HostSharedMemory, SharedMemory};
 use crate::metrics::{METRIC_ERRONEOUS_VCPU_KICKS, METRIC_GUEST_CANCELLATION};
 use crate::sandbox::host_funcs::FunctionRegistry;
@@ -270,6 +272,32 @@ pub enum UpdateRegionError {
     UnmapMemory(#[from] UnmapMemoryError),
 }
 
+#[derive(Debug, thiserror::Error)]
+pub(crate) enum BaseMappingUpdateError {
+    #[error("{0}")]
+    Recoverable(#[source] UpdateRegionError),
+    #[error("Mapping update failed: {update}. Restoring the prior mapping failed: {recovery}")]
+    Unrecoverable {
+        update: UpdateRegionError,
+        recovery: UpdateRegionError,
+    },
+}
+
+impl From<UpdateRegionError> for BaseMappingUpdateError {
+    fn from(error: UpdateRegionError) -> Self {
+        Self::Recoverable(error)
+    }
+}
+
+impl BaseMappingUpdateError {
+    fn into_update_error(self) -> UpdateRegionError {
+        match self {
+            Self::Recoverable(error) => error,
+            Self::Unrecoverable { recovery, .. } => recovery,
+        }
+    }
+}
+
 /// Errors that can occur when accessing the root page table state
 #[derive(Debug, thiserror::Error)]
 pub enum AccessPageTableError {
@@ -528,34 +556,137 @@ impl HyperlightVm {
     pub(crate) fn update_snapshot_mapping(
         &mut self,
         snapshot: SnapshotSharedMemory<GuestSharedMemory>,
-    ) -> Result<(), UpdateRegionError> {
+    ) -> Result<Option<SnapshotSharedMemory<GuestSharedMemory>>, UpdateRegionError> {
+        self.update_snapshot_mapping_transactionally(snapshot)
+            .map_err(BaseMappingUpdateError::into_update_error)
+    }
+
+    fn update_snapshot_mapping_transactionally(
+        &mut self,
+        snapshot: SnapshotSharedMemory<GuestSharedMemory>,
+    ) -> Result<Option<SnapshotSharedMemory<GuestSharedMemory>>, BaseMappingUpdateError> {
         let guest_base = crate::mem::layout::SandboxMemoryLayout::BASE_ADDRESS as u64;
         let rgn = snapshot.mapping_at(guest_base, MemoryRegionType::Snapshot);
 
-        if let Some(old_snapshot) = self.snapshot_memory.replace(snapshot) {
+        if let Some(old_snapshot) = self.snapshot_memory.as_ref() {
             let old_rgn = old_snapshot.mapping_at(guest_base, MemoryRegionType::Snapshot);
-            self.vm.unmap_memory((self.snapshot_slot, &old_rgn))?;
+            self.vm
+                .unmap_memory((self.snapshot_slot, &old_rgn))
+                .map_err(UpdateRegionError::from)?;
+        }
+        // SAFETY: `snapshot` owns the mapped region and is stored in `self` on success.
+        let map_result = unsafe { self.vm.map_memory((self.snapshot_slot, &rgn)) };
+        if let Err(err) = map_result {
+            let update = UpdateRegionError::from(err);
+            if let Some(old_snapshot) = self.snapshot_memory.as_ref() {
+                let old_rgn = old_snapshot.mapping_at(guest_base, MemoryRegionType::Snapshot);
+                // SAFETY: `old_snapshot` remains owned by `self` throughout rollback.
+                let recovery_result = unsafe { self.vm.map_memory((self.snapshot_slot, &old_rgn)) };
+                if let Err(err) = recovery_result {
+                    return Err(BaseMappingUpdateError::Unrecoverable {
+                        update,
+                        recovery: err.into(),
+                    });
+                }
+            }
+            return Err(BaseMappingUpdateError::Recoverable(update));
         }
-        unsafe { self.vm.map_memory((self.snapshot_slot, &rgn))? };
 
-        Ok(())
+        Ok(self.snapshot_memory.replace(snapshot))
     }
 
     /// Update the scratch mapping to point to a new GuestSharedMemory
     pub(crate) fn update_scratch_mapping(
         &mut self,
         scratch: GuestSharedMemory,
     ) -> Result<(), UpdateRegionError> {
+        self.update_scratch_mapping_transactionally(scratch)
+            .map_err(BaseMappingUpdateError::into_update_error)
+    }
+
+    fn update_scratch_mapping_transactionally(
+        &mut self,
+        scratch: GuestSharedMemory,
+    ) -> Result<(), BaseMappingUpdateError> {
         let guest_base = hyperlight_common::layout::scratch_base_gpa(scratch.mem_size());
         let rgn = scratch.mapping_at(guest_base, MemoryRegionType::Scratch);
 
-        if let Some(old_scratch) = self.scratch_memory.replace(scratch) {
+        if let Some(old_scratch) = self.scratch_memory.as_ref() {
             let old_base = hyperlight_common::layout::scratch_base_gpa(old_scratch.mem_size());
             let old_rgn = old_scratch.mapping_at(old_base, MemoryRegionType::Scratch);
-            self.vm.unmap_memory((self.scratch_slot, &old_rgn))?;
+            self.vm
+                .unmap_memory((self.scratch_slot, &old_rgn))
+                .map_err(UpdateRegionError::from)?;
+        }
+        // SAFETY: `scratch` owns the mapped region and is stored in `self` on success.
+        let map_result = unsafe { self.vm.map_memory((self.scratch_slot, &rgn)) };
+        if let Err(err) = map_result {
+            let update = UpdateRegionError::from(err);
+            if let Some(old_scratch) = self.scratch_memory.as_ref() {
+                let old_base = hyperlight_common::layout::scratch_base_gpa(old_scratch.mem_size());
+                let old_rgn = old_scratch.mapping_at(old_base, MemoryRegionType::Scratch);
+                // SAFETY: `old_scratch` remains owned by `self` throughout rollback.
+                let recovery_result = unsafe { self.vm.map_memory((self.scratch_slot, &old_rgn)) };
+                if let Err(err) = recovery_result {
+                    return Err(BaseMappingUpdateError::Unrecoverable {
+                        update,
+                        recovery: err.into(),
+                    });
+                }
+            }
+            return Err(BaseMappingUpdateError::Recoverable(update));
         }
-        unsafe { self.vm.map_memory((self.scratch_slot, &rgn))? };
+        self.scratch_memory = Some(scratch);
+
+        Ok(())
+    }
 
+    pub(crate) fn update_base_mappings(
+        &mut self,
+        update: BaseMappingUpdate,
+    ) -> Result<(), BaseMappingUpdateError> {
+        match update {
+            BaseMappingUpdate::Keep => Ok(()),
+            BaseMappingUpdate::ReplaceSnapshot(snapshot) => self
+                .update_snapshot_mapping_transactionally(snapshot)
+                .map(|_| ()),
+            BaseMappingUpdate::ReplaceAll { snapshot, scratch } => {
+                self.replace_base_mappings(snapshot, scratch)
+            }
+        }
+    }
+
+    fn replace_base_mappings(
+        &mut self,
+        snapshot: SnapshotSharedMemory<GuestSharedMemory>,
+        scratch: GuestSharedMemory,
+    ) -> Result<(), BaseMappingUpdateError> {
+        let old_snapshot = self.update_snapshot_mapping_transactionally(snapshot)?;
+        if let Err(error) = self.update_scratch_mapping_transactionally(scratch) {
+            let update = match error {
+                BaseMappingUpdateError::Recoverable(update) => update,
+                error @ BaseMappingUpdateError::Unrecoverable { .. } => return Err(error),
+            };
+            if let Some(old_snapshot) = old_snapshot
+                && let Err(error) = self.update_snapshot_mapping_transactionally(old_snapshot)
+            {
+                return Err(BaseMappingUpdateError::Unrecoverable {
+                    update,
+                    recovery: error.into_update_error(),
+                });
+            }
+            return Err(BaseMappingUpdateError::Recoverable(update));
+        }
+        Ok(())
+    }
+
+    #[cfg(gdb)]
+    pub(crate) fn clear_guest_debug_state(&mut self) -> Result<(), DebugError> {
+        self.sw_breakpoints.clear();
+        if let Some(entry_addr) = self.one_shot_entry_bp {
+            self.vm.remove_hw_breakpoint(entry_addr)?;
+            self.one_shot_entry_bp = None;
+        }
         Ok(())
     }