From 3344f91fc499ef7d91078f5b2498649fc9468a41 Mon Sep 17 00:00:00 2001 From: Liav A Date: Thu, 1 Jul 2021 19:18:38 +0300 Subject: [PATCH] Kernel/ProcFS: Clean dead processes properly Now we use WeakPtrs to break Ref-counting cycle. Also, we call the prepare_for_deletion method to ensure deleted objects are ready for deletion. This is necessary to ensure we don't keep dead processes, which would become zombies. In addition to that, add some debug prints to aid debug in the future. --- Kernel/Bus/USB/UHCIController.cpp | 6 +- Kernel/FileSystem/ProcFS.cpp | 7 ++- Kernel/ProcessExposed.cpp | 11 +++- Kernel/ProcessExposed.h | 20 ++++--- Kernel/ProcessSpecificExposed.cpp | 96 ++++++++++++++++++++++--------- Kernel/Thread.cpp | 6 +- 6 files changed, 103 insertions(+), 43 deletions(-) diff --git a/Kernel/Bus/USB/UHCIController.cpp b/Kernel/Bus/USB/UHCIController.cpp index 7c0a565ff4..e3d834c067 100644 --- a/Kernel/Bus/USB/UHCIController.cpp +++ b/Kernel/Bus/USB/UHCIController.cpp @@ -144,9 +144,11 @@ KResultOr ProcFSUSBBusFolder::entries_count() const KResult ProcFSUSBBusFolder::traverse_as_directory(unsigned fsid, Function callback) const { ScopedSpinLock lock(m_lock); - VERIFY(m_parent_folder); + auto parent_folder = m_parent_folder.strong_ref(); + // Note: if the parent folder is null, it means something bad happened as this should not happen for the USB folder. + VERIFY(parent_folder); callback({ ".", { fsid, component_index() }, 0 }); - callback({ "..", { fsid, m_parent_folder->component_index() }, 0 }); + callback({ "..", { fsid, parent_folder->component_index() }, 0 }); for (auto& device_node : m_device_nodes) { InodeIdentifier identifier = { fsid, device_node.component_index() }; diff --git a/Kernel/FileSystem/ProcFS.cpp b/Kernel/FileSystem/ProcFS.cpp index e90b96d064..6dac1946a0 100644 --- a/Kernel/FileSystem/ProcFS.cpp +++ b/Kernel/FileSystem/ProcFS.cpp @@ -56,9 +56,12 @@ void ProcFSComponentsRegistrar::register_new_process(Process& new_process) void ProcFSComponentsRegistrar::unregister_process(Process& deleted_process) { - auto process_folder = m_root_folder->process_folder_for(deleted_process); - VERIFY(process_folder); + auto process_folder = m_root_folder->process_folder_for(deleted_process).release_nonnull(); + process_folder->prepare_for_deletion(); process_folder->m_list_node.remove(); + dbgln_if(PROCFS_DEBUG, "ProcFSExposedFolder ref_count now: {}", process_folder->ref_count()); + // Note: Let's ensure we are the last holder of the ProcFSProcessFolder object before it can be deleted for good + VERIFY(process_folder->ref_count() == 1); } RefPtr ProcFS::create() diff --git a/Kernel/ProcessExposed.cpp b/Kernel/ProcessExposed.cpp index fb883cad7b..ef1100a3d2 100644 --- a/Kernel/ProcessExposed.cpp +++ b/Kernel/ProcessExposed.cpp @@ -157,7 +157,10 @@ KResult ProcFSProcessInformation::refresh_data(FileDescription& description) con // For process-specific inodes, hold the process's ptrace lock across refresh // and refuse to load data if the process is not dumpable. // Without this, files opened before a process went non-dumpable could still be used for dumping. - auto process = const_cast(*this).m_parent_folder->m_associated_process; + auto parent_folder = const_cast(*this).m_parent_folder.strong_ref(); + if (parent_folder.is_null()) + return KResult(EINVAL); + auto process = parent_folder->m_associated_process; process->ptrace_lock().lock(); if (!process->is_dumpable()) { process->ptrace_lock().unlock(); @@ -240,9 +243,11 @@ RefPtr ProcFSExposedFolder::lookup(StringView name) KResult ProcFSExposedFolder::traverse_as_directory(unsigned fsid, Function callback) const { Locker locker(ProcFSComponentsRegistrar::the().m_lock); - VERIFY(m_parent_folder); + auto parent_folder = m_parent_folder.strong_ref(); + if (parent_folder.is_null()) + return KResult(EINVAL); callback({ ".", { fsid, component_index() }, 0 }); - callback({ "..", { fsid, m_parent_folder->component_index() }, 0 }); + callback({ "..", { fsid, parent_folder->component_index() }, 0 }); for (auto& component : m_components) { InodeIdentifier identifier = { fsid, component.component_index() }; diff --git a/Kernel/ProcessExposed.h b/Kernel/ProcessExposed.h index d4cff9b8f1..0b3a3fd28f 100644 --- a/Kernel/ProcessExposed.h +++ b/Kernel/ProcessExposed.h @@ -92,7 +92,9 @@ private: InodeIndex m_component_index {}; }; -class ProcFSExposedFolder : public ProcFSExposedComponent { +class ProcFSExposedFolder + : public ProcFSExposedComponent + , public Weakable { friend class ProcFSProcessFolder; friend class ProcFSComponentsRegistrar; @@ -104,8 +106,9 @@ public: virtual void prepare_for_deletion() override { - m_components.clear(); - m_parent_folder.clear(); + for (auto& component : m_components) { + component.prepare_for_deletion(); + } } virtual mode_t required_mode() const override { return 0555; } @@ -115,7 +118,7 @@ protected: explicit ProcFSExposedFolder(StringView name); ProcFSExposedFolder(StringView name, const ProcFSExposedFolder& parent_folder); NonnullRefPtrVector m_components; - RefPtr m_parent_folder; + WeakPtr m_parent_folder; }; class ProcFSExposedLink : public ProcFSExposedComponent { @@ -134,7 +137,8 @@ protected: class ProcFSRootFolder; class ProcFSProcessInformation; -class ProcFSProcessFolder final : public ProcFSExposedFolder { +class ProcFSProcessFolder final + : public ProcFSExposedFolder { friend class ProcFSComponentsRegistrar; friend class ProcFSRootFolder; friend class ProcFSProcessInformation; @@ -235,8 +239,8 @@ public: virtual KResultOr read_bytes(off_t offset, size_t count, UserOrKernelBuffer& buffer, FileDescription* description) const override; - virtual uid_t owner_user() const override { return m_parent_folder->m_associated_process->uid(); } - virtual gid_t owner_group() const override { return m_parent_folder->m_associated_process->gid(); } + virtual uid_t owner_user() const override { return m_parent_folder.strong_ref()->m_associated_process->uid(); } + virtual gid_t owner_group() const override { return m_parent_folder.strong_ref()->m_associated_process->gid(); } protected: ProcFSProcessInformation(StringView name, const ProcFSProcessFolder& process_folder) @@ -248,7 +252,7 @@ protected: virtual KResult refresh_data(FileDescription&) const override; virtual bool output(KBufferBuilder& builder) = 0; - NonnullRefPtr m_parent_folder; + WeakPtr m_parent_folder; mutable SpinLock m_refresh_lock; }; diff --git a/Kernel/ProcessSpecificExposed.cpp b/Kernel/ProcessSpecificExposed.cpp index ab8fb29f6b..f711769edb 100644 --- a/Kernel/ProcessSpecificExposed.cpp +++ b/Kernel/ProcessSpecificExposed.cpp @@ -83,24 +83,29 @@ private: , m_process_folder(parent_folder) { } - RefPtr m_process_folder; + WeakPtr m_process_folder; mutable Lock m_lock; }; KResultOr ProcFSProcessStacks::entries_count() const { Locker locker(m_lock); - auto process = m_process_folder->m_associated_process; - return process->thread_count(); + auto parent_folder = m_process_folder.strong_ref(); + if (parent_folder.is_null()) + return KResult(EINVAL); + return parent_folder->m_associated_process->thread_count(); } KResult ProcFSProcessStacks::traverse_as_directory(unsigned fsid, Function callback) const { Locker locker(m_lock); + auto parent_folder = m_process_folder.strong_ref(); + if (parent_folder.is_null()) + return KResult(EINVAL); callback({ ".", { fsid, component_index() }, 0 }); - callback({ "..", { fsid, m_parent_folder->component_index() }, 0 }); + callback({ "..", { fsid, parent_folder->component_index() }, 0 }); - auto process = m_process_folder->m_associated_process; + auto process = parent_folder->m_associated_process; process->for_each_thread([&](const Thread& thread) { int tid = thread.tid().value(); InodeIdentifier identifier = { fsid, thread.global_procfs_inode_index() }; @@ -112,13 +117,16 @@ KResult ProcFSProcessStacks::traverse_as_directory(unsigned fsid, Function ProcFSProcessStacks::lookup(StringView name) { Locker locker(m_lock); - auto process = m_process_folder->m_associated_process; + auto parent_folder = m_process_folder.strong_ref(); + if (parent_folder.is_null()) + return nullptr; + auto process = parent_folder->m_associated_process; RefPtr procfd_stack; // FIXME: Try to exit the loop earlier process->for_each_thread([&](const Thread& thread) { int tid = thread.tid().value(); if (name == String::number(tid)) { - procfd_stack = ProcFSThreadStack::create(*m_process_folder, *this, thread); + procfd_stack = ProcFSThreadStack::create(*parent_folder, *this, thread); } }); return procfd_stack; @@ -177,24 +185,30 @@ private: , m_process_folder(parent_folder) { } - RefPtr m_process_folder; + WeakPtr m_process_folder; mutable Lock m_lock; }; KResultOr ProcFSProcessFileDescriptions::entries_count() const { Locker locker(m_lock); - return m_process_folder->m_associated_process->fds().open_count(); + auto parent_folder = m_process_folder.strong_ref(); + if (parent_folder.is_null()) + return KResult(EINVAL); + return parent_folder->m_associated_process->fds().open_count(); } KResult ProcFSProcessFileDescriptions::traverse_as_directory(unsigned fsid, Function callback) const { Locker locker(m_lock); + auto parent_folder = m_process_folder.strong_ref(); + if (parent_folder.is_null()) + return KResult(EINVAL); callback({ ".", { fsid, component_index() }, 0 }); - callback({ "..", { fsid, m_parent_folder->component_index() }, 0 }); + callback({ "..", { fsid, parent_folder->component_index() }, 0 }); - auto process = m_process_folder->m_associated_process; + auto process = parent_folder->m_associated_process; size_t count = 0; - m_process_folder->m_associated_process->fds().enumerate([&](auto& file_description_metadata) { + process->fds().enumerate([&](auto& file_description_metadata) { if (!file_description_metadata.is_valid()) { count++; return; @@ -208,11 +222,14 @@ KResult ProcFSProcessFileDescriptions::traverse_as_directory(unsigned fsid, Func RefPtr ProcFSProcessFileDescriptions::lookup(StringView name) { Locker locker(m_lock); - auto process = m_process_folder->m_associated_process; + auto parent_folder = m_process_folder.strong_ref(); + if (parent_folder.is_null()) + return nullptr; + auto process = parent_folder->m_associated_process; RefPtr procfd_fd; // FIXME: Try to exit the loop earlier size_t count = 0; - m_process_folder->m_associated_process->fds().enumerate([&](auto& file_description_metadata) { + process->fds().enumerate([&](auto& file_description_metadata) { if (!file_description_metadata.is_valid()) { count++; return; @@ -239,8 +256,11 @@ private: } virtual bool output(KBufferBuilder& builder) override { + auto parent_folder = m_parent_folder.strong_ref(); + if (parent_folder.is_null()) + return false; JsonArraySerializer array { builder }; - for (auto& unveiled_path : m_parent_folder->m_associated_process->unveiled_paths()) { + for (auto& unveiled_path : parent_folder->m_associated_process->unveiled_paths()) { if (!unveiled_path.was_explicitly_unveiled()) continue; auto obj = array.add_object(); @@ -278,11 +298,15 @@ private: virtual bool output(KBufferBuilder& builder) override { InterruptDisabler disabler; - if (!m_parent_folder->m_associated_process->perf_events()) { - dbgln("ProcFS: No perf events for {}", m_parent_folder->m_associated_process->pid()); + auto parent_folder = m_parent_folder.strong_ref(); + if (parent_folder.is_null()) + return false; + auto process = parent_folder->m_associated_process; + if (!process->perf_events()) { + dbgln("ProcFS: No perf events for {}", process->pid()); return false; } - return m_parent_folder->m_associated_process->perf_events()->to_json(builder); + return process->perf_events()->to_json(builder); } }; @@ -300,8 +324,11 @@ private: } virtual bool output(KBufferBuilder& builder) override { + auto parent_folder = m_parent_folder.strong_ref(); + if (parent_folder.is_null()) + return false; JsonArraySerializer array { builder }; - auto process = m_parent_folder->m_associated_process; + auto process = parent_folder->m_associated_process; if (process->fds().open_count() == 0) { array.finish(); return true; @@ -348,10 +375,13 @@ private: } virtual bool acquire_link(KBufferBuilder& builder) override { - builder.append_bytes(m_parent_process_directory->m_associated_process->root_directory_relative_to_global_root().absolute_path().to_byte_buffer()); + auto parent_folder = m_parent_process_directory.strong_ref(); + if (parent_folder.is_null()) + return false; + builder.append_bytes(parent_folder->m_associated_process->root_directory_relative_to_global_root().absolute_path().to_byte_buffer()); return true; } - NonnullRefPtr m_parent_process_directory; + WeakPtr m_parent_process_directory; }; class ProcFSProcessVirtualMemory final : public ProcFSProcessInformation { @@ -368,7 +398,10 @@ private: } virtual bool output(KBufferBuilder& builder) override { - auto process = m_parent_folder->m_associated_process; + auto parent_folder = m_parent_folder.strong_ref(); + if (parent_folder.is_null()) + return false; + auto process = parent_folder->m_associated_process; JsonArraySerializer array { builder }; { ScopedSpinLock lock(process->space().get_lock()); @@ -428,11 +461,14 @@ private: } virtual bool acquire_link(KBufferBuilder& builder) override { - builder.append_bytes(m_parent_process_directory->m_associated_process->current_directory().absolute_path().bytes()); + auto parent_folder = m_parent_process_directory.strong_ref(); + if (parent_folder.is_null()) + return false; + builder.append_bytes(parent_folder->m_associated_process->current_directory().absolute_path().bytes()); return true; } - NonnullRefPtr m_parent_process_directory; + WeakPtr m_parent_process_directory; }; class ProcFSProcessBinary final : public ProcFSExposedLink { @@ -444,7 +480,10 @@ public: virtual mode_t required_mode() const override { - if (!m_parent_process_directory->m_associated_process->executable()) + auto parent_folder = m_parent_process_directory.strong_ref(); + if (parent_folder.is_null()) + return false; + if (!parent_folder->m_associated_process->executable()) return 0; return ProcFSExposedComponent::required_mode(); } @@ -457,14 +496,17 @@ private: } virtual bool acquire_link(KBufferBuilder& builder) override { - auto* custody = m_parent_process_directory->m_associated_process->executable(); + auto parent_folder = m_parent_process_directory.strong_ref(); + if (parent_folder.is_null()) + return false; + auto* custody = parent_folder->m_associated_process->executable(); if (!custody) return false; builder.append(custody->absolute_path().bytes()); return true; } - NonnullRefPtr m_parent_process_directory; + WeakPtr m_parent_process_directory; }; NonnullRefPtr ProcFSProcessFolder::create(const Process& process) diff --git a/Kernel/Thread.cpp b/Kernel/Thread.cpp index 7794a2afdd..e11244ea8d 100644 --- a/Kernel/Thread.cpp +++ b/Kernel/Thread.cpp @@ -459,8 +459,12 @@ void Thread::finalize_dying_threads() }); } for (auto* thread : dying_threads) { + RefPtr process = thread->process(); + dbgln_if(PROCESS_DEBUG, "Before finalization, {} has {} refs and its process has {}", + *thread, thread->ref_count(), thread->process().ref_count()); thread->finalize(); - + dbgln_if(PROCESS_DEBUG, "After finalization, {} has {} refs and its process has {}", + *thread, thread->ref_count(), thread->process().ref_count()); // This thread will never execute again, drop the running reference // NOTE: This may not necessarily drop the last reference if anything // else is still holding onto this thread!