Skip to content

Commit

Permalink
Kernel: Fix some race conditions with Lock and waiting/waking threads
Browse files Browse the repository at this point in the history 
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.
  • Loading branch information
@tomuta @awesomekling
tomuta authored and awesomekling committed Jan 25, 2021
1 parent 4cf0859 commit bd73102
Show file tree
Hide file tree
Showing 3 changed files with 42 additions and 17 deletions.
38 changes: 25 additions & 13 deletions Kernel/Lock.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,7 @@ void Lock::lock(Mode mode)
Mode current_mode = m_mode;
switch (current_mode) {
case Mode::Unlocked: {
dbgln<LOCK_TRACE_DEBUG>("Lock::lock @ {}: acquire {}, currently unlocked", this, mode_to_string(mode));
dbgln<LOCK_TRACE_DEBUG>("Lock::lock @ ({}) {}: acquire {}, currently unlocked", this, m_name, mode_to_string(mode));
m_mode = mode;
ASSERT(!m_holder);
ASSERT(m_shared_holders.is_empty());
Expand All @@ -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;
}
Expand All @@ -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);
Expand All @@ -105,7 +106,7 @@ void Lock::lock(Mode mode)
if (mode != Mode::Shared)
break;

dbgln<LOCK_TRACE_DEBUG>("Lock::lock @ {}: acquire {}, currently shared, locks held {}", this, mode_to_string(mode), m_times_locked);
dbgln<LOCK_TRACE_DEBUG>("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++;
Expand All @@ -125,7 +126,9 @@ void Lock::lock(Mode mode)
ASSERT_NOT_REACHED();
}
m_lock.store(false, AK::memory_order_release);
dbgln<LOCK_TRACE_DEBUG>("Lock::lock @ {} ({}) waiting...", this, m_name);
m_queue.wait_on({}, m_name);
dbgln<LOCK_TRACE_DEBUG>("Lock::lock @ {} ({}) waited", this, m_name);
}
}

Expand All @@ -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);
Expand Down Expand Up @@ -174,17 +177,22 @@ 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
current_thread->holding_lock(*this, -1);
#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_TRACE_DEBUG>("Lock::unlock @ {} ({}) wake one ({})", this, m_name, did_wake);
}
return;
}
// I don't know *who* is using "m_lock", so just yield.
Expand Down Expand Up @@ -212,16 +220,16 @@ auto Lock::force_unlock_if_locked(u32& lock_count_to_restore) -> Mode
}

dbgln<LOCK_RESTORE_DEBUG>("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;
}
Expand All @@ -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;
Expand Down Expand Up @@ -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);
Expand All @@ -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);
Expand Down
12 changes: 9 additions & 3 deletions Kernel/WaitQueue.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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_DEBUG>("WaitQueue @ {}: do not block thread {}, wake was pending", this, data);
dbgln<WAITQUEUE_DEBUG>("WaitQueue @ {}: do not block thread {}, {}", this, data, m_should_block ? "wake was pending" : "not blocking");
return false;
}
dbgln<WAITQUEUE_DEBUG>("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_DEBUG>("WaitQueue @ {}: wake_one", this);
bool did_unblock_one = do_unblock([&](Thread::Blocker& b, void* data, bool& stop_iterating) {
Expand All @@ -55,11 +56,14 @@ void WaitQueue::wake_one()
dbgln<WAITQUEUE_DEBUG>("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_DEBUG>("WaitQueue @ {}: wake_one woke {} threads", this, did_wake);
return did_wake;
}

u32 WaitQueue::wake_n(u32 wake_count)
Expand All @@ -84,6 +88,7 @@ u32 WaitQueue::wake_n(u32 wake_count)
return false;
});
m_wake_requested = !did_unblock_some;
dbgln<WAITQUEUE_DEBUG>("WaitQueue @ {}: wake_n({}) woke {} threads", this, wake_count, did_wake);
return did_wake;
}

Expand All @@ -108,6 +113,7 @@ u32 WaitQueue::wake_all()
return false;
});
m_wake_requested = !did_unblock_any;
dbgln<WAITQUEUE_DEBUG>("WaitQueue @ {}: wake_all woke {} threads", this, did_wake);
return did_wake;
}

Expand Down
9 changes: 8 additions & 1 deletion Kernel/WaitQueue.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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<class... Args>
Thread::BlockResult wait_on(const Thread::BlockTimeout& timeout, Args&&... args)
{
Expand All @@ -49,6 +55,7 @@ class WaitQueue : public Thread::BlockCondition {

private:
bool m_wake_requested { false };
bool m_should_block { true };
};

}

0 comments on commit bd73102

Please sign in to comment.