LadybirdBrowser/ladybird
1
1
Fork 0
mirror of https://github.com/LadybirdBrowser/ladybird.git synced 2024-12-27 21:21:50 +03:00
Code Issues Packages Projects Releases Wiki Activity

Kernel: Fix futex race that could lead to thread waiting forever

Browse Source 
There is a race condition where we would remove a FutexQueue from
our futex map and in the meanwhile another thread started to queue
itself into that very same futex, leading to that thread to wait
forever as no other wake operation could discover that removed
FutexQueue.

This fixes the problem by:
* Tracking imminent waits, which prevents a new FutexQueue from being
  deleted that a thread will wait on momentarily
* Atomically marking a FutexQueue as removed, which prevents a thread
  from waiting on it before it is actually removed from the futex map.
This commit is contained in:
Tom 2021-07-06 12:48:48 -06:00 committed by Andreas Kling
parent dd27490ee1
commit 7593418b77
Notes: sideshowbarker 2024-07-18 10:15:32 +09:00 
Author: https://github.com/tomuta
Commit: https://github.com/SerenityOS/serenity/commit/7593418b777
Pull-request: https://github.com/SerenityOS/serenity/pull/8507
3 changed files with 86 additions and 18 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

51
Kernel/FutexQueue.cpp
View File

@ -16,6 +16,13 @@ bool FutexQueue::should_add_blocker(Thread::Blocker& b, void* data)
VERIFY(m_lock.is_locked());
VERIFY(b.blocker_type() == Thread::Blocker::Type::Futex);
VERIFY(m_imminent_waits > 0);
m_imminent_waits--;
if (m_was_removed) {
dbgln_if(FUTEXQUEUE_DEBUG, "FutexQueue @ {}: should not block thread {}: was removed", this, *static_cast<Thread*>(data));
return false;
}
dbgln_if(FUTEXQUEUE_DEBUG, "FutexQueue @ {}: should block thread {}", this, *static_cast<Thread*>(data));
return true;
@ -43,7 +50,7 @@ u32 FutexQueue::wake_n_requeue(u32 wake_count, const Function<FutexQueue*()>& ge
}
return false;
});
is_empty = is_empty_locked();
is_empty = is_empty_and_no_imminent_waits_locked();
if (requeue_count > 0) {
auto blockers_to_requeue = do_take_blockers(requeue_count);
if (!blockers_to_requeue.is_empty()) {
@ -69,7 +76,7 @@ u32 FutexQueue::wake_n_requeue(u32 wake_count, const Function<FutexQueue*()>& ge
blocker.finish_requeue(*target_futex_queue);
}
target_futex_queue->do_append_blockers(move(blockers_to_requeue));
is_empty_target = target_futex_queue->is_empty_locked();
is_empty_target = target_futex_queue->is_empty_and_no_imminent_waits_locked();
} else {
dbgln_if(FUTEXQUEUE_DEBUG, "FutexQueue @ {}: wake_n_requeue could not get target queue to requeue {} blockers", this, blockers_to_requeue.size());
do_append_blockers(move(blockers_to_requeue));
@ -81,8 +88,10 @@ u32 FutexQueue::wake_n_requeue(u32 wake_count, const Function<FutexQueue*()>& ge
u32 FutexQueue::wake_n(u32 wake_count, const Optional<u32>& bitset, bool& is_empty)
{
if (wake_count == 0)
if (wake_count == 0) {
is_empty = false;
return 0; // should we assert instead?
}
ScopedSpinLock lock(m_lock);
dbgln_if(FUTEXQUEUE_DEBUG, "FutexQueue @ {}: wake_n({})", this, wake_count);
u32 did_wake = 0;
@ -100,7 +109,7 @@ u32 FutexQueue::wake_n(u32 wake_count, const Optional<u32>& bitset, bool& is_emp
}
return false;
});
is_empty = is_empty_locked();
is_empty = is_empty_and_no_imminent_waits_locked();
return did_wake;
}
@ -120,8 +129,40 @@ u32 FutexQueue::wake_all(bool& is_empty)
}
return false;
});
is_empty = is_empty_locked();
is_empty = is_empty_and_no_imminent_waits_locked();
return did_wake;
}
bool FutexQueue::is_empty_and_no_imminent_waits_locked()
{
return m_imminent_waits == 0 && is_empty_locked();
}
bool FutexQueue::queue_imminent_wait()
{
ScopedSpinLock lock(m_lock);
if (m_was_removed)
return false;
m_imminent_waits++;
return true;
}
bool FutexQueue::try_remove()
{
ScopedSpinLock lock(m_lock);
if (m_was_removed)
return false;
if (!is_empty_and_no_imminent_waits_locked())
return false;
m_was_removed = true;
return true;
}
void FutexQueue::did_remove()
{
ScopedSpinLock lock(m_lock);
VERIFY(m_was_removed);
VERIFY(is_empty_and_no_imminent_waits_locked());
}
}

13
Kernel/FutexQueue.h
View File

