From 1c786710787a9d61f75132bc9575a7556faa3780 Mon Sep 17 00:00:00 2001 From: Bananymous Date: Wed, 6 Dec 2023 13:02:17 +0200 Subject: [PATCH] Kernel: Rework all signal code Signal handling code was way too complex. Now everything is simplified and there is no need for ThreadBlockers. Only complication that this patch includes is that blocking syscalls have to manually be made interruptable by signal. There might be some clever solution to combat this is make this happen automatically. --- kernel/arch/x86_64/Signal.S | 4 - kernel/include/kernel/Thread.h | 29 +++--- kernel/kernel/FS/Inode.cpp | 12 --- kernel/kernel/Process.cpp | 9 +- kernel/kernel/Scheduler.cpp | 4 +- kernel/kernel/Storage/StorageDevice.cpp | 6 -- kernel/kernel/Syscall.cpp | 15 ++-- kernel/kernel/Terminal/TTY.cpp | 4 + kernel/kernel/Thread.cpp | 115 +++++++----------------- libc/include/sys/syscall.h | 1 - 10 files changed, 61 insertions(+), 138 deletions(-) diff --git a/kernel/arch/x86_64/Signal.S b/kernel/arch/x86_64/Signal.S index 0e24f639be..cdb610baea 100644 --- a/kernel/arch/x86_64/Signal.S +++ b/kernel/arch/x86_64/Signal.S @@ -31,10 +31,6 @@ signal_trampoline: movq 120(%rsp), %rax call *%rax - movq $SYS_SIGNAL_DONE, %rax - movq 128(%rsp), %rbx - int $0x80 - popq %r15 popq %r14 popq %r13 diff --git a/kernel/include/kernel/Thread.h b/kernel/include/kernel/Thread.h index 3d5383b072..30b3527bc8 100644 --- a/kernel/include/kernel/Thread.h +++ b/kernel/include/kernel/Thread.h @@ -25,19 +25,9 @@ namespace Kernel { NotStarted, Executing, - Terminating, Terminated }; - class TerminateBlocker - { - public: - TerminateBlocker(Thread&); - ~TerminateBlocker(); - private: - Thread& m_thread; - }; - public: static BAN::ErrorOr create_kernel(entry_t, void*, Process*); static BAN::ErrorOr create_userspace(Process*); @@ -47,15 +37,20 @@ namespace Kernel void setup_exec(); void setup_process_cleanup(); - bool has_signal_to_execute() const; - void set_signal_done(int signal); + // Adds pending signals to thread if possible and + // returns true, if thread is going to trigger signal + bool is_interrupted_by_signal(); + + // Returns true if pending signal can be added to thread + bool can_add_signal_to_execute() const; + bool will_execute_signal() const; void handle_signal(int signal = 0); bool add_signal(int signal); void set_return_rsp(uintptr_t& rsp) { m_return_rsp = &rsp; } void set_return_rip(uintptr_t& rip) { m_return_rip = &rip; } - uintptr_t& return_rsp() { ASSERT(m_return_rsp); return *m_return_rsp; } - uintptr_t& return_rip() { ASSERT(m_return_rip); return *m_return_rip; } + uintptr_t return_rsp() { ASSERT(m_return_rsp); return *m_return_rsp; } + uintptr_t return_rip() { ASSERT(m_return_rip); return *m_return_rip; } pid_t tid() const { return m_tid; } @@ -65,7 +60,8 @@ namespace Kernel uintptr_t rip() const { return m_rip; } void set_started() { ASSERT(m_state == State::NotStarted); m_state = State::Executing; } - void set_terminating(); + // Thread will no longer execute. If called on current thread, does not return + void terminate(); State state() const { return m_state; } vaddr_t stack_base() const { return m_stack->vaddr(); } @@ -116,11 +112,8 @@ namespace Kernel uint64_t m_signal_pending_mask { 0 }; uint64_t m_signal_block_mask { 0 }; - int m_handling_signal { 0 }; static_assert(_SIGMAX < 64); - uint64_t m_terminate_blockers { 0 }; - #if __enable_sse alignas(16) uint8_t m_sse_storage[512] {}; #endif diff --git a/kernel/kernel/FS/Inode.cpp b/kernel/kernel/FS/Inode.cpp index 7227934285..7f0311a821 100644 --- a/kernel/kernel/FS/Inode.cpp +++ b/kernel/kernel/FS/Inode.cpp @@ -1,6 +1,5 @@ #include #include -#include #include @@ -60,7 +59,6 @@ namespace Kernel BAN::ErrorOr> Inode::find_inode(BAN::StringView name) { LockGuard _(m_lock); - Thread::TerminateBlocker blocker(Thread::current()); if (!mode().ifdir()) return BAN::Error::from_errno(ENOTDIR); return find_inode_impl(name); @@ -69,7 +67,6 @@ namespace Kernel BAN::ErrorOr Inode::list_next_inodes(off_t offset, DirectoryEntryList* list, size_t list_len) { LockGuard _(m_lock); - Thread::TerminateBlocker blocker(Thread::current()); if (!mode().ifdir()) return BAN::Error::from_errno(ENOTDIR); return list_next_inodes_impl(offset, list, list_len); @@ -78,7 +75,6 @@ namespace Kernel BAN::ErrorOr Inode::create_file(BAN::StringView name, mode_t mode, uid_t uid, gid_t gid) { LockGuard _(m_lock); - Thread::TerminateBlocker blocker(Thread::current()); if (!this->mode().ifdir()) return BAN::Error::from_errno(ENOTDIR); if (Mode(mode).ifdir()) @@ -89,7 +85,6 @@ namespace Kernel BAN::ErrorOr Inode::create_directory(BAN::StringView name, mode_t mode, uid_t uid, gid_t gid) { LockGuard _(m_lock); - Thread::TerminateBlocker blocker(Thread::current()); if (!this->mode().ifdir()) return BAN::Error::from_errno(ENOTDIR); if (!Mode(mode).ifdir()) @@ -100,7 +95,6 @@ namespace Kernel BAN::ErrorOr Inode::unlink(BAN::StringView name) { LockGuard _(m_lock); - Thread::TerminateBlocker blocker(Thread::current()); if (!mode().ifdir()) return BAN::Error::from_errno(ENOTDIR); if (name == "."sv || name == ".."sv) @@ -111,7 +105,6 @@ namespace Kernel BAN::ErrorOr Inode::link_target() { LockGuard _(m_lock); - Thread::TerminateBlocker blocker(Thread::current()); if (!mode().iflnk()) return BAN::Error::from_errno(EINVAL); return link_target_impl(); @@ -120,7 +113,6 @@ namespace Kernel BAN::ErrorOr Inode::read(off_t offset, BAN::ByteSpan buffer) { LockGuard _(m_lock); - Thread::TerminateBlocker blocker(Thread::current()); if (mode().ifdir()) return BAN::Error::from_errno(EISDIR); return read_impl(offset, buffer); @@ -129,7 +121,6 @@ namespace Kernel BAN::ErrorOr Inode::write(off_t offset, BAN::ConstByteSpan buffer) { LockGuard _(m_lock); - Thread::TerminateBlocker blocker(Thread::current()); if (mode().ifdir()) return BAN::Error::from_errno(EISDIR); return write_impl(offset, buffer); @@ -138,7 +129,6 @@ namespace Kernel BAN::ErrorOr Inode::truncate(size_t size) { LockGuard _(m_lock); - Thread::TerminateBlocker blocker(Thread::current()); if (mode().ifdir()) return BAN::Error::from_errno(EISDIR); return truncate_impl(size); @@ -148,14 +138,12 @@ namespace Kernel { ASSERT((mode & Inode::Mode::TYPE_MASK) == 0); LockGuard _(m_lock); - Thread::TerminateBlocker blocker(Thread::current()); return chmod_impl(mode); } bool Inode::has_data() const { LockGuard _(m_lock); - Thread::TerminateBlocker blocker(Thread::current()); return has_data_impl(); } diff --git a/kernel/kernel/Process.cpp b/kernel/kernel/Process.cpp index 82f93ede0e..cc205e254f 100644 --- a/kernel/kernel/Process.cpp +++ b/kernel/kernel/Process.cpp @@ -257,7 +257,10 @@ namespace Kernel LockGuard _(m_lock); m_exit_status.exit_code = __WGENEXITCODE(status, signal); for (auto* thread : m_threads) - thread->set_terminating(); + if (thread != &Thread::current()) + thread->terminate(); + if (this == &Process::current()) + Thread::current().terminate(); } size_t Process::proc_meminfo(off_t offset, BAN::ByteSpan buffer) const @@ -334,7 +337,7 @@ namespace Kernel BAN::ErrorOr Process::sys_exit(int status) { exit(status, 0); - Thread::TerminateBlocker _(Thread::current()); + Thread::current().terminate(); ASSERT_NOT_REACHED(); } @@ -1235,6 +1238,8 @@ namespace Kernel { CriticalScope _; process.m_signal_pending_mask |= 1 << signal; + // FIXME: This is super hacky + Scheduler::get().unblock_thread(process.m_threads.front()->tid()); } return (pid > 0) ? BAN::Iteration::Break : BAN::Iteration::Continue; } diff --git a/kernel/kernel/Scheduler.cpp b/kernel/kernel/Scheduler.cpp index 22ba59c234..b47c19ea4c 100644 --- a/kernel/kernel/Scheduler.cpp +++ b/kernel/kernel/Scheduler.cpp @@ -258,10 +258,8 @@ namespace Kernel current->set_started(); start_thread(current->rsp(), current->rip()); case Thread::State::Executing: - while (current->has_signal_to_execute()) + while (current->can_add_signal_to_execute()) current->handle_signal(); - // fall through - case Thread::State::Terminating: continue_thread(current->rsp(), current->rip()); case Thread::State::Terminated: ASSERT_NOT_REACHED(); diff --git a/kernel/kernel/Storage/StorageDevice.cpp b/kernel/kernel/Storage/StorageDevice.cpp index cd6c86e3a0..161af64415 100644 --- a/kernel/kernel/Storage/StorageDevice.cpp +++ b/kernel/kernel/Storage/StorageDevice.cpp @@ -225,7 +225,6 @@ namespace Kernel { LockGuard _(m_lock); - Thread::TerminateBlocker blocker(Thread::current()); if (!m_disk_cache.has_value()) return read_sectors_impl(lba, sector_count, buffer); } @@ -233,8 +232,6 @@ namespace Kernel for (uint64_t offset = 0; offset < sector_count; offset++) { LockGuard _(m_lock); - Thread::TerminateBlocker blocker(Thread::current()); - auto sector_buffer = buffer.slice(offset * sector_size(), sector_size()); if (m_disk_cache->read_from_cache(lba + offset, sector_buffer)) continue; @@ -251,7 +248,6 @@ namespace Kernel { LockGuard _(m_lock); - Thread::TerminateBlocker blocker(Thread::current()); if (!m_disk_cache.has_value()) return write_sectors_impl(lba, sector_count, buffer); } @@ -259,8 +255,6 @@ namespace Kernel for (uint8_t offset = 0; offset < sector_count; offset++) { LockGuard _(m_lock); - Thread::TerminateBlocker blocker(Thread::current()); - auto sector_buffer = buffer.slice(offset * sector_size(), sector_size()); if (m_disk_cache->write_to_cache(lba + offset, sector_buffer, true).is_error()) TRY(write_sectors_impl(lba + offset, 1, sector_buffer)); diff --git a/kernel/kernel/Syscall.cpp b/kernel/kernel/Syscall.cpp index 030a74f71c..128c8d86f6 100644 --- a/kernel/kernel/Syscall.cpp +++ b/kernel/kernel/Syscall.cpp @@ -26,12 +26,6 @@ namespace Kernel Thread::current().set_return_rsp(interrupt_stack.rsp); Thread::current().set_return_rip(interrupt_stack.rip); - if (syscall == SYS_SIGNAL_DONE) - { - Thread::current().set_signal_done((int)arg1); - return 0; - } - #if __enable_sse Thread::current().save_sse(); #endif @@ -154,9 +148,6 @@ namespace Kernel case SYS_SIGNAL: ret = Process::current().sys_signal((int)arg1, (void (*)(int))arg2); break; - case SYS_SIGNAL_DONE: - // Handled above - ASSERT_NOT_REACHED(); case SYS_TCSETPGRP: ret = Process::current().sys_tcsetpgrp((int)arg1, (pid_t)arg2); break; @@ -230,10 +221,14 @@ namespace Kernel if (ret.is_error() && ret.error().is_kernel_error()) Kernel::panic("Kernel error while returning to userspace {}", ret.error()); + auto& current_thread = Thread::current(); + if (current_thread.can_add_signal_to_execute()) + current_thread.handle_signal(); + ASSERT(Kernel::Thread::current().state() == Kernel::Thread::State::Executing); #if __enable_sse - Thread::current().load_sse(); + current_thread.load_sse(); #endif if (ret.is_error()) diff --git a/kernel/kernel/Terminal/TTY.cpp b/kernel/kernel/Terminal/TTY.cpp index 33e555fd4e..d02ef985a3 100644 --- a/kernel/kernel/Terminal/TTY.cpp +++ b/kernel/kernel/Terminal/TTY.cpp @@ -303,10 +303,14 @@ namespace Kernel LockGuard _(m_lock); while (!m_output.flush) { + if (Thread::current().is_interrupted_by_signal()) + return BAN::Error::from_errno(EINTR); m_lock.unlock(); m_output.semaphore.block(); m_lock.lock(); } + if (Thread::current().is_interrupted_by_signal()) + return BAN::Error::from_errno(EINTR); if (m_output.bytes == 0) { diff --git a/kernel/kernel/Thread.cpp b/kernel/kernel/Thread.cpp index 7ef49f916c..8ff9670918 100644 --- a/kernel/kernel/Thread.cpp +++ b/kernel/kernel/Thread.cpp @@ -1,6 +1,8 @@ #include #include +#include #include +#include #include #include #include @@ -25,63 +27,13 @@ namespace Kernel memcpy((void*)rsp, (void*)&value, sizeof(uintptr_t)); } - Thread::TerminateBlocker::TerminateBlocker(Thread& thread) - : m_thread(thread) + void Thread::terminate() { CriticalScope _; - - // FIXME: this should not be a requirement - ASSERT(&thread == &Thread::current()); - - if (m_thread.state() == State::Executing || m_thread.m_terminate_blockers > 0) - { - m_thread.m_terminate_blockers++; - return; - } - - if (m_thread.state() == State::Terminating && m_thread.m_terminate_blockers == 0) - { - m_thread.m_state = State::Terminated; + ASSERT(this == &Thread::current()); + m_state = Thread::State::Terminated; + if (this == &Thread::current()) Scheduler::get().execute_current_thread(); - } - - while (true) - Scheduler::get().reschedule(); - } - - Thread::TerminateBlocker::~TerminateBlocker() - { - CriticalScope _; - - m_thread.m_terminate_blockers--; - - if (m_thread.state() == State::Executing || m_thread.m_terminate_blockers > 0) - return; - - if (m_thread.state() == State::Terminating && m_thread.m_terminate_blockers == 0) - { - m_thread.m_state = State::Terminated; - Scheduler::get().execute_current_thread(); - } - - ASSERT_NOT_REACHED(); - } - - void Thread::set_terminating() - { - CriticalScope _; - if (m_terminate_blockers == 0) - { - m_state = State::Terminated; - if (this == &Thread::current()) - Scheduler::get().execute_current_thread(); - } - else - { - m_state = State::Terminating; - if (this == &Thread::current()) - Scheduler::get().unblock_thread(tid()); - } } static pid_t s_next_tid = 1; @@ -245,27 +197,30 @@ namespace Kernel } } - bool Thread::has_signal_to_execute() const + bool Thread::is_interrupted_by_signal() { - if (!is_userspace() || m_handling_signal || m_state != State::Executing) + while (can_add_signal_to_execute()) + handle_signal(); + return will_execute_signal(); + } + + bool Thread::can_add_signal_to_execute() const + { + if (!is_userspace() || m_state != State::Executing) + return false; + auto& interrupt_stack = *reinterpret_cast(interrupt_stack_base() + interrupt_stack_size() - sizeof(InterruptStack)); + if (!GDT::is_user_segment(interrupt_stack.cs)) return false; uint64_t full_pending_mask = m_signal_pending_mask | m_process->m_signal_pending_mask; return full_pending_mask & ~m_signal_block_mask; } - void Thread::set_signal_done(int signal) + bool Thread::will_execute_signal() const { - ASSERT(!interrupts_enabled()); - if (m_handling_signal == 0) - derrorln("set_signal_done called while not handling singal"); - else if (m_handling_signal != signal) - derrorln("set_signal_done called with invalid signal"); - else - m_handling_signal = 0; - - if (m_handling_signal == 0) - while (has_signal_to_execute()) - handle_signal(); + if (!is_userspace() || m_state != State::Executing) + return false; + auto& interrupt_stack = *reinterpret_cast(interrupt_stack_base() + interrupt_stack_size() - sizeof(InterruptStack)); + return interrupt_stack.rip == (uintptr_t)signal_trampoline; } void Thread::handle_signal(int signal) @@ -273,7 +228,9 @@ namespace Kernel ASSERT(!interrupts_enabled()); ASSERT(&Thread::current() == this); ASSERT(is_userspace()); - ASSERT(!m_handling_signal); + + auto& interrupt_stack = *reinterpret_cast(interrupt_stack_base() + interrupt_stack_size() - sizeof(InterruptStack)); + ASSERT(GDT::is_user_segment(interrupt_stack.cs)); if (signal == 0) { @@ -292,9 +249,6 @@ namespace Kernel ASSERT(signal <= _SIGMAX); } - uintptr_t& return_rsp = this->return_rsp(); - uintptr_t& return_rip = this->return_rip(); - vaddr_t signal_handler = process().m_signal_handlers[signal]; m_signal_pending_mask &= ~(1ull << signal); @@ -305,14 +259,11 @@ namespace Kernel else if (signal_handler != (vaddr_t)SIG_DFL) { // call userspace signal handlers - // FIXME: signal trampoline should take a hash etc - // to only allow marking signals done from it - m_handling_signal = signal; - return_rsp += 128; // skip possible red-zone - write_to_stack(return_rsp, return_rip); - write_to_stack(return_rsp, signal); - write_to_stack(return_rsp, signal_handler); - return_rip = (uintptr_t)signal_trampoline; + interrupt_stack.rsp -= 128; // skip possible red-zone + write_to_stack(interrupt_stack.rsp, interrupt_stack.rip); + write_to_stack(interrupt_stack.rsp, signal); + write_to_stack(interrupt_stack.rsp, signal_handler); + interrupt_stack.rip = (uintptr_t)signal_trampoline; } else { @@ -393,8 +344,8 @@ namespace Kernel void Thread::on_exit() { - set_terminating(); - TerminateBlocker(*this); + ASSERT(this == &Thread::current()); + terminate(); ASSERT_NOT_REACHED(); } diff --git a/libc/include/sys/syscall.h b/libc/include/sys/syscall.h index e9e93a793f..9cd90cd8e2 100644 --- a/libc/include/sys/syscall.h +++ b/libc/include/sys/syscall.h @@ -40,7 +40,6 @@ __BEGIN_DECLS #define SYS_DUP2 37 #define SYS_KILL 39 #define SYS_SIGNAL 40 -#define SYS_SIGNAL_DONE 41 #define SYS_TCSETPGRP 42 #define SYS_GET_PID 43 #define SYS_GET_PGID 44