From e72fd5a0e4ea19ca8fc490b6d08b5786ca4f1fd9 Mon Sep 17 00:00:00 2001 From: 4lDO2 <4lDO2@protonmail.com> Date: Sat, 9 Apr 2022 11:24:34 +0200 Subject: [PATCH 1/2] Fix a kernel deadlock in empty(). For more information, see https://gitlab.redox-os.org/4lDO2/kernel/-/commit/b3b5d1b864eec030d53ccd2ab907c68958f140aa --- src/context/memory.rs | 33 +++++++++++++++++++-------------- src/syscall/process.rs | 27 ++++++++++++++++++--------- 2 files changed, 37 insertions(+), 23 deletions(-) diff --git a/src/context/memory.rs b/src/context/memory.rs index cfac011..032c550 100644 --- a/src/context/memory.rs +++ b/src/context/memory.rs @@ -35,6 +35,17 @@ pub fn page_flags(flags: MapFlags) -> PageFlags { //TODO: PROT_READ } +pub struct UnmapResult { + pub file_desc: Option, +} +impl Drop for UnmapResult { + fn drop(&mut self) { + if let Some(fd) = self.file_desc.take() { + let _ = fd.close(); + } + } +} + #[derive(Debug, Default)] pub struct UserGrants { pub inner: BTreeSet, @@ -486,7 +497,7 @@ impl Grant { self.flags } - pub fn unmap(mut self) { + pub fn unmap(mut self) -> UnmapResult { assert!(self.mapped); let mut active_table = unsafe { ActivePageTable::new(self.start_address().kind()) }; @@ -507,16 +518,13 @@ impl Grant { flush_all.flush(); - if let Some(desc) = self.desc_opt.take() { - println!("Grant::unmap: close desc {:?}", desc); - //TODO: This imposes a large cost on unmapping, but that cost cannot be avoided without modifying fmap and funmap - let _ = desc.close(); - } - self.mapped = false; + + // TODO: This imposes a large cost on unmapping, but that cost cannot be avoided without modifying fmap and funmap + UnmapResult { file_desc: self.desc_opt.take() } } - pub fn unmap_inactive(mut self, new_table: &mut InactivePageTable) { + pub fn unmap_inactive(mut self, new_table: &mut InactivePageTable) -> UnmapResult { assert!(self.mapped); let start_page = Page::containing_address(self.start_address()); @@ -533,13 +541,10 @@ impl Grant { ipi(IpiKind::Tlb, IpiTarget::Other); - if let Some(desc) = self.desc_opt.take() { - println!("Grant::unmap_inactive: close desc {:?}", desc); - //TODO: This imposes a large cost on unmapping, but that cost cannot be avoided without modifying fmap and funmap - let _ = desc.close(); - } - self.mapped = false; + + // TODO: This imposes a large cost on unmapping, but that cost cannot be avoided without modifying fmap and funmap + UnmapResult { file_desc: self.desc_opt.take() } } /// Extract out a region into a separate grant. The return value is as diff --git a/src/syscall/process.rs b/src/syscall/process.rs index 3ef08d6..458f677 100644 --- a/src/syscall/process.rs +++ b/src/syscall/process.rs @@ -8,11 +8,11 @@ use alloc::{ use core::alloc::{GlobalAlloc, Layout}; use core::ops::DerefMut; use core::{intrinsics, mem, str}; -use spin::RwLock; +use spin::{RwLock, RwLockWriteGuard}; use crate::context::file::FileDescriptor; -use crate::context::{ContextId, WaitpidKey}; use crate::context::memory::{UserGrants, Region}; +use crate::context::{Context, ContextId, WaitpidKey}; use crate::context; #[cfg(not(feature="doc"))] use crate::elf::{self, program_header}; @@ -525,7 +525,7 @@ pub fn clone(flags: CloneFlags, stack_base: usize) -> Result { Ok(pid) } -fn empty(context: &mut context::Context, reaping: bool) { +fn empty<'lock>(context_lock: &'lock RwLock, mut context: RwLockWriteGuard<'lock, Context>, reaping: bool) -> RwLockWriteGuard<'lock, Context> { if reaping { // Memory should already be unmapped assert!(context.image.is_empty()); @@ -559,17 +559,26 @@ fn empty(context: &mut context::Context, reaping: bool) { let grants = mem::replace(&mut *grants_guard, UserGrants::default()); for grant in grants.inner.into_iter() { - if reaping { + let unmap_result = if reaping { log::error!("{}: {}: Grant should not exist: {:?}", context.id.into(), *context.name.read(), grant); let mut new_table = unsafe { InactivePageTable::from_address(context.arch.get_page_utable()) }; - grant.unmap_inactive(&mut new_table); + grant.unmap_inactive(&mut new_table) } else { - grant.unmap(); + grant.unmap() + }; + + if unmap_result.file_desc.is_some() { + drop(context); + + drop(unmap_result); + + context = context_lock.write(); } } } + context } struct ExecFile(FileHandle); @@ -607,7 +616,7 @@ fn fexec_noreturn( context.name = Arc::new(RwLock::new(name)); - empty(&mut context, false); + context = empty(&context_lock, context, false); context.grants.write().insert(phdr_grant); @@ -1091,7 +1100,7 @@ pub fn exit(status: usize) -> ! { let (vfork, children) = { let mut context = context_lock.write(); - empty(&mut context, false); + context = empty(&context_lock, context, false); let vfork = context.vfork; context.vfork = false; @@ -1439,7 +1448,7 @@ fn reap(pid: ContextId) -> Result { let context_lock = contexts.remove(pid).ok_or(Error::new(ESRCH))?; { let mut context = context_lock.write(); - empty(&mut context, true); + context = empty(&context_lock, context, true); } drop(context_lock); From 4d7da495f5bc5fb91222582243fc4dfcfca3f86b Mon Sep 17 00:00:00 2001 From: 4lDO2 <4lDO2@protonmail.com> Date: Fri, 8 Apr 2022 16:10:16 +0200 Subject: [PATCH 2/2] Fix "clone grant using fmap" This does the same as the previous MR, but fixes the issue where the parent process got the mapping (and at the wrong address) and not the child process. --- src/context/file.rs | 2 +- src/context/memory.rs | 16 +++++++++--- src/scheme/user.rs | 7 ++--- src/syscall/fs.rs | 18 +++++-------- src/syscall/process.rs | 59 ++++++++++++++++++++++++++++++------------ 5 files changed, 66 insertions(+), 36 deletions(-) diff --git a/src/context/file.rs b/src/context/file.rs index a274ef7..86ebf25 100644 --- a/src/context/file.rs +++ b/src/context/file.rs @@ -7,7 +7,7 @@ use crate::scheme::{self, SchemeNamespace, SchemeId}; use crate::syscall::error::{Result, Error, EBADF}; /// A file description -#[derive(Debug)] +#[derive(Clone, Copy, Debug)] pub struct FileDescription { /// The namespace the file was opened from (used for debugging) pub namespace: SchemeNamespace, diff --git a/src/context/memory.rs b/src/context/memory.rs index 032c550..209d908 100644 --- a/src/context/memory.rs +++ b/src/context/memory.rs @@ -36,12 +36,12 @@ pub fn page_flags(flags: MapFlags) -> PageFlags { } pub struct UnmapResult { - pub file_desc: Option, + pub file_desc: Option, } impl Drop for UnmapResult { fn drop(&mut self) { if let Some(fd) = self.file_desc.take() { - let _ = fd.close(); + let _ = fd.desc.close(); } } } @@ -301,7 +301,15 @@ pub struct Grant { mapped: bool, owned: bool, //TODO: This is probably a very heavy way to keep track of fmap'd files, perhaps move to the context? - pub desc_opt: Option, + pub desc_opt: Option, +} +#[derive(Clone, Debug)] +pub struct GrantFileRef { + pub desc: FileDescriptor, + pub offset: usize, + // TODO: Can the flags maybe be stored together with the page flags. Should some flags be kept, + // and others discarded when re-fmapping on clone? + pub flags: MapFlags, } impl Grant { @@ -374,7 +382,7 @@ impl Grant { } } - pub fn map_inactive(src: VirtualAddress, dst: VirtualAddress, size: usize, flags: PageFlags, desc_opt: Option, inactive_table: &mut InactivePageTable) -> Grant { + pub fn map_inactive(src: VirtualAddress, dst: VirtualAddress, size: usize, flags: PageFlags, desc_opt: Option, inactive_table: &mut InactivePageTable) -> Grant { let active_table = unsafe { ActivePageTable::new(src.kind()) }; let mut inactive_mapper = inactive_table.mapper(); diff --git a/src/scheme/user.rs b/src/scheme/user.rs index 6035bd5..5d7f9c6 100644 --- a/src/scheme/user.rs +++ b/src/scheme/user.rs @@ -8,7 +8,7 @@ use spin::{Mutex, RwLock}; use crate::context::{self, Context}; use crate::context::file::FileDescriptor; -use crate::context::memory::{DANGLING, page_flags, round_down_pages, Grant, Region}; +use crate::context::memory::{DANGLING, page_flags, round_down_pages, Grant, Region, GrantFileRef}; use crate::event; use crate::paging::{PAGE_SIZE, InactivePageTable, VirtualAddress}; use crate::scheme::{AtomicSchemeId, SchemeId}; @@ -123,7 +123,7 @@ impl UserInner { ).map(|addr| addr.data()) } - fn capture_inner(context_weak: &Weak>, dst_address: usize, address: usize, size: usize, flags: MapFlags, desc_opt: Option) + fn capture_inner(context_weak: &Weak>, dst_address: usize, address: usize, size: usize, flags: MapFlags, desc_opt: Option) -> Result { // TODO: More abstractions over grant creation! @@ -233,7 +233,8 @@ impl UserInner { if address % PAGE_SIZE > 0 { println!("scheme returned unaligned address, causing extra frame to be allocated"); } - let res = UserInner::capture_inner(&context_weak, map.address, address, map.size, map.flags, Some(desc)); + let file_ref = GrantFileRef { desc, offset: map.offset, flags: map.flags }; + let res = UserInner::capture_inner(&context_weak, map.address, address, map.size, map.flags, Some(file_ref)); if let Ok(grant_address) = res { if let Some(context_lock) = context_weak.upgrade() { let context = context_lock.read(); diff --git a/src/syscall/fs.rs b/src/syscall/fs.rs index 7052c0f..642a80a 100644 --- a/src/syscall/fs.rs +++ b/src/syscall/fs.rs @@ -489,11 +489,8 @@ pub fn funmap_old(virtual_address: usize) -> Result { } } - if let Some(desc) = desc_opt { - let scheme_id = { - let description = desc.description.read(); - description.scheme - }; + if let Some(file_ref) = desc_opt { + let scheme_id = { file_ref.desc.description.read().scheme }; let scheme = { let schemes = scheme::schemes(); @@ -502,7 +499,7 @@ pub fn funmap_old(virtual_address: usize) -> Result { }; let res = scheme.funmap_old(virtual_address); - let _ = desc.close(); + let _ = file_ref.desc.close(); res } else { @@ -555,11 +552,8 @@ pub fn funmap(virtual_address: usize, length: usize) -> Result { } } - for (desc, intersection) in notify_files { - let scheme_id = { - let description = desc.description.read(); - description.scheme - }; + for (file_ref, intersection) in notify_files { + let scheme_id = { file_ref.desc.description.read().scheme }; let scheme = { let schemes = scheme::schemes(); @@ -568,7 +562,7 @@ pub fn funmap(virtual_address: usize, length: usize) -> Result { }; let res = scheme.funmap(intersection.start_address().data(), intersection.size()); - let _ = desc.close(); + let _ = file_ref.desc.close(); res?; } diff --git a/src/syscall/process.rs b/src/syscall/process.rs index 458f677..c246568 100644 --- a/src/syscall/process.rs +++ b/src/syscall/process.rs @@ -10,7 +10,7 @@ use core::ops::DerefMut; use core::{intrinsics, mem, str}; use spin::{RwLock, RwLockWriteGuard}; -use crate::context::file::FileDescriptor; +use crate::context::file::{FileDescription, FileDescriptor}; use crate::context::memory::{UserGrants, Region}; use crate::context::{Context, ContextId, WaitpidKey}; use crate::context; @@ -56,7 +56,7 @@ pub fn clone(flags: CloneFlags, stack_base: usize) -> Result { let mut image = vec![]; let mut stack_opt = None; let mut sigstack_opt = None; - let grants; + let mut grants; let name; let cwd; let files; @@ -266,24 +266,51 @@ pub fn clone(flags: CloneFlags, stack_base: usize) -> Result { // If not cloning virtual memory, use fmap to re-obtain every grant where possible if !flags.contains(CLONE_VM) { - let mut grants = grants.write(); + let grants = Arc::get_mut(&mut grants).ok_or(Error::new(EBUSY))?.get_mut(); + let old_grants = mem::take(&mut grants.inner); - let mut to_remove = BTreeSet::new(); + // TODO: Find some way to do this without having to allocate. - // TODO: Use drain_filter if possible + // TODO: Check that the current process is not allowed to serve any scheme this logic + // could interfere with. Deadlocks would otherwise seem inevitable. - for grant in grants.iter() { - let remove = false; - if let Some(ref _desc) = grant.desc_opt { - println!("todo: clone grant using fmap: {:?}", grant); - } - if remove { - to_remove.insert(Region::from(grant)); - } - } + for mut grant in old_grants.into_iter() { + let region = *grant.region(); + let address = region.start_address().data(); + let size = region.size(); - for region in to_remove { - grants.remove(®ion); + let new_grant = if let Some(ref mut file_ref) = grant.desc_opt.take() { + // TODO: Technically this is redundant as the grants are already secret_cloned. + // Maybe grants with fds can be excluded from that step? + grant.unmap(); + + let FileDescription { scheme, number, .. } = { *file_ref.desc.description.read() }; + let scheme_arc = match crate::scheme::schemes().get(scheme) { + Some(s) => Arc::clone(s), + None => continue, + }; + let map = crate::syscall::data::Map { + address, + size, + offset: file_ref.offset, + flags: file_ref.flags | MapFlags::MAP_FIXED_NOREPLACE, + }; + + let ptr = match scheme_arc.fmap(number, &map) { + Ok(new_range) => new_range as *mut u8, + Err(_) => continue, + }; + + // This will eventually be freed from the parent context after move_to is + // called. + context::contexts().current().ok_or(Error::new(ESRCH))? + .read().grants.write() + .take(&Region::new(VirtualAddress::new(ptr as usize), map.size)) + .ok_or(Error::new(EFAULT))? + } else { + grant + }; + grants.insert(new_grant); } }