From 262038010624853e9becbd0d27b48fd0d0e37e3d Mon Sep 17 00:00:00 2001 From: Quentin Perret Date: Thu, 2 Dec 2021 09:48:43 +0000 Subject: [PATCH] Revert "vm-memory: mlock2(MLOCK_ONFAULT) guest memory for protected VMs" This reverts commit b975546c3ffa05a65e6b725d39baad593cbd3052. Reason for revert: mlock is insufficient to prevent migration/compacting of guest memory, and therefore pKVM has been modified to perform pinning in the kernel, making the mlock call superfluous. Original change's description: > vm-memory: mlock2(MLOCK_ONFAULT) guest memory for protected VMs > > By default, the memory of a protected VM is inaccessible to the host > and crosvm. Consequently, attempts to access guest memory are fatal and > must be avoided in order for the guest to run. > > Mlock guest pages as they are faulted in for protected VMs, ensuring > that the host doesn't try to age or swap them out as a result of memory > pressure. > > Bug: b:204298056 > Test: cargo test on x86 and arm64 > Cc: Quentin Perret > Cc: Andrew Walbran > Signed-off-by: Will Deacon > Change-Id: I618ec1e8b1136a47a8b3ef563e45bc41d75ab517 > Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform/crosvm/+/3257689 > Tested-by: kokoro > Reviewed-by: Chirantan Ekbote Bug: b:204298056 Change-Id: Ibdcc579805c47adf35412b732829c074ce038471 Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform/crosvm/+/3310884 Tested-by: kokoro Commit-Queue: Quentin Perret Auto-Submit: Quentin Perret Reviewed-by: Will Deacon Reviewed-by: Andrew Walbran Reviewed-by: Chirantan Ekbote --- common/base/src/mmap.rs | 4 ---- common/sys_util/src/mmap.rs | 19 ------------------- src/linux.rs | 7 +------ src/plugin/mod.rs | 7 ++----- vm_memory/src/guest_memory.rs | 18 +++--------------- 5 files changed, 6 insertions(+), 49 deletions(-) diff --git a/common/base/src/mmap.rs b/common/base/src/mmap.rs index 00e8a7c6a6..16444fb054 100644 --- a/common/base/src/mmap.rs +++ b/common/base/src/mmap.rs @@ -42,10 +42,6 @@ impl MemoryMapping { self.mapping.use_hugepages() } - pub fn mlock_on_fault(&self) -> Result<()> { - self.mapping.mlock_on_fault() - } - pub fn read_to_memory( &self, mem_offset: usize, diff --git a/common/sys_util/src/mmap.rs b/common/sys_util/src/mmap.rs index 67ca7fc981..3db2f0b715 100644 --- a/common/sys_util/src/mmap.rs +++ b/common/sys_util/src/mmap.rs @@ -19,8 +19,6 @@ use data_model::DataInit; use crate::{errno, pagesize}; -const MLOCK_ONFAULT: libc::c_int = 1; - #[sorted] #[derive(Debug, thiserror::Error)] pub enum Error { @@ -413,23 +411,6 @@ impl MemoryMapping { } } - /// Mlock the guest pages as they are faulted in - pub fn mlock_on_fault(&self) -> Result<()> { - let ret = unsafe { - // TODO: Switch to libc::mlock2 once https://github.com/rust-lang/libc/pull/2525 lands - libc::syscall( - libc::SYS_mlock2, - self.as_ptr() as *mut libc::c_void, - self.size(), - MLOCK_ONFAULT, - ) - }; - if ret == -1 { - return Err(Error::SystemCallFailed(errno::Error::last())); - } - Ok(()) - } - /// Calls msync with MS_SYNC on the mapping. pub fn msync(&self) -> Result<()> { // This is safe since we use the exact address and length of a known diff --git a/src/linux.rs b/src/linux.rs index be59e2ee65..644e90396c 100644 --- a/src/linux.rs +++ b/src/linux.rs @@ -2450,12 +2450,7 @@ pub fn run_config(cfg: Config) -> Result<()> { if components.hugepages { mem_policy |= MemoryPolicy::USE_HUGEPAGES; } - if components.protected_vm == ProtectionType::Protected { - mem_policy |= MemoryPolicy::MLOCK_ON_FAULT; - } - guest_mem - .set_memory_policy(mem_policy) - .context("failed to set guest memory policy")?; + guest_mem.set_memory_policy(mem_policy); let kvm = Kvm::new_with_path(&cfg.kvm_device_path).context("failed to create kvm")?; let vm = KvmVm::new(&kvm, guest_mem).context("failed to create vm")?; let vm_clone = vm.try_clone().context("failed to clone vm")?; diff --git a/src/plugin/mod.rs b/src/plugin/mod.rs index de43141062..79880baa45 100644 --- a/src/plugin/mod.rs +++ b/src/plugin/mod.rs @@ -35,7 +35,7 @@ use base::{ use kvm::{Cap, Datamatch, IoeventAddress, Kvm, Vcpu, VcpuExit, Vm}; use minijail::{self, Minijail}; use net_util::{Error as TapError, Tap, TapT}; -use vm_memory::{GuestMemory, GuestMemoryError, MemoryPolicy}; +use vm_memory::{GuestMemory, MemoryPolicy}; use self::process::*; use self::vcpu::*; @@ -138,8 +138,6 @@ pub enum Error { RootNotDir, #[error("failed to set gidmap for jail: {0}")] SetGidMap(minijail::Error), - #[error("failed to set guest memory policy: {0}")] - SetMemoryPolicy(GuestMemoryError), #[error("failed to set uidmap for jail: {0}")] SetUidMap(minijail::Error), #[error("process {pid} died with signal {signo}, status {status}, and code {code}")] @@ -697,8 +695,7 @@ pub fn run_config(cfg: Config) -> Result<()> { if cfg.hugepages { mem_policy |= MemoryPolicy::USE_HUGEPAGES; } - mem.set_memory_policy(mem_policy) - .map_err(Error::SetMemoryPolicy)?; + mem.set_memory_policy(mem_policy); let kvm = Kvm::new_with_path(&cfg.kvm_device_path).map_err(Error::CreateKvm)?; let mut vm = Vm::new(&kvm, mem).map_err(Error::CreateVm)?; vm.create_irq_chip().map_err(Error::CreateIrqChip)?; diff --git a/vm_memory/src/guest_memory.rs b/vm_memory/src/guest_memory.rs index e65f71075c..47c3ba4fd1 100644 --- a/vm_memory/src/guest_memory.rs +++ b/vm_memory/src/guest_memory.rs @@ -41,8 +41,6 @@ pub enum Error { MemoryAddSealsFailed(SysError), #[error("failed to create shm region")] MemoryCreationFailed(SysError), - #[error("failed to lock {0} bytes of guest memory: {1}")] - MemoryLockingFailed(usize, MmapError), #[error("failed to map guest memory: {0}")] MemoryMappingFailed(MmapError), #[error("shm regions must be page aligned")] @@ -66,7 +64,6 @@ pub type Result = result::Result; bitflags! { pub struct MemoryPolicy: u32 { const USE_HUGEPAGES = 1; - const MLOCK_ON_FAULT = 2; } } @@ -344,25 +341,16 @@ impl GuestMemory { } /// Handles guest memory policy hints/advices. - pub fn set_memory_policy(&self, mem_policy: MemoryPolicy) -> Result<()> { - for (_, region) in self.regions.iter().enumerate() { - if mem_policy.contains(MemoryPolicy::USE_HUGEPAGES) { + pub fn set_memory_policy(&self, mem_policy: MemoryPolicy) { + if mem_policy.contains(MemoryPolicy::USE_HUGEPAGES) { + for (_, region) in self.regions.iter().enumerate() { let ret = region.mapping.use_hugepages(); if let Err(err) = ret { println!("Failed to enable HUGEPAGE for mapping {}", err); } } - - if mem_policy.contains(MemoryPolicy::MLOCK_ON_FAULT) { - region - .mapping - .mlock_on_fault() - .map_err(|e| Error::MemoryLockingFailed(region.mapping.size(), e))?; - } } - - Ok(()) } /// Perform the specified action on each region's addresses.