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

@@ -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"

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]

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<ContextId, Arc<RwLock<Context>>>,
+    map: BTreeMap<ContextId, Arc<RwSpinlock<Context>>>,
     next_id: usize,
 }
 
@@ -23,7 +23,7 @@ impl ContextList {
     }
 
     /// Get the nth context.
-    pub fn get(&self, id: ContextId) -> Option<&Arc<RwLock<Context>>> {
+    pub fn get(&self, id: ContextId) -> Option<&Arc<RwSpinlock<Context>>> {
         self.map.get(&id)
     }
 
@@ -31,7 +31,7 @@ impl ContextList {
     pub fn ancestors(
         &'_ self,
         id: ContextId,
-    ) -> impl Iterator<Item = (ContextId, &Arc<RwLock<Context>>)> + '_ {
+    ) -> impl Iterator<Item = (ContextId, &Arc<RwSpinlock<Context>>)> + '_ {
         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<RwLock<Context>>> {
+    pub fn current(&self) -> Option<&Arc<RwSpinlock<Context>>> {
         self.map.get(&super::context_id())
     }
 
-    pub fn iter(&self) -> ::alloc::collections::btree_map::Iter<ContextId, Arc<RwLock<Context>>> {
+    pub fn iter(
+        &self,
+    ) -> ::alloc::collections::btree_map::Iter<ContextId, Arc<RwSpinlock<Context>>> {
         self.map.iter()
     }
 
     pub fn range(
         &self,
         range: impl core::ops::RangeBounds<ContextId>,
-    ) -> ::alloc::collections::btree_map::Range<'_, ContextId, Arc<RwLock<Context>>> {
+    ) -> ::alloc::collections::btree_map::Range<'_, ContextId, Arc<RwSpinlock<Context>>> {
         self.map.range(range)
     }
 
-    pub(crate) fn insert_context_raw(&mut self, id: ContextId) -> Result<&Arc<RwLock<Context>>> {
+    pub(crate) fn insert_context_raw(
+        &mut self,
+        id: ContextId,
+    ) -> Result<&Arc<RwSpinlock<Context>>> {
         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<RwLock<Context>>> {
+    pub fn new_context(&mut self) -> Result<&Arc<RwSpinlock<Context>>> {
         // 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<RwLock<Context>>> {
+    pub fn spawn(&mut self, func: extern "C" fn()) -> Result<&Arc<RwSpinlock<Context>>> {
         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<Arc<RwLock<Context>>> {
+    pub fn remove(&mut self, id: ContextId) -> Option<Arc<RwSpinlock<Context>>> {
         self.map.remove(&id)
     }
 }

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<Arc<RwLock<Context>>> {
+pub fn current() -> Result<Arc<RwSpinlock<Context>>> {
     contexts()
         .current()
         .ok_or(Error::new(ESRCH))

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<RwLock<Context>>,
-    next_lock: Arc<RwLock<Context>>,
+    _prev_guard: ArcRwSpinlockWriteGuard<Context>,
+    _next_guard: ArcRwSpinlockWriteGuard<Context>,
 }
 
 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);

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<usize> {
     Ok(id)
 }
 
-fn get_context(id: ContextId) -> Result<Arc<RwLock<Context>>> {
+fn get_context(id: ContextId) -> Result<Arc<RwSpinlock<Context>>> {
     context::contexts()
         .get(id)
         .ok_or(Error::new(ENOENT))

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<u64>,
-    context: Weak<RwLock<Context>>,
+    context: Weak<RwSpinlock<Context>>,
     todo: WaitQueue<Packet>,
     states: Mutex<HashMap<u64, State>>,
     unmounting: AtomicBool,
@@ -58,11 +59,11 @@ pub struct UserInner {
 
 enum State {
     Waiting {
-        context: Weak<RwLock<Context>>,
+        context: Weak<RwSpinlock<Context>>,
         fd: Option<Arc<RwLock<FileDescription>>>,
     },
     Responded(Response),
-    Fmap(Weak<RwLock<Context>>),
+    Fmap(Weak<RwSpinlock<Context>>),
     Placeholder,
 }
 
@@ -84,7 +85,7 @@ impl UserInner {
         handle_id: usize,
         name: Box<str>,
         flags: usize,
-        context: Weak<RwLock<Context>>,
+        context: Weak<RwSpinlock<Context>>,
     ) -> 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<const READ: bool, const WRITE: bool>(
-        context_weak: &Weak<RwLock<Context>>,
+        context_weak: &Weak<RwSpinlock<Context>>,
         user_buf: UserSlice<READ, WRITE>,
     ) -> Result<CaptureGuard<READ, WRITE>> {
         let (mode, map_flags) = match (READ, WRITE) {

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<Vec<Arc<RwLock<Context>>>>,
+    contexts: Mutex<Vec<Arc<RwSpinlock<Context>>>>,
 }
 
 impl WaitCondition {

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<FutexEntry>;
 
 pub struct FutexEntry {
     target_physaddr: PhysicalAddress,
-    context_lock: Arc<RwLock<Context>>,
+    context_lock: Arc<RwSpinlock<Context>>,
 }
 
 // TODO: Process-private futexes? In that case, put the futex table in each AddrSpace.