Skip to content

Commit

Permalink
Kernel: Change wait blocking to Process-only blocking
Browse files Browse the repository at this point in the history
This prevents zombies created by multi-threaded applications and brings
our model back to closer to what other OSs do.

This also means that SIGSTOP needs to halt all threads, and SIGCONT needs
to resume those threads.
  • Loading branch information
tomuta authored and awesomekling committed Dec 12, 2020
1 parent 47ede74 commit c455fc2
Show file tree
Hide file tree
Showing 11 changed files with 284 additions and 203 deletions.
2 changes: 1 addition & 1 deletion CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,7 @@ if (ALL_THE_DEBUG_MACROS)
set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -DTCP_DEBUG -DTCP_SOCKET_DEBUG -DTERMCAP_DEBUG -DTERMINAL_DEBUG -DTHREAD_DEBUG -DTLS_DEBUG -DTTY_DEBUG")
set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -DUCI_DEBUG -DUDP_DEBUG -DUPDATE_COALESCING_DEBUG")
set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -DVERY_DEBUG -DVFS_DEBUG -DVMWAREBACKDOOR_DEBUG -DVRA_DEBUG")
set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -DWAITQUEUE_DEBUG -DWEAKABLE_DEBUG -DWINDOWMANAGER_DEBUG -DWSMESSAGELOOP_DEBUG -DWSSCREEN_DEBUG")
set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -DWAITBLOCK_DEBUG -DWAITQUEUE_DEBUG -DWEAKABLE_DEBUG -DWINDOWMANAGER_DEBUG -DWSMESSAGELOOP_DEBUG -DWSSCREEN_DEBUG")
# False positive: IF_BMP_DEBUG is not actually a flag.
# set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -DIF_BMP_DEBUG")
# False positive: LOG_DEBUG is a flag, but for a bitset, not a feature.
Expand Down
14 changes: 8 additions & 6 deletions Kernel/Arch/i386/CPU.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -484,7 +484,8 @@ void debug_handler(TrapFrame* trap)
clac();
auto& regs = *trap->regs;
auto current_thread = Thread::current();
if (&current_thread->process() == nullptr || (regs.cs & 3) == 0) {
auto& process = current_thread->process();
if ((regs.cs & 3) == 0) {
klog() << "Debug Exception in Ring0";
Processor::halt();
return;
Expand All @@ -494,8 +495,8 @@ void debug_handler(TrapFrame* trap)
if (!is_reason_singlestep)
return;

if (current_thread->tracer()) {
current_thread->tracer()->set_regs(regs);
if (auto tracer = process.tracer()) {
tracer->set_regs(regs);
}
current_thread->send_urgent_signal_to_self(SIGTRAP);
}
Expand All @@ -506,13 +507,14 @@ void breakpoint_handler(TrapFrame* trap)
clac();
auto& regs = *trap->regs;
auto current_thread = Thread::current();
if (&current_thread->process() == nullptr || (regs.cs & 3) == 0) {
auto& process = current_thread->process();
if ((regs.cs & 3) == 0) {
klog() << "Breakpoint Trap in Ring0";
Processor::halt();
return;
}
if (current_thread->tracer()) {
current_thread->tracer()->set_regs(regs);
if (auto tracer = process.tracer()) {
tracer->set_regs(regs);
}
current_thread->send_urgent_signal_to_self(SIGTRAP);
}
Expand Down
25 changes: 21 additions & 4 deletions Kernel/Process.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -577,7 +577,7 @@ KResultOr<String> Process::get_syscall_path_argument(const Syscall::StringArgume
return get_syscall_path_argument(path.characters, path.length);
}

void Process::finalize(Thread& last_thread)
void Process::finalize()
{
ASSERT(Thread::current() == g_finalizer);
#ifdef PROCESS_DEBUG
Expand Down Expand Up @@ -627,7 +627,7 @@ void Process::finalize(Thread& last_thread)
}
}

unblock_waiters(last_thread, Thread::WaitBlocker::UnblockFlags::Terminated);
unblock_waiters(Thread::WaitBlocker::UnblockFlags::Terminated);

{
ScopedSpinLock lock(m_lock);
Expand All @@ -647,10 +647,10 @@ void Process::disowned_by_waiter(Process& process)
m_wait_block_condition.disowned_by_waiter(process);
}

void Process::unblock_waiters(Thread& thread, Thread::WaitBlocker::UnblockFlags flags, u8 signal)
void Process::unblock_waiters(Thread::WaitBlocker::UnblockFlags flags, u8 signal)
{
if (auto parent = Process::from_pid(ppid()))
parent->m_wait_block_condition.unblock(thread, flags, signal);
parent->m_wait_block_condition.unblock(*this, flags, signal);
}

void Process::die()
Expand Down Expand Up @@ -877,4 +877,21 @@ OwnPtr<Process::ELFBundle> Process::elf_bundle() const
return bundle;
}

void Process::start_tracing_from(ProcessID tracer)
{
m_tracer = ThreadTracer::create(tracer);
}

void Process::stop_tracing()
{
m_tracer = nullptr;
}

void Process::tracer_trap(Thread& thread, const RegisterState& regs)
{
ASSERT(m_tracer.ptr());
m_tracer->set_regs(regs);
thread.send_urgent_signal_to_self(SIGTRAP);
}

}
19 changes: 16 additions & 3 deletions Kernel/Process.h
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@
#include <Kernel/ProcessGroup.h>
#include <Kernel/StdLib.h>
#include <Kernel/Thread.h>
#include <Kernel/ThreadTracer.h>
#include <Kernel/UnixTypes.h>
#include <Kernel/VM/RangeAllocator.h>
#include <LibC/signal_numbers.h>
Expand Down Expand Up @@ -166,6 +167,9 @@ class Process

