Kernel: Rework all signal code

Signal handling code was way too complex. Now everything is
simplified and there is no need for ThreadBlockers.

Only complication that this patch includes is that blocking syscalls
have to manually be made interruptable by signal. There might be some
clever solution to combat this is make this happen automatically.

kernel/arch/x86_64/Signal.S

@@ -31,10 +31,6 @@ signal_trampoline:
 	movq 120(%rsp), %rax
 	call *%rax
 
-	movq $SYS_SIGNAL_DONE, %rax
-	movq 128(%rsp), %rbx
-	int $0x80
-
 	popq %r15
 	popq %r14
 	popq %r13

kernel/include/kernel/Thread.h

@@ -25,19 +25,9 @@ namespace Kernel
 		{
 			NotStarted,
 			Executing,
-			Terminating,
 			Terminated
 		};
 
-		class TerminateBlocker
-		{
-		public:
-			TerminateBlocker(Thread&);
-			~TerminateBlocker();
-		private:
-			Thread& m_thread;
-		};
-
 	public:
 		static BAN::ErrorOr<Thread*> create_kernel(entry_t, void*, Process*);
 		static BAN::ErrorOr<Thread*> create_userspace(Process*);
@@ -47,15 +37,20 @@ namespace Kernel
 		void setup_exec();
 		void setup_process_cleanup();
 
-		bool has_signal_to_execute() const;
-		void set_signal_done(int signal);
+		// Adds pending signals to thread if possible and
+		// returns true, if thread is going to trigger signal
+		bool is_interrupted_by_signal();
+
+		// Returns true if pending signal can be added to thread
+		bool can_add_signal_to_execute() const;
+		bool will_execute_signal() const;
 		void handle_signal(int signal = 0);
 		bool add_signal(int signal);
 
 		void set_return_rsp(uintptr_t& rsp) { m_return_rsp = &rsp; }
 		void set_return_rip(uintptr_t& rip) { m_return_rip = &rip; }
-		uintptr_t& return_rsp() { ASSERT(m_return_rsp); return *m_return_rsp; }
-		uintptr_t& return_rip() { ASSERT(m_return_rip); return *m_return_rip; }
+		uintptr_t return_rsp() { ASSERT(m_return_rsp); return *m_return_rsp; }
+		uintptr_t return_rip() { ASSERT(m_return_rip); return *m_return_rip; }
 
 		pid_t tid() const { return m_tid; }
 
@@ -65,7 +60,8 @@ namespace Kernel
 		uintptr_t rip() const { return m_rip; }
 
 		void set_started() { ASSERT(m_state == State::NotStarted); m_state = State::Executing; }
-		void set_terminating();
+		// Thread will no longer execute. If called on current thread, does not return
+		void terminate();
 		State state() const { return m_state; }
 
 		vaddr_t stack_base() const { return m_stack->vaddr(); }
@@ -116,11 +112,8 @@ namespace Kernel
 
 		uint64_t					m_signal_pending_mask	{ 0 };
 		uint64_t					m_signal_block_mask		{ 0 };
-		int							m_handling_signal		{ 0 };
 		static_assert(_SIGMAX < 64);
 
-		uint64_t m_terminate_blockers { 0 };
-
 #if __enable_sse
 		alignas(16) uint8_t m_sse_storage[512] {};
 #endif

kernel/kernel/FS/Inode.cpp

@@ -1,6 +1,5 @@
 #include <kernel/FS/Inode.h>
 #include <kernel/LockGuard.h>
-#include <kernel/Thread.h>
 
 #include <fcntl.h>
 
