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