From 782ec87f27ae4cb3ebc73a63cfdb63074339fe92 Mon Sep 17 00:00:00 2001 From: bjorn3 <17426603+bjorn3@users.noreply.github.com> Date: Sun, 25 Feb 2024 17:07:41 +0100 Subject: [PATCH] Fix UB in the locking code of context switching The spin crate considers it UB to call force_write_unlock while there is a threads trying to obtain a read lock on the same rwlock. This also switches to the spinning_lot crate for the context rwlock as it has support for a write guard keeping a reference to the rwlock using an Arc instead of a reference. --- Cargo.lock | 18 ++++++++++---- Cargo.toml | 4 ++-- src/context/list.rs | 29 +++++++++++++---------- src/context/mod.rs | 3 ++- src/context/switch.rs | 48 ++++++++++++++++---------------------- src/scheme/proc.rs | 3 ++- src/scheme/user.rs | 11 +++++---- src/sync/wait_condition.rs | 5 ++-- src/syscall/futex.rs | 3 ++- 9 files changed, 68 insertions(+), 56 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 8c4a9e5614..74337ae449 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -126,7 +126,8 @@ dependencies = [ "rustc-cfg", "rustc-demangle", "slab_allocator", - "spin 0.9.0", + "spin 0.9.8", + "spinning_top 0.3.0", "toml", "x86", ] @@ -146,7 +147,7 @@ version = "0.9.1" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "549ce1740e46b291953c4340adcd74c59bcf4308f4cac050fd33ba91b7168f4a" dependencies = [ - "spinning_top", + "spinning_top 0.2.4", ] [[package]] @@ -311,9 +312,9 @@ checksum = "6e63cff320ae2c57904679ba7cb63280a3dc4613885beafb148ee7bf9aa9042d" [[package]] name = "spin" -version = "0.9.0" +version = "0.9.8" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "b87bbf98cb81332a56c1ee8929845836f85e8ddd693157c30d76660196014478" +checksum = "6980e8d7511241f8acf4aebddbb1ff938df5eebe98691418c4468d0b72a96a67" dependencies = [ "lock_api", ] @@ -327,6 +328,15 @@ dependencies = [ "lock_api", ] +[[package]] +name = "spinning_top" +version = "0.3.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "d96d2d1d716fb500937168cc09353ffdc7a012be8475ac7308e1bdf0e3923300" +dependencies = [ + "lock_api", +] + [[package]] name = "syn" version = "2.0.32" diff --git a/Cargo.toml b/Cargo.toml index 83f226332f..59fa0b5a1b 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -18,8 +18,8 @@ log = "0.4" redox-path = "0.2.0" redox_syscall = { path = "syscall" } slab_allocator = { path = "slab_allocator", optional = true } -# FIXME: There is some undefined behavior probably in the kernel, which forces us to use spin 0.9.0 and not 0.9.2. -spin = "=0.9.0" +spin = "0.9.8" +spinning_top = { version = "0.3", features = ["arc_lock"] } rmm = { path = "rmm", default-features = false } [dependencies.goblin] diff --git a/src/context/list.rs b/src/context/list.rs index 99c94eb86a..e68b5ddf9c 100644 --- a/src/context/list.rs +++ b/src/context/list.rs @@ -1,7 +1,7 @@ use alloc::{collections::BTreeMap, sync::Arc}; use core::{iter, mem}; -use spin::RwLock; +use spinning_top::RwSpinlock; use super::context::{Context, ContextId}; use crate::syscall::error::{Error, Result, EAGAIN}; @@ -9,7 +9,7 @@ use crate::syscall::error::{Error, Result, EAGAIN}; /// Context list type pub struct ContextList { // Using a BTreeMap for it's range method - map: BTreeMap>>, + map: BTreeMap>>, next_id: usize, } @@ -23,7 +23,7 @@ impl ContextList { } /// Get the nth context. - pub fn get(&self, id: ContextId) -> Option<&Arc>> { + pub fn get(&self, id: ContextId) -> Option<&Arc>> { self.map.get(&id) } @@ -31,7 +31,7 @@ impl ContextList { pub fn ancestors( &'_ self, id: ContextId, - ) -> impl Iterator>)> + '_ { + ) -> impl Iterator>)> + '_ { iter::successors( self.get(id).map(|context| (id, context)), move |(_id, context)| { @@ -43,25 +43,30 @@ impl ContextList { } /// Get the current context. - pub fn current(&self) -> Option<&Arc>> { + pub fn current(&self) -> Option<&Arc>> { self.map.get(&super::context_id()) } - pub fn iter(&self) -> ::alloc::collections::btree_map::Iter>> { + pub fn iter( + &self, + ) -> ::alloc::collections::btree_map::Iter>> { self.map.iter() } pub fn range( &self, range: impl core::ops::RangeBounds, - ) -> ::alloc::collections::btree_map::Range<'_, ContextId, Arc>> { + ) -> ::alloc::collections::btree_map::Range<'_, ContextId, Arc>> { self.map.range(range) } - pub(crate) fn insert_context_raw(&mut self, id: ContextId) -> Result<&Arc>> { + pub(crate) fn insert_context_raw( + &mut self, + id: ContextId, + ) -> Result<&Arc>> { assert!(self .map - .insert(id, Arc::new(RwLock::new(Context::new(id)?))) + .insert(id, Arc::new(RwSpinlock::new(Context::new(id)?))) .is_none()); Ok(self @@ -71,7 +76,7 @@ impl ContextList { } /// Create a new context. - pub fn new_context(&mut self) -> Result<&Arc>> { + pub fn new_context(&mut self) -> Result<&Arc>> { // Zero is not a valid context ID, therefore add 1. // // FIXME: Ensure the number of CPUs can't switch between new_context calls. @@ -98,7 +103,7 @@ impl ContextList { } /// Spawn a context from a function. - pub fn spawn(&mut self, func: extern "C" fn()) -> Result<&Arc>> { + pub fn spawn(&mut self, func: extern "C" fn()) -> Result<&Arc>> { let context_lock = self.new_context()?; { let mut context = context_lock.write(); @@ -129,7 +134,7 @@ impl ContextList { Ok(context_lock) } - pub fn remove(&mut self, id: ContextId) -> Option>> { + pub fn remove(&mut self, id: ContextId) -> Option>> { self.map.remove(&id) } } diff --git a/src/context/mod.rs b/src/context/mod.rs index df0e5c0935..8639b20448 100644 --- a/src/context/mod.rs +++ b/src/context/mod.rs @@ -5,6 +5,7 @@ use alloc::{borrow::Cow, sync::Arc}; use spin::{RwLock, RwLockReadGuard, RwLockWriteGuard}; +use spinning_top::RwSpinlock; use crate::{ paging::{RmmA, RmmArch, TableKind}, @@ -102,7 +103,7 @@ pub fn context_id() -> ContextId { PercpuBlock::current().switch_internals.context_id() } -pub fn current() -> Result>> { +pub fn current() -> Result>> { contexts() .current() .ok_or(Error::new(ESRCH)) diff --git a/src/context/switch.rs b/src/context/switch.rs index a6d760f7f4..8870e97168 100644 --- a/src/context/switch.rs +++ b/src/context/switch.rs @@ -1,8 +1,6 @@ -use core::{cell::Cell, ops::Bound, sync::atomic::Ordering}; +use core::{cell::Cell, mem, ops::Bound, sync::atomic::Ordering}; -use alloc::sync::Arc; - -use spin::{RwLock, RwLockWriteGuard}; +use spinning_top::guard::ArcRwSpinlockWriteGuard; use crate::{ context::{arch, contexts, signal::signal_handler, Context}, @@ -89,8 +87,8 @@ unsafe fn update_runnable(context: &mut Context, cpu_id: LogicalCpuId) -> bool { } struct SwitchResult { - prev_lock: Arc>, - next_lock: Arc>, + _prev_guard: ArcRwSpinlockWriteGuard, + _next_guard: ArcRwSpinlockWriteGuard, } pub fn tick() { @@ -106,13 +104,8 @@ pub fn tick() { } pub unsafe extern "C" fn switch_finish_hook() { - if let Some(SwitchResult { - prev_lock, - next_lock, - }) = PercpuBlock::current().switch_internals.switch_result.take() - { - prev_lock.force_write_unlock(); - next_lock.force_write_unlock(); + if let Some(switch_result) = PercpuBlock::current().switch_internals.switch_result.take() { + drop(switch_result); } else { // TODO: unreachable_unchecked()? crate::arch::stop::emergency_reset(); @@ -151,7 +144,7 @@ pub unsafe fn switch() -> bool { let prev_context_lock = contexts .current() .expect("context::switch: not inside of context"); - let prev_context_guard = prev_context_lock.write(); + let prev_context_guard = prev_context_lock.write_arc(); let idle_id = percpu.switch_internals.idle_id(); let mut skip_idle = true; @@ -179,17 +172,12 @@ pub unsafe fn switch() -> bool { } // Lock next context - let mut next_context_guard = next_context_lock.write(); + let mut next_context_guard = next_context_lock.write_arc(); // Update state of next context and check if runnable if update_runnable(&mut *next_context_guard, cpu_id) { // Store locks for previous and next context - switch_context_opt = Some(( - Arc::clone(prev_context_lock), - RwLockWriteGuard::leak(prev_context_guard) as *mut Context, - Arc::clone(next_context_lock), - RwLockWriteGuard::leak(next_context_guard) as *mut Context, - )); + switch_context_opt = Some((prev_context_guard, next_context_guard)); break; } else { continue; @@ -198,16 +186,14 @@ pub unsafe fn switch() -> bool { }; // Switch process states, TSS stack pointer, and store new context ID - if let Some((prev_context_lock, prev_context_ptr, next_context_lock, next_context_ptr)) = - switch_context_opt - { + if let Some((mut prev_context_guard, mut next_context_guard)) = switch_context_opt { // Set old context as not running and update CPU time - let prev_context = &mut *prev_context_ptr; + let prev_context = &mut *prev_context_guard; prev_context.running = false; prev_context.cpu_time += switch_time.saturating_sub(prev_context.switch_time); // Set new context as running and set switch time - let next_context = &mut *next_context_ptr; + let next_context = &mut *next_context_guard; next_context.running = true; next_context.cpu_id = Some(cpu_id); next_context.switch_time = switch_time; @@ -233,12 +219,18 @@ pub unsafe fn switch() -> bool { } } + // FIXME set th switch result in arch::switch_to instead + let prev_context = + mem::transmute::<&'_ mut Context, &'_ mut Context>(&mut *prev_context_guard); + let next_context = + mem::transmute::<&'_ mut Context, &'_ mut Context>(&mut *next_context_guard); + percpu .switch_internals .switch_result .set(Some(SwitchResult { - prev_lock: prev_context_lock, - next_lock: next_context_lock, + _prev_guard: prev_context_guard, + _next_guard: next_context_guard, })); arch::switch_to(prev_context, next_context); diff --git a/src/scheme/proc.rs b/src/scheme/proc.rs index 27063ba9eb..75a306aa7d 100644 --- a/src/scheme/proc.rs +++ b/src/scheme/proc.rs @@ -34,6 +34,7 @@ use core::{ sync::atomic::{AtomicUsize, Ordering}, }; use spin::RwLock; +use spinning_top::RwSpinlock; use super::{CallerCtx, GlobalSchemes, KernelSchemes, OpenResult}; @@ -265,7 +266,7 @@ fn new_handle(handle: Handle) -> Result { Ok(id) } -fn get_context(id: ContextId) -> Result>> { +fn get_context(id: ContextId) -> Result>> { context::contexts() .get(id) .ok_or(Error::new(ENOENT)) diff --git a/src/scheme/user.rs b/src/scheme/user.rs index 35cb5e519d..9305729205 100644 --- a/src/scheme/user.rs +++ b/src/scheme/user.rs @@ -12,6 +12,7 @@ use core::{ }; use hashbrown::hash_map::{Entry, HashMap}; use spin::{Mutex, RwLock}; +use spinning_top::RwSpinlock; use syscall::{ FobtainFdFlags, MunmapFlags, SendFdFlags, MAP_FIXED_NOREPLACE, SKMSG_FOBTAINFD, SKMSG_FRETURNFD, SKMSG_PROVIDE_MMAP, @@ -50,7 +51,7 @@ pub struct UserInner { pub flags: usize, pub scheme_id: SchemeId, next_id: Mutex, - context: Weak>, + context: Weak>, todo: WaitQueue, states: Mutex>, unmounting: AtomicBool, @@ -58,11 +59,11 @@ pub struct UserInner { enum State { Waiting { - context: Weak>, + context: Weak>, fd: Option>>, }, Responded(Response), - Fmap(Weak>), + Fmap(Weak>), Placeholder, } @@ -84,7 +85,7 @@ impl UserInner { handle_id: usize, name: Box, flags: usize, - context: Weak>, + context: Weak>, ) -> UserInner { UserInner { root_id, @@ -274,7 +275,7 @@ impl UserInner { // TODO: Hypothetical accept_head_leak, accept_tail_leak options might be useful for // libc-controlled buffer pools. fn capture_inner( - context_weak: &Weak>, + context_weak: &Weak>, user_buf: UserSlice, ) -> Result> { let (mode, map_flags) = match (READ, WRITE) { diff --git a/src/sync/wait_condition.rs b/src/sync/wait_condition.rs index 002560ad88..7ffedb676f 100644 --- a/src/sync/wait_condition.rs +++ b/src/sync/wait_condition.rs @@ -1,11 +1,12 @@ use alloc::{sync::Arc, vec::Vec}; -use spin::{Mutex, MutexGuard, RwLock}; +use spin::{Mutex, MutexGuard}; +use spinning_top::RwSpinlock; use crate::context::{self, Context}; #[derive(Debug)] pub struct WaitCondition { - contexts: Mutex>>>, + contexts: Mutex>>>, } impl WaitCondition { diff --git a/src/syscall/futex.rs b/src/syscall/futex.rs index 40310fff2a..c0c3dc1133 100644 --- a/src/syscall/futex.rs +++ b/src/syscall/futex.rs @@ -6,6 +6,7 @@ use alloc::{collections::VecDeque, sync::Arc}; use core::sync::atomic::{AtomicU32, Ordering}; use rmm::Arch; use spin::RwLock; +use spinning_top::RwSpinlock; use crate::{ context::{self, memory::AddrSpace, Context}, @@ -26,7 +27,7 @@ type FutexList = VecDeque; pub struct FutexEntry { target_physaddr: PhysicalAddress, - context_lock: Arc>, + context_lock: Arc>, } // TODO: Process-private futexes? In that case, put the futex table in each AddrSpace.