From c3cf79cb39beeeb82dd1534c9f730554bc0a4392 Mon Sep 17 00:00:00 2001
From: PixelyIon <pixelyion@protonmail.com>
Date: Thu, 4 Aug 2022 17:15:13 +0530
Subject: [PATCH] Rework `KThread::waiterMutex` Locking

Two issues exist with locking of `KThread::waiterMutex`:
* It was not always locked when accessing waiter members such as `waitThread`, `waitKey` and `waitTag` which would lead to a race that could end up in a deadlock or most notably a segfault inside `UpdatePriorityInheritance`
* There could be a deadlock from `UpdatePriorityInheritance` locking `waiterMutex` of a thread and waiting to get the owner's `waiterMutex` while on another thread `MutexUnlock` holds the owner's `waiterMutex` and waits on locking the `waiterMutex` held by `UpdatePriorityInheritance`

This commit fixes both issues by adding appropriate locking to all locations where waiter members are accessed in addition to adding a fallback mechanism inside `UpdatePriorityInheritance` that unlocks `waiterMutex` on contention to avoid a deadlock.
---
 .../cpp/skyline/kernel/types/KProcess.cpp     |  2 +-
 .../main/cpp/skyline/kernel/types/KThread.cpp | 20 +++++++++++++++++--
 .../main/cpp/skyline/kernel/types/KThread.h   |  1 +
 3 files changed, 20 insertions(+), 3 deletions(-)

diff --git a/app/src/main/cpp/skyline/kernel/types/KProcess.cpp b/app/src/main/cpp/skyline/kernel/types/KProcess.cpp
index f6856789..211796db 100644
--- a/app/src/main/cpp/skyline/kernel/types/KProcess.cpp
+++ b/app/src/main/cpp/skyline/kernel/types/KProcess.cpp
@@ -122,7 +122,7 @@ namespace skyline::kernel::type {
 
         bool isHighestPriority;
         {
-            std::scoped_lock lock{owner->waiterMutex};
+            std::scoped_lock lock{owner->waiterMutex, state.thread->waiterMutex}; // We need to lock both mutexes at the same time as we mutate the owner and the current thread, the ordering of locks **must** match MutexUnlock to avoid deadlocks
 
             u32 value{};
             if (__atomic_compare_exchange_n(mutex, &value, tag, false, __ATOMIC_SEQ_CST, __ATOMIC_SEQ_CST))
diff --git a/app/src/main/cpp/skyline/kernel/types/KThread.cpp b/app/src/main/cpp/skyline/kernel/types/KThread.cpp
index a054f4b1..d69d32a4 100644
--- a/app/src/main/cpp/skyline/kernel/types/KThread.cpp
+++ b/app/src/main/cpp/skyline/kernel/types/KThread.cpp
@@ -274,7 +274,9 @@ namespace skyline::kernel::type {
     }
 
     void KThread::UpdatePriorityInheritance() {
-        auto waitingOn{waitThread};
+        std::unique_lock lock{waiterMutex};
+
+        std::shared_ptr<KThread> waitingOn{waitThread};
         i8 currentPriority{priority.load()};
         while (waitingOn) {
             i8 ownerPriority;
@@ -287,7 +289,21 @@ namespace skyline::kernel::type {
             } while (!waitingOn->priority.compare_exchange_strong(ownerPriority, currentPriority));
 
             if (ownerPriority != currentPriority) {
-                std::scoped_lock waiterLock{waitingOn->waiterMutex};
+                std::unique_lock waiterLock{waitingOn->waiterMutex, std::try_to_lock};
+                if (!waiterLock) {
+                    // We want to avoid a deadlock here from the thread holding waitingOn->waiterMutex waiting for waiterMutex
+                    // We use a fallback mechanism to avoid this, resetting the state and trying again after being able to successfully acquire waitingOn->waiterMutex once
+                    waitingOn->priority = ownerPriority;
+
+                    lock.unlock();
+                    waiterLock.lock();
+
+                    lock.lock();
+                    waitingOn = waitThread;
+
+                    continue;
+                }
+
                 auto nextThread{waitingOn->waitThread};
                 if (nextThread) {
                     // We need to update the location of the owner thread in the waiter queue of the thread it's waiting on
diff --git a/app/src/main/cpp/skyline/kernel/types/KThread.h b/app/src/main/cpp/skyline/kernel/types/KThread.h
index 3e79ad77..2dd8b4b9 100644
--- a/app/src/main/cpp/skyline/kernel/types/KThread.h
+++ b/app/src/main/cpp/skyline/kernel/types/KThread.h
@@ -109,6 +109,7 @@ namespace skyline {
             /**
              * @brief Recursively updates the priority for any threads this thread might be waiting on
              * @note PI is performed by temporarily upgrading a thread's priority if a thread waiting on it has a higher priority to prevent priority inversion
+             * @note This will lock `waiterMutex` internally and it must **not** be held when calling this function
              */
             void UpdatePriorityInheritance();