diff --git a/kernel/arch/i686/interrupts.S b/kernel/arch/i686/interrupts.S index c355dbe7..38ec5edd 100644 --- a/kernel/arch/i686/interrupts.S +++ b/kernel/arch/i686/interrupts.S @@ -1,20 +1,20 @@ -.macro maybe_load_kernel_segments, n - testb $3, \n(%esp) - jz 1f; jnp 1f - +.macro intr_header, n + pushal + testb $3, \n+8*4(%esp) + jz 1f movw $0x10, %ax movw %ax, %ds movw %ax, %es movw %ax, %fs movw $0x28, %ax movw %ax, %gs -1: +1: cld .endm -.macro maybe_load_userspace_segments, n - testb $3, \n(%esp) - jz 1f; jnp 1f - +.macro intr_footer, n + testb $3, \n+8*4(%esp) + jz 1f + call cpp_check_signal movw $(0x20 | 3), %bx movw %bx, %ds movw %bx, %es @@ -22,14 +22,11 @@ movw %bx, %fs movw $(0x38 | 3), %bx movw %bx, %gs -1: +1: popal .endm isr_stub: - pushal - maybe_load_kernel_segments 44 - cld - + intr_header 12 movl %cr0, %eax; pushl %eax movl %cr2, %eax; pushl %eax movl %cr3, %eax; pushl %eax @@ -57,15 +54,12 @@ isr_stub: movl %ebp, %esp addl $24, %esp - maybe_load_userspace_segments 44 - popal + intr_footer 12 addl $8, %esp iret irq_stub: - pushal - maybe_load_kernel_segments 44 - cld + intr_header 12 movl 32(%esp), %edi # interrupt number @@ -78,16 +72,13 @@ irq_stub: movl %ebp, %esp - maybe_load_userspace_segments 44 - popal + intr_footer 12 addl $8, %esp iret .global asm_ipi_handler asm_ipi_handler: - pushal - maybe_load_kernel_segments 36 - cld + intr_header 4 movl %esp, %ebp andl $-16, %esp @@ -96,15 +87,12 @@ asm_ipi_handler: movl %ebp, %esp - maybe_load_userspace_segments 36 - popal + intr_footer 4 iret .global asm_timer_handler asm_timer_handler: - pushal - maybe_load_kernel_segments 36 - cld + intr_header 4 movl %esp, %ebp andl $-16, %esp @@ -113,8 +101,7 @@ asm_timer_handler: movl %ebp, %esp - maybe_load_userspace_segments 36 - popal + intr_footer 4 iret .macro isr n diff --git a/kernel/arch/x86_64/interrupts.S b/kernel/arch/x86_64/interrupts.S index 401951fe..913053dd 100644 --- a/kernel/arch/x86_64/interrupts.S +++ b/kernel/arch/x86_64/interrupts.S @@ -1,12 +1,4 @@ -.macro swapgs_if_necessary, n - testb $3, \n(%rsp) - jz 1f; jnp 1f - swapgs -1: -.endm - -.macro pushaq, n - swapgs_if_necessary \n +.macro intr_header, n pushq %rax pushq %rcx pushq %rdx @@ -22,10 +14,18 @@ pushq %r13 pushq %r14 pushq %r15 + testb $3, \n+15*8(%rsp) + jz 1f + swapgs +1: cld .endm -.macro popaq, n - popq %r15 +.macro intr_footer, n + testb $3, \n+15*8(%rsp) + jz 1f + call cpp_check_signal + swapgs +1: popq %r15 popq %r14 popq %r13 popq %r12 @@ -40,12 +40,10 @@ popq %rdx popq %rcx popq %rax - swapgs_if_necessary \n .endm isr_stub: - pushaq 24 - cld + intr_header 24 movq %cr0, %rax; pushq %rax movq %cr2, %rax; pushq %rax movq %cr3, %rax; pushq %rax @@ -58,33 +56,33 @@ isr_stub: call cpp_isr_handler addq $32, %rsp - popaq 24 + intr_footer 24 addq $16, %rsp iretq irq_stub: - pushaq 24 - cld + intr_header 24 + xorq %rbp, %rbp movq 120(%rsp), %rdi # irq number call cpp_irq_handler - popaq 24 + intr_footer 24 addq $16, %rsp iretq .global asm_ipi_handler asm_ipi_handler: - pushaq 8 - cld + intr_header 8 + xorq %rbp, %rbp call cpp_ipi_handler - popaq 8 + intr_footer 8 iretq .global asm_timer_handler asm_timer_handler: - pushaq 8 - cld + intr_header 8 + xorq %rbp, %rbp call cpp_timer_handler - popaq 8 + intr_footer 8 iretq .macro isr n diff --git a/kernel/include/kernel/Thread.h b/kernel/include/kernel/Thread.h index 8dfde905..2fd2a90d 100644 --- a/kernel/include/kernel/Thread.h +++ b/kernel/include/kernel/Thread.h @@ -56,12 +56,10 @@ namespace Kernel // Returns true, if thread is going to trigger signal bool is_interrupted_by_signal(bool skip_stop_and_cont = false) const; - // Returns true if pending signal can be added to thread - bool can_add_signal_to_execute() const; - bool will_execute_signal() const; // Returns true if handled signal had SA_RESTART - bool handle_signal(int signal = 0, const siginfo_t& signal_info = {}); - void add_signal(int signal, const siginfo_t& info); + bool handle_signal_if_interrupted(); + bool handle_signal(int signal, const siginfo_t&); + void add_signal(int signal, const siginfo_t&); void set_suspend_signal_mask(uint64_t sigmask); static bool is_stopping_signal(int signal); @@ -153,6 +151,15 @@ namespace Kernel bool currently_on_alternate_stack() const; + struct signal_handle_info_t + { + vaddr_t signal_handler; + vaddr_t signal_stack_top; + bool has_sa_restart; + }; + signal_handle_info_t remove_signal_and_get_info(int signal); + void handle_signal_impl(int signal, const siginfo_t&, vaddr_t signal_handler, vaddr_t signal_stack_top); + private: // NOTE: this is the first member to force it being last destructed // {kernel,userspace}_stack has to be destroyed before page table diff --git a/kernel/kernel/IDT.cpp b/kernel/kernel/IDT.cpp index 60fcd220..086ce92f 100644 --- a/kernel/kernel/IDT.cpp +++ b/kernel/kernel/IDT.cpp @@ -221,7 +221,13 @@ namespace Kernel if (result.is_error()) { dwarnln("Demand paging: {}", result.error()); + + // TODO: this is too strict, we should maybe do SIGBUS and + // SIGKILL only on recursive exceptions + Processor::set_interrupt_state(InterruptState::Enabled); Thread::current().handle_signal(SIGKILL, {}); + Processor::set_interrupt_state(InterruptState::Disabled); + return; } @@ -321,7 +327,7 @@ namespace Kernel Debug::s_debug_lock.unlock(InterruptState::Disabled); - if (tid && Thread::current().is_userspace()) + if (tid && GDT::is_user_segment(interrupt_stack->cs)) { // TODO: Confirm and fix the exception to signal mappings @@ -366,7 +372,9 @@ namespace Kernel break; } + Processor::set_interrupt_state(InterruptState::Enabled); Thread::current().handle_signal(signal_info.si_signo, signal_info); + Processor::set_interrupt_state(InterruptState::Disabled); } else { @@ -401,10 +409,6 @@ namespace Kernel Process::update_alarm_queue(); Processor::scheduler().timer_interrupt(); - - auto& current_thread = Thread::current(); - if (current_thread.can_add_signal_to_execute()) - current_thread.handle_signal(); } extern "C" void cpp_irq_handler(uint32_t irq) @@ -428,15 +432,18 @@ namespace Kernel else dprintln("no handler for irq 0x{2H}", irq); - auto& current_thread = Thread::current(); - if (current_thread.can_add_signal_to_execute()) - current_thread.handle_signal(); - Processor::scheduler().reschedule_if_idle(); ASSERT(Thread::current().state() != Thread::State::Terminated); } + extern "C" void cpp_check_signal() + { + Processor::set_interrupt_state(InterruptState::Enabled); + Thread::current().handle_signal_if_interrupted(); + Processor::set_interrupt_state(InterruptState::Disabled); + } + void IDT::register_interrupt_handler(uint8_t index, void (*handler)(), uint8_t ist) { auto& desc = m_idt[index]; @@ -476,7 +483,6 @@ namespace Kernel IRQ_LIST_X #undef X - extern "C" void asm_yield_handler(); extern "C" void asm_ipi_handler(); extern "C" void asm_timer_handler(); #if ARCH(i686) diff --git a/kernel/kernel/Process.cpp b/kernel/kernel/Process.cpp index 47351bb0..255991f3 100644 --- a/kernel/kernel/Process.cpp +++ b/kernel/kernel/Process.cpp @@ -303,6 +303,8 @@ namespace Kernel void Process::exit(int status, int signal) { + ASSERT(Processor::get_interrupt_state() == InterruptState::Enabled); + bool expected = false; if (!m_is_exiting.compare_exchange(expected, true)) { @@ -310,9 +312,6 @@ namespace Kernel ASSERT_NOT_REACHED(); } - const auto state = Processor::get_interrupt_state(); - Processor::set_interrupt_state(InterruptState::Enabled); - if (m_parent) { Process* parent_process = nullptr; @@ -369,8 +368,6 @@ namespace Kernel while (m_threads.size() > 1) Processor::yield(); - Processor::set_interrupt_state(state); - Thread::current().on_exit(); ASSERT_NOT_REACHED(); @@ -2951,7 +2948,7 @@ namespace Kernel for (;;) { while (Thread::current().will_exit_because_of_signal()) - Thread::current().handle_signal(); + Thread::current().handle_signal_if_interrupted(); SpinLockGuard guard(m_signal_lock); if (!m_stopped) diff --git a/kernel/kernel/Syscall.cpp b/kernel/kernel/Syscall.cpp index 0649ecfb..469d734e 100644 --- a/kernel/kernel/Syscall.cpp +++ b/kernel/kernel/Syscall.cpp @@ -94,13 +94,11 @@ namespace Kernel Process::current().wait_while_stopped(); - Processor::set_interrupt_state(InterruptState::Disabled); + if (Thread::current().handle_signal_if_interrupted()) + if (ret.is_error() && ret.error().get_error_code() == EINTR && is_restartable_syscall(syscall)) + ret = BAN::Error::from_errno(ERESTART); - auto& current_thread = Thread::current(); - if (current_thread.can_add_signal_to_execute()) - if (current_thread.handle_signal()) - if (ret.is_error() && ret.error().get_error_code() == EINTR && is_restartable_syscall(syscall)) - ret = BAN::Error::from_errno(ERESTART); + Processor::set_interrupt_state(InterruptState::Disabled); ASSERT(Kernel::Thread::current().state() == Kernel::Thread::State::Executing); diff --git a/kernel/kernel/Thread.cpp b/kernel/kernel/Thread.cpp index 51dd51a6..15cc5217 100644 --- a/kernel/kernel/Thread.cpp +++ b/kernel/kernel/Thread.cpp @@ -572,19 +572,6 @@ namespace Kernel return false; } - bool Thread::can_add_signal_to_execute() const - { - return is_interrupted_by_signal() && m_mutex_count == 0; - } - - bool Thread::will_execute_signal() const - { - if (!is_userspace() || m_state != State::Executing) - return false; - auto& interrupt_stack = *reinterpret_cast(kernel_stack_top() - sizeof(InterruptStack)); - return interrupt_stack.ip == (uintptr_t)signal_trampoline; - } - bool Thread::will_exit_because_of_signal() const { const uint64_t full_pending_mask = m_signal_pending_mask | process().signal_pending_mask(); @@ -596,21 +583,17 @@ namespace Kernel return false; } - bool Thread::handle_signal(int signal, const siginfo_t& _signal_info) + bool Thread::handle_signal_if_interrupted() { - ASSERT(&Thread::current() == this); - ASSERT(is_userspace()); + int signal; + siginfo_t signal_info; + signal_handle_info_t handle_info; - auto state = m_signal_lock.lock(); - - auto& interrupt_stack = *reinterpret_cast(kernel_stack_top() - sizeof(InterruptStack)); - ASSERT(GDT::is_user_segment(interrupt_stack.cs)); - - auto signal_info = _signal_info; - - if (signal == 0) { - const uint64_t process_signal_pending_mask = process().signal_pending_mask(); + SpinLockGuard _1(m_signal_lock); + SpinLockGuard _2(m_process->m_signal_lock); + + const uint64_t process_signal_pending_mask = m_process->m_signal_pending_mask; const uint64_t full_pending_mask = m_signal_pending_mask | process_signal_pending_mask; for (signal = _SIGMIN; signal <= _SIGMAX; signal++) { @@ -618,42 +601,77 @@ namespace Kernel if ((full_pending_mask & mask) && !(m_signal_block_mask & mask)) break; } - ASSERT(signal <= _SIGMAX); + if (signal > _SIGMAX) + return false; + if (process_signal_pending_mask & (1ull << signal)) - signal_info = process().m_signal_infos[signal]; + signal_info = m_process->m_signal_infos[signal]; else signal_info = m_signal_infos[signal]; - } - else - { - ASSERT(signal >= _SIGMIN); - ASSERT(signal <= _SIGMAX); + + handle_info = remove_signal_and_get_info(signal); } - vaddr_t signal_handler; - bool has_sa_restart; + handle_signal_impl( + signal, + signal_info, + handle_info.signal_handler, + handle_info.signal_stack_top + ); + + return handle_info.has_sa_restart; + } + + bool Thread::handle_signal(int signal, const siginfo_t& signal_info) + { + ASSERT(&Thread::current() == this); + ASSERT(is_userspace()); + + ASSERT(signal >= _SIGMIN); + ASSERT(signal <= _SIGMAX); + + signal_handle_info_t handle_info; + + { + SpinLockGuard _1(m_signal_lock); + SpinLockGuard _2(m_process->m_signal_lock); + handle_info = remove_signal_and_get_info(signal); + } + + handle_signal_impl( + signal, + signal_info, + handle_info.signal_handler, + handle_info.signal_stack_top + ); + + return handle_info.has_sa_restart; + } + + Thread::signal_handle_info_t Thread::remove_signal_and_get_info(int signal) + { + ASSERT(m_signal_lock.current_processor_has_lock()); + ASSERT(m_process->m_signal_lock.current_processor_has_lock()); + + ASSERT(signal >= _SIGMIN); + ASSERT(signal <= _SIGMAX); + + auto& handler = m_process->m_signal_handlers[signal]; + + const vaddr_t signal_handler = (handler.sa_flags & SA_SIGINFO) + ? reinterpret_cast(handler.sa_sigaction) + : reinterpret_cast(handler.sa_handler); + const bool has_sa_restart = !!(handler.sa_flags & SA_RESTART); + vaddr_t signal_stack_top = 0; + if (m_signal_alt_stack.ss_flags != SS_DISABLE && (handler.sa_flags & SA_ONSTACK) && !currently_on_alternate_stack()) + signal_stack_top = reinterpret_cast(m_signal_alt_stack.ss_sp) + m_signal_alt_stack.ss_size; - { - SpinLockGuard _(m_process->m_signal_lock); - - auto& handler = m_process->m_signal_handlers[signal]; - - signal_handler = (handler.sa_flags & SA_SIGINFO) - ? reinterpret_cast(handler.sa_sigaction) - : reinterpret_cast(handler.sa_handler); - has_sa_restart = !!(handler.sa_flags & SA_RESTART); - - const auto& alt_stack = m_signal_alt_stack; - if (alt_stack.ss_flags != SS_DISABLE && (handler.sa_flags & SA_ONSTACK) && !currently_on_alternate_stack()) - signal_stack_top = reinterpret_cast(alt_stack.ss_sp) + alt_stack.ss_size; - - if (handler.sa_flags & SA_RESETHAND) - handler = { .sa_handler = SIG_DFL, .sa_mask = 0, .sa_flags = 0 }; - } + if (handler.sa_flags & SA_RESETHAND) + handler = { .sa_handler = SIG_DFL, .sa_mask = 0, .sa_flags = 0 }; m_signal_pending_mask &= ~(1ull << signal); - process().remove_pending_signal(signal); + m_process->m_signal_pending_mask &= ~(1ull << signal); if (m_signal_suspend_mask.has_value()) { @@ -661,7 +679,21 @@ namespace Kernel m_signal_suspend_mask.clear(); } - m_signal_lock.unlock(state); + return { + .signal_handler = signal_handler, + .signal_stack_top = signal_stack_top, + .has_sa_restart = has_sa_restart, + }; + } + + void Thread::handle_signal_impl(int signal, const siginfo_t& signal_info, vaddr_t signal_handler, vaddr_t signal_stack_top) + { + ASSERT(this == &Thread::current()); + ASSERT(is_userspace()); + ASSERT(Processor::get_interrupt_state() == InterruptState::Enabled); + + auto& interrupt_stack = *reinterpret_cast(kernel_stack_top() - sizeof(InterruptStack)); + ASSERT(GDT::is_user_segment(interrupt_stack.cs)); if (signal_handler == (vaddr_t)SIG_IGN) ; @@ -696,10 +728,8 @@ namespace Kernel { if (m_process->page_table().get_page_flags(pages[i]) & PageTable::Flags::Present) continue; - Processor::set_interrupt_state(InterruptState::Enabled); if (auto ret = m_process->allocate_page_for_demand_paging(pages[i], true, false); ret.is_error() || !ret.value()) m_process->exit(128 + SIGSEGV, SIGSEGV); - Processor::set_interrupt_state(InterruptState::Disabled); } } @@ -711,11 +741,13 @@ namespace Kernel write_to_stack(interrupt_stack.sp, interrupt_stack.flags); { - signal_info.si_signo = signal; - signal_info.si_addr = reinterpret_cast(interrupt_stack.ip); - interrupt_stack.sp -= sizeof(siginfo_t); - *reinterpret_cast(interrupt_stack.sp) = signal_info; + + auto& stack_signal_info = *reinterpret_cast(interrupt_stack.sp); + stack_signal_info = signal_info; + stack_signal_info.si_signo = signal; + stack_signal_info.si_addr = reinterpret_cast(interrupt_stack.ip); + static_assert(sizeof(siginfo_t) % sizeof(uintptr_t) == 0); } @@ -752,8 +784,6 @@ namespace Kernel panic("Executing unhandled signal {}", signal); } } - - return has_sa_restart; } void Thread::add_signal(int signal, const siginfo_t& info)