Kernel: More signal handling improvements.

Finally fixed the weird flaky crashing when resizing Terminal windows.
It was because we were dispatching a signal to "current" from the scheduler.
Yet another thing I dislike about even having a "current" process while
we're in the scheduler. Not sure yet how to fix this.

Let the signal handler's kernel stack be a kmalloc() allocation for now.
Once we can do allocation of consecutive physical pages in the supervisor
memory region, we can use that for all types of kernel stacks.

Kernel/BXVGADevice.cpp

@@ -61,8 +61,6 @@ void BXVGADevice::set_resolution(int width, int height)
     set_register(VBE_DISPI_INDEX_BPP, 32);
     set_register(VBE_DISPI_INDEX_ENABLE, VBE_DISPI_ENABLED | VBE_DISPI_LFB_ENABLED);
     set_register(VBE_DISPI_INDEX_BANK, 0);
-
-    m_framebuffer_size = { width, height };
 }
 
 void BXVGADevice::set_y_offset(int offset)

Kernel/Process.cpp

@@ -26,7 +26,7 @@
 //#define DEBUG_IO
 //#define TASK_DEBUG
 //#define FORK_DEBUG
-#define SIGNAL_DEBUG
+//#define SIGNAL_DEBUG
 #define MAX_PROCESS_GIDS 32
 //#define SHARED_BUFFER_DEBUG
 
@@ -370,7 +370,8 @@ int Process::do_exec(String path, Vector<String> arguments, Vector<String> envir
         }
     }
 
-    m_signal_stack_kernel_region = nullptr;
+    kfree(m_kernel_stack_for_signal_handler);
+    m_kernel_stack_for_signal_handler = nullptr;
     m_signal_stack_user_region = nullptr;
     m_display_framebuffer_region = nullptr;
     set_default_signal_dispositions();
@@ -725,6 +726,11 @@ Process::~Process()
         kfree(m_kernel_stack);
         m_kernel_stack = nullptr;
     }
+
+    if (m_kernel_stack_for_signal_handler) {
+        kfree(m_kernel_stack_for_signal_handler);
+        m_kernel_stack_for_signal_handler = nullptr;
+    }
 }
 
 void Process::dump_regions()
@@ -912,8 +918,11 @@ ShouldUnblockProcess Process::dispatch_signal(byte signal)
     word ret_cs = m_tss.cs;
     dword ret_eip = m_tss.eip;
     dword ret_eflags = m_tss.eflags;
-
     bool interrupting_in_kernel = (ret_cs & 3) == 0;
+
+    ProcessPagingScope paging_scope(*this);
+    create_signal_trampolines_if_needed();
+
     if (interrupting_in_kernel) {
 #ifdef SIGNAL_DEBUG
         kprintf("dispatch_signal to %s(%u) in state=%s with return to %w:%x\n", name().characters(), pid(), to_string(state()), ret_cs, ret_eip);
@@ -921,23 +930,23 @@ ShouldUnblockProcess Process::dispatch_signal(byte signal)
         ASSERT(is_blocked());
         m_tss_to_resume_kernel = m_tss;
 #ifdef SIGNAL_DEBUG
-        kprintf("resume tss pc: %w:%x\n", m_tss_to_resume_kernel.cs, m_tss_to_resume_kernel.eip);
+        kprintf("resume tss pc: %w:%x stack: %w:%x flags: %x cr3: %x\n", m_tss_to_resume_kernel.cs, m_tss_to_resume_kernel.eip, m_tss_to_resume_kernel.ss, m_tss_to_resume_kernel.esp, m_tss_to_resume_kernel.eflags, m_tss_to_resume_kernel.cr3);
 #endif
-    }
 
