diff --git a/src/context/mod.rs b/src/context/mod.rs index 678bd6f5..ac743231 100644 --- a/src/context/mod.rs +++ b/src/context/mod.rs @@ -6,7 +6,7 @@ use alloc::{ collections::{BTreeMap, BTreeSet, VecDeque}, sync::{Arc, Weak}, }; -use core::{cmp::Reverse, num::NonZeroUsize, ops::Deref, sync::atomic::AtomicUsize}; +use core::{cell::Ref, cmp::Reverse, num::NonZeroUsize, ops::Deref, sync::atomic::AtomicUsize}; use syscall::NumaMemoryPolicy; use crate::{ @@ -206,21 +206,24 @@ pub fn init(token: &mut CleanLockToken) { } } -pub fn current() -> Arc { - PercpuBlock::current() - .switch_internals - .with_context(Arc::clone) +// TODO: Maybe use lock tokens to forbid holding this reference across context::switch (where the +// RefCell's borrow_mut will fail if the reference is kept?)? If so, maybe even avoid `RefCell` +// entirely, if it can be done sufficiently rigorously and without breaking soundness? +pub fn current() -> Ref<'static, Arc> { + PercpuBlock::current().switch_internals.current_context() } -pub fn try_current() -> Option> { +pub fn try_current() -> Ref<'static, Option>> { PercpuBlock::current() .switch_internals - .try_with_context(|context| context.map(Arc::clone)) + .current_context_raw() } pub fn is_current(context: &Arc) -> bool { PercpuBlock::current() .switch_internals - .with_context(|current| Arc::ptr_eq(context, current)) + .current_context_raw() + .as_ref() + .map_or(false, |current| Arc::ptr_eq(current, context)) } #[derive(Clone)] diff --git a/src/context/signal.rs b/src/context/signal.rs index f1561d22..ec150005 100644 --- a/src/context/signal.rs +++ b/src/context/signal.rs @@ -1,5 +1,7 @@ use core::sync::atomic::Ordering; +use alloc::sync::Arc; + use crate::{context, sync::CleanLockToken, syscall::flag::SigcontrolFlags}; pub fn signal_handler(token: &mut CleanLockToken) { @@ -82,10 +84,11 @@ pub fn excp_handler(excp: syscall::Exception) { let Some(eh) = context.sig.as_ref().and_then(|s| s.excp_handler) else { // TODO: Let procmgr print this? info!( - "UNHANDLED EXCEPTION, CPU {}, PID {}, NAME {}, CONTEXT {current:p}", + "UNHANDLED EXCEPTION, CPU {}, PID {}, NAME {}, CONTEXT {:p}", crate::cpu_id(), context.pid, - context.name + context.name, + Arc::as_ptr(&*current), ); drop(context); drop(current); diff --git a/src/context/switch.rs b/src/context/switch.rs index c2b3493c..caa8bc9b 100644 --- a/src/context/switch.rs +++ b/src/context/switch.rs @@ -17,7 +17,7 @@ use alloc::{ vec::Vec, }; use core::{ - cell::{Cell, RefCell}, + cell::{Cell, Ref, RefCell}, cmp::Reverse, hint, matches, mem, option::Option::{None, Some}, @@ -60,9 +60,6 @@ unsafe fn opportunistic_write_arc(lock: &Arc) -> Option) -> Option UpdateResult { +fn update_runnable(context: &mut Context, cpu_id: LogicalCpuId, switch_time: u128) -> UpdateResult { // Ignore contexts that are already running. if context.running { return UpdateResult::Skip; @@ -211,9 +204,11 @@ pub fn switch(token: &mut CleanLockToken) -> SwitchResult { } // Lock the previous context. - let prev_context_lock = crate::context::current(); - // We are careful not to lock this context twice - let mut prev_context_guard = unsafe { prev_context_lock.write_arc() }; + let mut prev_context_guard = { + let prev_context_lock_ref = crate::context::current(); + // We are careful not to lock this context twice + unsafe { prev_context_lock_ref.write_arc() } + }; if !prev_context_guard.is_preemptable() { // Unset global lock @@ -266,7 +261,8 @@ pub fn switch(token: &mut CleanLockToken) -> SwitchResult { }; // TODO: can this happen? if !cfg!(opportunistic_context_locking) - && Weak::as_ptr(&context_ref.0) == Arc::as_ptr(&prev_context_lock) + && Weak::as_ptr(&context_ref.0) + == Arc::as_ptr(&ArcRwLockWriteGuard::rwlock(&prev_context_guard)) { continue; } @@ -452,9 +448,6 @@ pub fn switch(token: &mut CleanLockToken) -> SwitchResult { .being_sigkilled .set(next_context.being_sigkilled); - // Anything implement Drop must be manually dropped now - drop(prev_context_lock); - unsafe { percpu.new_addrsp_guard.set(addr_space_guard); arch::switch_to(prev_context, next_context); @@ -580,7 +573,7 @@ fn select_next_context( continue; }; - let sw = unsafe { update_runnable(&mut guard, cpu_id, switch_time) }; + let sw = update_runnable(&mut guard, cpu_id, switch_time); if matches!(sw, UpdateResult::Blocked) { if guard.is_active { @@ -994,39 +987,23 @@ impl ContextSwitchPercpu { } } - /// Applies a function to the current context, allowing controlled access. - /// - /// # Parameters - /// - `f`: A closure that receives a reference to the current context and returns a value. - /// - /// # Returns - /// The result of applying `f` to the current context. - pub fn with_context(&self, f: impl FnOnce(&Arc) -> T) -> T { - f(self - .current_ctxt - .borrow() - .as_ref() - .expect("not inside of context")) + /// Gets a reference to the raw current context slot. + pub fn current_context_raw(&self) -> Ref<'_, Option>> { + self.current_ctxt.borrow() } - /// Applies a function to the current context, allowing controlled access. - /// - /// # Parameters - /// - `f`: A closure that receives a reference to the current context and returns a value. - /// - /// # Returns - /// The result of applying `f` to the current context if any. - pub fn try_with_context(&self, f: impl FnOnce(Option<&Arc>) -> T) -> T { - f(self.current_ctxt.borrow().as_ref()) + /// Gets a reference to the current context, which will always be populated after the startup + /// code (unless there are kernel bugs). + pub fn current_context(&self) -> Ref<'_, Arc> { + Ref::map(self.current_ctxt.borrow(), |c| { + c.as_ref().expect("no current context present") + }) } /// Sets the current context to a new value. /// /// # Safety /// This function is unsafe as it modifies the context state directly. - /// - /// # Parameters - /// - `new`: The new context to be set as the current context. pub unsafe fn set_current_context(&self, new: Arc) { *self.current_ctxt.borrow_mut() = Some(new); } @@ -1035,23 +1012,14 @@ impl ContextSwitchPercpu { /// /// # Safety /// This function is unsafe as it modifies the idle context state directly. - /// - /// # Parameters - /// - `new`: The new context to be set as the idle context. pub unsafe fn set_idle_context(&self, new: Arc) { *self.idle_ctxt.borrow_mut() = Some(new); } /// Retrieves the current idle context. - /// - /// # Returns - /// A reference to the idle context. - pub fn idle_context(&self) -> Arc { - Arc::clone( - self.idle_ctxt - .borrow() - .as_ref() - .expect("no idle context present"), - ) + pub fn idle_context(&self) -> Ref<'_, Arc> { + Ref::map(self.idle_ctxt.borrow(), |opt| { + opt.as_ref().expect("no idle context present") + }) } } diff --git a/src/panic.rs b/src/panic.rs index 50c05dca..ded62e62 100644 --- a/src/panic.rs +++ b/src/panic.rs @@ -40,7 +40,8 @@ fn panic_handler_inner(info: &PanicInfo) -> ! { stack_trace(); } - let Some(context_lock) = context::try_current() else { + let current_ref = context::try_current(); + let Some(context_lock) = &*current_ref else { println!("CPU {}, CID ", cpu_id()); println!("HALT"); diff --git a/src/scheme/proc.rs b/src/scheme/proc.rs index d8936ad2..1c0b4d5f 100644 --- a/src/scheme/proc.rs +++ b/src/scheme/proc.rs @@ -326,7 +326,7 @@ impl ProcScheme { ); return Ok((id.get(), InternalFlags::empty())); } - "cur-context" => context::current(), + "cur-context" => Arc::clone(&*context::current()), _ => return Err(Error::new(ENOENT)), }; @@ -416,7 +416,7 @@ impl KernelScheme for ProcScheme { id, Handle { // TODO: placeholder - context: context::current(), + context: Arc::clone(&*context::current()), kind: ContextHandle::Authority, }, ); diff --git a/src/sync/wait_condition.rs b/src/sync/wait_condition.rs index 5c8c5679..44a3220d 100644 --- a/src/sync/wait_condition.rs +++ b/src/sync/wait_condition.rs @@ -70,8 +70,8 @@ impl WaitCondition { reason: &'static str, token: &'a mut LockToken<'a, L2>, ) -> bool { - let current_context_ref = context::current(); - { + let current_context_ptr = { + let current_context_ref = context::current(); // Avoid a context switch between blocking ourselves and adding // ourselves to the wait list as otherwise we might miss a wakeup. // We cannot add ourselves to the wait list first as that would lead @@ -94,12 +94,17 @@ impl WaitCondition { .push(Arc::downgrade(¤t_context_ref)); drop(guard); - } + + // It's perfectly valid to use raw pointers in Safe Rust for comparing... + Arc::as_ptr(&*current_context_ref) + }; { // SAFETY: Guaranteed by caller let token = unsafe { &mut CleanLockToken::new() }; context::switch(token); + // ... and in order for it to be possible to switch back, the context Arc and thus + // current_context_ptr must continue to be alive (meaningful to check against). } let mut waited = true; @@ -109,7 +114,7 @@ impl WaitCondition { if let Some(index) = contexts .iter() - .position(|c| Weak::as_ptr(c) == Arc::as_ptr(¤t_context_ref)) + .position(|c| Weak::as_ptr(c) == current_context_ptr) { contexts.swap_remove(index); waited = false; diff --git a/src/syscall/fs.rs b/src/syscall/fs.rs index 61218cf5..f04d6b18 100644 --- a/src/syscall/fs.rs +++ b/src/syscall/fs.rs @@ -319,13 +319,13 @@ fn call_normal( let mut nums = arrayvec::ArrayVec::<_, 2>::new(); - let current_lock = context::current(); + let current_lock_ref = context::current(); let consume = flags.contains(CallFlags::CONSUME); let mut fds = fds.iter(); let (target_file, scheme) = { let fd = FileHandle::from(fds.next().copied().unwrap()); - let mut current = current_lock.read(token.token()); + let mut current = current_lock_ref.read(token.token()); let (file, mut split_token) = match (current.token_split(), consume) { ((ctxt, mut split_token), true) => { @@ -346,7 +346,7 @@ fn call_normal( for &fd in fds { let fd = FileHandle::from(fd); - let mut current = current_lock.read(token.token()); + let mut current = current_lock_ref.read(token.token()); let (file, mut split_token) = match (current.token_split(), consume) { ((ctxt, mut split_token), true) => { @@ -363,6 +363,9 @@ fn call_normal( nums.push(desc.number); } + // must never be held when context::switch is called, or it will (predictably) panic + drop(current_lock_ref); + if flags.contains(CallFlags::STD_FS) { scheme.translate_std_fs_call( &nums, diff --git a/src/syscall/futex.rs b/src/syscall/futex.rs index c8e1deed..0bccc9f7 100644 --- a/src/syscall/futex.rs +++ b/src/syscall/futex.rs @@ -115,9 +115,9 @@ pub fn futex( .map(|buf| unsafe { buf.read_exact::() }) .transpose()?; - let context_lock = context::current(); - { + let context_lock = context::current(); + // TODO: Lock ordering violation let mut token = unsafe { CleanLockToken::new() }; let mut futexes = FUTEXES.lock(token.token()); @@ -196,11 +196,9 @@ pub fn futex( context::switch(token); - let context = context_lock.read(token.token()); - // The scheduler clears `wake` on timeout. Hence if a timeout was // set and `wake` is now `None`, we timed out. - if context.wake.is_none() && timeout_opt.is_some() { + if context::current().read(token.token()).wake.is_none() && timeout_opt.is_some() { Err(Error::new(ETIMEDOUT)) } else { Ok(0) diff --git a/src/syscall/process.rs b/src/syscall/process.rs index 7812436b..4045e25e 100644 --- a/src/syscall/process.rs +++ b/src/syscall/process.rs @@ -35,7 +35,7 @@ use super::usercopy::UserSliceWo; /// SAFETY: Returns Never type, all things that implement `Drop` must be dropped manually before calling this to prevent memory leak. /// pub fn exit_this_context(excp: Option, token: &mut CleanLockToken) -> ! { - let context_lock = context::current(); + let context_lock = Arc::clone(&*context::current()); let (addrspace_opt, mut close_files) = { let mut context = context_lock.write(token.token()); let (context, token) = context.token_split(); @@ -70,12 +70,14 @@ pub fn exit_this_context(excp: Option, token: &mut CleanLock ); } { - if !context::contexts_mut(token.downgrade()).remove(&ContextRef(context_lock)) { + let context_ref = ContextRef(context_lock); + if !context::contexts_mut(token.downgrade()).remove(&context_ref) { #[cfg(feature = "drop_panic")] { panic!("This context is not in the cpu") } } + drop(context_ref); } drop(close_files); context::switch(token); diff --git a/src/syscall/time.rs b/src/syscall/time.rs index e5b64fa0..69044638 100644 --- a/src/syscall/time.rs +++ b/src/syscall/time.rs @@ -36,9 +36,9 @@ pub fn nanosleep( let start = time::monotonic(token); let end = start + req.to_nanos(); - let current_context = context::current(); { - let context = current_context.upgradeable_read(token.token()); + let context_lock = context::current(); + let context = context_lock.upgradeable_read(token.token()); if let Some((tctl, pctl, _)) = context.sigcontrol() && tctl.currently_pending_unblocked(pctl) != 0 @@ -54,7 +54,11 @@ pub fn nanosleep( // reason? context::switch(token); - let was_interrupted = current_context.write(token.token()).wake.take().is_some(); + let was_interrupted = context::current() + .write(token.token()) + .wake + .take() + .is_some(); if let Some(rem_buf) = rem_buf_opt { let current = time::monotonic(token);