From 6542a037dfd87e12ecb1915c748b6d7acd863501 Mon Sep 17 00:00:00 2001 From: Bananymous Date: Fri, 30 May 2025 21:59:07 +0300 Subject: [PATCH] Kernel: Make spinlocks more safe Kernel now makes sure thread is not holding any spinlocks when it tries to lock a mutex or yield. Spinlocks are supposed to be only used for short times without the possibility of yielding --- kernel/CMakeLists.txt | 1 + kernel/include/kernel/Lock/Mutex.h | 4 ++ kernel/include/kernel/Lock/SpinLock.h | 37 ++---------------- kernel/include/kernel/Thread.h | 5 +++ kernel/kernel/Lock/SpinLock.cpp | 55 +++++++++++++++++++++++++++ kernel/kernel/Processor.cpp | 2 + kernel/kernel/Thread.cpp | 2 + 7 files changed, 72 insertions(+), 34 deletions(-) create mode 100644 kernel/kernel/Lock/SpinLock.cpp diff --git a/kernel/CMakeLists.txt b/kernel/CMakeLists.txt index 2f4e3bf489..dcc2276f84 100644 --- a/kernel/CMakeLists.txt +++ b/kernel/CMakeLists.txt @@ -43,6 +43,7 @@ set(KERNEL_SOURCES kernel/Interruptable.cpp kernel/InterruptController.cpp kernel/kernel.cpp + kernel/Lock/SpinLock.cpp kernel/Memory/DMARegion.cpp kernel/Memory/FileBackedRegion.cpp kernel/Memory/Heap.cpp diff --git a/kernel/include/kernel/Lock/Mutex.h b/kernel/include/kernel/Lock/Mutex.h index db9946afe5..a3aae89f12 100644 --- a/kernel/include/kernel/Lock/Mutex.h +++ b/kernel/include/kernel/Lock/Mutex.h @@ -88,6 +88,8 @@ namespace Kernel void lock() { const auto tid = Thread::current_tid(); + ASSERT(!tid || !Thread::current().has_spinlock()); + if (tid == m_locker) ASSERT(m_lock_depth > 0); else @@ -111,6 +113,8 @@ namespace Kernel bool try_lock() { const auto tid = Thread::current_tid(); + ASSERT(!tid || !Thread::current().has_spinlock()); + if (tid == m_locker) ASSERT(m_lock_depth > 0); else diff --git a/kernel/include/kernel/Lock/SpinLock.h b/kernel/include/kernel/Lock/SpinLock.h index ff2f6fa5a0..a10d43386e 100644 --- a/kernel/include/kernel/Lock/SpinLock.h +++ b/kernel/include/kernel/Lock/SpinLock.h @@ -18,42 +18,11 @@ namespace Kernel public: SpinLock() = default; - InterruptState lock() - { - auto state = Processor::get_interrupt_state(); - Processor::set_interrupt_state(InterruptState::Disabled); + InterruptState lock(); - auto id = Processor::current_id().as_u32(); - ASSERT(m_locker.load(BAN::MemoryOrder::memory_order_relaxed) != id); + bool try_lock_interrupts_disabled(); - auto expected = PROCESSOR_NONE.as_u32(); - while (!m_locker.compare_exchange(expected, id, BAN::MemoryOrder::memory_order_acquire)) - { - Processor::pause(); - expected = PROCESSOR_NONE.as_u32(); - } - - return state; - } - - bool try_lock_interrupts_disabled() - { - ASSERT(Processor::get_interrupt_state() == InterruptState::Disabled); - - auto id = Processor::current_id().as_u32(); - ASSERT(m_locker.load(BAN::MemoryOrder::memory_order_relaxed) != id); - - auto expected = PROCESSOR_NONE.as_u32(); - return m_locker.compare_exchange(expected, id, BAN::MemoryOrder::memory_order_acquire); - } - - void unlock(InterruptState state) - { - ASSERT(Processor::get_interrupt_state() == InterruptState::Disabled); - ASSERT(current_processor_has_lock()); - m_locker.store(PROCESSOR_NONE.as_u32(), BAN::MemoryOrder::memory_order_release); - Processor::set_interrupt_state(state); - } + void unlock(InterruptState state); bool current_processor_has_lock() const { diff --git a/kernel/include/kernel/Thread.h b/kernel/include/kernel/Thread.h index c9c2cd80ed..4bb081dcd7 100644 --- a/kernel/include/kernel/Thread.h +++ b/kernel/include/kernel/Thread.h @@ -101,6 +101,10 @@ namespace Kernel void save_sse(); void load_sse(); + void add_spinlock() { m_spinlock_count++; } + void remove_spinlock() { m_spinlock_count--; } + bool has_spinlock() const { return !!m_spinlock_count; } + void add_mutex() { m_mutex_count++; } void remove_mutex() { m_mutex_count--; } @@ -137,6 +141,7 @@ namespace Kernel SpinLock m_signal_lock; static_assert(_SIGMAX < 64); + BAN::Atomic m_spinlock_count { 0 }; BAN::Atomic m_mutex_count { 0 }; alignas(16) uint8_t m_sse_storage[512] {}; diff --git a/kernel/kernel/Lock/SpinLock.cpp b/kernel/kernel/Lock/SpinLock.cpp new file mode 100644 index 0000000000..2a61d0f46b --- /dev/null +++ b/kernel/kernel/Lock/SpinLock.cpp @@ -0,0 +1,55 @@ +#include +#include + +namespace Kernel +{ + + InterruptState SpinLock::lock() + { + auto state = Processor::get_interrupt_state(); + Processor::set_interrupt_state(InterruptState::Disabled); + + auto id = Processor::current_id().as_u32(); + ASSERT(m_locker.load(BAN::MemoryOrder::memory_order_relaxed) != id); + + auto expected = PROCESSOR_NONE.as_u32(); + while (!m_locker.compare_exchange(expected, id, BAN::MemoryOrder::memory_order_acquire)) + { + Processor::pause(); + expected = PROCESSOR_NONE.as_u32(); + } + + if (Thread::current_tid()) + Thread::current().add_spinlock(); + + return state; + } + + bool SpinLock::try_lock_interrupts_disabled() + { + ASSERT(Processor::get_interrupt_state() == InterruptState::Disabled); + + auto id = Processor::current_id().as_u32(); + ASSERT(m_locker.load(BAN::MemoryOrder::memory_order_relaxed) != id); + + auto expected = PROCESSOR_NONE.as_u32(); + if (!m_locker.compare_exchange(expected, id, BAN::MemoryOrder::memory_order_acquire)) + return false; + + if (Thread::current_tid()) + Thread::current().add_spinlock(); + + return true; + } + + void SpinLock::unlock(InterruptState state) + { + ASSERT(Processor::get_interrupt_state() == InterruptState::Disabled); + ASSERT(current_processor_has_lock()); + m_locker.store(PROCESSOR_NONE.as_u32(), BAN::MemoryOrder::memory_order_release); + if (Thread::current_tid()) + Thread::current().remove_spinlock(); + Processor::set_interrupt_state(state); + } + +} diff --git a/kernel/kernel/Processor.cpp b/kernel/kernel/Processor.cpp index 3258f8a477..8a30dce035 100644 --- a/kernel/kernel/Processor.cpp +++ b/kernel/kernel/Processor.cpp @@ -337,6 +337,8 @@ namespace Kernel auto state = get_interrupt_state(); set_interrupt_state(InterruptState::Disabled); + ASSERT(!Thread::current().has_spinlock()); + auto& processor_info = s_processors[current_id().as_u32()]; { diff --git a/kernel/kernel/Thread.cpp b/kernel/kernel/Thread.cpp index 952b894a81..542e4f38f4 100644 --- a/kernel/kernel/Thread.cpp +++ b/kernel/kernel/Thread.cpp @@ -140,6 +140,8 @@ namespace Kernel pid_t Thread::current_tid() { + if (Processor::count() == 0) + return 0; return Processor::scheduler().current_tid(); }