From 728c3fbd14252bd746f97dbb5441063992074b6b Mon Sep 17 00:00:00 2001 From: Andreas Kling Date: Sun, 21 Aug 2022 01:04:35 +0200 Subject: [PATCH] Kernel: Use RefPtr instead of LockRefPtr for Custody By protecting all the RefPtr objects that may be accessed from multiple threads at the same time (with spinlocks), we remove the need for using LockRefPtr (which is basically a RefPtr with a built-in spinlock.) --- Kernel/FileSystem/Custody.cpp | 9 +-- Kernel/FileSystem/Custody.h | 12 ++-- Kernel/FileSystem/Inode.cpp | 2 +- Kernel/FileSystem/Inode.h | 2 +- Kernel/FileSystem/Mount.cpp | 32 +++++----- Kernel/FileSystem/Mount.h | 7 ++- Kernel/FileSystem/OpenFileDescription.cpp | 24 +++++--- Kernel/FileSystem/OpenFileDescription.h | 7 ++- Kernel/FileSystem/VirtualFileSystem.cpp | 71 ++++++++++++----------- Kernel/FileSystem/VirtualFileSystem.h | 14 +++-- Kernel/Process.cpp | 22 +++++-- Kernel/Process.h | 14 ++--- Kernel/ProcessSpecificExposed.cpp | 2 +- Kernel/Syscalls/chdir.cpp | 5 +- Kernel/Syscalls/chmod.cpp | 2 +- Kernel/Syscalls/chown.cpp | 3 +- Kernel/Syscalls/execve.cpp | 2 +- Kernel/Syscalls/fork.cpp | 2 +- Kernel/Syscalls/open.cpp | 3 +- Kernel/Syscalls/stat.cpp | 3 +- Kernel/Syscalls/unlink.cpp | 2 +- Kernel/Syscalls/unveil.cpp | 3 +- Kernel/Syscalls/utimensat.cpp | 2 +- 23 files changed, 143 insertions(+), 102 deletions(-) diff --git a/Kernel/FileSystem/Custody.cpp b/Kernel/FileSystem/Custody.cpp index 8166cc15f4..0c40a26197 100644 --- a/Kernel/FileSystem/Custody.cpp +++ b/Kernel/FileSystem/Custody.cpp @@ -4,6 +4,7 @@ * SPDX-License-Identifier: BSD-2-Clause */ +#include #include #include #include @@ -20,20 +21,20 @@ SpinlockProtected& Custody::all_instances() return s_all_instances; } -ErrorOr> Custody::try_create(Custody* parent, StringView name, Inode& inode, int mount_flags) +ErrorOr> Custody::try_create(Custody* parent, StringView name, Inode& inode, int mount_flags) { - return all_instances().with([&](auto& all_custodies) -> ErrorOr> { + return all_instances().with([&](auto& all_custodies) -> ErrorOr> { for (Custody& custody : all_custodies) { if (custody.parent() == parent && custody.name() == name && &custody.inode() == &inode && custody.mount_flags() == mount_flags) { - return NonnullLockRefPtr { custody }; + return NonnullRefPtr { custody }; } } auto name_kstring = TRY(KString::try_create(name)); - auto custody = TRY(adopt_nonnull_lock_ref_or_enomem(new (nothrow) Custody(parent, move(name_kstring), inode, mount_flags))); + auto custody = TRY(adopt_nonnull_ref_or_enomem(new (nothrow) Custody(parent, move(name_kstring), inode, mount_flags))); all_custodies.prepend(*custody); return custody; }); diff --git a/Kernel/FileSystem/Custody.h b/Kernel/FileSystem/Custody.h index 26f2e277b6..f72cf071c6 100644 --- a/Kernel/FileSystem/Custody.h +++ b/Kernel/FileSystem/Custody.h @@ -8,22 +8,22 @@ #include #include +#include #include #include #include -#include #include namespace Kernel { -class Custody : public ListedRefCounted { +class Custody final : public ListedRefCounted { public: - static ErrorOr> try_create(Custody* parent, StringView name, Inode&, int mount_flags); + static ErrorOr> try_create(Custody* parent, StringView name, Inode&, int mount_flags); ~Custody(); - Custody* parent() { return m_parent.ptr(); } - Custody const* parent() const { return m_parent.ptr(); } + RefPtr parent() { return m_parent; } + RefPtr parent() const { return m_parent; } Inode& inode() { return *m_inode; } Inode const& inode() const { return *m_inode; } StringView name() const { return m_name->view(); } @@ -35,7 +35,7 @@ public: private: Custody(Custody* parent, NonnullOwnPtr name, Inode&, int mount_flags); - LockRefPtr m_parent; + RefPtr m_parent; NonnullOwnPtr m_name; NonnullLockRefPtr m_inode; int m_mount_flags { 0 }; diff --git a/Kernel/FileSystem/Inode.cpp b/Kernel/FileSystem/Inode.cpp index e0bf3d5955..108b47aee7 100644 --- a/Kernel/FileSystem/Inode.cpp +++ b/Kernel/FileSystem/Inode.cpp @@ -76,7 +76,7 @@ ErrorOr> Inode::read_entire(OpenFileDescription* descript return entire_file.release_nonnull(); } -ErrorOr> Inode::resolve_as_link(Custody& base, LockRefPtr* out_parent, int options, int symlink_recursion_level) const +ErrorOr> Inode::resolve_as_link(Custody& base, RefPtr* out_parent, int options, int symlink_recursion_level) const { // The default implementation simply treats the stored // contents as a path and resolves that. That is, it diff --git a/Kernel/FileSystem/Inode.h b/Kernel/FileSystem/Inode.h index 20c2ae6c49..155a8e2567 100644 --- a/Kernel/FileSystem/Inode.h +++ b/Kernel/FileSystem/Inode.h @@ -69,7 +69,7 @@ public: virtual ErrorOr chmod(mode_t) = 0; virtual ErrorOr chown(UserID, GroupID) = 0; virtual ErrorOr truncate(u64) { return {}; } - virtual ErrorOr> resolve_as_link(Custody& base, LockRefPtr* out_parent, int options, int symlink_recursion_level) const; + virtual ErrorOr> resolve_as_link(Custody& base, RefPtr* out_parent, int options, int symlink_recursion_level) const; virtual ErrorOr get_block_address(int) { return ENOTSUP; } diff --git a/Kernel/FileSystem/Mount.cpp b/Kernel/FileSystem/Mount.cpp index e934d2eb71..733147d746 100644 --- a/Kernel/FileSystem/Mount.cpp +++ b/Kernel/FileSystem/Mount.cpp @@ -14,7 +14,7 @@ namespace Kernel { Mount::Mount(FileSystem& guest_fs, Custody* host_custody, int flags) : m_guest(guest_fs.root_inode()) , m_guest_fs(guest_fs) - , m_host_custody(host_custody) + , m_host_custody(LockRank::None, host_custody) , m_flags(flags) { } @@ -22,30 +22,36 @@ Mount::Mount(FileSystem& guest_fs, Custody* host_custody, int flags) Mount::Mount(Inode& source, Custody& host_custody, int flags) : m_guest(source) , m_guest_fs(source.fs()) - , m_host_custody(host_custody) + , m_host_custody(LockRank::None, host_custody) , m_flags(flags) { } ErrorOr> Mount::absolute_path() const { - if (!m_host_custody) - return KString::try_create("/"sv); - return m_host_custody->try_serialize_absolute_path(); + return m_host_custody.with([&](auto& host_custody) -> ErrorOr> { + if (!host_custody) + return KString::try_create("/"sv); + return host_custody->try_serialize_absolute_path(); + }); } -Inode* Mount::host() +LockRefPtr Mount::host() { - if (!m_host_custody) - return nullptr; - return &m_host_custody->inode(); + return m_host_custody.with([](auto& host_custody) -> LockRefPtr { + if (!host_custody) + return nullptr; + return &host_custody->inode(); + }); } -Inode const* Mount::host() const +LockRefPtr Mount::host() const { - if (!m_host_custody) - return nullptr; - return &m_host_custody->inode(); + return m_host_custody.with([](auto& host_custody) -> LockRefPtr { + if (!host_custody) + return nullptr; + return &host_custody->inode(); + }); } } diff --git a/Kernel/FileSystem/Mount.h b/Kernel/FileSystem/Mount.h index 89f59a5925..de49c64caa 100644 --- a/Kernel/FileSystem/Mount.h +++ b/Kernel/FileSystem/Mount.h @@ -6,6 +6,7 @@ #pragma once +#include #include #include #include @@ -17,8 +18,8 @@ public: Mount(FileSystem&, Custody* host_custody, int flags); Mount(Inode& source, Custody& host_custody, int flags); - Inode const* host() const; - Inode* host(); + LockRefPtr host() const; + LockRefPtr host(); Inode const& guest() const { return *m_guest; } Inode& guest() { return *m_guest; } @@ -34,7 +35,7 @@ public: private: NonnullLockRefPtr m_guest; NonnullLockRefPtr m_guest_fs; - LockRefPtr m_host_custody; + SpinlockProtected> m_host_custody; int m_flags; }; diff --git a/Kernel/FileSystem/OpenFileDescription.cpp b/Kernel/FileSystem/OpenFileDescription.cpp index b7a1c5ecd2..f7405d52a9 100644 --- a/Kernel/FileSystem/OpenFileDescription.cpp +++ b/Kernel/FileSystem/OpenFileDescription.cpp @@ -27,7 +27,7 @@ ErrorOr> OpenFileDescription::try_create( auto inode_file = TRY(InodeFile::create(custody.inode())); auto description = TRY(adopt_nonnull_lock_ref_or_enomem(new (nothrow) OpenFileDescription(move(inode_file)))); - description->m_custody = custody; + description->m_state.with([&](auto& state) { state.custody = custody; }); TRY(description->attach()); return description; } @@ -72,7 +72,7 @@ ErrorOr OpenFileDescription::attach() void OpenFileDescription::set_original_custody(Badge, Custody& custody) { - m_custody = custody; + m_state.with([&](auto& state) { state.custody = custody; }); } Thread::FileBlocker::BlockFlags OpenFileDescription::should_unblock(Thread::FileBlocker::BlockFlags block_flags) const @@ -355,15 +355,15 @@ ErrorOr OpenFileDescription::close() ErrorOr> OpenFileDescription::original_absolute_path() const { - if (!m_custody) - return ENOENT; - return m_custody->try_serialize_absolute_path(); + if (auto custody = this->custody()) + return custody->try_serialize_absolute_path(); + return ENOENT; } ErrorOr> OpenFileDescription::pseudo_path() const { - if (m_custody) - return m_custody->try_serialize_absolute_path(); + if (auto custody = this->custody()) + return custody->try_serialize_absolute_path(); return m_file->pseudo_path(*this); } @@ -538,4 +538,14 @@ off_t OpenFileDescription::offset() const return m_state.with([](auto& state) { return state.current_offset; }); } +RefPtr OpenFileDescription::custody() const +{ + return m_state.with([](auto& state) { return state.custody; }); +} + +RefPtr OpenFileDescription::custody() +{ + return m_state.with([](auto& state) { return state.custody; }); +} + } diff --git a/Kernel/FileSystem/OpenFileDescription.h b/Kernel/FileSystem/OpenFileDescription.h index fc513f56cb..c4cdae2d49 100644 --- a/Kernel/FileSystem/OpenFileDescription.h +++ b/Kernel/FileSystem/OpenFileDescription.h @@ -8,6 +8,7 @@ #include #include +#include #include #include #include @@ -88,8 +89,8 @@ public: Inode* inode() { return m_inode.ptr(); } Inode const* inode() const { return m_inode.ptr(); } - Custody* custody() { return m_custody.ptr(); } - Custody const* custody() const { return m_custody.ptr(); } + RefPtr custody(); + RefPtr custody() const; ErrorOr mmap(Process&, Memory::VirtualRange const&, u64 offset, int prot, bool shared); @@ -138,12 +139,12 @@ private: blocker_set().unblock_all_blockers_whose_conditions_are_met(); } - LockRefPtr m_custody; LockRefPtr m_inode; NonnullLockRefPtr m_file; struct State { OwnPtr data; + RefPtr custody; off_t current_offset { 0 }; u32 file_flags { 0 }; bool readable : 1 { false }; diff --git a/Kernel/FileSystem/VirtualFileSystem.cpp b/Kernel/FileSystem/VirtualFileSystem.cpp index d995c29299..5b426e3c46 100644 --- a/Kernel/FileSystem/VirtualFileSystem.cpp +++ b/Kernel/FileSystem/VirtualFileSystem.cpp @@ -6,6 +6,7 @@ #include #include +#include #include #include #include @@ -38,6 +39,7 @@ VirtualFileSystem& VirtualFileSystem::the() } UNMAP_AFTER_INIT VirtualFileSystem::VirtualFileSystem() + : m_root_custody(LockRank::None) { } @@ -52,14 +54,15 @@ InodeIdentifier VirtualFileSystem::root_inode_id() const bool VirtualFileSystem::mount_point_exists_at_inode(InodeIdentifier inode_identifier) { return m_mounts.with([&](auto& mounts) -> bool { - return any_of(mounts, [&inode_identifier](Mount const& existing_mount) { - return existing_mount.host() && existing_mount.host()->identifier() == inode_identifier; + return any_of(mounts, [&inode_identifier](auto const& existing_mount) { + return existing_mount->host() && existing_mount->host()->identifier() == inode_identifier; }); }); } ErrorOr VirtualFileSystem::mount(FileSystem& fs, Custody& mount_point, int flags) { + auto new_mount = TRY(adopt_nonnull_own_or_enomem(new (nothrow) Mount(fs, &mount_point, flags))); return m_mounts.with([&](auto& mounts) -> ErrorOr { auto& inode = mount_point.inode(); dbgln("VirtualFileSystem: Mounting {} at inode {} with flags {}", @@ -70,14 +73,14 @@ ErrorOr VirtualFileSystem::mount(FileSystem& fs, Custody& mount_point, int dbgln("VirtualFileSystem: Mounting unsuccessful - inode {} is already a mount-point.", inode.identifier()); return EBUSY; } - Mount mount { fs, &mount_point, flags }; - mounts.append(move(mount)); + mounts.append(move(new_mount)); return {}; }); } ErrorOr VirtualFileSystem::bind_mount(Custody& source, Custody& mount_point, int flags) { + auto new_mount = TRY(adopt_nonnull_own_or_enomem(new (nothrow) Mount(source.inode(), mount_point, flags))); return m_mounts.with([&](auto& mounts) -> ErrorOr { auto& inode = mount_point.inode(); dbgln("VirtualFileSystem: Bind-mounting inode {} at inode {}", source.inode().identifier(), inode.identifier()); @@ -86,8 +89,7 @@ ErrorOr VirtualFileSystem::bind_mount(Custody& source, Custody& mount_poin mount_point.inode().identifier()); return EBUSY; } - Mount mount { source.inode(), mount_point, flags }; - mounts.append(move(mount)); + mounts.append(move(new_mount)); return {}; }); } @@ -111,11 +113,11 @@ ErrorOr VirtualFileSystem::unmount(Inode& guest_inode) return m_mounts.with([&](auto& mounts) -> ErrorOr { for (size_t i = 0; i < mounts.size(); ++i) { auto& mount = mounts[i]; - if (&mount.guest() != &guest_inode) + if (&mount->guest() != &guest_inode) continue; - TRY(mount.guest_fs().prepare_to_unmount()); - dbgln("VirtualFileSystem: Unmounting file system {}...", mount.guest_fs().fsid()); - mounts.unstable_take(i); + TRY(mount->guest_fs().prepare_to_unmount()); + dbgln("VirtualFileSystem: Unmounting file system {}...", mount->guest_fs().fsid()); + (void)mounts.unstable_take(i); return {}; } dbgln("VirtualFileSystem: Nothing mounted on inode {}", guest_inode.identifier()); @@ -130,7 +132,7 @@ ErrorOr VirtualFileSystem::mount_root(FileSystem& fs) return EEXIST; } - Mount mount { fs, nullptr, root_mount_flags }; + auto new_mount = TRY(adopt_nonnull_own_or_enomem(new (nothrow) Mount(fs, nullptr, root_mount_flags))); auto& root_inode = fs.root_inode(); if (!root_inode.is_directory()) { @@ -143,10 +145,13 @@ ErrorOr VirtualFileSystem::mount_root(FileSystem& fs) dmesgln("VirtualFileSystem: mounted root from {} ({})", fs.class_name(), pseudo_path); m_mounts.with([&](auto& mounts) { - mounts.append(move(mount)); + mounts.append(move(new_mount)); }); - m_root_custody = TRY(Custody::try_create(nullptr, ""sv, *m_root_inode, root_mount_flags)); + RefPtr new_root_custody = TRY(Custody::try_create(nullptr, ""sv, *m_root_inode, root_mount_flags)); + m_root_custody.with([&](auto& root_custody) { + swap(root_custody, new_root_custody); + }); return {}; } @@ -154,8 +159,8 @@ auto VirtualFileSystem::find_mount_for_host(InodeIdentifier id) -> Mount* { return m_mounts.with([&](auto& mounts) -> Mount* { for (auto& mount : mounts) { - if (mount.host() && mount.host()->identifier() == id) - return &mount; + if (mount->host() && mount->host()->identifier() == id) + return mount.ptr(); } return nullptr; }); @@ -165,8 +170,8 @@ auto VirtualFileSystem::find_mount_for_guest(InodeIdentifier id) -> Mount* { return m_mounts.with([&](auto& mounts) -> Mount* { for (auto& mount : mounts) { - if (mount.guest().identifier() == id) - return &mount; + if (mount->guest().identifier() == id) + return mount.ptr(); } return nullptr; }); @@ -244,7 +249,7 @@ ErrorOr> VirtualFileSystem::open(StringVi if ((options & O_CREAT) && (options & O_DIRECTORY)) return EINVAL; - LockRefPtr parent_custody; + RefPtr parent_custody; auto custody_or_error = resolve_path(path, base, &parent_custody, options); if (custody_or_error.is_error()) { // NOTE: ENOENT with a non-null parent custody signals us that the immediate parent @@ -332,7 +337,7 @@ ErrorOr VirtualFileSystem::mknod(StringView path, mode_t mode, dev_t dev, if (!is_regular_file(mode) && !is_block_device(mode) && !is_character_device(mode) && !is_fifo(mode) && !is_socket(mode)) return EINVAL; - LockRefPtr parent_custody; + RefPtr parent_custody; auto existing_file_or_error = resolve_path(path, base, &parent_custody); if (!existing_file_or_error.is_error()) return EEXIST; @@ -397,7 +402,7 @@ ErrorOr VirtualFileSystem::mkdir(StringView path, mode_t mode, Custody& ba path = "/"sv; } - LockRefPtr parent_custody; + RefPtr parent_custody; // FIXME: The errors returned by resolve_path_without_veil can leak information about paths that are not unveiled, // e.g. when the error is EACCESS or similar. auto result = resolve_path_without_veil(path, base, &parent_custody); @@ -446,7 +451,7 @@ ErrorOr VirtualFileSystem::access(StringView path, int mode, Custody& base return {}; } -ErrorOr> VirtualFileSystem::open_directory(StringView path, Custody& base) +ErrorOr> VirtualFileSystem::open_directory(StringView path, Custody& base) { auto custody = TRY(resolve_path(path, base)); auto& inode = custody->inode(); @@ -480,11 +485,11 @@ ErrorOr VirtualFileSystem::chmod(StringView path, mode_t mode, Custody& ba ErrorOr VirtualFileSystem::rename(StringView old_path, StringView new_path, Custody& base) { - LockRefPtr old_parent_custody; + RefPtr old_parent_custody; auto old_custody = TRY(resolve_path(old_path, base, &old_parent_custody, O_NOFOLLOW_NOERROR)); auto& old_inode = old_custody->inode(); - LockRefPtr new_parent_custody; + RefPtr new_parent_custody; auto new_custody_or_error = resolve_path(new_path, base, &new_parent_custody); if (new_custody_or_error.is_error()) { if (new_custody_or_error.error().code() != ENOENT || !new_parent_custody) @@ -630,7 +635,7 @@ ErrorOr VirtualFileSystem::link(StringView old_path, StringView new_path, auto old_custody = TRY(resolve_path(old_path, base)); auto& old_inode = old_custody->inode(); - LockRefPtr parent_custody; + RefPtr parent_custody; auto new_custody_or_error = resolve_path(new_path, base, &parent_custody); if (!new_custody_or_error.is_error()) return EEXIST; @@ -660,7 +665,7 @@ ErrorOr VirtualFileSystem::link(StringView old_path, StringView new_path, ErrorOr VirtualFileSystem::unlink(StringView path, Custody& base) { - LockRefPtr parent_custody; + RefPtr parent_custody; auto custody = TRY(resolve_path(path, base, &parent_custody, O_NOFOLLOW_NOERROR | O_UNLINK_INTERNAL)); auto& inode = custody->inode(); @@ -690,7 +695,7 @@ ErrorOr VirtualFileSystem::unlink(StringView path, Custody& base) ErrorOr VirtualFileSystem::symlink(StringView target, StringView linkpath, Custody& base) { - LockRefPtr parent_custody; + RefPtr parent_custody; auto existing_custody_or_error = resolve_path(linkpath, base, &parent_custody); if (!existing_custody_or_error.is_error()) return EEXIST; @@ -719,7 +724,7 @@ ErrorOr VirtualFileSystem::symlink(StringView target, StringView linkpath, ErrorOr VirtualFileSystem::rmdir(StringView path, Custody& base) { - LockRefPtr parent_custody; + RefPtr parent_custody; auto custody = TRY(resolve_path(path, base, &parent_custody)); auto& inode = custody->inode(); @@ -766,7 +771,7 @@ ErrorOr VirtualFileSystem::for_each_mount(Function(Mount con { return m_mounts.with([&](auto& mounts) -> ErrorOr { for (auto& mount : mounts) - TRY(callback(mount)); + TRY(callback(*mount)); return {}; }); } @@ -776,9 +781,9 @@ void VirtualFileSystem::sync() FileSystem::sync(); } -Custody& VirtualFileSystem::root_custody() +NonnullRefPtr VirtualFileSystem::root_custody() { - return *m_root_custody; + return m_root_custody.with([](auto& root_custody) -> NonnullRefPtr { return *root_custody; }); } UnveilNode const& VirtualFileSystem::find_matching_unveiled_path(StringView path) @@ -871,7 +876,7 @@ ErrorOr VirtualFileSystem::validate_path_against_process_veil(StringView p return {}; } -ErrorOr> VirtualFileSystem::resolve_path(StringView path, Custody& base, LockRefPtr* out_parent, int options, int symlink_recursion_level) +ErrorOr> VirtualFileSystem::resolve_path(StringView path, NonnullRefPtr base, RefPtr* out_parent, int options, int symlink_recursion_level) { // FIXME: The errors returned by resolve_path_without_veil can leak information about paths that are not unveiled, // e.g. when the error is EACCESS or similar. @@ -899,7 +904,7 @@ static bool safe_to_follow_symlink(Inode const& inode, InodeMetadata const& pare return false; } -ErrorOr> VirtualFileSystem::resolve_path_without_veil(StringView path, Custody& base, LockRefPtr* out_parent, int options, int symlink_recursion_level) +ErrorOr> VirtualFileSystem::resolve_path_without_veil(StringView path, NonnullRefPtr base, RefPtr* out_parent, int options, int symlink_recursion_level) { if (symlink_recursion_level >= symlink_recursion_limit) return ELOOP; @@ -910,7 +915,7 @@ ErrorOr> VirtualFileSystem::resolve_path_without_veil GenericLexer path_lexer(path); auto& current_process = Process::current(); - NonnullLockRefPtr custody = path[0] == '/' ? root_custody() : base; + NonnullRefPtr custody = path[0] == '/' ? root_custody() : base; bool extra_iteration = path[path.length() - 1] == '/'; while (!path_lexer.is_eof() || extra_iteration) { diff --git a/Kernel/FileSystem/VirtualFileSystem.h b/Kernel/FileSystem/VirtualFileSystem.h index a9b358eb01..26459474d8 100644 --- a/Kernel/FileSystem/VirtualFileSystem.h +++ b/Kernel/FileSystem/VirtualFileSystem.h @@ -67,7 +67,7 @@ public: ErrorOr utimensat(StringView path, Custody& base, timespec const& atime, timespec const& mtime, int options = 0); ErrorOr rename(StringView oldpath, StringView newpath, Custody& base); ErrorOr mknod(StringView path, mode_t, dev_t, Custody& base); - ErrorOr> open_directory(StringView path, Custody& base); + ErrorOr> open_directory(StringView path, Custody& base); ErrorOr for_each_mount(Function(Mount const&)>) const; @@ -75,9 +75,9 @@ public: static void sync(); - Custody& root_custody(); - ErrorOr> resolve_path(StringView path, Custody& base, LockRefPtr* out_parent = nullptr, int options = 0, int symlink_recursion_level = 0); - ErrorOr> resolve_path_without_veil(StringView path, Custody& base, LockRefPtr* out_parent = nullptr, int options = 0, int symlink_recursion_level = 0); + NonnullRefPtr root_custody(); + ErrorOr> resolve_path(StringView path, NonnullRefPtr base, RefPtr* out_parent = nullptr, int options = 0, int symlink_recursion_level = 0); + ErrorOr> resolve_path_without_veil(StringView path, NonnullRefPtr base, RefPtr* out_parent = nullptr, int options = 0, int symlink_recursion_level = 0); private: friend class OpenFileDescription; @@ -92,13 +92,15 @@ private: bool mount_point_exists_at_inode(InodeIdentifier inode); + // FIXME: These functions are totally unsafe as someone could unmount the returned Mount underneath us. Mount* find_mount_for_host(InodeIdentifier); Mount* find_mount_for_guest(InodeIdentifier); LockRefPtr m_root_inode; - LockRefPtr m_root_custody; - SpinlockProtected> m_mounts { LockRank::None }; + SpinlockProtected> m_root_custody; + + SpinlockProtected, 16>> m_mounts { LockRank::None }; }; } diff --git a/Kernel/Process.cpp b/Kernel/Process.cpp index 5adb6d85e7..c069d0f0ed 100644 --- a/Kernel/Process.cpp +++ b/Kernel/Process.cpp @@ -222,7 +222,7 @@ void Process::unprotect_data() }); } -ErrorOr> Process::try_create(LockRefPtr& first_thread, NonnullOwnPtr name, UserID uid, GroupID gid, ProcessID ppid, bool is_kernel_process, LockRefPtr current_directory, LockRefPtr executable, TTY* tty, Process* fork_parent) +ErrorOr> Process::try_create(LockRefPtr& first_thread, NonnullOwnPtr name, UserID uid, GroupID gid, ProcessID ppid, bool is_kernel_process, RefPtr current_directory, RefPtr executable, TTY* tty, Process* fork_parent) { auto space = TRY(Memory::AddressSpace::try_create(fork_parent ? &fork_parent->address_space() : nullptr)); auto unveil_tree = UnveilNode { TRY(KString::try_create("/"sv)), UnveilMetadata(TRY(KString::try_create("/"sv))) }; @@ -232,10 +232,10 @@ ErrorOr> Process::try_create(LockRefPtr& firs return process; } -Process::Process(NonnullOwnPtr name, NonnullRefPtr credentials, ProcessID ppid, bool is_kernel_process, LockRefPtr current_directory, LockRefPtr executable, TTY* tty, UnveilNode unveil_tree) +Process::Process(NonnullOwnPtr name, NonnullRefPtr credentials, ProcessID ppid, bool is_kernel_process, RefPtr current_directory, RefPtr executable, TTY* tty, UnveilNode unveil_tree) : m_name(move(name)) , m_is_kernel_process(is_kernel_process) - , m_executable(move(executable)) + , m_executable(LockRank::None, move(executable)) , m_current_directory(LockRank::None, move(current_directory)) , m_tty(tty) , m_unveil_data(LockRank::None, move(unveil_tree)) @@ -537,9 +537,9 @@ siginfo_t Process::wait_info() const return siginfo; } -NonnullLockRefPtr Process::current_directory() +NonnullRefPtr Process::current_directory() { - return m_current_directory.with([&](auto& current_directory) -> NonnullLockRefPtr { + return m_current_directory.with([&](auto& current_directory) -> NonnullRefPtr { if (!current_directory) current_directory = VirtualFileSystem::the().root_custody(); return *current_directory; @@ -642,7 +642,7 @@ void Process::finalize() TimerQueue::the().cancel_timer(m_alarm_timer.release_nonnull()); m_fds.with_exclusive([](auto& fds) { fds.clear(); }); m_tty = nullptr; - m_executable = nullptr; + m_executable.with([](auto& executable) { executable = nullptr; }); m_arguments.clear(); m_environment.clear(); @@ -971,4 +971,14 @@ NonnullRefPtr Process::credentials() const return *m_protected_values.credentials; } +RefPtr Process::executable() +{ + return m_executable.with([](auto& executable) { return executable; }); +} + +RefPtr Process::executable() const +{ + return m_executable.with([](auto& executable) { return executable; }); +} + } diff --git a/Kernel/Process.h b/Kernel/Process.h index 887b80aea0..345cc4e343 100644 --- a/Kernel/Process.h +++ b/Kernel/Process.h @@ -449,9 +449,9 @@ public: u32 m_ticks_in_user_for_dead_children { 0 }; u32 m_ticks_in_kernel_for_dead_children { 0 }; - NonnullLockRefPtr current_directory(); - Custody* executable() { return m_executable.ptr(); } - Custody const* executable() const { return m_executable.ptr(); } + NonnullRefPtr current_directory(); + RefPtr executable(); + RefPtr executable() const; static constexpr size_t max_arguments_size = Thread::default_userspace_stack_size / 8; static constexpr size_t max_environment_size = Thread::default_userspace_stack_size / 8; @@ -556,8 +556,8 @@ private: bool add_thread(Thread&); bool remove_thread(Thread&); - Process(NonnullOwnPtr name, NonnullRefPtr, ProcessID ppid, bool is_kernel_process, LockRefPtr current_directory, LockRefPtr executable, TTY* tty, UnveilNode unveil_tree); - static ErrorOr> try_create(LockRefPtr& first_thread, NonnullOwnPtr name, UserID, GroupID, ProcessID ppid, bool is_kernel_process, LockRefPtr current_directory = nullptr, LockRefPtr executable = nullptr, TTY* = nullptr, Process* fork_parent = nullptr); + Process(NonnullOwnPtr name, NonnullRefPtr, ProcessID ppid, bool is_kernel_process, RefPtr current_directory, RefPtr executable, TTY* tty, UnveilNode unveil_tree); + static ErrorOr> try_create(LockRefPtr& first_thread, NonnullOwnPtr name, UserID, GroupID, ProcessID ppid, bool is_kernel_process, RefPtr current_directory = nullptr, RefPtr executable = nullptr, TTY* = nullptr, Process* fork_parent = nullptr); ErrorOr attach_resources(NonnullOwnPtr&&, LockRefPtr& first_thread, Process* fork_parent); static ProcessID allocate_pid(); @@ -824,9 +824,9 @@ private: Atomic m_is_stopped { false }; bool m_should_generate_coredump { false }; - LockRefPtr m_executable; + SpinlockProtected> m_executable; - SpinlockProtected> m_current_directory; + SpinlockProtected> m_current_directory; NonnullOwnPtrVector m_arguments; NonnullOwnPtrVector m_environment; diff --git a/Kernel/ProcessSpecificExposed.cpp b/Kernel/ProcessSpecificExposed.cpp index 8e25c28c20..82fcf6735e 100644 --- a/Kernel/ProcessSpecificExposed.cpp +++ b/Kernel/ProcessSpecificExposed.cpp @@ -333,7 +333,7 @@ mode_t Process::binary_link_required_mode() const ErrorOr Process::procfs_get_binary_link(KBufferBuilder& builder) const { - auto const* custody = executable(); + auto custody = executable(); if (!custody) return Error::from_errno(ENOEXEC); return builder.append(TRY(custody->try_serialize_absolute_path())->view()); diff --git a/Kernel/Syscalls/chdir.cpp b/Kernel/Syscalls/chdir.cpp index 445a4fc03e..a10a7cbc5b 100644 --- a/Kernel/Syscalls/chdir.cpp +++ b/Kernel/Syscalls/chdir.cpp @@ -4,6 +4,7 @@ * SPDX-License-Identifier: BSD-2-Clause */ +#include #include #include #include @@ -15,10 +16,10 @@ ErrorOr Process::sys$chdir(Userspace user_path, size_t pat VERIFY_NO_PROCESS_BIG_LOCK(this); TRY(require_promise(Pledge::rpath)); auto path = TRY(get_syscall_path_argument(user_path, path_length)); - auto current_directory = m_current_directory.with([](auto& current_directory) -> NonnullLockRefPtr { + auto current_directory = m_current_directory.with([](auto& current_directory) -> NonnullRefPtr { return *current_directory; }); - LockRefPtr new_directory = TRY(VirtualFileSystem::the().open_directory(path->view(), *current_directory)); + RefPtr new_directory = TRY(VirtualFileSystem::the().open_directory(path->view(), *current_directory)); m_current_directory.with([&](auto& current_directory) { // NOTE: We use swap() here to avoid manipulating the ref counts while holding the lock. swap(current_directory, new_directory); diff --git a/Kernel/Syscalls/chmod.cpp b/Kernel/Syscalls/chmod.cpp index 564b87a61e..c1f2e2fc8d 100644 --- a/Kernel/Syscalls/chmod.cpp +++ b/Kernel/Syscalls/chmod.cpp @@ -18,7 +18,7 @@ ErrorOr Process::sys$chmod(Userspace u auto params = TRY(copy_typed_from_user(user_params)); auto path = TRY(get_syscall_path_argument(params.path)); - LockRefPtr base; + RefPtr base; if (params.dirfd == AT_FDCWD) { base = current_directory(); } else { diff --git a/Kernel/Syscalls/chown.cpp b/Kernel/Syscalls/chown.cpp index 70c49b2876..4a72df16a2 100644 --- a/Kernel/Syscalls/chown.cpp +++ b/Kernel/Syscalls/chown.cpp @@ -4,6 +4,7 @@ * SPDX-License-Identifier: BSD-2-Clause */ +#include #include #include #include @@ -27,7 +28,7 @@ ErrorOr Process::sys$chown(Userspace u auto params = TRY(copy_typed_from_user(user_params)); auto path = TRY(get_syscall_path_argument(params.path)); - LockRefPtr base; + RefPtr base; if (params.dirfd == AT_FDCWD) { base = current_directory(); } else { diff --git a/Kernel/Syscalls/execve.cpp b/Kernel/Syscalls/execve.cpp index 0a8dd5eeeb..405c9dbc8c 100644 --- a/Kernel/Syscalls/execve.cpp +++ b/Kernel/Syscalls/execve.cpp @@ -554,7 +554,7 @@ ErrorOr Process::do_exec(NonnullLockRefPtr main_progr m_space = load_result.space.release_nonnull(); - m_executable = main_program_description->custody(); + m_executable.with([&](auto& executable) { executable = main_program_description->custody(); }); m_arguments = move(arguments); m_environment = move(environment); diff --git a/Kernel/Syscalls/fork.cpp b/Kernel/Syscalls/fork.cpp index 772b38764b..2009f743a7 100644 --- a/Kernel/Syscalls/fork.cpp +++ b/Kernel/Syscalls/fork.cpp @@ -28,7 +28,7 @@ ErrorOr Process::sys$fork(RegisterState& regs) }; auto child_name = TRY(m_name->try_clone()); - auto child = TRY(Process::try_create(child_first_thread, move(child_name), uid(), gid(), pid(), m_is_kernel_process, current_directory(), m_executable, m_tty, this)); + auto child = TRY(Process::try_create(child_first_thread, move(child_name), uid(), gid(), pid(), m_is_kernel_process, current_directory(), executable(), m_tty, this)); // NOTE: All user processes have a leaked ref on them. It's balanced by Thread::WaitBlockerSet::finalize(). child->ref(); diff --git a/Kernel/Syscalls/open.cpp b/Kernel/Syscalls/open.cpp index 77294cd0f7..2a0c773255 100644 --- a/Kernel/Syscalls/open.cpp +++ b/Kernel/Syscalls/open.cpp @@ -4,6 +4,7 @@ * SPDX-License-Identifier: BSD-2-Clause */ +#include #include #include #include @@ -54,7 +55,7 @@ ErrorOr Process::sys$open(Userspace use dbgln_if(IO_DEBUG, "sys$open(dirfd={}, path='{}', options={}, mode={})", dirfd, path->view(), options, mode); auto fd_allocation = TRY(allocate_fd()); - LockRefPtr base; + RefPtr base; if (dirfd == AT_FDCWD) { base = current_directory(); } else { diff --git a/Kernel/Syscalls/stat.cpp b/Kernel/Syscalls/stat.cpp index 39eea5b7e9..89194d32d0 100644 --- a/Kernel/Syscalls/stat.cpp +++ b/Kernel/Syscalls/stat.cpp @@ -4,6 +4,7 @@ * SPDX-License-Identifier: BSD-2-Clause */ +#include #include #include #include @@ -29,7 +30,7 @@ ErrorOr Process::sys$stat(Userspace use auto path = TRY(get_syscall_path_argument(params.path)); - LockRefPtr base; + RefPtr base; if (params.dirfd == AT_FDCWD) { base = current_directory(); } else { diff --git a/Kernel/Syscalls/unlink.cpp b/Kernel/Syscalls/unlink.cpp index 21a5edbc3c..5f3a62330e 100644 --- a/Kernel/Syscalls/unlink.cpp +++ b/Kernel/Syscalls/unlink.cpp @@ -19,7 +19,7 @@ ErrorOr Process::sys$unlink(int dirfd, Userspace user_path if (flags & ~AT_REMOVEDIR) return Error::from_errno(EINVAL); - LockRefPtr base; + RefPtr base; if (dirfd == AT_FDCWD) { base = current_directory(); } else { diff --git a/Kernel/Syscalls/unveil.cpp b/Kernel/Syscalls/unveil.cpp index a993f00952..a27ebefbbb 100644 --- a/Kernel/Syscalls/unveil.cpp +++ b/Kernel/Syscalls/unveil.cpp @@ -5,6 +5,7 @@ * SPDX-License-Identifier: BSD-2-Clause */ +#include #include #include #include @@ -79,7 +80,7 @@ ErrorOr Process::sys$unveil(Userspace // However, if the user specified unveil() with "c" permissions, we don't set errno if ENOENT is encountered, // because they most likely intend the program to create the file for them later on. // If this case is encountered, the parent node of the path is returned and the custody of that inode is used instead. - LockRefPtr parent_custody; // Parent inode in case of ENOENT + RefPtr parent_custody; // Parent inode in case of ENOENT OwnPtr new_unveiled_path; auto custody_or_error = VirtualFileSystem::the().resolve_path_without_veil(path->view(), VirtualFileSystem::the().root_custody(), &parent_custody); if (!custody_or_error.is_error()) { diff --git a/Kernel/Syscalls/utimensat.cpp b/Kernel/Syscalls/utimensat.cpp index 45bfd1cb1b..73b13ecb2e 100644 --- a/Kernel/Syscalls/utimensat.cpp +++ b/Kernel/Syscalls/utimensat.cpp @@ -41,7 +41,7 @@ ErrorOr Process::sys$utimensat(Userspace path; LockRefPtr description; - LockRefPtr base; + RefPtr base; auto path_or_error = get_syscall_path_argument(params.path); if (path_or_error.is_error()) {