From bd73102513d51bd19533658edf9f77bb2d987c26 Mon Sep 17 00:00:00 2001 From: Tom Date: Sat, 23 Jan 2021 22:30:10 -0700 Subject: [PATCH] Kernel: Fix some race conditions with Lock and waiting/waking threads There is a window between acquiring/releasing the lock with the atomic variables and subsequently waiting or waking threads. With a single processor this window was closed by using a critical section, but this doesn't prevent other processors from running these code paths. To solve this, set a flag in the WaitQueue while holding m_lock which determines if threads should be blocked at all. --- Kernel/Lock.cpp | 38 +++++++++++++++++++++++++------------- Kernel/WaitQueue.cpp | 12 +++++++++--- Kernel/WaitQueue.h | 9 ++++++++- 3 files changed, 42 insertions(+), 17 deletions(-) diff --git a/Kernel/Lock.cpp b/Kernel/Lock.cpp index 7191ef7bba..f1c523054e 100644 --- a/Kernel/Lock.cpp +++ b/Kernel/Lock.cpp @@ -60,7 +60,7 @@ void Lock::lock(Mode mode) Mode current_mode = m_mode; switch (current_mode) { case Mode::Unlocked: { - dbgln("Lock::lock @ {}: acquire {}, currently unlocked", this, mode_to_string(mode)); + dbgln("Lock::lock @ ({}) {}: acquire {}, currently unlocked", this, m_name, mode_to_string(mode)); m_mode = mode; ASSERT(!m_holder); ASSERT(m_shared_holders.is_empty()); @@ -75,6 +75,7 @@ void Lock::lock(Mode mode) #if LOCK_DEBUG current_thread->holding_lock(*this, 1, file, line); #endif + m_queue.should_block(true); m_lock.store(false, AK::memory_order_release); return; } @@ -86,9 +87,9 @@ void Lock::lock(Mode mode) if constexpr (LOCK_TRACE_DEBUG) { if (mode == Mode::Exclusive) - dbgln("Lock::lock @ {}: acquire {}, currently exclusive, holding: {}", this, mode_to_string(mode), m_times_locked); + dbgln("Lock::lock @ {} ({}): acquire {}, currently exclusive, holding: {}", this, m_name, mode_to_string(mode), m_times_locked); else - dbgln("Lock::lock @ {}: acquire exclusive (requested {}), currently exclusive, holding: {}", this, mode_to_string(mode), m_times_locked); + dbgln("Lock::lock @ {} ({}): acquire exclusive (requested {}), currently exclusive, holding: {}", this, m_name, mode_to_string(mode), m_times_locked); } ASSERT(mode == Mode::Exclusive || mode == Mode::Shared); @@ -105,7 +106,7 @@ void Lock::lock(Mode mode) if (mode != Mode::Shared) break; - dbgln("Lock::lock @ {}: acquire {}, currently shared, locks held {}", this, mode_to_string(mode), m_times_locked); + dbgln("Lock::lock @ {} ({}): acquire {}, currently shared, locks held {}", this, m_name, mode_to_string(mode), m_times_locked); ASSERT(m_times_locked > 0); m_times_locked++; @@ -125,7 +126,9 @@ void Lock::lock(Mode mode) ASSERT_NOT_REACHED(); } m_lock.store(false, AK::memory_order_release); + dbgln("Lock::lock @ {} ({}) waiting...", this, m_name); m_queue.wait_on({}, m_name); + dbgln("Lock::lock @ {} ({}) waited", this, m_name); } } @@ -141,9 +144,9 @@ void Lock::unlock() Mode current_mode = m_mode; if constexpr (LOCK_TRACE_DEBUG) { if (current_mode == Mode::Shared) - dbgln("Lock::unlock @ {}: release {}, locks held: {}", this, mode_to_string(current_mode), m_times_locked); + dbgln("Lock::unlock @ {} ({}): release {}, locks held: {}", this, m_name, mode_to_string(current_mode), m_times_locked); else - dbgln("Lock::unlock @ {}: release {}, holding: {}", this, mode_to_string(current_mode), m_times_locked); + dbgln("Lock::unlock @ {} ({}): release {}, holding: {}", this, m_name, mode_to_string(current_mode), m_times_locked); } ASSERT(current_mode != Mode::Unlocked); @@ -174,9 +177,11 @@ void Lock::unlock() ASSERT_NOT_REACHED(); } - if (m_times_locked == 0) { + bool unlocked_last = (m_times_locked == 0); + if (unlocked_last) { ASSERT(current_mode == Mode::Exclusive ? !m_holder : m_shared_holders.is_empty()); m_mode = Mode::Unlocked; + m_queue.should_block(false); } #if LOCK_DEBUG @@ -184,7 +189,10 @@ void Lock::unlock() #endif m_lock.store(false, AK::memory_order_release); - m_queue.wake_one(); + if (unlocked_last) { + u32 did_wake = m_queue.wake_one(); + dbgln("Lock::unlock @ {} ({}) wake one ({})", this, m_name, did_wake); + } return; } // I don't know *who* is using "m_lock", so just yield. @@ -212,16 +220,16 @@ auto Lock::force_unlock_if_locked(u32& lock_count_to_restore) -> Mode } dbgln("Lock::force_unlock_if_locked @ {}: unlocking exclusive with lock count: {}", this, m_times_locked); - +#if LOCK_DEBUG + m_holder->holding_lock(*this, -(int)lock_count_to_restore); +#endif m_holder = nullptr; ASSERT(m_times_locked > 0); lock_count_to_restore = m_times_locked; m_times_locked = 0; m_mode = Mode::Unlocked; + m_queue.should_block(false); m_lock.store(false, AK::memory_order_release); -#if LOCK_DEBUG - m_holder->holding_lock(*this, -(int)lock_count_to_restore); -#endif previous_mode = Mode::Exclusive; break; } @@ -246,8 +254,10 @@ auto Lock::force_unlock_if_locked(u32& lock_count_to_restore) -> Mode m_shared_holders.remove(it); ASSERT(m_times_locked >= lock_count_to_restore); m_times_locked -= lock_count_to_restore; - if (m_times_locked == 0) + if (m_times_locked == 0) { m_mode = Mode::Unlocked; + m_queue.should_block(false); + } m_lock.store(false, AK::memory_order_release); previous_mode = Mode::Shared; break; @@ -300,6 +310,7 @@ void Lock::restore_lock(Mode mode, u32 lock_count) ASSERT(!m_holder); ASSERT(m_shared_holders.is_empty()); m_holder = current_thread; + m_queue.should_block(true); m_lock.store(false, AK::memory_order_release); #if LOCK_DEBUG m_holder->holding_lock(*this, (int)lock_count, file, line); @@ -321,6 +332,7 @@ void Lock::restore_lock(Mode mode, u32 lock_count) auto set_result = m_shared_holders.set(current_thread, lock_count); // There may be other shared lock holders already, but we should not have an entry yet ASSERT(set_result == AK::HashSetResult::InsertedNewEntry); + m_queue.should_block(true); m_lock.store(false, AK::memory_order_release); #if LOCK_DEBUG m_holder->holding_lock(*this, (int)lock_count, file, line); diff --git a/Kernel/WaitQueue.cpp b/Kernel/WaitQueue.cpp index ce2d74bba0..6d6f3d9611 100644 --- a/Kernel/WaitQueue.cpp +++ b/Kernel/WaitQueue.cpp @@ -35,17 +35,18 @@ bool WaitQueue::should_add_blocker(Thread::Blocker& b, void* data) ASSERT(data != nullptr); // Thread that is requesting to be blocked ASSERT(m_lock.is_locked()); ASSERT(b.blocker_type() == Thread::Blocker::Type::Queue); - if (m_wake_requested) { + if (m_wake_requested || !m_should_block) { m_wake_requested = false; - dbgln("WaitQueue @ {}: do not block thread {}, wake was pending", this, data); + dbgln("WaitQueue @ {}: do not block thread {}, {}", this, data, m_should_block ? "wake was pending" : "not blocking"); return false; } dbgln("WaitQueue @ {}: should block thread {}", this, data); return true; } -void WaitQueue::wake_one() +u32 WaitQueue::wake_one() { + u32 did_wake = 0; ScopedSpinLock lock(m_lock); dbgln("WaitQueue @ {}: wake_one", this); bool did_unblock_one = do_unblock([&](Thread::Blocker& b, void* data, bool& stop_iterating) { @@ -55,11 +56,14 @@ void WaitQueue::wake_one() dbgln("WaitQueue @ {}: wake_one unblocking {}", this, data); if (blocker.unblock()) { stop_iterating = true; + did_wake = 1; return true; } return false; }); m_wake_requested = !did_unblock_one; + dbgln("WaitQueue @ {}: wake_one woke {} threads", this, did_wake); + return did_wake; } u32 WaitQueue::wake_n(u32 wake_count) @@ -84,6 +88,7 @@ u32 WaitQueue::wake_n(u32 wake_count) return false; }); m_wake_requested = !did_unblock_some; + dbgln("WaitQueue @ {}: wake_n({}) woke {} threads", this, wake_count, did_wake); return did_wake; } @@ -108,6 +113,7 @@ u32 WaitQueue::wake_all() return false; }); m_wake_requested = !did_unblock_any; + dbgln("WaitQueue @ {}: wake_all woke {} threads", this, did_wake); return did_wake; } diff --git a/Kernel/WaitQueue.h b/Kernel/WaitQueue.h index 338844383e..0121273958 100644 --- a/Kernel/WaitQueue.h +++ b/Kernel/WaitQueue.h @@ -34,10 +34,16 @@ namespace Kernel { class WaitQueue : public Thread::BlockCondition { public: - void wake_one(); + u32 wake_one(); u32 wake_n(u32 wake_count); u32 wake_all(); + void should_block(bool block) + { + ScopedSpinLock lock(m_lock); + m_should_block = block; + } + template Thread::BlockResult wait_on(const Thread::BlockTimeout& timeout, Args&&... args) { @@ -49,6 +55,7 @@ protected: private: bool m_wake_requested { false }; + bool m_should_block { true }; }; }