From 82428e2a05eafd5bbbf7a6da76951635eeb19577 Mon Sep 17 00:00:00 2001
From: Liav A <liavalb@gmail.com>
Date: Sat, 8 Jul 2023 14:41:00 +0300
Subject: [PATCH] Kernel/TTY: Protect SlavePTY pointer with proper spinlock

Instead of using a LockRefPtr, we could easily use SpinlockProtected to
ensure proper locking of this pointer.
---
 Kernel/Devices/TTY/MasterPTY.cpp | 82 +++++++++++++++++++-------------
 Kernel/Devices/TTY/MasterPTY.h   |  2 +-
 2 files changed, 50 insertions(+), 34 deletions(-)

diff --git a/Kernel/Devices/TTY/MasterPTY.cpp b/Kernel/Devices/TTY/MasterPTY.cpp
index eda6d5f27f..8a934e4c12 100644
--- a/Kernel/Devices/TTY/MasterPTY.cpp
+++ b/Kernel/Devices/TTY/MasterPTY.cpp
@@ -22,7 +22,9 @@ ErrorOr<NonnullLockRefPtr<MasterPTY>> MasterPTY::try_create(unsigned int index)
     auto master_pty = TRY(adopt_nonnull_lock_ref_or_enomem(new (nothrow) MasterPTY(index, move(buffer))));
     auto credentials = Process::current().credentials();
     auto slave_pty = TRY(adopt_nonnull_lock_ref_or_enomem(new (nothrow) SlavePTY(*master_pty, credentials->uid(), credentials->gid(), index)));
-    master_pty->m_slave = slave_pty;
+    master_pty->m_slave.with([&slave_pty](auto& slave) {
+        slave = *slave_pty;
+    });
     TRY(master_pty->after_inserting());
     TRY(slave_pty->after_inserting());
     return master_pty;
@@ -34,8 +36,10 @@ MasterPTY::MasterPTY(unsigned index, NonnullOwnPtr<DoubleBuffer> buffer)
     , m_buffer(move(buffer))
 {
     m_buffer->set_unblock_callback([this]() {
-        if (m_slave)
-            evaluate_block_conditions();
+        m_slave.with([this](auto& slave) {
+            if (slave)
+                evaluate_block_conditions();
+        });
     });
 }
 
@@ -47,24 +51,30 @@ MasterPTY::~MasterPTY()
 
 ErrorOr<size_t> MasterPTY::read(OpenFileDescription&, u64, UserOrKernelBuffer& buffer, size_t size)
 {
-    if (!m_slave && m_buffer->is_empty())
-        return 0;
-    return m_buffer->read(buffer, size);
+    return m_slave.with([this, &buffer, size](auto& slave) -> ErrorOr<size_t> {
+        if (!slave && m_buffer->is_empty())
+            return 0;
+        return m_buffer->read(buffer, size);
+    });
 }
 
 ErrorOr<size_t> MasterPTY::write(OpenFileDescription&, u64, UserOrKernelBuffer const& buffer, size_t size)
 {
-    if (!m_slave)
-        return EIO;
-    m_slave->on_master_write(buffer, size);
-    return size;
+    return m_slave.with([&](auto& slave) -> ErrorOr<size_t> {
+        if (!slave)
+            return EIO;
+        slave->on_master_write(buffer, size);
+        return size;
+    });
 }
 
 bool MasterPTY::can_read(OpenFileDescription const&, u64) const
 {
-    if (!m_slave)
-        return true;
-    return !m_buffer->is_empty();
+    return m_slave.with([this](auto& slave) -> bool {
+        if (!slave)
+            return true;
+        return !m_buffer->is_empty();
+    });
 }
 
 bool MasterPTY::can_write(OpenFileDescription const&, u64) const
@@ -74,11 +84,13 @@ bool MasterPTY::can_write(OpenFileDescription const&, u64) const
 
 void MasterPTY::notify_slave_closed(Badge<SlavePTY>)
 {
-    dbgln_if(MASTERPTY_DEBUG, "MasterPTY({}): slave closed, my retains: {}, slave retains: {}", m_index, ref_count(), m_slave->ref_count());
-    // +1 ref for my MasterPTY::m_slave
-    // +1 ref for OpenFileDescription::m_device
-    if (m_slave->ref_count() == 2)
-        m_slave = nullptr;
+    m_slave.with([this](auto& slave) {
+        dbgln_if(MASTERPTY_DEBUG, "MasterPTY({}): slave closed, my retains: {}, slave retains: {}", m_index, ref_count(), slave->ref_count());
+        // +1 ref for my MasterPTY::m_slave
+        // +1 ref for OpenFileDescription::m_device
+        if (slave->ref_count() == 2)
+            slave = nullptr;
+    });
 }
 
 ErrorOr<size_t> MasterPTY::on_slave_write(UserOrKernelBuffer const& data, size_t size)
@@ -102,8 +114,10 @@ ErrorOr<void> MasterPTY::close()
     // From this point, let's consider ourselves closed.
     m_closed = true;
 
-    if (m_slave)
-        m_slave->hang_up();
+    m_slave.with([](auto& slave) {
+        if (slave)
+            slave->hang_up();
+    });
 
     return {};
 }
@@ -111,19 +125,21 @@ ErrorOr<void> MasterPTY::close()
 ErrorOr<void> MasterPTY::ioctl(OpenFileDescription& description, unsigned request, Userspace<void*> arg)
 {
     TRY(Process::current().require_promise(Pledge::tty));
-    if (!m_slave)
-        return EIO;
-    switch (request) {
-    case TIOCGPTN: {
-        int master_pty_index = index();
-        return copy_to_user(static_ptr_cast<int*>(arg), &master_pty_index);
-    }
-    case TIOCSWINSZ:
-    case TIOCGPGRP:
-        return m_slave->ioctl(description, request, arg);
-    default:
-        return EINVAL;
-    }
+    return m_slave.with([&](auto& slave) -> ErrorOr<void> {
+        if (!slave)
+            return EIO;
+        switch (request) {
+        case TIOCGPTN: {
+            int master_pty_index = index();
+            return copy_to_user(static_ptr_cast<int*>(arg), &master_pty_index);
+        }
+        case TIOCSWINSZ:
+        case TIOCGPGRP:
+            return slave->ioctl(description, request, arg);
+        default:
+            return EINVAL;
+        }
+    });
 }
 
 ErrorOr<NonnullOwnPtr<KString>> MasterPTY::pseudo_path(OpenFileDescription const&) const
diff --git a/Kernel/Devices/TTY/MasterPTY.h b/Kernel/Devices/TTY/MasterPTY.h
index 7c45f1642c..4334056f67 100644
--- a/Kernel/Devices/TTY/MasterPTY.h
+++ b/Kernel/Devices/TTY/MasterPTY.h
@@ -43,7 +43,7 @@ private:
     virtual ErrorOr<void> ioctl(OpenFileDescription&, unsigned request, Userspace<void*> arg) override;
     virtual StringView class_name() const override { return "MasterPTY"sv; }
 
-    LockRefPtr<SlavePTY> m_slave;
+    SpinlockProtected<RefPtr<SlavePTY>, LockRank::None> m_slave;
     unsigned m_index;
     bool m_closed { false };
     NonnullOwnPtr<DoubleBuffer> m_buffer;