From 633006926fd26eba7116526c760d2ab7d2a55fe3 Mon Sep 17 00:00:00 2001 From: Liav A Date: Thu, 12 Jan 2023 22:06:51 +0200 Subject: [PATCH] Kernel: Make the Jails' internal design a lot more sane This is done with 2 major steps: 1. Remove JailManagement singleton and use a structure that resembles what we have with the Process object. This is required later for the second step in this commit, but on its own, is a major change that removes this clunky singleton that had no real usage by itself. 2. Use IntrusiveLists to keep references to Process objects in the same Jail so it will be much more straightforward to iterate on this kind of objects when needed. Previously we locked the entire Process list and we did a simple pointer comparison to check if the checked Process we iterate on is in the same Jail or not, which required taking multiple Spinlocks in a very clumsy and heavyweight way. --- Kernel/Arch/aarch64/init.cpp | 3 - Kernel/Arch/x86_64/init.cpp | 2 - Kernel/CMakeLists.txt | 2 +- .../SysFS/Subsystems/Kernel/Jails.cpp | 6 +- Kernel/Jail.cpp | 59 +++++++- Kernel/Jail.h | 24 +++- Kernel/JailManagement.cpp | 76 ---------- Kernel/JailManagement.h | 42 ------ Kernel/Process.cpp | 130 +++++++++--------- Kernel/Process.h | 28 +++- Kernel/ProcessList.cpp | 16 +++ Kernel/Syscalls/execve.cpp | 6 + Kernel/Syscalls/fork.cpp | 21 +++ Kernel/Syscalls/jail.cpp | 13 +- 14 files changed, 214 insertions(+), 214 deletions(-) delete mode 100644 Kernel/JailManagement.cpp delete mode 100644 Kernel/JailManagement.h create mode 100644 Kernel/ProcessList.cpp diff --git a/Kernel/Arch/aarch64/init.cpp b/Kernel/Arch/aarch64/init.cpp index 7a690cce94..326b32051e 100644 --- a/Kernel/Arch/aarch64/init.cpp +++ b/Kernel/Arch/aarch64/init.cpp @@ -26,7 +26,6 @@ #include #include #include -#include #include #include #include @@ -175,8 +174,6 @@ extern "C" [[noreturn]] void init() Processor::disable_interrupts(); TimeManagement::initialize(0); - JailManagement::the(); - Process::initialize(); Scheduler::initialize(); diff --git a/Kernel/Arch/x86_64/init.cpp b/Kernel/Arch/x86_64/init.cpp index cc2b2757c2..4188128d9e 100644 --- a/Kernel/Arch/x86_64/init.cpp +++ b/Kernel/Arch/x86_64/init.cpp @@ -38,7 +38,6 @@ #include #include #include -#include #include #include #include @@ -231,7 +230,6 @@ extern "C" [[noreturn]] UNMAP_AFTER_INIT void init(BootInfo const& boot_info) __stack_chk_guard = get_fast_random(); - JailManagement::the(); Process::initialize(); Scheduler::initialize(); diff --git a/Kernel/CMakeLists.txt b/Kernel/CMakeLists.txt index f880747190..7168aa494e 100644 --- a/Kernel/CMakeLists.txt +++ b/Kernel/CMakeLists.txt @@ -93,7 +93,6 @@ set(KERNEL_SOURCES Graphics/VirtIOGPU/GraphicsAdapter.cpp IOWindow.cpp Jail.cpp - JailManagement.cpp SanCov.cpp Storage/ATA/AHCI/Controller.cpp Storage/ATA/AHCI/Port.cpp @@ -256,6 +255,7 @@ set(KERNEL_SOURCES PerformanceEventBuffer.cpp Process.cpp ProcessGroup.cpp + ProcessList.cpp Random.cpp Scheduler.cpp ScopedCritical.cpp diff --git a/Kernel/FileSystem/SysFS/Subsystems/Kernel/Jails.cpp b/Kernel/FileSystem/SysFS/Subsystems/Kernel/Jails.cpp index dbe37ce092..7c8f24f7c6 100644 --- a/Kernel/FileSystem/SysFS/Subsystems/Kernel/Jails.cpp +++ b/Kernel/FileSystem/SysFS/Subsystems/Kernel/Jails.cpp @@ -1,12 +1,12 @@ /* - * Copyright (c) 2022, Liav A. + * Copyright (c) 2022-2023, Liav A. * * SPDX-License-Identifier: BSD-2-Clause */ #include #include -#include +#include #include namespace Kernel { @@ -24,7 +24,7 @@ UNMAP_AFTER_INIT NonnullLockRefPtr SysFSJails::must_create(SysFSDire ErrorOr SysFSJails::try_generate(KBufferBuilder& builder) { auto array = TRY(JsonArraySerializer<>::try_create(builder)); - TRY(JailManagement::the().for_each_in_same_jail([&array](Jail& jail) -> ErrorOr { + TRY(Jail::for_each_when_process_is_not_jailed([&array](Jail const& jail) -> ErrorOr { auto obj = TRY(array.add_object()); TRY(obj.add("index"sv, jail.index().value())); TRY(obj.add("name"sv, jail.name())); diff --git a/Kernel/Jail.cpp b/Kernel/Jail.cpp index faeed507f1..3b546ee32b 100644 --- a/Kernel/Jail.cpp +++ b/Kernel/Jail.cpp @@ -1,21 +1,70 @@ /* - * Copyright (c) 2022, Liav A. + * Copyright (c) 2022-2023, Liav A. * * SPDX-License-Identifier: BSD-2-Clause */ +#include +#include #include +#include namespace Kernel { -ErrorOr> Jail::create(Badge, NonnullOwnPtr name, JailIndex index) +static Atomic s_jail_id; +static Singleton> s_all_instances {}; + +static JailIndex generate_jail_id() { - return adopt_nonnull_lock_ref_or_enomem(new (nothrow) Jail(move(name), index)); + return s_jail_id.fetch_add(1); } -Jail::Jail(NonnullOwnPtr name, JailIndex index) +NonnullRefPtr Jail::process_list() +{ + return m_process_list; +} + +ErrorOr> Jail::create(NonnullOwnPtr name) +{ + return s_all_instances->with([&](auto& list) -> ErrorOr> { + auto process_list = TRY(ProcessList::create()); + auto jail = TRY(adopt_nonnull_lock_ref_or_enomem(new (nothrow) Jail(move(name), generate_jail_id(), move(process_list)))); + list.append(jail); + return jail; + }); +} + +ErrorOr Jail::for_each_when_process_is_not_jailed(Function(Jail const&)> callback) +{ + return Process::current().jail().with([&](auto const& my_jail) -> ErrorOr { + // Note: If we are in a jail, don't reveal anything about the outside world, + // not even the fact that we are in which jail... + if (my_jail) + return {}; + return s_all_instances->with([&](auto& list) -> ErrorOr { + for (auto& jail : list) { + TRY(callback(jail)); + } + return {}; + }); + }); +} + +LockRefPtr Jail::find_by_index(JailIndex index) +{ + return s_all_instances->with([&](auto& list) -> LockRefPtr { + for (auto& jail : list) { + if (jail.index() == index) + return jail; + } + return {}; + }); +} + +Jail::Jail(NonnullOwnPtr name, JailIndex index, NonnullRefPtr process_list) : m_name(move(name)) , m_index(index) + , m_process_list(move(process_list)) { } @@ -26,7 +75,7 @@ void Jail::detach(Badge) VERIFY(my_attach_count > 0); my_attach_count--; if (my_attach_count == 0) { - m_jail_list_node.remove(); + m_list_node.remove(); } }); } diff --git a/Kernel/Jail.h b/Kernel/Jail.h index bfbc6c833a..355c15c4ab 100644 --- a/Kernel/Jail.h +++ b/Kernel/Jail.h @@ -1,5 +1,5 @@ /* - * Copyright (c) 2022, Liav A. + * Copyright (c) 2022-2023, Liav A. * * SPDX-License-Identifier: BSD-2-Clause */ @@ -9,6 +9,7 @@ #include #include #include +#include #include #include #include @@ -16,19 +17,21 @@ #include #include #include -#include namespace Kernel { -class JailManagement; +class ProcessList; AK_TYPEDEF_DISTINCT_ORDERED_ID(u64, JailIndex); class Jail : public RefCounted { - friend class JailManagement; public: - static ErrorOr> create(Badge, NonnullOwnPtr, JailIndex); + NonnullRefPtr process_list(); + + static LockRefPtr find_by_index(JailIndex); + static ErrorOr> create(NonnullOwnPtr name); + static ErrorOr for_each_when_process_is_not_jailed(Function(Jail const&)> callback); StringView name() const { return m_name->view(); } JailIndex index() const { return m_index; } @@ -37,12 +40,19 @@ public: SpinlockProtected& attach_count() { return m_attach_count; } private: - Jail(NonnullOwnPtr, JailIndex); + Jail(NonnullOwnPtr, JailIndex, NonnullRefPtr); NonnullOwnPtr m_name; JailIndex const m_index; - IntrusiveListNode> m_jail_list_node; + IntrusiveListNode> m_list_node; + +public: + using List = IntrusiveListRelaxedConst<&Jail::m_list_node>; + +private: + NonnullRefPtr m_process_list; + SpinlockProtected m_attach_count { 0 }; }; diff --git a/Kernel/JailManagement.cpp b/Kernel/JailManagement.cpp deleted file mode 100644 index df927ced6d..0000000000 --- a/Kernel/JailManagement.cpp +++ /dev/null @@ -1,76 +0,0 @@ -/* - * Copyright (c) 2022, Liav A. - * - * SPDX-License-Identifier: BSD-2-Clause - */ - -#include -#include -#include -#include - -namespace Kernel { - -static Singleton s_the; -static Atomic s_jail_id; - -UNMAP_AFTER_INIT JailManagement::JailManagement() = default; - -JailIndex JailManagement::generate_jail_id() -{ - return s_jail_id.fetch_add(1); -} - -JailManagement& JailManagement::the() -{ - return *s_the; -} - -LockRefPtr JailManagement::find_jail_by_index(JailIndex index) -{ - return m_jails.with([&](auto& list) -> LockRefPtr { - for (auto& jail : list) { - if (jail.index() == index) - return jail; - } - return {}; - }); -} - -ErrorOr JailManagement::for_each_in_same_jail(Function(Jail&)> callback) -{ - return Process::current().jail().with([&](auto const& my_jail) -> ErrorOr { - // Note: If we are in a jail, don't reveal anything about the outside world, - // not even the fact that we are in which jail... - if (my_jail) - return {}; - return m_jails.with([&](auto& list) -> ErrorOr { - for (auto& jail : list) { - TRY(callback(jail)); - } - return {}; - }); - }); -} - -LockRefPtr JailManagement::find_first_jail_by_name(StringView name) -{ - return m_jails.with([&](auto& list) -> LockRefPtr { - for (auto& jail : list) { - if (jail.name() == name) - return jail; - } - return {}; - }); -} - -ErrorOr> JailManagement::create_jail(NonnullOwnPtr name) -{ - return m_jails.with([&](auto& list) -> ErrorOr> { - auto jail = TRY(Jail::create({}, move(name), generate_jail_id())); - list.append(jail); - return jail; - }); -} - -} diff --git a/Kernel/JailManagement.h b/Kernel/JailManagement.h deleted file mode 100644 index 76fe91cf82..0000000000 --- a/Kernel/JailManagement.h +++ /dev/null @@ -1,42 +0,0 @@ -/* - * Copyright (c) 2022, Liav A. - * - * SPDX-License-Identifier: BSD-2-Clause - */ - -#pragma once - -#include -#include -#include -#include -#include -#include -#include -#include -#include -#include -#include - -namespace Kernel { - -class JailManagement { - -public: - JailManagement(); - static JailManagement& the(); - - LockRefPtr find_jail_by_index(JailIndex); - LockRefPtr find_first_jail_by_name(StringView); - - ErrorOr> create_jail(NonnullOwnPtr name); - - ErrorOr for_each_in_same_jail(Function(Jail&)>); - -private: - JailIndex generate_jail_id(); - - SpinlockProtected, LockRank::None> m_jails {}; -}; - -} diff --git a/Kernel/Process.cpp b/Kernel/Process.cpp index 504484976f..2783dce4aa 100644 --- a/Kernel/Process.cpp +++ b/Kernel/Process.cpp @@ -48,7 +48,7 @@ extern ProcessID g_init_pid; RecursiveSpinlock g_profiling_lock {}; static Atomic next_pid; -static Singleton> s_all_instances; +static Singleton> s_all_instances; READONLY_AFTER_INIT Memory::Region* g_signal_trampoline_region; static Singleton>> s_hostname; @@ -58,98 +58,89 @@ MutexProtected>& hostname() return *s_hostname; } -SpinlockProtected& Process::all_instances() +SpinlockProtected& Process::all_instances() { return *s_all_instances; } ErrorOr Process::for_each_in_same_jail(Function(Process&)> callback) { - ErrorOr result {}; - Process::all_instances().with([&](auto const& list) { - Process::current().jail().with([&](auto const& my_jail) { - for (auto& process : list) { - if (!my_jail) { + return Process::current().m_jail_process_list.with([&](auto const& list_ptr) -> ErrorOr { + ErrorOr result {}; + if (list_ptr) { + list_ptr->attached_processes().with([&](auto const& list) { + for (auto& process : list) { result = callback(process); - } else { - // Note: Don't acquire the process jail spinlock twice if it's the same process - // we are currently inspecting. - if (&Process::current() == &process) { - result = callback(process); - } else { - process.jail().with([&](auto const& their_jail) { - if (their_jail.ptr() == my_jail.ptr()) - result = callback(process); - }); - } + if (result.is_error()) + break; } + }); + return result; + } + all_instances().with([&](auto const& list) { + for (auto& process : list) { + result = callback(process); if (result.is_error()) break; } }); + return result; }); - return result; } ErrorOr Process::for_each_child_in_same_jail(Function(Process&)> callback) { ProcessID my_pid = pid(); - ErrorOr result {}; - Process::all_instances().with([&](auto const& list) { - jail().with([&](auto const& my_jail) { - for (auto& process : list) { - if (!my_jail) { + return m_jail_process_list.with([&](auto const& list_ptr) -> ErrorOr { + ErrorOr result {}; + if (list_ptr) { + list_ptr->attached_processes().with([&](auto const& list) { + for (auto& process : list) { if (process.ppid() == my_pid || process.has_tracee_thread(pid())) result = callback(process); - } else { - // FIXME: Is it possible to have a child process being pointing to itself - // as the parent process under normal conditions? - // Note: Don't acquire the process jail spinlock twice if it's the same process - // we are currently inspecting. - if (&Process::current() == &process && (process.ppid() == my_pid || process.has_tracee_thread(pid()))) { - result = callback(process); - } else { - process.jail().with([&](auto const& their_jail) { - if ((their_jail.ptr() == my_jail.ptr()) && (process.ppid() == my_pid || process.has_tracee_thread(pid()))) - result = callback(process); - }); - } + if (result.is_error()) + break; } + }); + return result; + } + all_instances().with([&](auto const& list) { + for (auto& process : list) { + if (process.ppid() == my_pid || process.has_tracee_thread(pid())) + result = callback(process); if (result.is_error()) break; } }); + return result; }); - return result; } ErrorOr Process::for_each_in_pgrp_in_same_jail(ProcessGroupID pgid, Function(Process&)> callback) { - ErrorOr result {}; - Process::all_instances().with([&](auto const& list) { - jail().with([&](auto const& my_jail) { - for (auto& process : list) { - if (!my_jail) { + return m_jail_process_list.with([&](auto const& list_ptr) -> ErrorOr { + ErrorOr result {}; + if (list_ptr) { + list_ptr->attached_processes().with([&](auto const& list) { + for (auto& process : list) { if (!process.is_dead() && process.pgid() == pgid) result = callback(process); - } else { - // Note: Don't acquire the process jail spinlock twice if it's the same process - // we are currently inspecting. - if (&Process::current() == &process && !process.is_dead() && process.pgid() == pgid) { - result = callback(process); - } else { - process.jail().with([&](auto const& their_jail) { - if ((their_jail.ptr() == my_jail.ptr()) && !process.is_dead() && process.pgid() == pgid) - result = callback(process); - }); - } + if (result.is_error()) + break; } + }); + return result; + } + all_instances().with([&](auto const& list) { + for (auto& process : list) { + if (!process.is_dead() && process.pgid() == pgid) + result = callback(process); if (result.is_error()) break; } }); + return result; }); - return result; } ProcessID Process::allocate_pid() @@ -502,23 +493,21 @@ void Process::crash(int signal, Optional regs, bool out_of LockRefPtr Process::from_pid_in_same_jail(ProcessID pid) { - return Process::current().jail().with([&](auto const& my_jail) -> LockRefPtr { - return all_instances().with([&](auto const& list) -> LockRefPtr { - if (!my_jail) { + return Process::current().m_jail_process_list.with([&](auto const& list_ptr) -> LockRefPtr { + if (list_ptr) { + return list_ptr->attached_processes().with([&](auto const& list) -> LockRefPtr { for (auto& process : list) { if (process.pid() == pid) { return process; } } - } else { - for (auto& process : list) { - if (process.pid() == pid) { - return process.jail().with([&](auto const& other_process_jail) -> LockRefPtr { - if (other_process_jail.ptr() == my_jail.ptr()) - return process; - return {}; - }); - } + return {}; + }); + } + return all_instances().with([&](auto const& list) -> LockRefPtr { + for (auto& process : list) { + if (process.pid() == pid) { + return process; } } return {}; @@ -773,6 +762,13 @@ void Process::finalize() m_fds.with_exclusive([](auto& fds) { fds.clear(); }); m_tty = nullptr; m_executable.with([](auto& executable) { executable = nullptr; }); + m_jail_process_list.with([this](auto& list_ptr) { + if (list_ptr) { + list_ptr->attached_processes().with([&](auto& list) { + list.remove(*this); + }); + } + }); m_attached_jail.with([](auto& jail) { if (jail) jail->detach({}); diff --git a/Kernel/Process.h b/Kernel/Process.h index 703cbb1dc9..a5e1e11341 100644 --- a/Kernel/Process.h +++ b/Kernel/Process.h @@ -104,6 +104,8 @@ static_assert(sizeof(GlobalFutexKey) == (sizeof(FlatPtr) * 2)); struct LoadResult; +class ProcessList; + class Process final : public ListedRefCounted , public LockWeakable { @@ -665,8 +667,6 @@ private: return nullptr; } - IntrusiveListNode m_list_node; - SpinlockProtected, LockRank::None> m_name; SpinlockProtected, LockRank::None> m_space; @@ -852,7 +852,15 @@ private: LockWeakPtr m_master_tls_region; - IntrusiveListNode m_jail_list_node; + IntrusiveListNode m_jail_process_list_node; + IntrusiveListNode m_all_processes_list_node; + +public: + using AllProcessesList = IntrusiveListRelaxedConst<&Process::m_all_processes_list_node>; + using JailProcessList = IntrusiveListRelaxedConst<&Process::m_jail_process_list_node>; + +private: + SpinlockProtected, LockRank::None> m_jail_process_list; SpinlockProtected, LockRank::Process> m_attached_jail {}; size_t m_master_tls_size { 0 }; @@ -895,8 +903,18 @@ private: u8 m_protected_values_padding[PAGE_SIZE - sizeof(ProtectedValues)]; public: - using List = IntrusiveListRelaxedConst<&Process::m_list_node>; - static SpinlockProtected& all_instances(); + static SpinlockProtected& all_instances(); +}; + +class ProcessList : public RefCounted { +public: + static ErrorOr> create(); + SpinlockProtected& attached_processes() { return m_attached_processes; } + SpinlockProtected const& attached_processes() const { return m_attached_processes; } + +private: + ProcessList() = default; + SpinlockProtected m_attached_processes; }; // Note: Process object should be 2 pages of 4096 bytes each. diff --git a/Kernel/ProcessList.cpp b/Kernel/ProcessList.cpp new file mode 100644 index 0000000000..aae12f7250 --- /dev/null +++ b/Kernel/ProcessList.cpp @@ -0,0 +1,16 @@ +/* + * Copyright (c) 2023, Liav A. + * + * SPDX-License-Identifier: BSD-2-Clause + */ + +#include + +namespace Kernel { + +ErrorOr> ProcessList::create() +{ + return adopt_nonnull_ref_or_enomem(new (nothrow) ProcessList()); +} + +} diff --git a/Kernel/Syscalls/execve.cpp b/Kernel/Syscalls/execve.cpp index 0f68783717..83e2b37a66 100644 --- a/Kernel/Syscalls/execve.cpp +++ b/Kernel/Syscalls/execve.cpp @@ -516,6 +516,7 @@ ErrorOr Process::do_exec(NonnullRefPtr main_program_d auto old_credentials = this->credentials(); auto new_credentials = old_credentials; auto old_process_attached_jail = m_attached_jail.with([&](auto& jail) -> RefPtr { return jail; }); + auto old_scoped_list = m_jail_process_list.with([&](auto& list) -> RefPtr { return list; }); bool executable_is_setid = false; @@ -578,6 +579,11 @@ ErrorOr Process::do_exec(NonnullRefPtr main_program_d m_attached_jail.with([&](auto& jail) { jail = old_process_attached_jail; }); + + m_jail_process_list.with([&](auto& list) { + list = old_scoped_list; + }); + m_environment = move(environment); TRY(m_unveil_data.with([&](auto& unveil_data) -> ErrorOr { diff --git a/Kernel/Syscalls/fork.cpp b/Kernel/Syscalls/fork.cpp index 148926e3e9..4c9924bb55 100644 --- a/Kernel/Syscalls/fork.cpp +++ b/Kernel/Syscalls/fork.cpp @@ -70,6 +70,26 @@ ErrorOr Process::sys$fork(RegisterState& regs) return {}; })); + ArmedScopeGuard remove_from_jail_process_list = [&]() { + m_jail_process_list.with([&](auto& list_ptr) { + if (list_ptr) { + list_ptr->attached_processes().with([&](auto& list) { + list.remove(*child); + }); + } + }); + }; + m_jail_process_list.with([&](auto& list_ptr) { + if (list_ptr) { + child->m_jail_process_list.with([&](auto& child_list_ptr) { + child_list_ptr = list_ptr; + }); + list_ptr->attached_processes().with([&](auto& list) { + list.append(child); + }); + } + }); + TRY(child->m_fds.with_exclusive([&](auto& child_fds) { return m_fds.with_exclusive([&](auto& parent_fds) { return child_fds.try_clone(parent_fds); @@ -150,6 +170,7 @@ ErrorOr Process::sys$fork(RegisterState& regs) })); thread_finalizer_guard.disarm(); + remove_from_jail_process_list.disarm(); Process::register_new(*child); diff --git a/Kernel/Syscalls/jail.cpp b/Kernel/Syscalls/jail.cpp index 4d84935fb6..5a36bcbce1 100644 --- a/Kernel/Syscalls/jail.cpp +++ b/Kernel/Syscalls/jail.cpp @@ -7,7 +7,6 @@ #include #include #include -#include #include #include @@ -31,7 +30,7 @@ ErrorOr Process::sys$jail_create(Userspaceindex().value(); })); // Note: We do the copy_to_user outside of the m_attached_jail Spinlock locked scope because @@ -64,13 +63,21 @@ ErrorOr Process::sys$jail_attach(Userspace(params.index)); + auto jail = Jail::find_by_index(static_cast(params.index)); if (!jail) return EINVAL; my_jail = *jail; my_jail->attach_count().with([&](auto& attach_count) { attach_count++; }); + m_jail_process_list.with([&](auto& list_ptr) { + list_ptr = my_jail->process_list(); + if (list_ptr) { + list_ptr->attached_processes().with([&](auto& list) { + list.append(*this); + }); + } + }); return 0; }); }