bool is_dead() const { return m_dead; }

bool is_stopped() const { return m_is_stopped.load(AK::MemoryOrder::memory_order_relaxed); }
bool set_stopped(bool stopped) { return m_is_stopped.exchange(stopped, AK::MemoryOrder::memory_order_relaxed); }

bool is_kernel_process() const { return m_is_kernel_process; }
bool is_user_process() const { return !m_is_kernel_process; }

Expand Down Expand Up @@ -209,10 +213,16 @@ class Process
void for_each_thread(Callback) const;

void die();
void finalize(Thread&);
void finalize();

ALWAYS_INLINE SpinLock<u32>& get_lock() const { return m_lock; }

ThreadTracer* tracer() { return m_tracer.ptr(); }
bool is_traced() const { return !!m_tracer; }
void start_tracing_from(ProcessID tracer);
void stop_tracing();
void tracer_trap(Thread&, const RegisterState&);

int sys$yield();
int sys$sync();
int sys$beep();
Expand Down Expand Up @@ -489,7 +499,7 @@ class Process
KResult poke_user_data(Userspace<u32*> address, u32 data);

void disowned_by_waiter(Process& process);
void unblock_waiters(Thread&, Thread::WaitBlocker::UnblockFlags, u8 signal = 0);
void unblock_waiters(Thread::WaitBlocker::UnblockFlags, u8 signal = 0);
Thread::WaitBlockCondition& wait_block_condition() { return m_wait_block_condition; }

private:
Expand Down Expand Up @@ -530,7 +540,7 @@ class Process
}
KResultOr<String> get_syscall_path_argument(const Syscall::StringArgument&) const;

bool has_tracee_thread(ProcessID tracer_pid) const;
bool has_tracee_thread(ProcessID tracer_pid);

RefPtr<PageDirectory> m_page_directory;

Expand All @@ -554,6 +564,8 @@ class Process
FlatPtr m_load_offset { 0U };
FlatPtr m_entry_eip { 0U };

OwnPtr<ThreadTracer> m_tracer;

static const int m_max_open_file_descriptors { FD_SETSIZE };

