From 662143e0a9bcb750ba0644cb93eaff8cf4240903 Mon Sep 17 00:00:00 2001 From: Idan Horowitz Date: Sat, 16 Dec 2023 00:12:37 +0200 Subject: [PATCH] Kernel: Resolve deadlock in MasterPTY due to mutex in spinlock scope MasterPTY::read called DoubleBuffer::read which takes a mutex (which may block) while holding m_slave's spinlock. If it did block, and was later rescheduled on a different physical CPU, we would deadlock on re-locking m_slave inside the unblock callback. (Since our recursive spinlock implementation is processor based and not process based) --- Kernel/Devices/TTY/MasterPTY.cpp | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/Kernel/Devices/TTY/MasterPTY.cpp b/Kernel/Devices/TTY/MasterPTY.cpp index a6a571fea5..0fcd9edee4 100644 --- a/Kernel/Devices/TTY/MasterPTY.cpp +++ b/Kernel/Devices/TTY/MasterPTY.cpp @@ -55,11 +55,16 @@ MasterPTY::~MasterPTY() ErrorOr MasterPTY::read(OpenFileDescription&, u64, UserOrKernelBuffer& buffer, size_t size) { - return m_slave.with([this, &buffer, size](auto& slave) -> ErrorOr { - if (!slave && m_buffer->is_empty()) - return 0; - return m_buffer->read(buffer, size); - }); + // Note that has_slave() takes and then releases the m_slave spinlock. + // Not holding the spinlock while calling m_buffer->read is legal, because slave starts non-null, + // and can only change its state to null once (and never back to non-null) in notify_slave_closed. + // So if the check happens, and it returns non-null, and then it turns null concurrently, + // and we call m_buffer->read, the behaviour from the perspective of the read caller is + // the same as if the slave turned null after we called m_buffer->read. On the other hand, + // if the check happens and returns null, then it can't possibly change to non-null after. + if (!has_slave() && m_buffer->is_empty()) + return 0; + return m_buffer->read(buffer, size); } ErrorOr MasterPTY::write(OpenFileDescription&, u64, UserOrKernelBuffer const& buffer, size_t size)