-    ProcessPagingScope paging_scope(*this);
-
-    if (interrupting_in_kernel) {
         if (!m_signal_stack_user_region) {
-            m_signal_stack_user_region = allocate_region(LinearAddress(), default_userspace_stack_size, "signal stack (user)");
+            m_signal_stack_user_region = allocate_region(LinearAddress(), default_userspace_stack_size, "Signal stack (user)");
             ASSERT(m_signal_stack_user_region);
-            m_signal_stack_kernel_region = allocate_region(LinearAddress(), default_userspace_stack_size, "signal stack (kernel)");
-            ASSERT(m_signal_stack_kernel_region);
+        }
+        if (!m_kernel_stack_for_signal_handler) {
+            m_kernel_stack_for_signal_handler = kmalloc(default_kernel_stack_size);
+            ASSERT(m_kernel_stack_for_signal_handler);
         }
         m_tss.ss = 0x23;
         m_tss.esp = m_signal_stack_user_region->laddr().offset(default_userspace_stack_size).get();
         m_tss.ss0 = 0x10;
-        m_tss.esp0 = m_signal_stack_kernel_region->laddr().offset(default_userspace_stack_size).get();
+        m_tss.esp0 = (dword)m_kernel_stack_for_signal_handler + default_kernel_stack_size;
+
+        push_value_on_stack(0);
     } else {
         push_value_on_stack(ret_eip);
         push_value_on_stack(ret_eflags);
@@ -952,6 +961,9 @@ ShouldUnblockProcess Process::dispatch_signal(byte signal)
         push_value_on_stack(m_tss.ebp);
         push_value_on_stack(m_tss.esi);
         push_value_on_stack(m_tss.edi);
+
+        // Align the stack.
+        m_tss.esp -= 12;
     }
 
     // PUSH old_signal_mask
@@ -964,42 +976,6 @@ ShouldUnblockProcess Process::dispatch_signal(byte signal)
     m_tss.gs = 0x23;
     m_tss.eip = handler_laddr.get();
 
-    if (m_return_to_ring3_from_signal_trampoline.is_null()) {
-        // FIXME: This should be a global trampoline shared by all processes, not one created per process!
-        // FIXME: Remap as read-only after setup.
-        auto* region = allocate_region(LinearAddress(), PAGE_SIZE, "signal_trampoline", true, true);
-        m_return_to_ring3_from_signal_trampoline = region->laddr();
-        byte* code_ptr = m_return_to_ring3_from_signal_trampoline.as_ptr();
-        *code_ptr++ = 0x58; // pop eax (Skip over signal argument)
-        *code_ptr++ = 0x5a; // pop edx
-        *code_ptr++ = 0xb8; // mov eax, <dword>
-        *(dword*)code_ptr = Syscall::SC_restore_signal_mask;
-        code_ptr += sizeof(dword);
-        *code_ptr++ = 0xcd; // int 0x82
-        *code_ptr++ = 0x82;
-        *code_ptr++ = 0x61; // popa
-        *code_ptr++ = 0x9d; // popf
-        *code_ptr++ = 0xc3; // ret
-        *code_ptr++ = 0x0f; // ud2
-        *code_ptr++ = 0x0b;
-
-        m_return_to_ring0_from_signal_trampoline = LinearAddress((dword)code_ptr);
-        *code_ptr++ = 0x58; // pop eax (Skip over signal argument)
-        *code_ptr++ = 0x5a; // pop edx
-        *code_ptr++ = 0xb8; // mov eax, <dword>
-        *(dword*)code_ptr = Syscall::SC_restore_signal_mask;
-        code_ptr += sizeof(dword);
-        *code_ptr++ = 0xcd; // int 0x82
-        *code_ptr++ = 0x82;
-        *code_ptr++ = 0xb8; // mov eax, <dword>
-        *(dword*)code_ptr = Syscall::SC_sigreturn;
-        code_ptr += sizeof(dword);
-        *code_ptr++ = 0xcd; // int 0x82
-        *code_ptr++ = 0x82;
-        *code_ptr++ = 0x0f; // ud2
-        *code_ptr++ = 0x0b;
-    }
-
     // FIXME: Should we worry about the stack being 16 byte aligned when entering a signal handler?
     push_value_on_stack(signal);
 
@@ -1008,6 +984,8 @@ ShouldUnblockProcess Process::dispatch_signal(byte signal)
     else
         push_value_on_stack(m_return_to_ring3_from_signal_trampoline.get());
 
