Kernel: Fix intermittent assertion failure in sys$exec()

While setting up the main thread stack for a new process, we'd incur
some zero-fill page faults. This was to be expected, since we allocate
a huge stack but lazily populate it with physical pages.

The problem is that page fault handlers may enable interrupts in order
to grab a VMObject lock (or to page in from an inode.)

During exec(), a process is reorganizing itself and will be in a very
unrunnable state if the scheduler should interrupt it and then later
ask it to run again. Which is exactly what happens if the process gets
pre-empted while the new stack's zero-fill page fault grabs the lock.

This patch fixes the issue by creating new main thread stacks before
disabling interrupts and going into the critical part of exec().

Kernel/Process.cpp

@@ -616,12 +616,18 @@ int Process::do_exec(String path, Vector<String> arguments, Vector<String> envir
         }
     }
 
+    // NOTE: We create the new stack before disabling interrupts since it will zero-fault
+    //       and we don't want to deal with faults after this point.
+    u32 new_userspace_esp = main_thread().make_userspace_stack_for_main_thread(move(arguments), move(environment));
+
     // We cli() manually here because we don't want to get interrupted between do_exec() and Schedule::yield().
     // The reason is that the task redirection we've set up above will be clobbered by the timer IRQ.
     // If we used an InterruptDisabler that sti()'d on exit, we might timer tick'd too soon in exec().
     if (&current->process() == this)
         cli();
 
+    // NOTE: Be careful to not trigger any page faults below!
+
     Scheduler::prepare_to_modify_tss(main_thread());
 
     m_name = parts.take_last();
@@ -645,7 +651,7 @@ int Process::do_exec(String path, Vector<String> arguments, Vector<String> envir
     main_thread().m_tss.gs = thread_specific_selector() | 3;
     main_thread().m_tss.ss = 0x23;
     main_thread().m_tss.cr3 = page_directory().cr3();
-    main_thread().make_userspace_stack_for_main_thread(move(arguments), move(environment));
+    main_thread().m_tss.esp = new_userspace_esp;
     main_thread().m_tss.ss0 = 0x10;
     main_thread().m_tss.esp0 = old_esp0;
     main_thread().m_tss.ss2 = m_pid;

Kernel/Thread.cpp

@@ -579,13 +579,16 @@ RegisterDump& Thread::get_register_dump_from_stack()
     return *(RegisterDump*)(kernel_stack_top() - sizeof(RegisterDump));
 }
 
-void Thread::make_userspace_stack_for_main_thread(Vector<String> arguments, Vector<String> environment)
+u32 Thread::make_userspace_stack_for_main_thread(Vector<String> arguments, Vector<String> environment)
 {
     auto* region = m_process.allocate_region(VirtualAddress(), default_userspace_stack_size, "Stack (Main thread)", PROT_READ | PROT_WRITE, false);
     ASSERT(region);
     region->set_stack(true);
-    m_tss.esp = region->vaddr().offset(default_userspace_stack_size).get();
 
+    u32 new_esp = region->vaddr().offset(default_userspace_stack_size).get();
+
+    // FIXME: This is weird, we put the argument contents at the base of the stack,
+    //        and the argument pointers at the top? Why?
     char* stack_base = (char*)region->vaddr().get();
     int argc = arguments.size();
     char** argv = (char**)stack_base;
@@ -608,11 +611,19 @@ void Thread::make_userspace_stack_for_main_thread(Vector<String> arguments, Vect
     }
     env[environment.size()] = nullptr;
 
+    auto push_on_new_stack = [&new_esp](u32 value)
+    {
+        new_esp -= 4;
+        u32* stack_ptr = (u32*)new_esp;
+        *stack_ptr = value;
+    };
+
     // NOTE: The stack needs to be 16-byte aligned.
-    push_value_on_stack((u32)env);
-    push_value_on_stack((u32)argv);
-    push_value_on_stack((u32)argc);
-    push_value_on_stack(0);
+    push_on_new_stack((u32)env);
+    push_on_new_stack((u32)argv);
+    push_on_new_stack((u32)argc);
+    push_on_new_stack(0);
+    return new_esp;
 }
 
 Thread* Thread::clone(Process& process)

Kernel/Thread.h

@@ -351,7 +351,8 @@ public:
 
     void set_default_signal_dispositions();
     void push_value_on_stack(u32);
-    void make_userspace_stack_for_main_thread(Vector<String> arguments, Vector<String> environment);
+
+    u32 make_userspace_stack_for_main_thread(Vector<String> arguments, Vector<String> environment);
 
     void make_thread_specific_region(Badge<Process>);