@@ -60,7 +59,6 @@ namespace Kernel
 	BAN::ErrorOr<BAN::RefPtr<Inode>> Inode::find_inode(BAN::StringView name)
 	{
 		LockGuard _(m_lock);
-		Thread::TerminateBlocker blocker(Thread::current());
 		if (!mode().ifdir())
 			return BAN::Error::from_errno(ENOTDIR);
 		return find_inode_impl(name);
@@ -69,7 +67,6 @@ namespace Kernel
 	BAN::ErrorOr<void> Inode::list_next_inodes(off_t offset, DirectoryEntryList* list, size_t list_len)
 	{
 		LockGuard _(m_lock);
-		Thread::TerminateBlocker blocker(Thread::current());
 		if (!mode().ifdir())
 			return BAN::Error::from_errno(ENOTDIR);
 		return list_next_inodes_impl(offset, list, list_len);
@@ -78,7 +75,6 @@ namespace Kernel
 	BAN::ErrorOr<void> Inode::create_file(BAN::StringView name, mode_t mode, uid_t uid, gid_t gid)
 	{
 		LockGuard _(m_lock);
-		Thread::TerminateBlocker blocker(Thread::current());
 		if (!this->mode().ifdir())
 			return BAN::Error::from_errno(ENOTDIR);
 		if (Mode(mode).ifdir())
@@ -89,7 +85,6 @@ namespace Kernel
 	BAN::ErrorOr<void> Inode::create_directory(BAN::StringView name, mode_t mode, uid_t uid, gid_t gid)
 	{
 		LockGuard _(m_lock);
-		Thread::TerminateBlocker blocker(Thread::current());
 		if (!this->mode().ifdir())
 			return BAN::Error::from_errno(ENOTDIR);
 		if (!Mode(mode).ifdir())
@@ -100,7 +95,6 @@ namespace Kernel
 	BAN::ErrorOr<void> Inode::unlink(BAN::StringView name)
 	{
 		LockGuard _(m_lock);
-		Thread::TerminateBlocker blocker(Thread::current());
 		if (!mode().ifdir())
 			return BAN::Error::from_errno(ENOTDIR);
 		if (name == "."sv || name == ".."sv)
@@ -111,7 +105,6 @@ namespace Kernel
 	BAN::ErrorOr<BAN::String> Inode::link_target()
 	{
 		LockGuard _(m_lock);
-		Thread::TerminateBlocker blocker(Thread::current());
 		if (!mode().iflnk())
 			return BAN::Error::from_errno(EINVAL);
 		return link_target_impl();
@@ -120,7 +113,6 @@ namespace Kernel
 	BAN::ErrorOr<size_t> Inode::read(off_t offset, BAN::ByteSpan buffer)
 	{
 		LockGuard _(m_lock);
-		Thread::TerminateBlocker blocker(Thread::current());
 		if (mode().ifdir())
 			return BAN::Error::from_errno(EISDIR);
 		return read_impl(offset, buffer);
@@ -129,7 +121,6 @@ namespace Kernel
 	BAN::ErrorOr<size_t> Inode::write(off_t offset, BAN::ConstByteSpan buffer)
 	{
 		LockGuard _(m_lock);
-		Thread::TerminateBlocker blocker(Thread::current());
 		if (mode().ifdir())
 			return BAN::Error::from_errno(EISDIR);
 		return write_impl(offset, buffer);
@@ -138,7 +129,6 @@ namespace Kernel
 	BAN::ErrorOr<void> Inode::truncate(size_t size)
 	{
 		LockGuard _(m_lock);
-		Thread::TerminateBlocker blocker(Thread::current());
 		if (mode().ifdir())
 			return BAN::Error::from_errno(EISDIR);
 		return truncate_impl(size);
@@ -148,14 +138,12 @@ namespace Kernel
 	{
 		ASSERT((mode & Inode::Mode::TYPE_MASK) == 0);
 		LockGuard _(m_lock);
-		Thread::TerminateBlocker blocker(Thread::current());
 		return chmod_impl(mode);
 	}
 
 	bool Inode::has_data() const
 	{
 		LockGuard _(m_lock);
-		Thread::TerminateBlocker blocker(Thread::current());
 		return has_data_impl();
 	}
 

kernel/kernel/Process.cpp

@@ -257,7 +257,10 @@ namespace Kernel
 		LockGuard _(m_lock);
 		m_exit_status.exit_code = __WGENEXITCODE(status, signal);
 		for (auto* thread : m_threads)
-			thread->set_terminating();
+			if (thread != &Thread::current())
+				thread->terminate();
+		if (this == &Process::current())
+			Thread::current().terminate();
 	}
 
 	size_t Process::proc_meminfo(off_t offset, BAN::ByteSpan buffer) const
@@ -334,7 +337,7 @@ namespace Kernel
 	BAN::ErrorOr<long> Process::sys_exit(int status)
 	{
 		exit(status, 0);
-		Thread::TerminateBlocker _(Thread::current());
+		Thread::current().terminate();
 		ASSERT_NOT_REACHED();
 	}
 
@@ -1235,6 +1238,8 @@ namespace Kernel
 					{
 						CriticalScope _;
 						process.m_signal_pending_mask |= 1 << signal;
+						// FIXME: This is super hacky
+						Scheduler::get().unblock_thread(process.m_threads.front()->tid());
 					}
 					return (pid > 0) ? BAN::Iteration::Break : BAN::Iteration::Continue;
 				}

kernel/kernel/Scheduler.cpp

@@ -258,10 +258,8 @@ namespace Kernel
 				current->set_started();
 				start_thread(current->rsp(), current->rip());
 			case Thread::State::Executing:
-				while (current->has_signal_to_execute())
+				while (current->can_add_signal_to_execute())
 					current->handle_signal();
-				// fall through
-			case Thread::State::Terminating:
 				continue_thread(current->rsp(), current->rip());
 			case Thread::State::Terminated:
 				ASSERT_NOT_REACHED();

kernel/kernel/Storage/StorageDevice.cpp

@@ -225,7 +225,6 @@ namespace Kernel
 
 		{
 			LockGuard _(m_lock);
-			Thread::TerminateBlocker blocker(Thread::current());
 			if (!m_disk_cache.has_value())
 				return read_sectors_impl(lba, sector_count, buffer);
 		}
@@ -233,8 +232,6 @@ namespace Kernel
 		for (uint64_t offset = 0; offset < sector_count; offset++)
 		{
 			LockGuard _(m_lock);
-			Thread::TerminateBlocker blocker(Thread::current());
-
 			auto sector_buffer = buffer.slice(offset * sector_size(), sector_size());
 			if (m_disk_cache->read_from_cache(lba + offset, sector_buffer))
 				continue;
@@ -251,7 +248,6 @@ namespace Kernel
 
 		{
 			LockGuard _(m_lock);
-			Thread::TerminateBlocker blocker(Thread::current());
 			if (!m_disk_cache.has_value())
 				return write_sectors_impl(lba, sector_count, buffer);
 		}
@@ -259,8 +255,6 @@ namespace Kernel
 		for (uint8_t offset = 0; offset < sector_count; offset++)
 		{
 			LockGuard _(m_lock);
-			Thread::TerminateBlocker blocker(Thread::current());
-
 			auto sector_buffer = buffer.slice(offset * sector_size(), sector_size());
 			if (m_disk_cache->write_to_cache(lba + offset, sector_buffer, true).is_error())
 				TRY(write_sectors_impl(lba + offset, 1, sector_buffer));

kernel/kernel/Syscall.cpp

@@ -26,12 +26,6 @@ namespace Kernel
 		Thread::current().set_return_rsp(interrupt_stack.rsp);
 		Thread::current().set_return_rip(interrupt_stack.rip);
 
-		if (syscall == SYS_SIGNAL_DONE)
-		{
-			Thread::current().set_signal_done((int)arg1);
-			return 0;
-		}
-
 #if __enable_sse
 		Thread::current().save_sse();
 #endif
@@ -154,9 +148,6 @@ namespace Kernel
 		case SYS_SIGNAL:
 			ret = Process::current().sys_signal((int)arg1, (void (*)(int))arg2);
 			break;
-		case SYS_SIGNAL_DONE:
-			// Handled above
-			ASSERT_NOT_REACHED();
 		case SYS_TCSETPGRP:
 			ret = Process::current().sys_tcsetpgrp((int)arg1, (pid_t)arg2);
 			break;
@@ -230,10 +221,14 @@ namespace Kernel
 		if (ret.is_error() && ret.error().is_kernel_error())
 			Kernel::panic("Kernel error while returning to userspace {}", ret.error());
 
+		auto& current_thread = Thread::current();
+		if (current_thread.can_add_signal_to_execute())
+			current_thread.handle_signal();
+
 		ASSERT(Kernel::Thread::current().state() == Kernel::Thread::State::Executing);
 
 #if __enable_sse
-		Thread::current().load_sse();
+		current_thread.load_sse();
 #endif
 
 		if (ret.is_error())

kernel/kernel/Terminal/TTY.cpp

@@ -303,10 +303,14 @@ namespace Kernel
 		LockGuard _(m_lock);
 		while (!m_output.flush)
 		{
+			if (Thread::current().is_interrupted_by_signal())
+				return BAN::Error::from_errno(EINTR);
 			m_lock.unlock();
 			m_output.semaphore.block();
 			m_lock.lock();
 		}
+		if (Thread::current().is_interrupted_by_signal())
+			return BAN::Error::from_errno(EINTR);
 
 		if (m_output.bytes == 0)
 		{

kernel/kernel/Thread.cpp

@@ -1,6 +1,8 @@
 #include <BAN/Errors.h>
 #include <BAN/ScopeGuard.h>
+#include <kernel/GDT.h>
 #include <kernel/InterruptController.h>
+#include <kernel/InterruptStack.h>
 #include <kernel/Memory/kmalloc.h>
 #include <kernel/Memory/PageTableScope.h>
 #include <kernel/Process.h>
@@ -25,63 +27,13 @@ namespace Kernel
 			memcpy((void*)rsp, (void*)&value, sizeof(uintptr_t));
 	}
 
-	Thread::TerminateBlocker::TerminateBlocker(Thread& thread)
-		: m_thread(thread)
+	void Thread::terminate()
 	{
 		CriticalScope _;
-
-		// FIXME: this should not be a requirement
-		ASSERT(&thread == &Thread::current());
-
-		if (m_thread.state() == State::Executing || m_thread.m_terminate_blockers > 0)
-		{
-			m_thread.m_terminate_blockers++;
-			return;
-		}
-
-		if (m_thread.state() == State::Terminating && m_thread.m_terminate_blockers == 0)
-		{
-			m_thread.m_state = State::Terminated;
+		ASSERT(this == &Thread::current());
+		m_state = Thread::State::Terminated;
+		if (this == &Thread::current())
 			Scheduler::get().execute_current_thread();
-		}
-
-		while (true)
-			Scheduler::get().reschedule();
-	}
-
-	Thread::TerminateBlocker::~TerminateBlocker()
-	{
-		CriticalScope _;
-
-		m_thread.m_terminate_blockers--;
-
-		if (m_thread.state() == State::Executing || m_thread.m_terminate_blockers > 0)
-			return;
-
-		if (m_thread.state() == State::Terminating && m_thread.m_terminate_blockers == 0)
-		{
-			m_thread.m_state = State::Terminated;
-			Scheduler::get().execute_current_thread();
-		}
-
-		ASSERT_NOT_REACHED();
-	}
-
-	void Thread::set_terminating()
-	{
-		CriticalScope _;
-		if (m_terminate_blockers == 0)
-		{
-			m_state = State::Terminated;
-			if (this == &Thread::current())
-				Scheduler::get().execute_current_thread();
-		}
-		else
-		{
-			m_state = State::Terminating;
-			if (this == &Thread::current())
-				Scheduler::get().unblock_thread(tid());
-		}
 	}
 
 	static pid_t s_next_tid = 1;
@@ -245,27 +197,30 @@ namespace Kernel
 		}
 	}
 
-	bool Thread::has_signal_to_execute() const
+	bool Thread::is_interrupted_by_signal()
 	{
-		if (!is_userspace() || m_handling_signal || m_state != State::Executing)
+		while (can_add_signal_to_execute())
+			handle_signal();
+		return will_execute_signal();
+	}
+
+	bool Thread::can_add_signal_to_execute() const
+	{
+		if (!is_userspace() || m_state != State::Executing)
+			return false;
+		auto& interrupt_stack = *reinterpret_cast<InterruptStack*>(interrupt_stack_base() + interrupt_stack_size() - sizeof(InterruptStack));
+		if (!GDT::is_user_segment(interrupt_stack.cs))
 			return false;
 		uint64_t full_pending_mask = m_signal_pending_mask | m_process->m_signal_pending_mask;
 		return full_pending_mask & ~m_signal_block_mask;
 	}
 
-	void Thread::set_signal_done(int signal)
+	bool Thread::will_execute_signal() const
 	{
-		ASSERT(!interrupts_enabled());
-		if (m_handling_signal == 0)
-			derrorln("set_signal_done called while not handling singal");
-		else if (m_handling_signal != signal)
-			derrorln("set_signal_done called with invalid signal");
-		else
-			m_handling_signal = 0;
-
-		if (m_handling_signal == 0)
-			while (has_signal_to_execute())
-				handle_signal();
+		if (!is_userspace() || m_state != State::Executing)
+			return false;
+		auto& interrupt_stack = *reinterpret_cast<InterruptStack*>(interrupt_stack_base() + interrupt_stack_size() - sizeof(InterruptStack));
+		return interrupt_stack.rip == (uintptr_t)signal_trampoline;
 	}
 
 	void Thread::handle_signal(int signal)
@@ -273,7 +228,9 @@ namespace Kernel
 		ASSERT(!interrupts_enabled());
 		ASSERT(&Thread::current() == this);
 		ASSERT(is_userspace());
-		ASSERT(!m_handling_signal);
+
+		auto& interrupt_stack = *reinterpret_cast<InterruptStack*>(interrupt_stack_base() + interrupt_stack_size() - sizeof(InterruptStack));
+		ASSERT(GDT::is_user_segment(interrupt_stack.cs));
 
 		if (signal == 0)
 		{
@@ -292,9 +249,6 @@ namespace Kernel
 			ASSERT(signal <= _SIGMAX);
 		}
 
-		uintptr_t& return_rsp = this->return_rsp();
-		uintptr_t& return_rip = this->return_rip();
-
 		vaddr_t signal_handler = process().m_signal_handlers[signal];
 
 		m_signal_pending_mask &= ~(1ull << signal);
@@ -305,14 +259,11 @@ namespace Kernel
 		else if (signal_handler != (vaddr_t)SIG_DFL)
 		{
 			// call userspace signal handlers
-			// FIXME: signal trampoline should take a hash etc
-			//        to only allow marking signals done from it
-			m_handling_signal = signal;
-			return_rsp += 128; // skip possible red-zone
-			write_to_stack(return_rsp, return_rip);
-			write_to_stack(return_rsp, signal);
-			write_to_stack(return_rsp, signal_handler);
-			return_rip = (uintptr_t)signal_trampoline;
+			interrupt_stack.rsp -= 128; // skip possible red-zone
+			write_to_stack(interrupt_stack.rsp, interrupt_stack.rip);
+			write_to_stack(interrupt_stack.rsp, signal);
+			write_to_stack(interrupt_stack.rsp, signal_handler);
+			interrupt_stack.rip = (uintptr_t)signal_trampoline;
 		}
 		else
 		{
@@ -393,8 +344,8 @@ namespace Kernel
 
 	void Thread::on_exit()
 	{
-		set_terminating();
-		TerminateBlocker(*this);
+		ASSERT(this == &Thread::current());
+		terminate();
 		ASSERT_NOT_REACHED();
 	}
 

libc/include/sys/syscall.h

@@ -40,7 +40,6 @@ __BEGIN_DECLS
 #define SYS_DUP2 37
 #define SYS_KILL 39
 #define SYS_SIGNAL 40
-#define SYS_SIGNAL_DONE 41
 #define SYS_TCSETPGRP 42
 #define SYS_GET_PID 43
 #define SYS_GET_PGID 44