From 7a29900ea21793a2b71e8694b816770c338ae530 Mon Sep 17 00:00:00 2001 From: ion098 <146852218+ion098@users.noreply.github.com> Date: Wed, 18 Sep 2024 13:52:22 -0700 Subject: [PATCH 1/3] fix: :bug: Prevent mutexes from deadlocking kernel --- packages/kernel/src/sync/mutex.rs | 24 +++++++++--------------- 1 file changed, 9 insertions(+), 15 deletions(-) diff --git a/packages/kernel/src/sync/mutex.rs b/packages/kernel/src/sync/mutex.rs index bc609a3..3a51a6d 100644 --- a/packages/kernel/src/sync/mutex.rs +++ b/packages/kernel/src/sync/mutex.rs @@ -1,23 +1,23 @@ -use core::sync::atomic::{AtomicU8, Ordering}; +use std::cell::UnsafeCell; +use critical_section::RestoreState; pub use lock_api::MutexGuard; pub type Mutex = lock_api::Mutex; -struct MutexState(AtomicU8); +struct MutexState(UnsafeCell); impl MutexState { const fn new() -> Self { - Self(AtomicU8::new(0)) + Self(UnsafeCell::new(RestoreState::invalid())) } /// Returns true if the lock was acquired. fn try_lock(&self) -> bool { - self.0 - .compare_exchange(0, 1, Ordering::Acquire, Ordering::Acquire) - .is_ok() + unsafe { *self.0.get() = critical_section::acquire() } + true } fn unlock(&self) { - self.0.store(0, Ordering::Release); + unsafe { critical_section::release(self.0.into_inner()) } } } @@ -42,11 +42,7 @@ unsafe impl lock_api::RawMutex for RawMutex { type GuardMarker = lock_api::GuardSend; fn lock(&self) { - critical_section::with(|_| { - while !self.state.try_lock() { - core::hint::spin_loop(); - } - }) + critical_section::with(|_| self.state.try_lock()) } fn try_lock(&self) -> bool { @@ -54,8 +50,6 @@ unsafe impl lock_api::RawMutex for RawMutex { } unsafe fn unlock(&self) { - critical_section::with(|_| { - self.state.unlock(); - }) + critical_section::with(|_| self.state.unlock()) } } From da15505819502c97e03e12f34b6d12d19d1e7e67 Mon Sep 17 00:00:00 2001 From: ion098 <146852218+ion098@users.noreply.github.com> Date: Thu, 19 Sep 2024 08:56:29 -0700 Subject: [PATCH 2/3] fix: :bug: Prevent mutex from being acquired twice --- packages/kernel/src/sync/mutex.rs | 43 +++++++++++++++++++++++++------ 1 file changed, 35 insertions(+), 8 deletions(-) diff --git a/packages/kernel/src/sync/mutex.rs b/packages/kernel/src/sync/mutex.rs index 3a51a6d..3bcde4c 100644 --- a/packages/kernel/src/sync/mutex.rs +++ b/packages/kernel/src/sync/mutex.rs @@ -1,19 +1,37 @@ -use std::cell::UnsafeCell; +use std::{cell::UnsafeCell, hint::spin_loop}; use critical_section::RestoreState; pub use lock_api::MutexGuard; pub type Mutex = lock_api::Mutex; -struct MutexState(UnsafeCell); +fn in_interrupt() -> bool { + unsafe { + let mut cpsr: u32; + asm!("mrs {0}, cpsr", out(reg) cpsr); + let is_system_mode = (cpsr & 0b11111) == 0b11111; + !is_system_mode + } +} + +struct MutexState(UnsafeCell, bool); impl MutexState { const fn new() -> Self { - Self(UnsafeCell::new(RestoreState::invalid())) + Self(UnsafeCell::new(RestoreState::invalid()), false) } /// Returns true if the lock was acquired. fn try_lock(&self) -> bool { - unsafe { *self.0.get() = critical_section::acquire() } - true + unsafe { + let state = critical_section::acquire(); + if self.1 { + critical_section::release(state); + false + } else { + *self.0.get() = state; + self.1 = true; + true + } + } } fn unlock(&self) { @@ -42,14 +60,23 @@ unsafe impl lock_api::RawMutex for RawMutex { type GuardMarker = lock_api::GuardSend; fn lock(&self) { - critical_section::with(|_| self.state.try_lock()) + if self.state.try_lock() { + () + } else if in_interrupt() { + panic!("Deadlock in kernel detected!"); + } else { + while !self.state.try_lock() { + spin_loop() + } + () + } } fn try_lock(&self) -> bool { - critical_section::with(|_| self.state.try_lock()) + self.state.try_lock() } unsafe fn unlock(&self) { - critical_section::with(|_| self.state.unlock()) + self.state.unlock() } } From 6ed83a6fd5649e6affad7777523455a3a4261de7 Mon Sep 17 00:00:00 2001 From: ion098 <146852218+ion098@users.noreply.github.com> Date: Thu, 19 Sep 2024 13:01:08 -0700 Subject: [PATCH 3/3] fix: :bug: Prevent interleaved mutex locks/unlocks from breaking mutex --- packages/kernel/src/sync/mutex.rs | 45 ++++++++++++++++++++++--------- 1 file changed, 33 insertions(+), 12 deletions(-) diff --git a/packages/kernel/src/sync/mutex.rs b/packages/kernel/src/sync/mutex.rs index 3bcde4c..8f92aac 100644 --- a/packages/kernel/src/sync/mutex.rs +++ b/packages/kernel/src/sync/mutex.rs @@ -1,4 +1,4 @@ -use std::{cell::UnsafeCell, hint::spin_loop}; +use std::{cell::SyncUnsafeCell, hint::spin_loop, sync::atomic::{AtomicBool, AtomicU32, Ordering}}; use critical_section::RestoreState; pub use lock_api::MutexGuard; @@ -13,29 +13,50 @@ fn in_interrupt() -> bool { } } -struct MutexState(UnsafeCell, bool); +static MUTEX_COUNTER: AtomicU32 = AtomicU32::new(0); +struct GlobalMutexState(SyncUnsafeCell); +/// safety: this variable should only be accessed in a critical section +static GLOBAL_MUTEX_STATE: SyncUnsafeCell = SyncUnsafeCell::new(RestoreState::invalid()); + +struct MutexState(AtomicBool); impl MutexState { const fn new() -> Self { - Self(UnsafeCell::new(RestoreState::invalid()), false) + Self(AtomicBool::new(false)) } /// Returns true if the lock was acquired. fn try_lock(&self) -> bool { unsafe { let state = critical_section::acquire(); - if self.1 { + if self.0.compare_exchange(false, true, Ordering::Acquire, Ordering::Acquire).is_ok() { + if MUTEX_COUNTER.fetch_add(1, Ordering::AcqRel) == 0 { + // we're the first mutex to be locked, we need to save the RestoreState + // to be released later + *GLOBAL_MUTEX_STATE.get() = state; + } else { + // another mutex has already entered a critical section before us so + // we need to release the nested critical section we entered + critical_section::release(state); + } + true + } else { + // the mutex is already locked so release the critical section we acquired critical_section::release(state); false - } else { - *self.0.get() = state; - self.1 = true; - true } } } - fn unlock(&self) { - unsafe { critical_section::release(self.0.into_inner()) } + /// safety: this function must only be called after a successful call to try_lock + unsafe fn unlock(&self) { + unsafe { + if MUTEX_COUNTER.fetch_sub(1, Ordering::AcqRel) == 1 { + // we are the last mutex to be unlocked so we need to unlock the critical section + critical_section::release(GLOBAL_MUTEX_STATE.into()); + } + self.0.store(false, Ordering::Release); + } + () } } @@ -63,7 +84,7 @@ unsafe impl lock_api::RawMutex for RawMutex { if self.state.try_lock() { () } else if in_interrupt() { - panic!("Deadlock in kernel detected!"); + unreachable!("Deadlock in kernel detected!"); } else { while !self.state.try_lock() { spin_loop() @@ -77,6 +98,6 @@ unsafe impl lock_api::RawMutex for RawMutex { } unsafe fn unlock(&self) { - self.state.unlock() + unsafe { self.state.unlock() } } }