From ee3aafa1d98683db26d0e1518a717e7b160b08dd Mon Sep 17 00:00:00 2001 From: 4lDO2 <4lDO2@protonmail.com> Date: Sat, 9 Dec 2023 12:15:26 +0100 Subject: [PATCH] Remove PhysBorrowed hacks. --- src/context/memory.rs | 83 ++++++++++++++++++++---------------------- src/debugger.rs | 48 +++++++++++++++++------- src/memory/mod.rs | 16 ++++++-- src/syscall/process.rs | 2 + 4 files changed, 88 insertions(+), 61 deletions(-) diff --git a/src/context/memory.rs b/src/context/memory.rs index 35901f98f6..77749d5690 100644 --- a/src/context/memory.rs +++ b/src/context/memory.rs @@ -14,7 +14,7 @@ use syscall::{ use rmm::{Arch as _, PageFlush}; use crate::arch::paging::PAGE_SIZE; -use crate::memory::{Enomem, Frame, get_page_info, PageInfo, deallocate_frames, RefKind, AddRefError, RefCount, the_zeroed_frame}; +use crate::memory::{Enomem, Frame, get_page_info, PageInfo, deallocate_frames, RefKind, AddRefError, RefCount, the_zeroed_frame, init_frame}; use crate::paging::mapper::{Flusher, InactiveFlusher, PageFlushAll}; use crate::paging::{Page, PageFlags, PageMapper, RmmA, TableKind, VirtualAddress}; use crate::scheme::{self, KernelSchemes}; @@ -694,13 +694,8 @@ pub enum Provider { AllocatedShared { is_pinned_userscheme_borrow: bool }, /// The grant is not owned, but borrowed from physical memory frames that do not belong to the - /// frame allocator. - /// - /// This is true for MMIO, or where the frames are managed externally (UserScheme head/tail - /// buffers). - /// - // TODO: Stop using PhysBorrowed for head/tail pages when doing scheme calls! Force userspace - // to provide it, perhaps from relibc? + /// frame allocator. The kernel will forbid borrowing any physical memory range, that the + /// memory map has indicated is regular allocatable RAM. PhysBorrowed { base: Frame }, /// The memory is borrowed directly from another address space. @@ -735,9 +730,9 @@ impl Grant { // TODO: // - // This may not necessarily hold, as even pinned memory can remain shared (e.g. - // proc: borrow), but it would probably be possible to forbid borrowing memory - // there as well. + // This may not necessarily hold, as even pinned memory can remain shared (e.g. proc: + // borrow), but it would probably be possible to forbid borrowing memory there as well. + // Maybe make it exclusive first using cow(), unless that is too expensive. // // assert_eq!(info.refcount(), RefCount::One); @@ -913,10 +908,6 @@ impl Grant { let (_, _, flush) = guard.table.utable.remap_with_full(src_page.start_address(), |_, flags| (new_cow_frame.start_address(), flags)).expect("page did exist"); src.flusher.consume(flush); - if page_info.remove_ref() == RefCount::Zero { - deallocate_frames(frame, 1); - } - new_cow_frame } Err(AddRefError::SharedToCow) => unreachable!(), @@ -1001,8 +992,7 @@ impl Grant { }; let writable = match get_page_info(Frame::containing_address(phys)) { - // TODO: this is a hack for PhysBorrowed pages - None => false, + None => true, Some(i) => { if i.add_ref(RefKind::Shared).is_err() { continue; @@ -1013,7 +1003,7 @@ impl Grant { }; unsafe { - let flush = dst_mapper.map_phys(dst_base.next_by(i).start_address(), phys, flags.write(writable)).ok_or(Error::new(ENOMEM))?; + let flush = dst_mapper.map_phys(dst_base.next_by(i).start_address(), phys, flags.write(flags.has_write() && writable)).ok_or(Error::new(ENOMEM))?; dst_flusher.consume(flush); } } @@ -1030,7 +1020,6 @@ impl Grant { }, }) } - // TODO: This is limited to one grant. Should it be (if some magic new proc: API is introduced)? pub fn copy_mappings( src_base: Page, dst_base: Page, @@ -1070,7 +1059,8 @@ impl Grant { if let Some((phys, _)) = src_mapper.translate(src_page.start_address()) { Frame::containing_address(phys) } else { - let new_frame = init_frame(RefCount::Shared(NonZeroUsize::new(2).unwrap())).expect("TODO: handle OOM"); + // TODO: Omit the unnecessary subsequent add_ref call. + let new_frame = init_frame(RefCount::One).expect("TODO: handle OOM"); let src_flush = unsafe { src_mapper.map_phys(src_page.start_address(), new_frame.start_address(), flags).expect("TODO: handle OOM") }; src_flusher.consume(src_flush); @@ -1350,7 +1340,7 @@ impl GrantInfo { self.page_count } pub fn can_have_flags(&self, flags: MapFlags) -> bool { - // TODO: read + // TODO: read (some architectures support execute-only pages) let is_downgrade = (self.flags.has_write() || !flags.contains(MapFlags::PROT_WRITE)) && (self.flags.has_execute() || !flags.contains(MapFlags::PROT_EXEC)); match self.provider { @@ -1367,7 +1357,6 @@ impl GrantInfo { match (&self.provider, &with.provider) { (Provider::Allocated { cow_file_ref: None, phys_contiguous: false }, Provider::Allocated { cow_file_ref: None, phys_contiguous: false }) => true, //(Provider::PhysBorrowed { base: ref lhs }, Provider::PhysBorrowed { base: ref rhs }) => lhs.next_by(self.page_count) == rhs.clone(), - // TODO: Add merge function that merges the page array. //(Provider::External { address_space: ref lhs_space, src_base: ref lhs_base, cow: lhs_cow, .. }, Provider::External { address_space: ref rhs_space, src_base: ref rhs_base, cow: rhs_cow, .. }) => Arc::ptr_eq(lhs_space, rhs_space) && lhs_cow == rhs_cow && lhs_base.next_by(self.page_count) == rhs_base.clone(), _ => false, @@ -1408,8 +1397,6 @@ impl GrantInfo { flags } pub fn file_ref(&self) -> Option<&GrantFileRef> { - // TODO: This would be bad for PhysBorrowed head/tail buffers, but otherwise the physical - // base address could be included in offset, for PhysBorrowed. if let Provider::FmapBorrowed { ref file_ref, .. } | Provider::Allocated { cow_file_ref: Some(ref file_ref), .. } = self.provider { Some(file_ref) } else { @@ -1435,7 +1422,13 @@ pub struct Table { impl Drop for AddrSpace { fn drop(&mut self) { - for grant in core::mem::take(&mut self.grants).into_iter() { + for mut grant in core::mem::take(&mut self.grants).into_iter() { + // Unpinning the grant is allowed, because pinning only occurs in UserScheme calls to + // prevent unmapping the mapped range twice (which would corrupt only the scheme + // provider), but it won't be able to double free any range after this address space + // has been dropped! + grant.info.unpin(); + // TODO: Optimize away clearing the actual page tables? Since this address space is no // longer arc-rwlock wrapped, it cannot be referenced `External`ly by borrowing grants, // so it should suffice to iterate over PageInfos and decrement and maybe deallocate @@ -1551,34 +1544,37 @@ pub enum PfError { RecursionLimitExceeded, } +/// Consumes an existing reference to old_frame, and then returns an exclusive frame, with refcount +/// either preinitialized to One or Shared(2) depending on initial_ref_kind. This may be the same +/// frame, or (if the refcount is modified simultaneously) a new frame whereas the old frame is +/// deallocated. fn cow(old_frame: Frame, old_info: &PageInfo, initial_ref_kind: RefKind) -> Result { - assert_ne!(old_info.refcount(), RefCount::Zero); + let old_refcount = old_info.refcount(); + assert_ne!(old_refcount, RefCount::Zero); - if old_info.refcount() == RefCount::One { - old_info.add_ref(initial_ref_kind).expect("must succeed, knows current value"); + let initial_rc = match initial_ref_kind { + RefKind::Cow => RefCount::One, + RefKind::Shared => RefCount::Shared(NonZeroUsize::new(2).unwrap()), + }; + + if old_refcount == RefCount::One { + // This reference, that is being copied on write, was the only reference to old_frame, so + // no copy is necessary. + if initial_ref_kind == RefKind::Shared { + old_info.refcount.store(initial_rc.to_raw(), Ordering::Relaxed); + } return Ok(old_frame); } - let new_frame = init_frame(match initial_ref_kind { - RefKind::Cow => RefCount::One, - RefKind::Shared => RefCount::Shared(NonZeroUsize::new(2).unwrap()), - })?; + let new_frame = init_frame(initial_rc)?; - // TODO: omit this step if old_frame == the_zeroed_frame() if old_frame != the_zeroed_frame().0 { unsafe { copy_frame_to_frame_directly(new_frame, old_frame); } } - let _ = old_info.remove_ref(); - - Ok(new_frame) -} - -pub fn init_frame(init_rc: RefCount) -> Result { - let new_frame = crate::memory::allocate_frames(1).ok_or(PfError::Oom)?; - let page_info = get_page_info(new_frame).unwrap_or_else(|| panic!("all allocated frames need an associated page info, {:?} didn't", new_frame)); - assert_eq!(page_info.refcount(), RefCount::Zero); - page_info.refcount.store(init_rc.to_raw(), Ordering::Relaxed); + if old_info.remove_ref() == RefCount::Zero { + crate::memory::deallocate_frames(old_frame, 1); + } Ok(new_frame) } @@ -1765,6 +1761,7 @@ fn correct_inner<'l>(addr_space_lock: &'l Arc>, mut addr_space let mut guard = RwLockUpgradableGuard::upgrade(guard); // TODO: Should this be called? + log::warn!("Mapped zero page since grant didn't exist"); map_zeroed(&mut guard.table.utable, src_page, grant_flags, access == AccessMode::Write)? } } diff --git a/src/debugger.rs b/src/debugger.rs index 8679987862..7d31bd8200 100644 --- a/src/debugger.rs +++ b/src/debugger.rs @@ -1,3 +1,5 @@ +use alloc::collections::BTreeSet; + use crate::paging::{RmmA, RmmArch, TableKind, PAGE_SIZE}; //TODO: combine arches into one function (aarch64 one is newest) @@ -176,7 +178,7 @@ pub unsafe fn debugger(target_id: Option) { pub unsafe fn debugger(target_id: Option) { use hashbrown::HashSet; - use crate::memory::{RefCount, get_page_info}; + use crate::memory::{RefCount, get_page_info, the_zeroed_frame}; unsafe { x86::bits64::rflags::stac(); } @@ -186,6 +188,10 @@ pub unsafe fn debugger(target_id: Option) { let mut tree = HashMap::new(); let mut spaces = HashSet::new(); + let mut temporarily_taken_htbufs = 0; + + tree.insert(the_zeroed_frame().0, (1, false)); + let old_table = RmmA::table(TableKind::User); for (id, context_lock) in crate::context::contexts().iter() { @@ -193,6 +199,17 @@ pub unsafe fn debugger(target_id: Option) { let context = context_lock.read(); println!("{}: {}", (*id).get(), context.name); + if let Some(ref head) = context.syscall_head { + tree.insert(head.get(), (1, false)); + } else { + temporarily_taken_htbufs += 1; + } + if let Some(ref tail) = context.syscall_tail { + tree.insert(tail.get(), (1, false)); + } else { + temporarily_taken_htbufs += 1; + } + // Switch to context page table to ensure syscall debug and stack dump will work if let Some(ref space) = context.addr_space { let was_new = spaces.insert(space.read().table.utable.table().phys().data()); @@ -250,18 +267,23 @@ pub unsafe fn debugger(target_id: Option) { println!(); } - for (frame, count) in tree { - let rc = get_page_info(frame).unwrap().refcount(); + for (frame, (count, p)) in tree { + let Some(info) = get_page_info(frame) else { + assert!(p); + continue; + }; + let rc = info.refcount(); let c = match rc { RefCount::Zero => 0, RefCount::One => 1, RefCount::Cow(c) => c.get(), RefCount::Shared(s) => s.get(), }; - if c < count { - println!("undercounted frame {:?} ({} < {})", frame, c, count); + if c != count { + println!("frame refcount mismatch for {:?} ({} != {})", frame, c, count); } } + println!("({} kernel-owned references were not counted)", temporarily_taken_htbufs); println!("DEBUGGER END"); unsafe { x86::bits64::rflags::clac(); } @@ -270,9 +292,8 @@ pub unsafe fn debugger(target_id: Option) { use {hashbrown::HashMap, crate::memory::Frame}; #[cfg(any(target_arch = "aarch64", target_arch = "x86_64"))] -pub unsafe fn check_consistency(addr_space: &mut crate::context::memory::AddrSpace, new_as: bool, tree: &mut HashMap) { - - use crate::context::memory::PageSpan; +pub unsafe fn check_consistency(addr_space: &mut crate::context::memory::AddrSpace, new_as: bool, tree: &mut HashMap) { + use crate::context::memory::{PageSpan, Provider}; use crate::memory::{get_page_info, RefCount}; use crate::paging::*; @@ -319,22 +340,21 @@ pub unsafe fn check_consistency(addr_space: &mut crate::context::memory::AddrSpa if grant.flags().write(false).data() & !EXCLUDE != flags.write(false).data() & !EXCLUDE { log::error!("FLAG MISMATCH: {:?} != {:?}, address {:p} in grant at {:?}", grant.flags(), flags, address.data() as *const u8, PageSpan::new(base, grant.page_count())); } + let p = matches!(grant.provider, Provider::PhysBorrowed { .. } | Provider::External { .. } | Provider::FmapBorrowed { .. }); let frame = Frame::containing_address(physaddr); if new_as { - *tree.entry(frame).or_insert(0) += 1; + tree.entry(frame).or_insert((0, p)).0 += 1; } if let Some(page) = get_page_info(frame) { match page.refcount() { - // TODO: Remove physalloc, and ensure physmap cannot map - // allocator-owned memory! This is a hack! - - //RefCount::Zero => panic!("mapped page with zero refcount"), - RefCount::Zero => (), + RefCount::Zero => panic!("mapped page with zero refcount"), RefCount::One | RefCount::Shared(_) => assert!(!(flags.has_write() && !grant.flags().has_write()), "page entry has higher permissions than grant!"), RefCount::Cow(_) => assert!(!flags.has_write(), "directly writable CoW page!"), } + } else { + //println!("!OWNED {:?}", frame); } } } diff --git a/src/memory/mod.rs b/src/memory/mod.rs index 14bef0b38d..45c9d8bdab 100644 --- a/src/memory/mod.rs +++ b/src/memory/mod.rs @@ -7,7 +7,7 @@ use core::num::NonZeroUsize; use core::sync::atomic::{AtomicUsize, Ordering}; use crate::arch::rmm::LockedAllocator; -use crate::context::{self, memory::{init_frame, AccessMode, PfError}}; +use crate::context::{self, memory::{AccessMode, PfError}}; use crate::kernel_executable_offsets::{__usercopy_start, __usercopy_end}; use crate::paging::Page; pub use crate::paging::{PAGE_SIZE, PhysicalAddress, RmmA, RmmArch}; @@ -139,7 +139,6 @@ pub struct RaiiFrame { } impl RaiiFrame { pub fn allocate() -> Result { - // TODO: Use special tag? init_frame(RefCount::One).map_err(|_| Enomem).map(|inner| Self { inner }) } pub fn get(&self) -> Frame { @@ -292,7 +291,7 @@ pub fn init_mm() { unsafe { let the_frame = allocate_frames(1).expect("failed to allocate static zeroed frame"); let the_info = get_page_info(the_frame).expect("static zeroed frame had no PageInfo"); - the_info.refcount.store(RefCount::Cow(NonZeroUsize::new(2).unwrap()).to_raw(), Ordering::Relaxed); + the_info.refcount.store(RefCount::One.to_raw(), Ordering::Relaxed); THE_ZEROED_FRAME.get().write(Some((the_frame, the_info))); } @@ -342,7 +341,7 @@ impl PageInfo { RefCount::from_raw(refcount) } } -#[derive(Clone, Copy, Debug)] +#[derive(Clone, Copy, Debug, PartialEq)] pub enum RefKind { Cow, Shared, @@ -478,3 +477,12 @@ pub fn the_zeroed_frame() -> (Frame, &'static PageInfo) { THE_ZEROED_FRAME.get().read().expect("zeroed frame must be initialized") } } + +pub fn init_frame(init_rc: RefCount) -> Result { + let new_frame = crate::memory::allocate_frames(1).ok_or(PfError::Oom)?; + let page_info = get_page_info(new_frame).unwrap_or_else(|| panic!("all allocated frames need an associated page info, {:?} didn't", new_frame)); + assert_eq!(page_info.refcount(), RefCount::Zero); + page_info.refcount.store(init_rc.to_raw(), Ordering::Relaxed); + + Ok(new_frame) +} diff --git a/src/syscall/process.rs b/src/syscall/process.rs index 5feeeaa1f6..76c219d296 100644 --- a/src/syscall/process.rs +++ b/src/syscall/process.rs @@ -38,6 +38,8 @@ pub fn exit(status: usize) -> ! { let mut context = context_lock.write(); close_files = Arc::try_unwrap(mem::take(&mut context.files)).map_or_else(|_| Vec::new(), RwLock::into_inner); addrspace_opt = mem::take(&mut context.addr_space).and_then(|a| Arc::try_unwrap(a).ok()); + drop(context.syscall_head.take()); + drop(context.syscall_tail.take()); context.id };