From b844aecb9e35083477a30df015d4f44aef09b38f Mon Sep 17 00:00:00 2001 From: Eladash Date: Sat, 18 Mar 2023 11:36:55 +0200 Subject: [PATCH] sys_lwmutex/mutex: Fix race on lock timeout --- rpcs3/Emu/Cell/lv2/sys_lwmutex.cpp | 32 +++++++++++++++++++++++++++--- rpcs3/Emu/Cell/lv2/sys_lwmutex.h | 18 ++++++++--------- rpcs3/Emu/Cell/lv2/sys_mutex.cpp | 32 +++++++++++++++++++++++++++--- rpcs3/Emu/Cell/lv2/sys_mutex.h | 16 +++++++-------- rpcs3/Emu/Cell/lv2/sys_sync.h | 26 ++++++++++++++++++------ 5 files changed, 95 insertions(+), 29 deletions(-) diff --git a/rpcs3/Emu/Cell/lv2/sys_lwmutex.cpp b/rpcs3/Emu/Cell/lv2/sys_lwmutex.cpp index adc3e6ab8a..0f9edf77c3 100644 --- a/rpcs3/Emu/Cell/lv2/sys_lwmutex.cpp +++ b/rpcs3/Emu/Cell/lv2/sys_lwmutex.cpp @@ -236,12 +236,38 @@ error_code _sys_lwmutex_lock(ppu_thread& ppu, u32 lwmutex_id, u64 timeout) std::lock_guard lock(mutex->mutex); - if (!mutex->unqueue(mutex->lv2_control.raw().sq, &ppu)) + bool success = false; + + mutex->lv2_control.fetch_op([&](lv2_lwmutex::control_data_t& data) { - break; + success = false; + + ppu_thread* sq = static_cast(data.sq); + + const bool retval = &ppu == sq; + + if (!mutex->unqueue(sq, &ppu)) + { + return false; + } + + success = true; + + if (!retval) + { + return false; + } + + data.sq = sq; + return true; + }); + + if (success) + { + ppu.next_cpu = nullptr; + ppu.gpr[3] = CELL_ETIMEDOUT; } - ppu.gpr[3] = CELL_ETIMEDOUT; break; } } diff --git a/rpcs3/Emu/Cell/lv2/sys_lwmutex.h b/rpcs3/Emu/Cell/lv2/sys_lwmutex.h index c49d006800..e3d30fe4dd 100644 --- a/rpcs3/Emu/Cell/lv2/sys_lwmutex.h +++ b/rpcs3/Emu/Cell/lv2/sys_lwmutex.h @@ -152,21 +152,15 @@ struct lv2_lwmutex final : lv2_obj template T* reown(bool unlock2 = false) { - T* res{}; - T* restore_next{}; + T* res = nullptr; lv2_control.fetch_op([&](control_data_t& data) { - if (res) - { - res->next_cpu = restore_next; - res = nullptr; - } + res = nullptr; if (auto sq = static_cast(data.sq)) { - restore_next = sq->next_cpu; - res = schedule(data.sq, protocol); + res = schedule(data.sq, protocol, false); if (sq == data.sq) { @@ -182,6 +176,12 @@ struct lv2_lwmutex final : lv2_obj } }); + if (res && cpu_flag::again - res->state) + { + // Detach manually (fetch_op can fail, so avoid side-effects on the first node in this case) + res->next_cpu = nullptr; + } + return res; } }; diff --git a/rpcs3/Emu/Cell/lv2/sys_mutex.cpp b/rpcs3/Emu/Cell/lv2/sys_mutex.cpp index bc6906a67e..6eb48c4265 100644 --- a/rpcs3/Emu/Cell/lv2/sys_mutex.cpp +++ b/rpcs3/Emu/Cell/lv2/sys_mutex.cpp @@ -245,12 +245,38 @@ error_code sys_mutex_lock(ppu_thread& ppu, u32 mutex_id, u64 timeout) std::lock_guard lock(mutex->mutex); - if (!mutex->unqueue(mutex->control.raw().sq, &ppu)) + bool success = false; + + mutex->control.fetch_op([&](lv2_mutex::control_data_t& data) { - break; + success = false; + + ppu_thread* sq = static_cast(data.sq); + + const bool retval = &ppu == sq; + + if (!mutex->unqueue(sq, &ppu)) + { + return false; + } + + success = true; + + if (!retval) + { + return false; + } + + data.sq = sq; + return true; + }); + + if (success) + { + ppu.next_cpu = nullptr; + ppu.gpr[3] = CELL_ETIMEDOUT; } - ppu.gpr[3] = CELL_ETIMEDOUT; break; } } diff --git a/rpcs3/Emu/Cell/lv2/sys_mutex.h b/rpcs3/Emu/Cell/lv2/sys_mutex.h index 175279441a..23d8f36034 100644 --- a/rpcs3/Emu/Cell/lv2/sys_mutex.h +++ b/rpcs3/Emu/Cell/lv2/sys_mutex.h @@ -157,20 +157,14 @@ struct lv2_mutex final : lv2_obj T* reown() { T* res{}; - T* restore_next{}; control.fetch_op([&](control_data_t& data) { - if (res) - { - res->next_cpu = restore_next; - res = nullptr; - } + res = nullptr; if (auto sq = static_cast(data.sq)) { - restore_next = sq->next_cpu; - res = schedule(data.sq, protocol); + res = schedule(data.sq, protocol, false); if (sq == data.sq) { @@ -188,6 +182,12 @@ struct lv2_mutex final : lv2_obj } }); + if (res && cpu_flag::again - res->state) + { + // Detach manually (fetch_op can fail, so avoid side-effects on the first node in this case) + res->next_cpu = nullptr; + } + return res; } }; diff --git a/rpcs3/Emu/Cell/lv2/sys_sync.h b/rpcs3/Emu/Cell/lv2/sys_sync.h index d394413b84..5e6e608ed6 100644 --- a/rpcs3/Emu/Cell/lv2/sys_sync.h +++ b/rpcs3/Emu/Cell/lv2/sys_sync.h @@ -106,7 +106,7 @@ public: } // Find and remove the object from the linked list - template + template static T* unqueue(T*& first, T* object, T* T::* mem_ptr = &T::next_cpu) { auto it = +first; @@ -114,7 +114,12 @@ public: if (it == object) { atomic_storage::release(first, it->*mem_ptr); - atomic_storage::release(it->*mem_ptr, nullptr); + + if constexpr (ModifyNode) + { + atomic_storage::release(it->*mem_ptr, nullptr); + } + return it; } @@ -125,7 +130,12 @@ public: if (next == object) { atomic_storage::release(it->*mem_ptr, next->*mem_ptr); - atomic_storage::release(next->*mem_ptr, nullptr); + + if constexpr (ModifyNode) + { + atomic_storage::release(next->*mem_ptr, nullptr); + } + return next; } @@ -137,7 +147,7 @@ public: // Remove an object from the linked set according to the protocol template - static E* schedule(T& first, u32 protocol) + static E* schedule(T& first, u32 protocol, bool modify_node = true) { auto it = static_cast(first); @@ -161,7 +171,7 @@ public: continue; } - if (it && cpu_flag::again - it->state) + if (cpu_flag::again - it->state) { atomic_storage::release(*parent_found, nullptr); } @@ -199,7 +209,11 @@ public: if (cpu_flag::again - found->state) { atomic_storage::release(*parent_found, found->next_cpu); - atomic_storage::release(found->next_cpu, nullptr); + + if (modify_node) + { + atomic_storage::release(found->next_cpu, nullptr); + } } return found;