From 7593418b777929a42bfdf176a54b9df518e6df17 Mon Sep 17 00:00:00 2001 From: Tom Date: Tue, 6 Jul 2021 12:48:48 -0600 Subject: [PATCH] Kernel: Fix futex race that could lead to thread waiting forever 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. --- Kernel/FutexQueue.cpp | 51 +++++++++++++++++++++++++++++++++++---- Kernel/FutexQueue.h | 13 ++++++++++ Kernel/Syscalls/futex.cpp | 40 ++++++++++++++++++++---------- 3 files changed, 86 insertions(+), 18 deletions(-) diff --git a/Kernel/FutexQueue.cpp b/Kernel/FutexQueue.cpp index 16c7cf45b9a..f15828ea626 100644 --- a/Kernel/FutexQueue.cpp +++ b/Kernel/FutexQueue.cpp @@ -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(data)); + return false; + } dbgln_if(FUTEXQUEUE_DEBUG, "FutexQueue @ {}: should block thread {}", this, *static_cast(data)); return true; @@ -43,7 +50,7 @@ u32 FutexQueue::wake_n_requeue(u32 wake_count, const Function& 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& 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& ge u32 FutexQueue::wake_n(u32 wake_count, const Optional& 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& 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()); +} + } diff --git a/Kernel/FutexQueue.h b/Kernel/FutexQueue.h index b63af4e135f..bc61ed66cd2 100644 --- a/Kernel/FutexQueue.h +++ b/Kernel/FutexQueue.h @@ -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 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 }; }; } diff --git a/Kernel/Syscalls/futex.cpp b/Kernel/Syscalls/futex.cpp index cd669e1dc76..208a49bf6c2 100644 --- a/Kernel/Syscalls/futex.cpp +++ b/Kernel/Syscalls/futex.cpp @@ -164,8 +164,9 @@ KResultOr Process::sys$futex(Userspace return nullptr; }; - auto find_futex_queue = [&](VMObject* vmobject, FlatPtr user_address_or_offset, bool create_if_not_found) -> RefPtr { + auto find_futex_queue = [&](VMObject* vmobject, FlatPtr user_address_or_offset, bool create_if_not_found, bool* did_create = nullptr) -> RefPtr { 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 Process::sys$futex(Userspace 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 Process::sys$futex(Userspace 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 Process::sys$futex(Userspace 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 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 Process::sys$futex(Userspace 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); }