@ -33,6 +33,17 @@ public:
virtual void vmobject_deleted(VMObject&) override;
bool queue_imminent_wait();
void did_remove();
bool try_remove();
bool is_empty_and_no_imminent_waits()
{
ScopedSpinLock lock(m_lock);
return is_empty_and_no_imminent_waits_locked();
}
bool is_empty_and_no_imminent_waits_locked();
protected:
virtual bool should_add_blocker(Thread::Blocker& b, void* data) override;
@ -42,6 +53,8 @@ private:
const FlatPtr m_user_address_or_offset;
WeakPtr<VMObject> m_vmobject;
const bool m_is_global;
size_t m_imminent_waits { 1 }; // We only create this object if we're going to be waiting, so start out with 1
bool m_was_removed { false };
};
}

40
Kernel/Syscalls/futex.cpp
View File

@ -164,8 +164,9 @@ KResultOr<FlatPtr> Process::sys$futex(Userspace<const Syscall::SC_futex_params*>
return nullptr;
};
auto find_futex_queue = [&](VMObject* vmobject, FlatPtr user_address_or_offset, bool create_if_not_found) -> RefPtr<FutexQueue> {
auto find_futex_queue = [&](VMObject* vmobject, FlatPtr user_address_or_offset, bool create_if_not_found, bool* did_create = nullptr) -> RefPtr<FutexQueue> {
VERIFY(is_private || vmobject);
VERIFY(!create_if_not_found || did_create != nullptr);
auto* queues = is_private ? &m_futex_queues : find_global_futex_queues(*vmobject, create_if_not_found);
if (!queues)
return {};
@ -173,6 +174,7 @@ KResultOr<FlatPtr> Process::sys$futex(Userspace<const Syscall::SC_futex_params*>
if (it != queues->end())
return it->value;
if (create_if_not_found) {
*did_create = true;
auto futex_queue = adopt_ref(*new FutexQueue(user_address_or_offset, vmobject));
auto result = queues->set(user_address_or_offset, futex_queue);
VERIFY(result == AK::HashSetResult::InsertedNewEntry);
@ -184,7 +186,12 @@ KResultOr<FlatPtr> Process::sys$futex(Userspace<const Syscall::SC_futex_params*>
auto remove_futex_queue = [&](VMObject* vmobject, FlatPtr user_address_or_offset) {
auto* queues = is_private ? &m_futex_queues : find_global_futex_queues(*vmobject, false);
if (queues) {
queues->remove(user_address_or_offset);
if (auto it = queues->find(user_address_or_offset); it != queues->end()) {
if (it->value->try_remove()) {
it->value->did_remove();
queues->remove(it);
}
}
if (!is_private && queues->is_empty())
g_global_futex_queues->remove(vmobject);
}
@ -208,17 +215,24 @@ KResultOr<FlatPtr> Process::sys$futex(Userspace<const Syscall::SC_futex_params*>
ScopedSpinLock lock(queue_lock);
auto do_wait = [&](u32 bitset) -> int {
auto user_value = user_atomic_load_relaxed(params.userspace_address);
if (!user_value.has_value())
return EFAULT;
if (user_value.value() != params.val) {
dbgln("futex wait: EAGAIN. user value: {:p} @ {:p} != val: {}", user_value.value(), params.userspace_address, params.val);
return EAGAIN;
}
atomic_thread_fence(AK::MemoryOrder::memory_order_acquire);
bool did_create;
RefPtr<FutexQueue> futex_queue;
do {
auto user_value = user_atomic_load_relaxed(params.userspace_address);
if (!user_value.has_value())
return EFAULT;
if (user_value.value() != params.val) {
dbgln_if(FUTEX_DEBUG, "futex wait: EAGAIN. user value: {:p} @ {:p} != val: {}", user_value.value(), params.userspace_address, params.val);
return EAGAIN;
}
atomic_thread_fence(AK::MemoryOrder::memory_order_acquire);
auto futex_queue = find_futex_queue(vmobject.ptr(), user_address_or_offset, true);
VERIFY(futex_queue);
did_create = false;
futex_queue = find_futex_queue(vmobject.ptr(), user_address_or_offset, true, &did_create);
VERIFY(futex_queue);
// We need to try again if we didn't create this queue and the existing queue
// was removed before we were able to queue an imminent wait.
} while (!did_create && !futex_queue->queue_imminent_wait());
// We need to release the lock before blocking. But we have a reference
// to the FutexQueue so that we can keep it alive.
@ -227,7 +241,7 @@ KResultOr<FlatPtr> Process::sys$futex(Userspace<const Syscall::SC_futex_params*>
Thread::BlockResult block_result = futex_queue->wait_on(timeout, bitset);
lock.lock();
if (futex_queue->is_empty()) {
if (futex_queue->is_empty_and_no_imminent_waits()) {
// If there are no more waiters, we want to get rid of the futex!
remove_futex_queue(vmobject, user_address_or_offset);
}