From 717cd5015e91495fc28cbbe83576530905047faf Mon Sep 17 00:00:00 2001 From: Cristian-Bogdan SIRB Date: Tue, 18 Feb 2020 14:28:28 +0200 Subject: [PATCH] Kernel: Allow process with multiple threads to call exec and exit This allows a process wich has more than 1 thread to call exec, even from a thread. This kills all the other threads, but it won't wait for them to finish, just makes sure that they are not in a running/runable state. In the case where a thread does exec, the new program PID will be the thread TID, to keep the PID == TID in the new process. This introduces a new function inside the Process class, kill_threads_except_self which is called on exit() too (exit with multiple threads wasn't properly working either). Inside the Lock class, there is the need for a new function, clear_waiters, which removes all the waiters from the Process::big_lock. This is needed since after a exit/exec, there should be no other threads waiting for this lock, the threads should be simply killed. Only queued threads should wait for this lock at this point, since blocked threads are handled in set_should_die. --- Kernel/Lock.cpp | 6 ++++ Kernel/Lock.h | 1 + Kernel/Process.cpp | 66 ++++++++++++++++++++++++++++++-------------- Kernel/Process.h | 7 +++++ Kernel/Scheduler.cpp | 3 ++ Kernel/Thread.cpp | 10 +++++-- Kernel/WaitQueue.cpp | 6 ++++ Kernel/WaitQueue.h | 1 + 8 files changed, 78 insertions(+), 22 deletions(-) diff --git a/Kernel/Lock.cpp b/Kernel/Lock.cpp index 3d9e0fd3371..4d3625f2ee2 100644 --- a/Kernel/Lock.cpp +++ b/Kernel/Lock.cpp @@ -86,4 +86,10 @@ bool Lock::force_unlock_if_locked() return true; } +void Lock::clear_waiters() +{ + InterruptDisabler disabler; + m_queue.clear(); +} + } diff --git a/Kernel/Lock.h b/Kernel/Lock.h index 9e1cd9bc41e..28ab1832f54 100644 --- a/Kernel/Lock.h +++ b/Kernel/Lock.h @@ -47,6 +47,7 @@ public: void unlock(); bool force_unlock_if_locked(); bool is_locked() const { return m_holder; } + void clear_waiters(); const char* name() const { return m_name; } diff --git a/Kernel/Process.cpp b/Kernel/Process.cpp index 0f917caa841..7d162fe5242 100644 --- a/Kernel/Process.cpp +++ b/Kernel/Process.cpp @@ -760,21 +760,45 @@ pid_t Process::sys$fork(RegisterState& regs) return child->pid(); } +void Process::kill_threads_except_self() +{ + InterruptDisabler disabler; + + if (m_thread_count <= 1) + return; + + for_each_thread([&](Thread& thread) { + if (&thread == Thread::current + || thread.state() == Thread::State::Dead + || thread.state() == Thread::State::Dying) + return IterationDecision::Continue; + + // At this point, we have no joiner anymore + thread.m_joiner = nullptr; + thread.set_should_die(); + + if (thread.state() != Thread::State::Dead) + thread.set_state(Thread::State::Dying); + + return IterationDecision::Continue; + }); + + big_lock().clear_waiters(); +} + +void Process::kill_all_threads() +{ + for_each_thread([&](Thread& thread) { + thread.set_should_die(); + return IterationDecision::Continue; + }); +} + int Process::do_exec(NonnullRefPtr main_program_description, Vector arguments, Vector environment, RefPtr interpreter_description) { ASSERT(is_ring3()); auto path = main_program_description->absolute_path(); dbg() << "do_exec(" << path << ")"; - // FIXME(Thread): Kill any threads the moment we commit to the exec(). - if (thread_count() != 1) { - dbgprintf("Gonna die because I have many threads! These are the threads:\n"); - for_each_thread([](Thread& thread) { - dbgprintf("Thread{%p}: TID=%d, PID=%d\n", &thread, thread.tid(), thread.pid()); - return IterationDecision::Continue; - }); - ASSERT(thread_count() == 1); - ASSERT_NOT_REACHED(); - } size_t total_blob_size = 0; for (auto& a : arguments) @@ -808,6 +832,10 @@ int Process::do_exec(NonnullRefPtr main_program_description, Ve bool was_profiling = is_profiling(); TemporaryChange profiling_disabler(m_profiling, false); + // Mark this thread as the current thread that does exec + // No other thread from this process will be scheduled to run + m_exec_tid = Thread::current->tid(); + auto old_page_directory = move(m_page_directory); auto old_regions = move(m_regions); m_page_directory = PageDirectory::create_for_userspace(*this); @@ -854,9 +882,7 @@ int Process::do_exec(NonnullRefPtr main_program_description, Ve m_regions = move(old_regions); MM.enter_process_paging_scope(*this); }); - loader = make(region->vaddr().as_ptr(), loader_metadata.size); - // Load the correct executable -- either interp or main program. // FIXME: Once we actually load both interp and main, we'll need to be more clever about this. // In that case, both will be ET_DYN objects, so they'll both be completely relocatable. @@ -920,6 +946,8 @@ int Process::do_exec(NonnullRefPtr main_program_description, Ve // NOTE: At this point, we've committed to the new executable. entry_eip = loader->entry().offset(totally_random_offset).get(); + kill_threads_except_self(); + #ifdef EXEC_DEBUG kprintf("Memory layout after ELF load:"); dump_regions(); @@ -999,6 +1027,7 @@ int Process::do_exec(NonnullRefPtr main_program_description, Ve m_master_tls_size = master_tls_size; m_master_tls_alignment = master_tls_alignment; + m_pid = new_main_thread->tid(); new_main_thread->make_thread_specific_region({}); new_main_thread->reset_fpu_state(); @@ -1198,6 +1227,9 @@ int Process::exec(String path, Vector arguments, Vector environm // The bulk of exec() is done by do_exec(), which ensures that all locals // are cleaned up by the time we yield-teleport below. int rc = do_exec(move(description), move(arguments), move(environment), move(interpreter_description)); + + m_exec_tid = 0; + if (rc < 0) return rc; @@ -1211,6 +1243,7 @@ int Process::exec(String path, Vector arguments, Vector environm int Process::sys$execve(const Syscall::SC_execve_params* user_params) { REQUIRE_PROMISE(exec); + // NOTE: Be extremely careful with allocating any kernel memory in exec(). // On success, the kernel stack will be lost. Syscall::SC_execve_params params; @@ -3053,14 +3086,7 @@ void Process::die() if (m_tracer) m_tracer->set_dead(); - { - // Tell the threads to unwind and die. - InterruptDisabler disabler; - for_each_thread([](Thread& thread) { - thread.set_should_die(); - return IterationDecision::Continue; - }); - } + kill_all_threads(); } size_t Process::amount_dirty_private() const diff --git a/Kernel/Process.h b/Kernel/Process.h index 8804f0cbc20..edc6dbd6528 100644 --- a/Kernel/Process.h +++ b/Kernel/Process.h @@ -141,6 +141,8 @@ public: gid_t egid() const { return m_egid; } pid_t ppid() const { return m_ppid; } + pid_t exec_tid() const { return m_exec_tid; } + mode_t umask() const { return m_umask; } bool in_group(gid_t) const; @@ -417,6 +419,9 @@ private: Region& add_region(NonnullOwnPtr); + void kill_threads_except_self(); + void kill_all_threads(); + int do_exec(NonnullRefPtr main_program_description, Vector arguments, Vector environment, RefPtr interpreter_description); ssize_t do_write(FileDescription&, const u8*, int data_size); @@ -448,6 +453,8 @@ private: pid_t m_sid { 0 }; pid_t m_pgid { 0 }; + pid_t m_exec_tid { 0 }; + static const int m_max_open_file_descriptors { FD_SETSIZE }; struct FileDescriptionAndFlags { diff --git a/Kernel/Scheduler.cpp b/Kernel/Scheduler.cpp index e8b67b1d596..8b7b8d7124d 100644 --- a/Kernel/Scheduler.cpp +++ b/Kernel/Scheduler.cpp @@ -409,6 +409,9 @@ bool Scheduler::pick_next() if (thread->process().is_being_inspected()) continue; + if (thread->process().exec_tid() && thread->process().exec_tid() != thread->tid()) + continue; + ASSERT(thread->state() == Thread::Runnable || thread->state() == Thread::Running); if (!thread_to_schedule) { diff --git a/Kernel/Thread.cpp b/Kernel/Thread.cpp index 8f38cb658ae..cb20b00083e 100644 --- a/Kernel/Thread.cpp +++ b/Kernel/Thread.cpp @@ -162,11 +162,17 @@ Thread::~Thread() void Thread::unblock() { if (current == this) { - set_state(Thread::Running); + if (m_should_die) + set_state(Thread::Dying); + else + set_state(Thread::Running); return; } ASSERT(m_state != Thread::Runnable && m_state != Thread::Running); - set_state(Thread::Runnable); + if (m_should_die) + set_state(Thread::Dying); + else + set_state(Thread::Runnable); } void Thread::set_should_die() diff --git a/Kernel/WaitQueue.cpp b/Kernel/WaitQueue.cpp index 2f866c302d5..2cafbb477fc 100644 --- a/Kernel/WaitQueue.cpp +++ b/Kernel/WaitQueue.cpp @@ -65,4 +65,10 @@ void WaitQueue::wake_all() Scheduler::stop_idling(); } +void WaitQueue::clear() +{ + InterruptDisabler disabler; + m_threads.clear(); +} + } diff --git a/Kernel/WaitQueue.h b/Kernel/WaitQueue.h index c8d7bf3810b..faa77b62781 100644 --- a/Kernel/WaitQueue.h +++ b/Kernel/WaitQueue.h @@ -40,6 +40,7 @@ public: void enqueue(Thread&); void wake_one(Atomic* lock = nullptr); void wake_all(); + void clear(); private: typedef IntrusiveList ThreadList;