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.
This commit is contained in:
PixelyIon 2022-08-04 17:15:13 +05:30
parent 68615703c1
commit c3cf79cb39
No known key found for this signature in database
GPG Key ID: 11BC6C3201BC2C05
3 changed files with 20 additions and 3 deletions

View File

@ -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))

View File

@ -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

View File

@ -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();