From 710cf14c55d5480a2b4f44c3b12c96315dcec8d5 Mon Sep 17 00:00:00 2001 From: Tom Date: Thu, 15 Jul 2021 16:10:52 -0600 Subject: [PATCH] Kernel: Fix some Lock problems and VERIFY statements When a Lock blocks (e.g. due to a mode mismatch or because someone else holds it) the lock mode will be updated to what was requested. There were also some cases where restoring locks may have not worked as intended as it may have been held already by the same thread. Fixes #8787 --- Kernel/Lock.cpp | 128 +++++++++++++++++++++++++++++++++++------------- Kernel/Lock.h | 17 ++++++- 2 files changed, 110 insertions(+), 35 deletions(-) diff --git a/Kernel/Lock.cpp b/Kernel/Lock.cpp index 45cb54208f..6afc672ea5 100644 --- a/Kernel/Lock.cpp +++ b/Kernel/Lock.cpp @@ -55,9 +55,19 @@ void Lock::lock(Mode mode) if (m_holder != current_thread) { block(*current_thread, mode, lock, 1); did_block = true; + // If we blocked then m_mode should have been updated to what we requested + VERIFY(m_mode == mode); } - VERIFY(m_shared_holders.is_empty()); + if (m_mode == Mode::Exclusive) { + VERIFY(m_holder == current_thread); + VERIFY(m_shared_holders.is_empty()); + } else if (did_block && mode == Mode::Shared) { + // Only if we blocked trying to acquire a shared lock the lock would have been converted + VERIFY(!m_holder); + VERIFY(!m_shared_holders.is_empty()); + VERIFY(m_shared_holders.find(current_thread) != m_shared_holders.end()); + } if constexpr (LOCK_TRACE_DEBUG) { if (mode == Mode::Exclusive) @@ -66,10 +76,12 @@ void Lock::lock(Mode mode) dbgln("Lock::lock @ {} ({}): acquire exclusive (requested {}), currently exclusive, holding: {}", this, m_name, mode_to_string(mode), m_times_locked); } - VERIFY(mode == Mode::Exclusive || mode == Mode::Shared); VERIFY(m_times_locked > 0); - if (!did_block) + if (!did_block) { + // if we didn't block we must still be an exclusive lock + VERIFY(m_mode == Mode::Exclusive); m_times_locked++; + } #if LOCK_DEBUG current_thread->holding_lock(*this, 1, location); @@ -78,29 +90,40 @@ void Lock::lock(Mode mode) } case Mode::Shared: { VERIFY(!m_holder); - if (mode == Mode::Exclusive && m_shared_holders.size() == 1) { - auto it = m_shared_holders.begin(); - if (it->key == current_thread) { - it->value++; - m_times_locked++; - m_mode = Mode::Exclusive; - m_holder = current_thread; - m_shared_holders.clear(); - dbgln_if(LOCK_TRACE_DEBUG, "Lock::lock @ {} ({}): acquire {}, converted shared to exclusive lock, locks held {}", this, m_name, mode_to_string(mode), m_times_locked); - return; + if (mode == Mode::Exclusive) { + if (m_shared_holders.size() == 1) { + auto it = m_shared_holders.begin(); + if (it->key == current_thread) { + it->value++; + m_times_locked++; + m_mode = Mode::Exclusive; + m_holder = current_thread; + m_shared_holders.clear(); + dbgln_if(LOCK_TRACE_DEBUG, "Lock::lock @ {} ({}): acquire {}, converted shared to exclusive lock, locks held {}", this, m_name, mode_to_string(mode), m_times_locked); + return; + } } - } - if (mode != Mode::Shared) { + block(*current_thread, mode, lock, 1); did_block = true; + VERIFY(m_mode == mode); } dbgln_if(LOCK_TRACE_DEBUG, "Lock::lock @ {} ({}): acquire {}, currently shared, locks held {}", this, m_name, mode_to_string(mode), m_times_locked); VERIFY(m_times_locked > 0); - if (did_block) { - VERIFY(m_shared_holders.contains(current_thread)); - } else { + if (m_mode == Mode::Shared) { + VERIFY(!m_holder); + VERIFY(!did_block || m_shared_holders.contains(current_thread)); + } else if (did_block) { + VERIFY(mode == Mode::Exclusive); + VERIFY(m_holder == current_thread); + VERIFY(m_shared_holders.is_empty()); + } + + if (!did_block) { + // if we didn't block we must still be a shared lock + VERIFY(m_mode == Mode::Shared); m_times_locked++; VERIFY(!m_shared_holders.is_empty()); auto it = m_shared_holders.find(current_thread); @@ -307,24 +330,47 @@ void Lock::restore_lock(Mode mode, u32 lock_count) ScopedSpinLock lock(m_lock); switch (mode) { case Mode::Exclusive: { - if (m_mode != Mode::Unlocked) { + auto previous_mode = m_mode; + bool need_to_block = false; + if (m_mode == Mode::Exclusive && m_holder != current_thread) + need_to_block = true; + else if (m_mode == Mode::Shared && (m_shared_holders.size() != 1 || !m_shared_holders.contains(current_thread))) + need_to_block = true; + if (need_to_block) { block(*current_thread, Mode::Exclusive, lock, lock_count); did_block = true; + // If we blocked then m_mode should have been updated to what we requested + VERIFY(m_mode == Mode::Exclusive); } - dbgln_if(LOCK_RESTORE_DEBUG, "Lock::restore_lock @ {}: restoring {} with lock count {}, was unlocked", this, mode_to_string(mode), lock_count); + dbgln_if(LOCK_RESTORE_DEBUG, "Lock::restore_lock @ {}: restoring {} with lock count {}, was {}", this, mode_to_string(mode), lock_count, mode_to_string(previous_mode)); + VERIFY(m_mode != Mode::Shared); VERIFY(m_shared_holders.is_empty()); if (did_block) { - VERIFY(m_mode == Mode::Exclusive); VERIFY(m_times_locked > 0); VERIFY(m_holder == current_thread); } else { - m_mode = Mode::Exclusive; - VERIFY(m_times_locked == 0); - m_times_locked = lock_count; - VERIFY(!m_holder); - m_holder = current_thread; + if (m_mode == Mode::Unlocked) { + m_mode = Mode::Exclusive; + VERIFY(m_times_locked == 0); + m_times_locked = lock_count; + VERIFY(!m_holder); + m_holder = current_thread; + } else if (m_mode == Mode::Shared) { + // Upgrade the shared lock to an exclusive lock + VERIFY(!m_holder); + VERIFY(m_shared_holders.size() == 1); + VERIFY(m_shared_holders.contains(current_thread)); + m_mode = Mode::Exclusive; + m_holder = current_thread; + m_shared_holders.clear(); + } else { + VERIFY(m_mode == Mode::Exclusive); + VERIFY(m_holder == current_thread); + VERIFY(m_times_locked > 0); + m_times_locked += lock_count; + } } #if LOCK_DEBUG @@ -334,9 +380,11 @@ void Lock::restore_lock(Mode mode, u32 lock_count) } case Mode::Shared: { auto previous_mode = m_mode; - if (m_mode != Mode::Unlocked && m_mode != Mode::Shared) { + if (m_mode == Mode::Exclusive && m_holder != current_thread) { block(*current_thread, Mode::Shared, lock, lock_count); did_block = true; + // If we blocked then m_mode should have been updated to what we requested + VERIFY(m_mode == Mode::Shared); } dbgln_if(LOCK_RESTORE_DEBUG, "Lock::restore_lock @ {}: restoring {} with lock count {}, was {}", @@ -344,15 +392,29 @@ void Lock::restore_lock(Mode mode, u32 lock_count) VERIFY(!m_holder); if (did_block) { - VERIFY(m_mode == Mode::Shared); VERIFY(m_times_locked > 0); VERIFY(m_shared_holders.contains(current_thread)); } else { - m_mode = Mode::Shared; - m_times_locked += 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 - VERIFY(set_result == AK::HashSetResult::InsertedNewEntry); + if (m_mode == Mode::Unlocked) { + m_mode = Mode::Shared; + m_times_locked += 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 + VERIFY(set_result == AK::HashSetResult::InsertedNewEntry); + } else if (m_mode == Mode::Shared) { + m_times_locked += lock_count; + if (auto it = m_shared_holders.find(current_thread); it != m_shared_holders.end()) { + it->value += lock_count; + } else { + 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 + VERIFY(set_result == AK::HashSetResult::InsertedNewEntry); + } + } else { + VERIFY(m_mode == Mode::Exclusive); + VERIFY(m_holder == current_thread); + m_times_locked += lock_count; + } } #if LOCK_DEBUG diff --git a/Kernel/Lock.h b/Kernel/Lock.h index cfd69eb7f9..c0908dc136 100644 --- a/Kernel/Lock.h +++ b/Kernel/Lock.h @@ -41,7 +41,20 @@ public: void unlock(); [[nodiscard]] Mode force_unlock_if_locked(u32&); - [[nodiscard]] bool is_locked() const { return m_mode != Mode::Unlocked; } + [[nodiscard]] bool is_locked() const + { + ScopedSpinLock lock(m_lock); + return m_mode != Mode::Unlocked; + } + [[nodiscard]] bool own_lock() const + { + ScopedSpinLock lock(m_lock); + if (m_mode == Mode::Exclusive) + return m_holder == Thread::current(); + if (m_mode == Mode::Shared) + return m_shared_holders.contains(Thread::current()); + return false; + } [[nodiscard]] const char* name() const { return m_name; } @@ -89,7 +102,7 @@ private: BlockedThreadList m_blocked_threads_list_exclusive; BlockedThreadList m_blocked_threads_list_shared; - SpinLock m_lock; + mutable SpinLock m_lock; }; class Locker {