From ab06a76920438a0f5e7ee010f9da454c83e1f094 Mon Sep 17 00:00:00 2001 From: Idan Horowitz Date: Fri, 15 Dec 2023 23:27:53 +0200 Subject: [PATCH] Kernel: Resolve lock-inversion based deadlock in MasterPTY & BlockSet MasterPTY's double buffer unblock callback would take m_slave's spinlock and then call evaluate_block_conditions() which would take BlockerSet's spinlock, while on the other hand, BlockerSet's add_blocker would take BlockerSet's spinlock, and then call should_add_blocker, which would call unblock_if_conditions_are_met, which would then call should_unblock, which will finally call MasterPTY::can_read() which will take m_slave's spinlock. Resolve this by moving the call to evaluate_block_conditions() out of the scope of m_slave's spinlock, as there's no need to hold the lock while calling it anyways. --- Kernel/Devices/TTY/MasterPTY.cpp | 12 ++++++++---- Kernel/Devices/TTY/MasterPTY.h | 5 +++++ 2 files changed, 13 insertions(+), 4 deletions(-) diff --git a/Kernel/Devices/TTY/MasterPTY.cpp b/Kernel/Devices/TTY/MasterPTY.cpp index 7c262b6c7d..a6a571fea5 100644 --- a/Kernel/Devices/TTY/MasterPTY.cpp +++ b/Kernel/Devices/TTY/MasterPTY.cpp @@ -36,10 +36,14 @@ MasterPTY::MasterPTY(unsigned index, NonnullOwnPtr buffer) , m_buffer(move(buffer)) { m_buffer->set_unblock_callback([this]() { - m_slave.with([this](auto& slave) { - if (slave) - evaluate_block_conditions(); - }); + // Note that has_slave() takes and then releases the m_slave spinlock. + // Not holding the spinlock while calling evaluate_block_conditions is legal, + // as the call will trigger a check to see if waiters may be unblocked, + // and if it was called spuriously (i.e. because the slave disappeared between + // calling the unblock callback and the actual block condition evaluation), + // the waiters will simply not unblock. + if (has_slave()) + evaluate_block_conditions(); }); } diff --git a/Kernel/Devices/TTY/MasterPTY.h b/Kernel/Devices/TTY/MasterPTY.h index 9d10d49c70..803af8abd6 100644 --- a/Kernel/Devices/TTY/MasterPTY.h +++ b/Kernel/Devices/TTY/MasterPTY.h @@ -43,6 +43,11 @@ private: virtual ErrorOr ioctl(OpenFileDescription&, unsigned request, Userspace arg) override; virtual StringView class_name() const override { return "MasterPTY"sv; } + bool has_slave() const + { + return m_slave.with([](auto& slave) -> bool { return slave; }); + } + SpinlockProtected, LockRank::None> m_slave; unsigned m_index; bool m_closed { false };