class FileDescriptionAndFlags {
Expand Down Expand Up @@ -582,6 +594,7 @@ class Process
const bool m_is_kernel_process;
bool m_dead { false };
bool m_profiling { false };
Atomic<bool> m_is_stopped { false };

RefPtr<Custody> m_executable;
RefPtr<Custody> m_cwd;
Expand Down
11 changes: 6 additions & 5 deletions Kernel/Ptrace.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ KResultOr<u32> handle_syscall(const Kernel::Syscall::SC_ptrace_params& params, P
{
ScopedSpinLock scheduler_lock(g_scheduler_lock);
if (params.request == PT_TRACE_ME) {
if (Thread::current()->tracer())
if (Process::current()->tracer())
return KResult(-EBUSY);

caller.set_wait_for_tracer_at_next_execve(true);
Expand All @@ -59,19 +59,20 @@ KResultOr<u32> handle_syscall(const Kernel::Syscall::SC_ptrace_params& params, P
|| (peer->process().uid() != peer->process().euid())) // Disallow tracing setuid processes
return KResult(-EACCES);

auto& peer_process = peer->process();
if (params.request == PT_ATTACH) {
if (peer->tracer()) {
if (peer_process.tracer()) {
return KResult(-EBUSY);
}
peer->start_tracing_from(caller.pid());
peer_process.start_tracing_from(caller.pid());
ScopedSpinLock lock(peer->get_lock());
if (peer->state() != Thread::State::Stopped) {
peer->send_signal(SIGSTOP, &caller);
}
return KSuccess;
}

auto* tracer = peer->tracer();
auto* tracer = peer_process.tracer();

if (!tracer)
return KResult(-EPERM);
Expand All @@ -88,7 +89,7 @@ KResultOr<u32> handle_syscall(const Kernel::Syscall::SC_ptrace_params& params, P
break;

case PT_DETACH:
peer->stop_tracing();
peer_process.stop_tracing();
peer->send_signal(SIGCONT, &caller);
break;

Expand Down
14 changes: 7 additions & 7 deletions Kernel/Syscall.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -94,10 +94,10 @@ int handle(RegisterState& regs, u32 function, u32 arg1, u32 arg2, u32 arg3)
if (function == SC_exit || function == SC_exit_thread) {
// These syscalls need special handling since they never return to the caller.

if (auto* tracer = current_thread->tracer(); tracer && tracer->is_tracing_syscalls()) {
if (auto* tracer = process.tracer(); tracer && tracer->is_tracing_syscalls()) {
regs.eax = 0;
tracer->set_trace_syscalls(false);
current_thread->tracer_trap(regs); // this triggers SIGTRAP and stops the thread!
process.tracer_trap(*current_thread, regs); // this triggers SIGTRAP and stops the thread!
}

cli();
Expand Down Expand Up @@ -137,10 +137,11 @@ void syscall_handler(TrapFrame* trap)
{
auto& regs = *trap->regs;
auto current_thread = Thread::current();
auto& process = current_thread->process();

if (auto* tracer = current_thread->tracer(); tracer && tracer->is_tracing_syscalls()) {
if (auto tracer = process.tracer(); tracer && tracer->is_tracing_syscalls()) {
tracer->set_trace_syscalls(false);
current_thread->tracer_trap(regs); // this triggers SIGTRAP and stops the thread!
process.tracer_trap(*current_thread, regs); // this triggers SIGTRAP and stops the thread!
}

current_thread->yield_if_stopped();
Expand All @@ -160,7 +161,6 @@ void syscall_handler(TrapFrame* trap)
asm volatile(""
: "=m"(*ptr));

auto& process = current_thread->process();
if (!MM.validate_user_stack(process, VirtualAddress(regs.userspace_esp))) {
dbgln("Invalid stack pointer: {:p}", regs.userspace_esp);
handle_crash(regs, "Bad stack on syscall entry", SIGSTKFLT);
Expand Down Expand Up @@ -189,9 +189,9 @@ void syscall_handler(TrapFrame* trap)

process.big_lock().unlock();

if (auto* tracer = current_thread->tracer(); tracer && tracer->is_tracing_syscalls()) {
if (auto tracer = process.tracer(); tracer && tracer->is_tracing_syscalls()) {
tracer->set_trace_syscalls(false);
current_thread->tracer_trap(regs); // this triggers SIGTRAP and stops the thread!
process.tracer_trap(*current_thread, regs); // this triggers SIGTRAP and stops the thread!
}

current_thread->yield_if_stopped();
Expand Down
15 changes: 4 additions & 11 deletions Kernel/Syscalls/ptrace.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -48,18 +48,11 @@ int Process::sys$ptrace(Userspace<const Syscall::SC_ptrace_params*> user_params)
/**
* "Does this process have a thread that is currently being traced by the provided process?"
*/
bool Process::has_tracee_thread(ProcessID tracer_pid) const
bool Process::has_tracee_thread(ProcessID tracer_pid)
{
bool has_tracee = false;

for_each_thread([&](Thread& t) {
if (t.tracer() && t.tracer()->tracer_pid() == tracer_pid) {
has_tracee = true;
return IterationDecision::Break;
}
return IterationDecision::Continue;
});
return has_tracee;
if (auto tracer = this->tracer())
return tracer->tracer_pid() == tracer_pid;
return false;
}

KResultOr<u32> Process::peek_user_data(Userspace<const u32*> address)
Expand Down
1 change: 1 addition & 0 deletions Kernel/Syscalls/thread.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -136,6 +136,7 @@ int Process::sys$join_thread(pid_t tid, Userspace<void**> exit_value)
}
if (result == Thread::BlockResult::InterruptedByDeath)
break;
dbg() << "join_thread: retrying";
}

if (exit_value && !copy_to_user(exit_value, &joinee_exit_value))
Expand Down
Loading

0 comments on commit c455fc2

Please sign in to comment.