+    ASSERT((m_tss.esp % 16) == 0);
+
     // FIXME: This state is such a hack. It avoids trouble if 'current' is the process receiving a signal.
     set_state(Skip1SchedulerPass);
 
@@ -1017,6 +995,54 @@ ShouldUnblockProcess Process::dispatch_signal(byte signal)
     return ShouldUnblockProcess::Yes;
 }
 
+void Process::create_signal_trampolines_if_needed()
+{
+    if (!m_return_to_ring3_from_signal_trampoline.is_null())
+        return;
+    // FIXME: This should be a global trampoline shared by all processes, not one created per process!
+    // FIXME: Remap as read-only after setup.
+    auto* region = allocate_region(LinearAddress(), PAGE_SIZE, "Signal trampolines", true, true);
+    m_return_to_ring3_from_signal_trampoline = region->laddr();
+    byte* code_ptr = m_return_to_ring3_from_signal_trampoline.as_ptr();
+    *code_ptr++ = 0x58; // pop eax (Argument to signal handler (ignored here))
+    *code_ptr++ = 0x5a; // pop edx (Original signal mask to restore)
+    *code_ptr++ = 0xb8; // mov eax, <dword>
+    *(dword*)code_ptr = Syscall::SC_restore_signal_mask;
+    code_ptr += sizeof(dword);
+    *code_ptr++ = 0xcd; // int 0x82
+    *code_ptr++ = 0x82;
+
+    *code_ptr++ = 0x83; // add esp, (stack alignment padding)
+    *code_ptr++ = 0xc4;
+    *code_ptr++ = sizeof(dword) * 3;
+
+    *code_ptr++ = 0x61; // popa
+    *code_ptr++ = 0x9d; // popf
+    *code_ptr++ = 0xc3; // ret
+    *code_ptr++ = 0x0f; // ud2
+    *code_ptr++ = 0x0b;
+
+    m_return_to_ring0_from_signal_trampoline = LinearAddress((dword)code_ptr);
+    *code_ptr++ = 0x58; // pop eax (Argument to signal handler (ignored here))
+    *code_ptr++ = 0x5a; // pop edx (Original signal mask to restore)
+    *code_ptr++ = 0xb8; // mov eax, <dword>
+    *(dword*)code_ptr = Syscall::SC_restore_signal_mask;
+    code_ptr += sizeof(dword);
+    *code_ptr++ = 0xcd; // int 0x82
+
+    // NOTE: Stack alignment padding doesn't matter when returning to ring0.
+    //       Nothing matters really, as we're returning by replacing the entire TSS.
+
+    *code_ptr++ = 0x82;
+    *code_ptr++ = 0xb8; // mov eax, <dword>
+    *(dword*)code_ptr = Syscall::SC_sigreturn;
+    code_ptr += sizeof(dword);
+    *code_ptr++ = 0xcd; // int 0x82
+    *code_ptr++ = 0x82;
+    *code_ptr++ = 0x0f; // ud2
+    *code_ptr++ = 0x0b;
+}
+
 int Process::sys$restore_signal_mask(dword mask)
 {
     m_signal_mask = mask;
@@ -1030,7 +1056,7 @@ void Process::sys$sigreturn()
     m_tss = m_tss_to_resume_kernel;
 #ifdef SIGNAL_DEBUG
     kprintf("sys$sigreturn in %s(%u)\n", name().characters(), pid());
-    kprintf(" -> resuming execution at %w:%x\n", m_tss.cs, m_tss.eip);
+    kprintf(" -> resuming execution at %w:%x stack %w:%x flags %x cr3 %x\n", m_tss.cs, m_tss.eip, m_tss.ss, m_tss.esp, m_tss.eflags, m_tss.cr3);
 #endif
     set_state(Skip1SchedulerPass);
     Scheduler::yield();

Kernel/Process.h

@@ -146,6 +146,9 @@ public:
     void set_ticks_left(dword t) { m_ticks_left = t; }
     dword ticks_left() const { return m_ticks_left; }
 
+    dword kernel_stack_base() const { return (dword)m_kernel_stack; };
+    dword kernel_stack_for_signal_handler_base() const { return (dword)m_kernel_stack_for_signal_handler; };
+
     void set_selector(word s) { m_far_ptr.selector = s; }
     void set_state(State s) { m_state = s; }
     void die();
@@ -319,6 +322,8 @@ private:
     void set_default_signal_dispositions();
     void disown_all_shared_buffers();
 
+    void create_signal_trampolines_if_needed();
+
     RetainPtr<PageDirectory> m_page_directory;
 
     Process* m_prev { nullptr };
@@ -355,6 +360,7 @@ private:
     RingLevel m_ring { Ring0 };
     int m_error { 0 };
     void* m_kernel_stack { nullptr };
+    void* m_kernel_stack_for_signal_handler { nullptr };
     dword m_times_scheduled { 0 };
     pid_t m_waitee_pid { -1 };
     int m_blocked_fd { -1 };
@@ -397,7 +403,6 @@ private:
     HashTable<gid_t> m_gids;
 
     Region* m_signal_stack_user_region { nullptr };
-    Region* m_signal_stack_kernel_region { nullptr };
 
     RetainPtr<Region> m_display_framebuffer_region;
 

Kernel/Scheduler.cpp

@@ -162,6 +162,11 @@ bool Scheduler::pick_next()
     Process::for_each_living([] (auto& process) {
         if (!process.has_unmasked_pending_signals())
             return true;
+        // FIXME: It would be nice if the Scheduler didn't have to worry about who is "current"
+        //        For now, avoid dispatching signals to "current" and do it in a scheduling pass
+        //        while some other process is interrupted. Otherwise a mess will be made.
+        if (&process == current)
+            return true;
         // We know how to interrupt blocked processes, but if they are just executing
         // at some random point in the kernel, let them continue. They'll be in userspace
         // sooner or later and we can deliver the signal then.
@@ -199,7 +204,7 @@ bool Scheduler::pick_next()
 
         if (process->state() == Process::Runnable || process->state() == Process::Running) {
 #ifdef SCHEDULER_DEBUG
-            dbgprintf("switch to %s(%u) @ %w:%x\n", process->name().characters(), process->pid(), process->tss().cs, process->tss().eip);
+            kprintf("switch to %s(%u) @ %w:%x\n", process->name().characters(), process->pid(), process->tss().cs, process->tss().eip);
 #endif
             return context_switch(*process);
         }

Kernel/i386.cpp

@@ -137,8 +137,10 @@ static void dump(const DumpType& regs)
     if constexpr (IsSame<DumpType, RegisterDumpWithExceptionCode>::value) {
         kprintf("exception code: %w\n", regs.exception_code);
     }
-    kprintf("pc=%w:%x ds=%w es=%w fs=%w gs=%w\n", regs.cs, regs.eip, regs.ds, regs.es, regs.fs, regs.gs);
-    kprintf("stk=%w:%x\n", ss, esp);
+    kprintf("  pc=%w:%x ds=%w es=%w fs=%w gs=%w\n", regs.cs, regs.eip, regs.ds, regs.es, regs.fs, regs.gs);
+    kprintf(" stk=%w:%x\n", ss, esp);
+    if (current)
+        kprintf("kstk=%w:%x, base=%x, sigbase=%x\n", current->tss().ss0, current->tss().esp0, current->kernel_stack_base(), current->kernel_stack_for_signal_handler_base());
     kprintf("eax=%x ebx=%x ecx=%x edx=%x\n", regs.eax, regs.ebx, regs.ecx, regs.edx);
     kprintf("ebp=%x esp=%x esi=%x edi=%x\n", regs.ebp, esp, regs.esi, regs.edi);
 

SharedGraphics/Painter.cpp

@@ -75,6 +75,8 @@ void Painter::fill_rect(const Rect& a_rect, Color color)
     if (rect.is_empty())
         return;
 
+    ASSERT(m_target->rect().contains(rect));
+
     RGBA32* dst = m_target->scanline(rect.top()) + rect.left();
     const unsigned dst_skip = m_target->width();