From 42b3541dcbbc4cecea631c2dbfbaf2b1a42d8305 Mon Sep 17 00:00:00 2001 From: Wang Siyuan Date: Sun, 17 Nov 2024 15:22:06 +0000 Subject: [PATCH] Fix releasing lock during the creation of `VmMapping` --- kernel/src/vm/vmar/dyn_cap.rs | 2 +- kernel/src/vm/vmar/mod.rs | 284 +++++++++++++++++++++++++------ kernel/src/vm/vmar/static_cap.rs | 2 +- kernel/src/vm/vmar/vm_mapping.rs | 259 ++-------------------------- 4 files changed, 247 insertions(+), 300 deletions(-) diff --git a/kernel/src/vm/vmar/dyn_cap.rs b/kernel/src/vm/vmar/dyn_cap.rs index 8ef02ca6a..7cc6a8150 100644 --- a/kernel/src/vm/vmar/dyn_cap.rs +++ b/kernel/src/vm/vmar/dyn_cap.rs @@ -4,7 +4,7 @@ use core::ops::Range; use aster_rights::Rights; -use super::{vm_mapping::VmarMapOptions, VmPerms, Vmar, VmarRightsOp, Vmar_}; +use super::{VmPerms, Vmar, VmarMapOptions, VmarRightsOp, Vmar_}; use crate::{ prelude::*, thread::exception::PageFaultInfo, vm::page_fault_handler::PageFaultHandler, }; diff --git a/kernel/src/vm/vmar/mod.rs b/kernel/src/vm/vmar/mod.rs index f5264947a..6c1d2ea0b 100644 --- a/kernel/src/vm/vmar/mod.rs +++ b/kernel/src/vm/vmar/mod.rs @@ -7,7 +7,7 @@ mod interval_set; mod static_cap; pub mod vm_mapping; -use core::ops::Range; +use core::{num::NonZeroUsize, ops::Range}; use align_ext::AlignExt; use aster_rights::Rights; @@ -18,13 +18,16 @@ use ostd::{ use self::{ interval_set::{Interval, IntervalSet}, - vm_mapping::VmMapping, + vm_mapping::{MappedVmo, VmMapping}, }; use super::page_fault_handler::PageFaultHandler; use crate::{ prelude::*, thread::exception::{handle_page_fault_from_vm_space, PageFaultInfo}, - vm::perms::VmPerms, + vm::{ + perms::VmPerms, + vmo::{Vmo, VmoRightsOp}, + }, }; /// Virtual Memory Address Regions (VMARs) are a type of capability that manages @@ -364,61 +367,11 @@ impl Vmar_ { Ok(()) } - fn check_overwrite(&self, mapping_range: Range, can_overwrite: bool) -> Result<()> { - let inner = self.inner.read(); - - if !can_overwrite && inner.vm_mappings.find(&mapping_range).next().is_some() { - return_errno_with_message!( - Errno::EACCES, - "mapping range overlapped with another mapping" - ); - } - - Ok(()) - } - /// Returns the attached `VmSpace`. fn vm_space(&self) -> &Arc { &self.vm_space } - /// Maps a `VmMapping` to this VMAR. - fn add_mapping(&self, mapping: VmMapping) { - self.inner.write().vm_mappings.insert(mapping); - } - - fn allocate_free_region_for_mapping( - &self, - map_size: usize, - offset: Option, - align: usize, - can_overwrite: bool, - ) -> Result { - trace!("allocate free region, map_size = 0x{:x}, offset = {:x?}, align = 0x{:x}, can_overwrite = {}", map_size, offset, align, can_overwrite); - - if can_overwrite { - // If can overwrite, the offset is ensured not to be `None`. - let offset = offset.ok_or(Error::with_message( - Errno::EINVAL, - "offset cannot be None since can overwrite is set", - ))?; - self.inner.write().alloc_free_region_exact_truncate( - &self.vm_space, - offset, - map_size, - )?; - Ok(offset) - } else if let Some(offset) = offset { - self.inner - .write() - .alloc_free_region_exact(offset, map_size)?; - Ok(offset) - } else { - let free_region = self.inner.write().alloc_free_region(map_size, align)?; - Ok(free_region.start) - } - } - pub(super) fn new_fork_root(self: &Arc) -> Result> { let new_vmar_ = { let vmar_inner = VmarInner::new(); @@ -482,6 +435,231 @@ impl Vmar { } } +/// Options for creating a new mapping. The mapping is not allowed to overlap +/// with any child VMARs. And unless specified otherwise, it is not allowed +/// to overlap with any existing mapping, either. +pub struct VmarMapOptions { + parent: Vmar, + vmo: Option>, + perms: VmPerms, + vmo_offset: usize, + vmo_limit: usize, + size: usize, + offset: Option, + align: usize, + can_overwrite: bool, + // Whether the mapping is mapped with `MAP_SHARED` + is_shared: bool, + // Whether the mapping needs to handle surrounding pages when handling page fault. + handle_page_faults_around: bool, +} + +impl VmarMapOptions { + /// Creates a default set of options with the VMO and the memory access + /// permissions. + /// + /// The VMO must have access rights that correspond to the memory + /// access permissions. For example, if `perms` contains `VmPerms::Write`, + /// then `vmo.rights()` should contain `Rights::WRITE`. + pub fn new(parent: Vmar, size: usize, perms: VmPerms) -> Self { + Self { + parent, + vmo: None, + perms, + vmo_offset: 0, + vmo_limit: usize::MAX, + size, + offset: None, + align: PAGE_SIZE, + can_overwrite: false, + is_shared: false, + handle_page_faults_around: false, + } + } + + /// Binds a VMO to the mapping. + /// + /// If the mapping is a private mapping, its size may not be equal to that of the VMO. + /// For example, it is ok to create a mapping whose size is larger than + /// that of the VMO, although one cannot read from or write to the + /// part of the mapping that is not backed by the VMO. + /// + /// So you may wonder: what is the point of supporting such _oversized_ + /// mappings? The reason is two-fold. + /// 1. VMOs are resizable. So even if a mapping is backed by a VMO whose + /// size is equal to that of the mapping initially, we cannot prevent + /// the VMO from shrinking. + /// 2. Mappings are not allowed to overlap by default. As a result, + /// oversized mappings can serve as a placeholder to prevent future + /// mappings from occupying some particular address ranges accidentally. + pub fn vmo(mut self, vmo: Vmo) -> Self { + self.vmo = Some(vmo); + + self + } + + /// Sets the offset of the first memory page in the VMO that is to be + /// mapped into the VMAR. + /// + /// The offset must be page-aligned and within the VMO. + /// + /// The default value is zero. + pub fn vmo_offset(mut self, offset: usize) -> Self { + self.vmo_offset = offset; + self + } + + /// Sets the access limit offset for the binding VMO. + pub fn vmo_limit(mut self, limit: usize) -> Self { + self.vmo_limit = limit; + self + } + + /// Sets the mapping's alignment. + /// + /// The default value is the page size. + /// + /// The provided alignment must be a power of two and a multiple of the + /// page size. + pub fn align(mut self, align: usize) -> Self { + self.align = align; + self + } + + /// Sets the mapping's offset inside the VMAR. + /// + /// The offset must satisfy the alignment requirement. + /// Also, the mapping's range `[offset, offset + size)` must be within + /// the VMAR. + /// + /// If not set, the system will choose an offset automatically. + pub fn offset(mut self, offset: usize) -> Self { + self.offset = Some(offset); + self + } + + /// Sets whether the mapping can overwrite existing mappings. + /// + /// The default value is false. + /// + /// If this option is set to true, then the `offset` option must be + /// set. + pub fn can_overwrite(mut self, can_overwrite: bool) -> Self { + self.can_overwrite = can_overwrite; + self + } + + /// Sets whether the mapping can be shared with other process. + /// + /// The default value is false. + /// + /// If this value is set to true, the mapping will be shared with child + /// process when forking. + pub fn is_shared(mut self, is_shared: bool) -> Self { + self.is_shared = is_shared; + self + } + + /// Sets the mapping to handle surrounding pages when handling page fault. + pub fn handle_page_faults_around(mut self) -> Self { + self.handle_page_faults_around = true; + self + } + + /// Creates the mapping and adds it to the parent VMAR. + /// + /// All options will be checked at this point. + /// + /// On success, the virtual address of the new mapping is returned. + pub fn build(self) -> Result { + self.check_options()?; + let Self { + parent, + vmo, + perms, + vmo_offset, + vmo_limit, + size: map_size, + offset, + align, + can_overwrite, + is_shared, + handle_page_faults_around, + } = self; + + // Allocates a free region. + trace!("allocate free region, map_size = 0x{:x}, offset = {:x?}, align = 0x{:x}, can_overwrite = {}", map_size, offset, align, can_overwrite); + let mut inner = parent.0.inner.write(); + let map_to_addr = if can_overwrite { + // If can overwrite, the offset is ensured not to be `None`. + let offset = offset.ok_or(Error::with_message( + Errno::EINVAL, + "offset cannot be None since can overwrite is set", + ))?; + inner.alloc_free_region_exact_truncate(parent.vm_space(), offset, map_size)?; + offset + } else if let Some(offset) = offset { + inner.alloc_free_region_exact(offset, map_size)?; + offset + } else { + let free_region = inner.alloc_free_region(map_size, align)?; + free_region.start + }; + + // Build the mapping. + let vmo = vmo.map(|vmo| MappedVmo::new(vmo.to_dyn(), vmo_offset..vmo_limit)); + let vm_mapping = VmMapping::new( + NonZeroUsize::new(map_size).unwrap(), + map_to_addr, + vmo, + is_shared, + handle_page_faults_around, + perms, + ); + + // Add the mapping to the VMAR. + inner.vm_mappings.insert(vm_mapping); + + Ok(map_to_addr) + } + + /// Checks whether all options are valid. + fn check_options(&self) -> Result<()> { + // Check align. + debug_assert!(self.align % PAGE_SIZE == 0); + debug_assert!(self.align.is_power_of_two()); + if self.align % PAGE_SIZE != 0 || !self.align.is_power_of_two() { + return_errno_with_message!(Errno::EINVAL, "invalid align"); + } + debug_assert!(self.size % self.align == 0); + if self.size % self.align != 0 { + return_errno_with_message!(Errno::EINVAL, "invalid mapping size"); + } + debug_assert!(self.vmo_offset % self.align == 0); + if self.vmo_offset % self.align != 0 { + return_errno_with_message!(Errno::EINVAL, "invalid vmo offset"); + } + if let Some(offset) = self.offset { + debug_assert!(offset % self.align == 0); + if offset % self.align != 0 { + return_errno_with_message!(Errno::EINVAL, "invalid offset"); + } + } + self.check_perms()?; + Ok(()) + } + + /// Checks whether the permissions of the mapping is subset of vmo rights. + fn check_perms(&self) -> Result<()> { + let Some(vmo) = &self.vmo else { + return Ok(()); + }; + + let perm_rights = Rights::from(self.perms); + vmo.check_rights(perm_rights) + } +} + /// Determines whether two ranges are intersected. /// returns false if one of the ranges has a length of 0 pub fn is_intersected(range1: &Range, range2: &Range) -> bool { diff --git a/kernel/src/vm/vmar/static_cap.rs b/kernel/src/vm/vmar/static_cap.rs index 19d224e4d..5f273cc71 100644 --- a/kernel/src/vm/vmar/static_cap.rs +++ b/kernel/src/vm/vmar/static_cap.rs @@ -5,7 +5,7 @@ use core::ops::Range; use aster_rights::{Dup, Rights, TRightSet, TRights, Write}; use aster_rights_proc::require; -use super::{vm_mapping::VmarMapOptions, VmPerms, Vmar, VmarRightsOp, Vmar_}; +use super::{VmPerms, Vmar, VmarMapOptions, VmarRightsOp, Vmar_}; use crate::{ prelude::*, thread::exception::PageFaultInfo, vm::page_fault_handler::PageFaultHandler, }; diff --git a/kernel/src/vm/vmar/vm_mapping.rs b/kernel/src/vm/vmar/vm_mapping.rs index 64fdbedde..0939da861 100644 --- a/kernel/src/vm/vmar/vm_mapping.rs +++ b/kernel/src/vm/vmar/vm_mapping.rs @@ -7,21 +7,16 @@ use core::{ }; use align_ext::AlignExt; -use aster_rights::Rights; use ostd::mm::{ tlb::TlbFlushOp, vm_space::VmItem, CachePolicy, Frame, FrameAllocOptions, PageFlags, PageProperty, VmSpace, }; -use super::{interval_set::Interval, Vmar}; +use super::interval_set::Interval; use crate::{ prelude::*, thread::exception::PageFaultInfo, - vm::{ - perms::VmPerms, - util::duplicate_frame, - vmo::{Vmo, VmoRightsOp}, - }, + vm::{perms::VmPerms, util::duplicate_frame, vmo::Vmo}, }; /// Mapping a range of physical pages into a `Vmar`. @@ -78,39 +73,22 @@ impl Interval for VmMapping { /***************************** Basic methods *********************************/ impl VmMapping { - pub fn build_mapping(option: VmarMapOptions) -> Result { - let VmarMapOptions { - parent, - vmo, - perms, - vmo_offset, - vmo_limit, - size, - offset, - align, - can_overwrite, - is_shared, - handle_page_faults_around, - } = option; - let Vmar(parent_vmar, _) = parent; - let map_to_addr = - parent_vmar.allocate_free_region_for_mapping(size, offset, align, can_overwrite)?; - trace!( - "build mapping, map_range = 0x{:x}- 0x{:x}", + pub(super) fn new( + map_size: NonZeroUsize, + map_to_addr: Vaddr, + vmo: Option, + is_shared: bool, + handle_page_faults_around: bool, + perms: VmPerms, + ) -> Self { + Self { + map_size, map_to_addr, - map_to_addr + size - ); - - let vmo = vmo.map(|vmo| MappedVmo::new(vmo.to_dyn(), vmo_offset..vmo_limit)); - - Ok(Self { vmo, is_shared, handle_page_faults_around, - map_size: NonZeroUsize::new(size).unwrap(), - map_to_addr, perms, - }) + } } pub(super) fn new_fork(&self) -> Result { @@ -431,215 +409,6 @@ impl VmMapping { } } -/// Options for creating a new mapping. The mapping is not allowed to overlap -/// with any child VMARs. And unless specified otherwise, it is not allowed -/// to overlap with any existing mapping, either. -pub struct VmarMapOptions { - parent: Vmar, - vmo: Option>, - perms: VmPerms, - vmo_offset: usize, - vmo_limit: usize, - size: usize, - offset: Option, - align: usize, - can_overwrite: bool, - // Whether the mapping is mapped with `MAP_SHARED` - is_shared: bool, - // Whether the mapping needs to handle surrounding pages when handling page fault. - handle_page_faults_around: bool, -} - -impl VmarMapOptions { - /// Creates a default set of options with the VMO and the memory access - /// permissions. - /// - /// The VMO must have access rights that correspond to the memory - /// access permissions. For example, if `perms` contains `VmPerms::Write`, - /// then `vmo.rights()` should contain `Rights::WRITE`. - pub fn new(parent: Vmar, size: usize, perms: VmPerms) -> Self { - Self { - parent, - vmo: None, - perms, - vmo_offset: 0, - vmo_limit: usize::MAX, - size, - offset: None, - align: PAGE_SIZE, - can_overwrite: false, - is_shared: false, - handle_page_faults_around: false, - } - } - - /// Binds a VMO to the mapping. - /// - /// If the mapping is a private mapping, its size may not be equal to that of the VMO. - /// For example, it is ok to create a mapping whose size is larger than - /// that of the VMO, although one cannot read from or write to the - /// part of the mapping that is not backed by the VMO. - /// - /// So you may wonder: what is the point of supporting such _oversized_ - /// mappings? The reason is two-fold. - /// 1. VMOs are resizable. So even if a mapping is backed by a VMO whose - /// size is equal to that of the mapping initially, we cannot prevent - /// the VMO from shrinking. - /// 2. Mappings are not allowed to overlap by default. As a result, - /// oversized mappings can serve as a placeholder to prevent future - /// mappings from occupying some particular address ranges accidentally. - pub fn vmo(mut self, vmo: Vmo) -> Self { - self.vmo = Some(vmo); - - self - } - - /// Sets the offset of the first memory page in the VMO that is to be - /// mapped into the VMAR. - /// - /// The offset must be page-aligned and within the VMO. - /// - /// The default value is zero. - pub fn vmo_offset(mut self, offset: usize) -> Self { - self.vmo_offset = offset; - self - } - - /// Sets the access limit offset for the binding VMO. - pub fn vmo_limit(mut self, limit: usize) -> Self { - self.vmo_limit = limit; - self - } - - /// Sets the mapping's alignment. - /// - /// The default value is the page size. - /// - /// The provided alignment must be a power of two and a multiple of the - /// page size. - pub fn align(mut self, align: usize) -> Self { - self.align = align; - self - } - - /// Sets the mapping's offset inside the VMAR. - /// - /// The offset must satisfy the alignment requirement. - /// Also, the mapping's range `[offset, offset + size)` must be within - /// the VMAR. - /// - /// If not set, the system will choose an offset automatically. - pub fn offset(mut self, offset: usize) -> Self { - self.offset = Some(offset); - self - } - - /// Sets whether the mapping can overwrite existing mappings. - /// - /// The default value is false. - /// - /// If this option is set to true, then the `offset` option must be - /// set. - pub fn can_overwrite(mut self, can_overwrite: bool) -> Self { - self.can_overwrite = can_overwrite; - self - } - - /// Sets whether the mapping can be shared with other process. - /// - /// The default value is false. - /// - /// If this value is set to true, the mapping will be shared with child - /// process when forking. - pub fn is_shared(mut self, is_shared: bool) -> Self { - self.is_shared = is_shared; - self - } - - /// Sets the mapping to handle surrounding pages when handling page fault. - pub fn handle_page_faults_around(mut self) -> Self { - self.handle_page_faults_around = true; - self - } - - /// Creates the mapping. - /// - /// All options will be checked at this point. - /// - /// On success, the virtual address of the new mapping is returned. - pub fn build(self) -> Result { - self.check_options()?; - let parent_vmar = self.parent.0.clone(); - let vm_mapping = VmMapping::build_mapping(self)?; - let map_to_addr = vm_mapping.map_to_addr(); - parent_vmar.add_mapping(vm_mapping); - Ok(map_to_addr) - } - - /// Checks whether all options are valid. - fn check_options(&self) -> Result<()> { - // Check align. - debug_assert!(self.align % PAGE_SIZE == 0); - debug_assert!(self.align.is_power_of_two()); - if self.align % PAGE_SIZE != 0 || !self.align.is_power_of_two() { - return_errno_with_message!(Errno::EINVAL, "invalid align"); - } - debug_assert!(self.size % self.align == 0); - if self.size % self.align != 0 { - return_errno_with_message!(Errno::EINVAL, "invalid mapping size"); - } - debug_assert!(self.vmo_offset % self.align == 0); - if self.vmo_offset % self.align != 0 { - return_errno_with_message!(Errno::EINVAL, "invalid vmo offset"); - } - if let Some(offset) = self.offset { - debug_assert!(offset % self.align == 0); - if offset % self.align != 0 { - return_errno_with_message!(Errno::EINVAL, "invalid offset"); - } - } - self.check_perms()?; - self.check_overwrite()?; - Ok(()) - } - - /// Checks whether the permissions of the mapping is subset of vmo rights. - fn check_perms(&self) -> Result<()> { - let Some(vmo) = &self.vmo else { - return Ok(()); - }; - - let perm_rights = Rights::from(self.perms); - vmo.check_rights(perm_rights) - } - - /// Checks whether the mapping will overwrite with any existing mapping or vmar. - fn check_overwrite(&self) -> Result<()> { - if self.can_overwrite { - // If `can_overwrite` is set, the offset cannot be None. - debug_assert!(self.offset.is_some()); - if self.offset.is_none() { - return_errno_with_message!( - Errno::EINVAL, - "offset can not be none when can overwrite is true" - ); - } - } - if self.offset.is_none() { - // If does not specify the offset, we assume the map can always find suitable free region. - // FIXME: is this always true? - return Ok(()); - } - let offset = self.offset.unwrap(); - // We should spare enough space at least for the whole mapping. - let size = self.size; - let mapping_range = offset..(offset + size); - self.parent - .0 - .check_overwrite(mapping_range, self.can_overwrite) - } -} - /// A wrapper that represents a mapped [`Vmo`] and provide required functionalities /// that need to be provided to mappings from the VMO. #[derive(Debug)] @@ -651,7 +420,7 @@ pub(super) struct MappedVmo { impl MappedVmo { /// Creates a `MappedVmo` used for mapping. - fn new(vmo: Vmo, range: Range) -> Self { + pub(super) fn new(vmo: Vmo, range: Range) -> Self { Self { vmo, range } }