From 1014aefe649aa50d69aa10aba647872e2ff99a93 Mon Sep 17 00:00:00 2001 From: Sam Atkins Date: Sat, 4 Feb 2023 14:11:35 +0000 Subject: [PATCH] Kernel: Protect Thread::m_name with a spinlock This replaces manually grabbing the thread's main lock. This lets us remove the `get_thread_name` and `set_thread_name` syscalls from the big lock. :^) --- Kernel/API/Syscall.h | 4 ++-- .../SysFS/Subsystems/Kernel/Processes.cpp | 2 +- Kernel/Syscalls/thread.cpp | 24 +++++++++---------- Kernel/Thread.cpp | 8 +++++++ Kernel/Thread.h | 16 ++++--------- 5 files changed, 27 insertions(+), 27 deletions(-) diff --git a/Kernel/API/Syscall.h b/Kernel/API/Syscall.h index eff0fadc4f2..342b4daabc3 100644 --- a/Kernel/API/Syscall.h +++ b/Kernel/API/Syscall.h @@ -85,7 +85,7 @@ enum class NeedsBigProcessLock { S(get_process_name, NeedsBigProcessLock::No) \ S(get_root_session_id, NeedsBigProcessLock::No) \ S(get_stack_bounds, NeedsBigProcessLock::No) \ - S(get_thread_name, NeedsBigProcessLock::Yes) \ + S(get_thread_name, NeedsBigProcessLock::No) \ S(getcwd, NeedsBigProcessLock::No) \ S(getegid, NeedsBigProcessLock::No) \ S(geteuid, NeedsBigProcessLock::No) \ @@ -158,7 +158,7 @@ enum class NeedsBigProcessLock { S(set_coredump_metadata, NeedsBigProcessLock::No) \ S(set_mmap_name, NeedsBigProcessLock::Yes) \ S(set_process_name, NeedsBigProcessLock::No) \ - S(set_thread_name, NeedsBigProcessLock::Yes) \ + S(set_thread_name, NeedsBigProcessLock::No) \ S(setegid, NeedsBigProcessLock::No) \ S(seteuid, NeedsBigProcessLock::No) \ S(setgid, NeedsBigProcessLock::No) \ diff --git a/Kernel/FileSystem/SysFS/Subsystems/Kernel/Processes.cpp b/Kernel/FileSystem/SysFS/Subsystems/Kernel/Processes.cpp index e53d1047b79..85663ce4753 100644 --- a/Kernel/FileSystem/SysFS/Subsystems/Kernel/Processes.cpp +++ b/Kernel/FileSystem/SysFS/Subsystems/Kernel/Processes.cpp @@ -118,7 +118,7 @@ ErrorOr SysFSOverallProcesses::try_generate(KBufferBuilder& builder) TRY(thread_object.add("lock_count"sv, thread.lock_count())); #endif TRY(thread_object.add("tid"sv, thread.tid().value())); - TRY(thread_object.add("name"sv, thread.name())); + TRY(thread.name().with([&](auto& thread_name) { return thread_object.add("name"sv, thread_name->view()); })); TRY(thread_object.add("times_scheduled"sv, thread.times_scheduled())); TRY(thread_object.add("time_user"sv, thread.time_in_user())); TRY(thread_object.add("time_kernel"sv, thread.time_in_kernel())); diff --git a/Kernel/Syscalls/thread.cpp b/Kernel/Syscalls/thread.cpp index 1093f48fca1..46e120a74ac 100644 --- a/Kernel/Syscalls/thread.cpp +++ b/Kernel/Syscalls/thread.cpp @@ -180,7 +180,7 @@ ErrorOr Process::sys$kill_thread(pid_t tid, int signal) ErrorOr Process::sys$set_thread_name(pid_t tid, Userspace user_name, size_t user_name_length) { - VERIFY_PROCESS_BIG_LOCK_ACQUIRED(this); + VERIFY_NO_PROCESS_BIG_LOCK(this); TRY(require_promise(Pledge::stdio)); auto name = TRY(try_copy_kstring_from_user(user_name, user_name_length)); @@ -199,7 +199,7 @@ ErrorOr Process::sys$set_thread_name(pid_t tid, Userspace ErrorOr Process::sys$get_thread_name(pid_t tid, Userspace buffer, size_t buffer_size) { - VERIFY_PROCESS_BIG_LOCK_ACQUIRED(this); + VERIFY_NO_PROCESS_BIG_LOCK(this); TRY(require_promise(Pledge::thread)); if (buffer_size == 0) return EINVAL; @@ -208,19 +208,19 @@ ErrorOr Process::sys$get_thread_name(pid_t tid, Userspace buffer if (!thread || thread->pid() != pid()) return ESRCH; - SpinlockLocker locker(thread->get_lock()); - auto thread_name = thread->name(); + TRY(thread->name().with([&](auto& thread_name) -> ErrorOr { + if (thread_name->view().is_null()) { + char null_terminator = '\0'; + TRY(copy_to_user(buffer, &null_terminator, sizeof(null_terminator))); + return {}; + } - if (thread_name.is_null()) { - char null_terminator = '\0'; - TRY(copy_to_user(buffer, &null_terminator, sizeof(null_terminator))); - return 0; - } + if (thread_name->length() + 1 > buffer_size) + return ENAMETOOLONG; - if (thread_name.length() + 1 > buffer_size) - return ENAMETOOLONG; + return copy_to_user(buffer, thread_name->characters(), thread_name->length() + 1); + })); - TRY(copy_to_user(buffer, thread_name.characters_without_null_termination(), thread_name.length() + 1)); return 0; } diff --git a/Kernel/Thread.cpp b/Kernel/Thread.cpp index 0c047e4df8e..28f8a0d6ea3 100644 --- a/Kernel/Thread.cpp +++ b/Kernel/Thread.cpp @@ -1466,6 +1466,14 @@ void Thread::track_lock_release(LockRank rank) m_lock_rank_mask ^= rank; } + +void Thread::set_name(NonnullOwnPtr name) +{ + m_name.with([&](auto& this_name) { + this_name = move(name); + }); +} + } ErrorOr AK::Formatter::format(FormatBuilder& builder, Kernel::Thread const& value) diff --git a/Kernel/Thread.h b/Kernel/Thread.h index a45889c2ea4..7e7e8559e36 100644 --- a/Kernel/Thread.h +++ b/Kernel/Thread.h @@ -94,19 +94,11 @@ public: Process& process() { return m_process; } Process const& process() const { return m_process; } - // NOTE: This returns a null-terminated string. - StringView name() const + SpinlockProtected, LockRank::None> const& name() const { - // NOTE: Whoever is calling this needs to be holding our lock while reading the name. - VERIFY(m_lock.is_locked_by_current_processor()); - return m_name->view(); - } - - void set_name(NonnullOwnPtr name) - { - SpinlockLocker lock(m_lock); - m_name = move(name); + return m_name; } + void set_name(NonnullOwnPtr name); void finalize(); @@ -1229,7 +1221,7 @@ private: FPUState m_fpu_state {}; State m_state { Thread::State::Invalid }; - NonnullOwnPtr m_name; + SpinlockProtected, LockRank::None> m_name; u32 m_priority { THREAD_PRIORITY_NORMAL }; State m_stop_state { Thread::State::Invalid };