From 6aa77d19998cc99bc10716c659ca307de32439ea Mon Sep 17 00:00:00 2001 From: Robin Burchell Date: Tue, 16 Jul 2019 15:03:39 +0200 Subject: [PATCH] SharedBuffer: Fix deadlock on destroy We were locking the list of references, and then destroying the reference, which made things go a little crazy. It's more straightforward to just remove the per-reference lock: the syscalls all have to lock the full list anyway, so let's just do that and avoid the hassle. While I'm at it, also move the SharedBuffer code out to its own file as it's getting a little long and unwieldly, and Process.cpp is already huge. --- Kernel/Makefile | 1 + Kernel/Process.cpp | 152 +--------------------------------------- Kernel/SharedBuffer.cpp | 117 +++++++++++++++++++++++++++++++ Kernel/SharedBuffer.h | 50 +++++++++++++ 4 files changed, 170 insertions(+), 150 deletions(-) create mode 100644 Kernel/SharedBuffer.cpp create mode 100644 Kernel/SharedBuffer.h diff --git a/Kernel/Makefile b/Kernel/Makefile index 936fe02eb02..272334ba084 100644 --- a/Kernel/Makefile +++ b/Kernel/Makefile @@ -6,6 +6,7 @@ KERNEL_OBJS = \ StdLib.o \ Arch/i386/CPU.o \ Process.o \ + SharedBuffer.o \ Thread.o \ Arch/i386/PIT.o \ Devices/KeyboardDevice.o \ diff --git a/Kernel/Process.cpp b/Kernel/Process.cpp index 06e5e304184..7ce78fadd07 100644 --- a/Kernel/Process.cpp +++ b/Kernel/Process.cpp @@ -20,9 +20,9 @@ #include #include #include +#include #include #include -#include #include #include #include @@ -2366,155 +2366,6 @@ int Process::sys$setsockopt(const Syscall::SC_setsockopt_params* params) return socket.setsockopt(level, option, value, value_size); } -struct SharedBuffer { -private: - struct Reference { - Reference(pid_t pid) - : pid(pid) - { - } - - pid_t pid; - unsigned count { 1 }; - Region* region { nullptr }; - }; -public: - SharedBuffer(int id, int size) - : m_shared_buffer_id(id) - , m_vmo(VMObject::create_anonymous(size)) - { -#ifdef SHARED_BUFFER_DEBUG - dbgprintf("Created shared buffer %d of size %d\n", m_shared_buffer_id, size); -#endif - } - - ~SharedBuffer() - { -#ifdef SHARED_BUFFER_DEBUG - dbgprintf("Destroyed shared buffer %d of size %d\n", m_shared_buffer_id, size()); -#endif - } - - bool is_shared_with(pid_t peer_pid) - { - LOCKER(m_refs.lock()); - for (auto& ref : m_refs.resource()) { - if (ref.pid == peer_pid) { - return true; - } - } - - return false; - } - - void* get_address(Process& process) - { - LOCKER(m_refs.lock()); - ASSERT(is_shared_with(process.pid())); - for (auto& ref : m_refs.resource()) { - if (ref.pid == process.pid()) { - if (ref.region == nullptr) { - ref.region = process.allocate_region_with_vmo(VirtualAddress(), size(), m_vmo, 0, "SharedBuffer", PROT_READ | (m_writable ? PROT_WRITE : 0)); - ref.region->set_shared(true); - } - return ref.region->vaddr().as_ptr(); - } - } - ASSERT_NOT_REACHED(); - } - - void share_with(pid_t peer_pid) - { - LOCKER(m_refs.lock()); - for (auto& ref : m_refs.resource()) { - if (ref.pid == peer_pid) { - ref.count++; - return; - } - } - - m_refs.resource().append(Reference(peer_pid)); - } - - void release(Process& process) - { - LOCKER(m_refs.lock()); -#ifdef SHARED_BUFFER_DEBUG - dbgprintf("Releasing shared buffer %d of size %d by PID %d\n", m_shared_buffer_id, size(), process.pid()); -#endif - for (int i = 0; i < m_refs.resource().size(); ++i) { - auto& ref = m_refs.resource()[i]; - if (ref.pid == process.pid()) { - if (--ref.count == 0) { - process.deallocate_region(*ref.region); - m_refs.resource().remove(i); - destroy_if_unused(); - return; - } - } - } - } - - void disown(pid_t pid) - { - LOCKER(m_refs.lock()); -#ifdef SHARED_BUFFER_DEBUG - dbgprintf("Disowning shared buffer %d of size %d by PID %d\n", m_shared_buffer_id, size(), pid); -#endif - for (int i = 0; i < m_refs.resource().size(); ++i) { - auto& ref = m_refs.resource()[i]; - if (ref.pid == pid) { - m_refs.resource().remove(i); - destroy_if_unused(); - return; - } - } - } - - size_t size() const { return m_vmo->size(); } - void destroy_if_unused(); - - void seal() - { - LOCKER(m_refs.lock()); - m_writable = false; - for (auto& ref : m_refs.resource()) { - if (ref.region) { - ref.region->set_writable(false); - MM.remap_region(*ref.region->page_directory(), *ref.region); - } - } - } - - int m_shared_buffer_id { -1 }; - bool m_writable { true }; - NonnullRefPtr m_vmo; - Lockable> m_refs; -}; - -static int s_next_shared_buffer_id; -Lockable>>& shared_buffers() -{ - static Lockable>>* map; - if (!map) - map = new Lockable>>; - return *map; -} - -void SharedBuffer::destroy_if_unused() -{ - LOCKER(m_refs.lock()); - if (m_refs.resource().size() == 0) { - LOCKER(shared_buffers().lock()); -#ifdef SHARED_BUFFER_DEBUG - kprintf("Destroying unused SharedBuffer{%p} id: %d\n", this, m_shared_buffer_id); -#endif - auto count_before = shared_buffers().resource().size(); - shared_buffers().resource().remove(m_shared_buffer_id); - ASSERT(count_before != shared_buffers().resource().size()); - } -} - void Process::disown_all_shared_buffers() { LOCKER(shared_buffers().lock()); @@ -2542,6 +2393,7 @@ int Process::sys$create_shared_buffer(pid_t peer_pid, int size, void** buffer) return -ESRCH; } LOCKER(shared_buffers().lock()); + static int s_next_shared_buffer_id; int shared_buffer_id = ++s_next_shared_buffer_id; auto shared_buffer = make(shared_buffer_id, size); shared_buffer->share_with(m_pid); diff --git a/Kernel/SharedBuffer.cpp b/Kernel/SharedBuffer.cpp new file mode 100644 index 00000000000..c5574f12dfd --- /dev/null +++ b/Kernel/SharedBuffer.cpp @@ -0,0 +1,117 @@ +#include +#include + +Lockable>>& shared_buffers() +{ + static Lockable>>* map; + if (!map) + map = new Lockable>>; + return *map; +} + +bool SharedBuffer::is_shared_with(pid_t peer_pid) +{ + LOCKER(shared_buffers().lock()); + for (auto& ref : m_refs) { + if (ref.pid == peer_pid) { + return true; + } + } + + return false; +} + +void* SharedBuffer::get_address(Process& process) +{ + LOCKER(shared_buffers().lock()); + ASSERT(is_shared_with(process.pid())); + for (auto& ref : m_refs) { + if (ref.pid == process.pid()) { + if (ref.region == nullptr) { + ref.region = process.allocate_region_with_vmo(VirtualAddress(), size(), m_vmo, 0, "SharedBuffer", PROT_READ | (m_writable ? PROT_WRITE : 0)); + ref.region->set_shared(true); + } + return ref.region->vaddr().as_ptr(); + } + } + ASSERT_NOT_REACHED(); +} + +void SharedBuffer::share_with(pid_t peer_pid) +{ + LOCKER(shared_buffers().lock()); + for (auto& ref : m_refs) { + if (ref.pid == peer_pid) { + ref.count++; + return; + } + } + + m_refs.append(Reference(peer_pid)); +} + +void SharedBuffer::release(Process& process) +{ + LOCKER(shared_buffers().lock()); + for (int i = 0; i < m_refs.size(); ++i) { + auto& ref = m_refs[i]; + if (ref.pid == process.pid()) { + if (--ref.count == 0) { +#ifdef SHARED_BUFFER_DEBUG + dbgprintf("Releasing shared buffer reference on %d of size %d by PID %d\n", m_shared_buffer_id, size(), process.pid()); +#endif + process.deallocate_region(*ref.region); + m_refs.remove(i); +#ifdef SHARED_BUFFER_DEBUG + dbgprintf("Released shared buffer reference on %d of size %d by PID %d\n", m_shared_buffer_id, size(), process.pid()); +#endif + destroy_if_unused(); + return; + } + } + } +} + +void SharedBuffer::disown(pid_t pid) +{ + LOCKER(shared_buffers().lock()); + for (int i = 0; i < m_refs.size(); ++i) { + auto& ref = m_refs[i]; + if (ref.pid == pid) { +#ifdef SHARED_BUFFER_DEBUG + dbgprintf("Disowning shared buffer %d of size %d by PID %d\n", m_shared_buffer_id, size(), pid); +#endif + m_refs.remove(i); +#ifdef SHARED_BUFFER_DEBUG + dbgprintf("Disowned shared buffer %d of size %d by PID %d\n", m_shared_buffer_id, size(), pid); +#endif + destroy_if_unused(); + return; + } + } +} + +void SharedBuffer::destroy_if_unused() +{ + LOCKER(shared_buffers().lock()); + if (m_refs.size() == 0) { +#ifdef SHARED_BUFFER_DEBUG + kprintf("Destroying unused SharedBuffer{%p} id: %d\n", this, m_shared_buffer_id); +#endif + auto count_before = shared_buffers().resource().size(); + shared_buffers().resource().remove(m_shared_buffer_id); + ASSERT(count_before != shared_buffers().resource().size()); + } +} + +void SharedBuffer::seal() +{ + LOCKER(shared_buffers().lock()); + m_writable = false; + for (auto& ref : m_refs) { + if (ref.region) { + ref.region->set_writable(false); + MM.remap_region(*ref.region->page_directory(), *ref.region); + } + } +} diff --git a/Kernel/SharedBuffer.h b/Kernel/SharedBuffer.h new file mode 100644 index 00000000000..c2187f10680 --- /dev/null +++ b/Kernel/SharedBuffer.h @@ -0,0 +1,50 @@ +#pragma once + +#include +#include + +struct SharedBuffer { +private: + struct Reference { + Reference(pid_t pid) + : pid(pid) + { + } + + pid_t pid; + unsigned count { 1 }; + Region* region { nullptr }; + }; +public: + SharedBuffer(int id, int size) + : m_shared_buffer_id(id) + , m_vmo(VMObject::create_anonymous(size)) + { +#ifdef SHARED_BUFFER_DEBUG + dbgprintf("Created shared buffer %d of size %d\n", m_shared_buffer_id, size); +#endif + } + + ~SharedBuffer() + { +#ifdef SHARED_BUFFER_DEBUG + dbgprintf("Destroyed shared buffer %d of size %d\n", m_shared_buffer_id, size()); +#endif + } + + bool is_shared_with(pid_t peer_pid); + void* get_address(Process& process); + void share_with(pid_t peer_pid); + void release(Process& process); + void disown(pid_t pid); + size_t size() const { return m_vmo->size(); } + void destroy_if_unused(); + void seal(); + + int m_shared_buffer_id { -1 }; + bool m_writable { true }; + NonnullRefPtr m_vmo; + Vector m_refs; +}; + +Lockable>>& shared_buffers();