From 788ef803fc425c9812fffd8d0834997b888d6c34 Mon Sep 17 00:00:00 2001 From: FlareCoding Date: Thu, 27 Aug 2026 21:23:42 -0700 Subject: [PATCH 01/12] docs(sched): documented the task pointer pin contract on the wake paths --- kernel/sched/sched.h | 8 +++++++- kernel/sync/wait_queue.cpp | 5 ++--- kernel/sync/wait_queue.h | 10 ++++------ 3 files changed, 13 insertions(+), 10 deletions(-) diff --git a/kernel/sched/sched.h b/kernel/sched/sched.h index b4529821..c10238e1 100644 --- a/kernel/sched/sched.h +++ b/kernel/sched/sched.h @@ -140,9 +140,14 @@ __PRIVILEGED_CODE void enqueue(task* t); __PRIVILEGED_CODE void enqueue_on(task* t, uint32_t cpu_id); /** - * @brief Resume a blocked task by placing it on the local runqueue. + * @brief Resume a blocked task by placing it on its CPU's runqueue. * Atomically transitions BLOCKED -> READY via CAS. * Called by sync::wake_one / sync::wake_all. + * + * Caller must pin t: hold a lock t re-acquires before it can exit, so the + * reaper cannot reclaim it mid-call. Blocking paths that re-acquire nothing + * are pinned only by the registry lock. A remote wake spins for t to go + * off-CPU, so holding a spinlock with interrupts off across it can deadlock. * @note Privilege: **required** */ __PRIVILEGED_CODE void wake(task* t); @@ -151,6 +156,7 @@ __PRIVILEGED_CODE void wake(task* t); * @brief Mark a task for termination and wake it if blocked. * Fire-and-forget: the target is force-woken now or observes the kill * at its next killable blocking attempt (sleep, futex, poll). + * Same pin and spin rules as wake. * @note Privilege: **required** */ __PRIVILEGED_CODE void force_wake_for_kill(task* t); diff --git a/kernel/sync/wait_queue.cpp b/kernel/sync/wait_queue.cpp index 152a51b3..b1a4652e 100644 --- a/kernel/sync/wait_queue.cpp +++ b/kernel/sync/wait_queue.cpp @@ -43,9 +43,8 @@ __PRIVILEGED_CODE static void notify_observers_and_unlock( // Overflow: re-scan and wake all observers under the lock. // Some were already woken above, sched::wake() is idempotent. - // sched::wake() only acquires rq.lock (never wq.lock), so holding - // wq.lock here is deadlock-free. This path is extremely rare - // (requires >16 concurrent pollers on one wait queue). + // Lock order is fine (wake takes only rq.lock) but its off-CPU spin is + // not: the CPU owing that publication may be waiting for this wq.lock. if (total > n) { irq = spin_lock_irqsave(wq.lock); for (auto& obs : wq.observers) { diff --git a/kernel/sync/wait_queue.h b/kernel/sync/wait_queue.h index ac5c25ec..495b125d 100644 --- a/kernel/sync/wait_queue.h +++ b/kernel/sync/wait_queue.h @@ -25,10 +25,8 @@ struct wait_queue { * Block current task until woken, atomically releasing a held lock. * * Caller MUST hold `lock` via spin_lock_irqsave (IRQs disabled). - * Internally acquires wq.lock via spin_lock_irqsave for both the - * enqueue and post-yield cleanup, ensuring it is - * safe to call from any context, including paths where an ISR may - * concurrently call wake_one() or wake_all(). + * Takes wq.lock for the enqueue and the post-yield cleanup, so an ISR + * running wake_one() or wake_all() cannot race the wait entry. * * On wake, re-acquires `lock` via spin_lock_irqsave and returns * the new irq_state. Caller MUST re-check its condition (spurious @@ -48,14 +46,14 @@ irq_state wait(wait_queue& wq, spinlock& lock, irq_state saved); /** * Wake the first waiting task (FIFO order). - * No-op if the queue is empty. Safe from IRQ context. + * No-op if the queue is empty. Same pin and spin rules as sched::wake. * @note Privilege: **required** */ __PRIVILEGED_CODE void wake_one(wait_queue& wq); /** * Wake all waiting tasks. - * No-op if the queue is empty. Safe from IRQ context. + * No-op if the queue is empty. Same pin and spin rules as sched::wake. * @note Privilege: **required** */ __PRIVILEGED_CODE void wake_all(wait_queue& wq); From a01a542c545fa6b02979c366654cc4c80a5db61f Mon Sep 17 00:00:00 2001 From: FlareCoding Date: Thu, 27 Aug 2026 22:17:00 -0700 Subject: [PATCH 02/12] feat(sched): made the task struct a reference counted object --- kernel/resource/providers/proc_provider.cpp | 46 ++++++--------------- kernel/sched/sched.cpp | 20 +++++---- kernel/sched/task.h | 19 ++++++--- kernel/tests/resource/resource.test.cpp | 19 +++++---- kernel/tests/sched/task_registry.test.cpp | 2 +- 5 files changed, 50 insertions(+), 56 deletions(-) diff --git a/kernel/resource/providers/proc_provider.cpp b/kernel/resource/providers/proc_provider.cpp index d4f667c9..20434a88 100644 --- a/kernel/resource/providers/proc_provider.cpp +++ b/kernel/resource/providers/proc_provider.cpp @@ -1,12 +1,8 @@ #include "resource/providers/proc_provider.h" #include "sched/sched.h" #include "sched/task.h" -#include "sched/task_registry.h" -#include "mm/mm.h" #include "mm/vma.h" -#include "mm/vmm.h" #include "mm/heap.h" -#include "fs/node.h" #include "common/logging.h" #include "sync/poll.h" #include "sync/wait_queue.h" @@ -164,44 +160,28 @@ __PRIVILEGED_CODE proc_resource* get_proc_resource(resource_object* obj) { __PRIVILEGED_CODE void destroy_unstarted_task(sched::task* t) { // Claim the task, a concurrent group teardown may have already - // moved it to dead and handed the memory to the reaper + // moved it to dead and handed it to the reaper uint32_t expected = sched::TASK_STATE_CREATED; if (!t->state.cmpxchg_strong_acq_rel(expected, sched::TASK_STATE_DEAD)) { return; } - // Leave the registry before the group teardown so registry walkers - // never see a task whose group is being freed (same order as reap_task) - sched::g_task_registry.remove(*t); - - resource::release_task_handles(t); - if (t->cwd) { - if (t->cwd->release()) { - fs::node::ref_destroy(t->cwd); - } - t->cwd = nullptr; + // Unlink from the group list here, reap_task releases the group + // reference but never touches the thread list + if (t->group && t->group->leader != t && t->group_link.is_linked()) { + sync::irq_state irq = sync::spin_lock_irqsave(t->group->lock); + t->group->threads.remove(t); + t->group->thread_count--; + sync::spin_unlock_irqrestore(t->group->lock, irq); } - if (t->exec.mm_ctx) { - mm::mm_context_release(t->exec.mm_ctx); - t->exec.mm_ctx = nullptr; - } + // Never started means already off-CPU, so the task goes straight to + // the reaper for the same staged teardown as a normal exit + t->cleanup_stage.store_release(sched::TASK_CLEANUP_STAGE_SCHEDULER_DETACHED); - if (t->group) { - if (t->group->leader != t && t->group_link.is_linked()) { - sync::irq_state irq = sync::spin_lock_irqsave(t->group->lock); - t->group->threads.remove(t); - t->group->thread_count--; - sync::spin_unlock_irqrestore(t->group->lock, irq); - } - if (t->group->release()) { - sched::thread_group::ref_destroy(t->group); - } - t->group = nullptr; + if (t->release()) { + sched::task::ref_destroy(t); } - - vmm::free(t->sys_stack_base); - heap::kfree_delete(t); } } // namespace resource::proc_provider diff --git a/kernel/sched/sched.cpp b/kernel/sched/sched.cpp index a5231984..0ef8ed21 100644 --- a/kernel/sched/sched.cpp +++ b/kernel/sched/sched.cpp @@ -52,6 +52,10 @@ __PRIVILEGED_CODE void thread_group::ref_destroy(thread_group* self) { heap::kfree_delete(self); } +__PRIVILEGED_CODE void task::ref_destroy(task* self) { + rc::reaper::defer(&self->reaper_node); +} + constexpr size_t TASK_STACK_PAGES = 4; constexpr uint16_t TASK_GUARD_PAGES = 1; @@ -284,8 +288,11 @@ __PRIVILEGED_CODE void finalize_pending_off_cpu() { cpu::send_event(); if (load_cleanup_stage(pending) == TASK_CLEANUP_STAGE_SCHEDULER_DETACHED) { - // The reaper must only start cleanup after off-CPU publication is visible. - rc::reaper::defer(&pending->reaper_node); + // Reclamation must not begin before the off-CPU store above is + // visible, so the reference the task was created with drops here. + if (pending->release()) { + task::ref_destroy(pending); + } } } @@ -548,7 +555,9 @@ __PRIVILEGED_CODE void sleep_ms(uint64_t ms) { } store_cleanup_stage(&thread, TASK_CLEANUP_STAGE_SCHEDULER_DETACHED); - rc::reaper::defer(&thread.reaper_node); + if (thread.release()) { + task::ref_destroy(&thread); + } } else { force_wake_for_kill(&thread); } @@ -1301,11 +1310,6 @@ __PRIVILEGED_CODE int32_t init_ap(uint32_t cpu_id, uintptr_t task_stack_top, return ERR_NO_MEM; } - auto* dst = reinterpret_cast(idle); - for (size_t i = 0; i < sizeof(task); i++) { - dst[i] = 0; - } - idle->exec.flags = TASK_FLAG_IDLE | TASK_FLAG_ELEVATED | TASK_FLAG_KERNEL | TASK_FLAG_CAN_ELEVATE | TASK_FLAG_PREEMPTIBLE; idle->exec.cpu = cpu_id; diff --git a/kernel/sched/task.h b/kernel/sched/task.h index 33bc675e..bb81cc94 100644 --- a/kernel/sched/task.h +++ b/kernel/sched/task.h @@ -45,7 +45,12 @@ struct task_tlb_sync_ticket { struct thread_group; -struct task { +/** + * A schedulable unit of execution. Refcounted: a task starts with one + * reference, dropped once the scheduler has detached it after death, and + * the last release hands reclamation to the reaper. + */ +struct task : rc::ref_counted { // Execution core task_exec_core exec; @@ -89,12 +94,14 @@ struct task { // when a thread is created with POSIX file table semantics resource::handle_table* handles; resource::proc_provider::proc_resource* proc_res; -}; -// Assembly accesses task_exec_core fields via offsets from the task pointer. -// exec must be at offset 0 so &task == &task.exec. -static_assert(__builtin_offsetof(task, exec) == 0, - "task.exec must be at offset 0 for assembly compatibility"); + /** + * Defers reclamation to the reaper, which owns the staged teardown + * and the TLB grace period for stack pages. + * @note Privilege: **required** + */ + __PRIVILEGED_CODE static void ref_destroy(task* self); +}; /** * Groups all tasks sharing an address space. Every userland task belongs to diff --git a/kernel/tests/resource/resource.test.cpp b/kernel/tests/resource/resource.test.cpp index db4bf830..f7f00ce5 100644 --- a/kernel/tests/resource/resource.test.cpp +++ b/kernel/tests/resource/resource.test.cpp @@ -90,20 +90,23 @@ TEST(resource_test, rights_enforced_for_read_and_write) { TEST(resource_test, releasing_task_handles_invalidates_existing_handles) { // A scratch task exercises table teardown without touching the // test runner's own live handle table - sched::task scratch{}; - ASSERT_EQ(resource::init_task_handles(&scratch), resource::OK); + sched::task* scratch = heap::kalloc_new(); + ASSERT_NOT_NULL(scratch); + ASSERT_EQ(resource::init_task_handles(scratch), resource::OK); resource::handle_t h1 = -1; resource::handle_t h2 = -1; - ASSERT_EQ(resource::open(&scratch, "/resource_close_all_1", fs::O_CREAT | fs::O_RDWR, &h1), resource::OK); - ASSERT_EQ(resource::open(&scratch, "/resource_close_all_2", fs::O_CREAT | fs::O_RDWR, &h2), resource::OK); + ASSERT_EQ(resource::open(scratch, "/resource_close_all_1", fs::O_CREAT | fs::O_RDWR, &h1), resource::OK); + ASSERT_EQ(resource::open(scratch, "/resource_close_all_2", fs::O_CREAT | fs::O_RDWR, &h2), resource::OK); - resource::release_task_handles(&scratch); + resource::release_task_handles(scratch); - EXPECT_NULL(scratch.handles); - EXPECT_EQ(resource::close(&scratch, h1), resource::ERR_BADF); - EXPECT_EQ(resource::close(&scratch, h2), resource::ERR_BADF); + EXPECT_NULL(scratch->handles); + EXPECT_EQ(resource::close(scratch, h1), resource::ERR_BADF); + EXPECT_EQ(resource::close(scratch, h2), resource::ERR_BADF); + + heap::kfree_delete(scratch); } TEST(resource_test, used_handle_slots_never_have_unknown_type) { diff --git a/kernel/tests/sched/task_registry.test.cpp b/kernel/tests/sched/task_registry.test.cpp index d2d95da8..c61ff53b 100644 --- a/kernel/tests/sched/task_registry.test.cpp +++ b/kernel/tests/sched/task_registry.test.cpp @@ -17,7 +17,7 @@ static sched::task s_tasks[MAX_MOCK_TASKS]; // Zero-initialize a mock task and set its TID. Only the tid and // task_registry_link fields matter for registry operations. static void init_mock_task(sched::task& t, uint32_t tid) { - new (&t) sched::task{}; + new (&t) sched::task(); t.tid = tid; } From 7871c273fea209a303550ccbb13955afd96d0fde Mon Sep 17 00:00:00 2001 From: FlareCoding Date: Thu, 27 Aug 2026 22:46:37 -0700 Subject: [PATCH 03/12] fix(proc): fixed a use-after-free when killing or closing an exiting child --- kernel/resource/providers/proc_provider.cpp | 32 ++++++++++++++------- kernel/sched/sched.cpp | 7 +++++ kernel/sched/sched.h | 18 +++++++++--- kernel/syscall/handlers/sys_proc.cpp | 19 ++++++++---- 4 files changed, 55 insertions(+), 21 deletions(-) diff --git a/kernel/resource/providers/proc_provider.cpp b/kernel/resource/providers/proc_provider.cpp index 20434a88..4afc4d23 100644 --- a/kernel/resource/providers/proc_provider.cpp +++ b/kernel/resource/providers/proc_provider.cpp @@ -35,24 +35,34 @@ __PRIVILEGED_CODE static void proc_close(resource_object* obj) { auto* impl = static_cast(obj->impl); auto* pr = impl->proc.ptr(); + // The reference taken under pr->lock keeps the child reclaim-safe + // after the lock drops, even if it exits and is reaped concurrently sync::irq_state irq = sync::spin_lock_irqsave(pr->lock); + rc::strong_ref child; + bool unstarted = false; - if (pr->child && pr->child->state.load_relaxed() == sched::TASK_STATE_CREATED) { - auto* child = pr->child; - pr->child = nullptr; - sync::spin_unlock_irqrestore(pr->lock, irq); + if (pr->child) { + unstarted = pr->child->state.load_relaxed() == sched::TASK_STATE_CREATED; + if (unstarted || (!pr->exited && !pr->detached)) { + child = sched::task_ref(pr->child); + } + + if (unstarted) { + pr->child = nullptr; + } + } + sync::spin_unlock_irqrestore(pr->lock, irq); + + if (child && unstarted) { if (child->proc_res) { (void)child->proc_res->release(); child->proc_res = nullptr; } - destroy_unstarted_task(child); - } else if (pr->child && !pr->exited && !pr->detached) { - sched::task* child = pr->child; - sync::spin_unlock_irqrestore(pr->lock, irq); - sched::force_wake_for_kill(child); - } else { - sync::spin_unlock_irqrestore(pr->lock, irq); + + destroy_unstarted_task(child.ptr()); + } else if (child) { + sched::force_wake_for_kill(child.ptr()); } heap::kfree_delete(impl); diff --git a/kernel/sched/sched.cpp b/kernel/sched/sched.cpp index 0ef8ed21..abddcc3d 100644 --- a/kernel/sched/sched.cpp +++ b/kernel/sched/sched.cpp @@ -427,6 +427,13 @@ __PRIVILEGED_CODE void enqueue_on(task* t, uint32_t cpu_id) { sync::spin_unlock_irqrestore(rq.lock, irq); } +/** + * @note Privilege: **required** + */ +__PRIVILEGED_CODE rc::strong_ref task_ref(task* t) { + return rc::strong_ref::try_from_raw(t); +} + /** * @note Privilege: **required** */ diff --git a/kernel/sched/sched.h b/kernel/sched/sched.h index c10238e1..6302458c 100644 --- a/kernel/sched/sched.h +++ b/kernel/sched/sched.h @@ -2,6 +2,7 @@ #define STELLUX_SCHED_SCHED_H #include "common/types.h" +#include "rc/strong_ref.h" namespace exec { struct loaded_image; } @@ -144,10 +145,10 @@ __PRIVILEGED_CODE void enqueue_on(task* t, uint32_t cpu_id); * Atomically transitions BLOCKED -> READY via CAS. * Called by sync::wake_one / sync::wake_all. * - * Caller must pin t: hold a lock t re-acquires before it can exit, so the - * reaper cannot reclaim it mid-call. Blocking paths that re-acquire nothing - * are pinned only by the registry lock. A remote wake spins for t to go - * off-CPU, so holding a spinlock with interrupts off across it can deadlock. + * The caller must pin t so the reaper cannot free it mid-call: hold a + * counted reference (task_ref) or a lock t must take before it can exit. + * A remote wake spins until t leaves its CPU, so never hold a spinlock + * with interrupts off across the call. * @note Privilege: **required** */ __PRIVILEGED_CODE void wake(task* t); @@ -161,6 +162,15 @@ __PRIVILEGED_CODE void wake(task* t); */ __PRIVILEGED_CODE void force_wake_for_kill(task* t); +/** + * @brief Acquire a counted reference to a task from a raw pointer. + * The raw pointer must still be protected here: hold a lock the task must + * take before it can finish exiting, or another counted reference. Returns + * a null reference if the task is already tearing down. + * @note Privilege: **required** + */ +[[nodiscard]] __PRIVILEGED_CODE rc::strong_ref task_ref(task* t); + /** * @brief Publish intent to block: moves the current task to BLOCKED. * Pair with block_task_interrupted before yielding. diff --git a/kernel/syscall/handlers/sys_proc.cpp b/kernel/syscall/handlers/sys_proc.cpp index 081245ab..ee9da6ae 100644 --- a/kernel/syscall/handlers/sys_proc.cpp +++ b/kernel/syscall/handlers/sys_proc.cpp @@ -486,17 +486,24 @@ DEFINE_SYSCALL1(proc_kill, u_handle) { return syscall::EINVAL; } + // The reference taken under pr->lock keeps the child reclaim-safe + // after the lock drops, even if it exits and is reaped concurrently sync::irq_state irq = sync::spin_lock_irqsave(pr->lock); - if (!pr->child || pr->exited) { - sync::spin_unlock_irqrestore(pr->lock, irq); - resource::resource_release(obj); - return pr->exited ? 0 : syscall::EINVAL; + bool exited = pr->exited; + + rc::strong_ref child; + if (pr->child && !exited) { + child = sched::task_ref(pr->child); } - sched::task* child = pr->child; sync::spin_unlock_irqrestore(pr->lock, irq); - RUN_ELEVATED(sched::force_wake_for_kill(child)); + if (!child) { + resource::resource_release(obj); + return exited ? 0 : syscall::EINVAL; + } + + RUN_ELEVATED(sched::force_wake_for_kill(child.ptr())); resource::resource_release(obj); return 0; From 1579c760cd21fab04d773efbb73370ae2eac55d8 Mon Sep 17 00:00:00 2001 From: FlareCoding Date: Thu, 27 Aug 2026 23:18:35 -0700 Subject: [PATCH 04/12] fix(sync): implemented holding task references across batch wakes in futex and wait queues --- kernel/sync/futex.cpp | 16 +++++--- kernel/sync/wait_queue.cpp | 80 +++++++++++++++++++------------------- kernel/sync/wait_queue.h | 6 ++- 3 files changed, 55 insertions(+), 47 deletions(-) diff --git a/kernel/sync/futex.cpp b/kernel/sync/futex.cpp index 4354cb66..84572c84 100644 --- a/kernel/sync/futex.cpp +++ b/kernel/sync/futex.cpp @@ -128,7 +128,7 @@ __PRIVILEGED_CODE int32_t futex_wake(uintptr_t uaddr, uint32_t count) { uint32_t total_woken = 0; for (;;) { - sched::task* batch[WAKE_BATCH_SIZE]; + rc::strong_ref batch[WAKE_BATCH_SIZE]; uint32_t n = 0; bool done = false; @@ -141,7 +141,7 @@ __PRIVILEGED_CODE int32_t futex_wake(uintptr_t uaddr, uint32_t count) { ++it; // advance before removal if (w.mm == mm && w.addr == uaddr) { bucket->waiters.remove(&w); - batch[n++] = w.task; + batch[n++] = sched::task_ref(w.task); if (total_woken + n >= count) { done = true; break; @@ -153,7 +153,9 @@ __PRIVILEGED_CODE int32_t futex_wake(uintptr_t uaddr, uint32_t count) { spin_unlock_irqrestore(bucket->lock, irq); for (uint32_t i = 0; i < n; i++) { - sched::wake(batch[i]); + if (batch[i]) { + sched::wake(batch[i].ptr()); + } } total_woken += n; @@ -174,7 +176,7 @@ __PRIVILEGED_CODE int32_t futex_wake_all(uintptr_t uaddr) { uint32_t total_woken = 0; for (;;) { - sched::task* batch[WAKE_BATCH_SIZE]; + rc::strong_ref batch[WAKE_BATCH_SIZE]; uint32_t n = 0; irq_state irq = spin_lock_irqsave(bucket->lock); @@ -186,7 +188,7 @@ __PRIVILEGED_CODE int32_t futex_wake_all(uintptr_t uaddr) { ++it; if (w.mm == mm && w.addr == uaddr) { bucket->waiters.remove(&w); - batch[n++] = w.task; + batch[n++] = sched::task_ref(w.task); } } @@ -194,7 +196,9 @@ __PRIVILEGED_CODE int32_t futex_wake_all(uintptr_t uaddr) { spin_unlock_irqrestore(bucket->lock, irq); for (uint32_t i = 0; i < n; i++) { - sched::wake(batch[i]); + if (batch[i]) { + sched::wake(batch[i].ptr()); + } } total_woken += n; diff --git a/kernel/sync/wait_queue.cpp b/kernel/sync/wait_queue.cpp index b1a4652e..18eb84d2 100644 --- a/kernel/sync/wait_queue.cpp +++ b/kernel/sync/wait_queue.cpp @@ -8,14 +8,11 @@ namespace sync { /** * Set triggered on all observers and wake their tasks. * - * Under wq.lock: set triggered on every observer and snapshot all task - * pointers into a stack batch. After releasing the lock, wake each task. - * - * The observer count per wait_queue is bounded in practice by the number - * of tasks simultaneously polling the same source (typically 1-2). - * The batch is sized to 16 which covers all realistic cases. If more - * observers exist, the excess are handled by a single-entry fallback - * that re-scans under the lock. + * Whoever flips an observer's triggered flag from 0 to 1 owes it exactly + * one wake, delivered after wq.lock drops so sched::wake's off-CPU spin + * never runs under the lock. Already-triggered observers are skipped, an + * earlier notify owes their wake. A full batch forces a rescan, the flag + * marks who was already handled. */ constexpr uint32_t OBSERVER_BATCH_SIZE = 16; constexpr uint32_t WAITER_BATCH_SIZE = 16; @@ -23,34 +20,38 @@ constexpr uint32_t WAITER_BATCH_SIZE = 16; __PRIVILEGED_CODE static void notify_observers_and_unlock( wait_queue& wq, irq_state irq ) { - sched::task* batch[OBSERVER_BATCH_SIZE]; - uint32_t n = 0; - uint32_t total = 0; - - for (auto& obs : wq.observers) { - obs.table->triggered.store_release(1); - if (n < OBSERVER_BATCH_SIZE) { - batch[n++] = obs.table->task; - } - total++; - } + for (;;) { + rc::strong_ref batch[OBSERVER_BATCH_SIZE]; + uint32_t n = 0; + bool rescan = false; - spin_unlock_irqrestore(wq.lock, irq); + for (auto& obs : wq.observers) { + if (obs.table->triggered.load_acquire()) { + continue; + } - for (uint32_t i = 0; i < n; i++) { - sched::wake(batch[i]); - } + if (n == OBSERVER_BATCH_SIZE) { + rescan = true; + break; + } - // Overflow: re-scan and wake all observers under the lock. - // Some were already woken above, sched::wake() is idempotent. - // Lock order is fine (wake takes only rq.lock) but its off-CPU spin is - // not: the CPU owing that publication may be waiting for this wq.lock. - if (total > n) { - irq = spin_lock_irqsave(wq.lock); - for (auto& obs : wq.observers) { - sched::wake(obs.table->task); + obs.table->triggered.store_release(1); + batch[n++] = sched::task_ref(obs.table->task); } + spin_unlock_irqrestore(wq.lock, irq); + + for (uint32_t i = 0; i < n; i++) { + if (batch[i]) { + sched::wake(batch[i].ptr()); + } + } + + if (!rescan) { + return; + } + + irq = spin_lock_irqsave(wq.lock); } } @@ -92,7 +93,7 @@ irq_state wait(wait_queue& wq, spinlock& lock, irq_state saved) { */ __PRIVILEGED_CODE void wake_one(wait_queue& wq) { irq_state irq = spin_lock_irqsave(wq.lock); - sched::task* t = wq.waiters.pop_front(); + rc::strong_ref t = sched::task_ref(wq.waiters.pop_front()); if (!wq.observers.empty()) { // notify_observers_and_unlock releases wq.lock @@ -102,13 +103,13 @@ __PRIVILEGED_CODE void wake_one(wait_queue& wq) { } if (t) { - sched::wake(t); + sched::wake(t.ptr()); } } /** - * Wake all waiting tasks. Snapshots waiter pointers into a stack batch - * so wait_link is fully unlinked (prev=next=nullptr) before any task + * Wake all waiting tasks. Snapshots counted waiter references into a stack + * batch so wait_link is fully unlinked (prev=next=nullptr) before any task * can be scheduled. This prevents a concurrent force_wake_for_kill from * racing with post-yield cleanup in sync::wait, which assumes is_linked * means "still on wq.waiters". @@ -116,14 +117,13 @@ __PRIVILEGED_CODE void wake_one(wait_queue& wq) { * @note Privilege: **required** */ __PRIVILEGED_CODE void wake_all(wait_queue& wq) { - sched::task* batch[WAITER_BATCH_SIZE]; - for (;;) { + rc::strong_ref batch[WAITER_BATCH_SIZE]; uint32_t n = 0; irq_state irq = spin_lock_irqsave(wq.lock); while (!wq.waiters.empty() && n < WAITER_BATCH_SIZE) { - batch[n++] = wq.waiters.pop_front(); + batch[n++] = sched::task_ref(wq.waiters.pop_front()); } bool drained = wq.waiters.empty(); @@ -134,7 +134,9 @@ __PRIVILEGED_CODE void wake_all(wait_queue& wq) { } for (uint32_t i = 0; i < n; i++) { - sched::wake(batch[i]); + if (batch[i]) { + sched::wake(batch[i].ptr()); + } } if (drained) break; diff --git a/kernel/sync/wait_queue.h b/kernel/sync/wait_queue.h index 495b125d..19eaf57d 100644 --- a/kernel/sync/wait_queue.h +++ b/kernel/sync/wait_queue.h @@ -46,14 +46,16 @@ irq_state wait(wait_queue& wq, spinlock& lock, irq_state saved); /** * Wake the first waiting task (FIFO order). - * No-op if the queue is empty. Same pin and spin rules as sched::wake. + * No-op if the queue is empty. Waiters are pinned internally, but the + * off-CPU spin rule of sched::wake still applies to the caller. * @note Privilege: **required** */ __PRIVILEGED_CODE void wake_one(wait_queue& wq); /** * Wake all waiting tasks. - * No-op if the queue is empty. Same pin and spin rules as sched::wake. + * No-op if the queue is empty. Waiters are pinned internally, but the + * off-CPU spin rule of sched::wake still applies to the caller. * @note Privilege: **required** */ __PRIVILEGED_CODE void wake_all(wait_queue& wq); From 25485a18584932ccdbb6d7532c3b69784c5e186c Mon Sep 17 00:00:00 2001 From: FlareCoding Date: Thu, 27 Aug 2026 23:38:15 -0700 Subject: [PATCH 05/12] fix(sched): stopped waking tasks while holding thread group and proc locks --- kernel/sched/sched.cpp | 119 +++++++++++++++++++-------- kernel/signals/signal.cpp | 39 +++++++-- kernel/syscall/handlers/sys_task.cpp | 8 +- 3 files changed, 122 insertions(+), 44 deletions(-) diff --git a/kernel/sched/sched.cpp b/kernel/sched/sched.cpp index abddcc3d..bab0ec27 100644 --- a/kernel/sched/sched.cpp +++ b/kernel/sched/sched.cpp @@ -64,6 +64,8 @@ constexpr uint16_t SYSTEM_GUARD_PAGES = 1; constexpr uint64_t TLB_SYNC_CPU_IGNORED = ~0ULL; +constexpr uint32_t TEARDOWN_BATCH_SIZE = 16; + constexpr uint64_t AT_NULL = 0; constexpr uint64_t AT_PHDR = 3; constexpr uint64_t AT_PHENT = 4; @@ -533,45 +535,87 @@ __PRIVILEGED_CODE void sleep_ms(uint64_t ms) { reap_status = static_cast(es) & 0x7F; } - sync::irq_state irq = sync::spin_lock_irqsave(tg->lock); - auto it = tg->threads.begin(); - auto end = tg->threads.end(); - - while (it != end) { - sched::task& thread = *it; - ++it; // advance before potential removal - uint32_t expected = TASK_STATE_CREATED; - if (thread.state.cmpxchg_strong_acq_rel(expected, - TASK_STATE_DEAD)) { - tg->threads.remove(&thread); - tg->thread_count--; - if (thread.proc_res) { - auto* pr = thread.proc_res; - - sync::irq_state pr_irq = sync::spin_lock_irqsave(pr->lock); - pr->wait_status = reap_status; - pr->exited = true; - pr->child = nullptr; - sync::wake_all(pr->wait_queue); - sync::spin_unlock_irqrestore(pr->lock, pr_irq); - - thread.proc_res = nullptr; - if (pr->release()) { - resource::proc_provider::proc_resource::ref_destroy(pr); + // Threads are handled in batches: kill references and orphaned + // proc resources collected under tg->lock are woken only after + // it drops, keeping the off-CPU spin in wake outside the lock + for (;;) { + rc::strong_ref kill_batch[TEARDOWN_BATCH_SIZE]; + resource::proc_provider::proc_resource* pr_batch[TEARDOWN_BATCH_SIZE]; + uint32_t kills = 0; + uint32_t prs = 0; + bool rescan = false; + + sync::irq_state irq = sync::spin_lock_irqsave(tg->lock); + auto it = tg->threads.begin(); + auto end = tg->threads.end(); + + while (it != end) { + sched::task& thread = *it; + ++it; // advance before potential removal + + if (kills == TEARDOWN_BATCH_SIZE || prs == TEARDOWN_BATCH_SIZE) { + rescan = true; + break; + } + + uint32_t expected = TASK_STATE_CREATED; + if (thread.state.cmpxchg_strong_acq_rel(expected, + TASK_STATE_DEAD)) { + tg->threads.remove(&thread); + tg->thread_count--; + + if (thread.proc_res) { + auto* pr = thread.proc_res; + + sync::irq_state pr_irq = sync::spin_lock_irqsave(pr->lock); + pr->wait_status = reap_status; + pr->exited = true; + pr->child = nullptr; + sync::spin_unlock_irqrestore(pr->lock, pr_irq); + + // The thread's resource reference moves to the + // batch and is released after the deferred wake + thread.proc_res = nullptr; + pr_batch[prs++] = pr; + } + + store_cleanup_stage(&thread, TASK_CLEANUP_STAGE_SCHEDULER_DETACHED); + if (thread.release()) { + task::ref_destroy(&thread); } + } else if (!(thread.sig.pending.load_acquire() + & signals::sig_bit(signals::SIGKILL))) { + // Setting the kill bit under tg->lock marks the + // thread handled, so a rescan cannot batch it twice + thread.sig.pending.fetch_or_acq_rel( + signals::sig_bit(signals::SIGKILL)); + + kill_batch[kills++] = task_ref(&thread); } + } - store_cleanup_stage(&thread, TASK_CLEANUP_STAGE_SCHEDULER_DETACHED); - if (thread.release()) { - task::ref_destroy(&thread); + if (!rescan) { + tg->leader = nullptr; + } + sync::spin_unlock_irqrestore(tg->lock, irq); + + for (uint32_t i = 0; i < prs; i++) { + sync::wake_all(pr_batch[i]->wait_queue); + if (pr_batch[i]->release()) { + resource::proc_provider::proc_resource::ref_destroy(pr_batch[i]); } - } else { - force_wake_for_kill(&thread); } - } - tg->leader = nullptr; - sync::spin_unlock_irqrestore(tg->lock, irq); + for (uint32_t i = 0; i < kills; i++) { + if (kill_batch[i]) { + force_wake_for_kill(kill_batch[i].ptr()); + } + } + + if (!rescan) { + break; + } + } } else if (task->group_link.is_linked()) { sync::irq_state irq = sync::spin_lock_irqsave(tg->lock); tg->threads.remove(task); @@ -586,6 +630,7 @@ __PRIVILEGED_CODE void sleep_ms(uint64_t ms) { uint32_t ges = task->group ? task->group->group_exit_status.load_acquire() : 0; + bool announce = false; sync::irq_state irq = sync::spin_lock_irqsave(pr->lock); if (!pr->detached) { @@ -600,12 +645,18 @@ __PRIVILEGED_CODE void sleep_ms(uint64_t ms) { } pr->exited = true; pr->child = nullptr; - sync::wake_all(pr->wait_queue); + announce = true; } else { pr->child = nullptr; } sync::spin_unlock_irqrestore(pr->lock, irq); + // Waiters recheck pr->exited under pr->lock, so waking after + // the lock drops cannot lose the wakeup + if (announce) { + sync::wake_all(pr->wait_queue); + } + task->proc_res = nullptr; if (pr->release()) { resource::proc_provider::proc_resource::ref_destroy(pr); diff --git a/kernel/signals/signal.cpp b/kernel/signals/signal.cpp index 6987e5c8..f84378c0 100644 --- a/kernel/signals/signal.cpp +++ b/kernel/signals/signal.cpp @@ -173,13 +173,20 @@ __PRIVILEGED_CODE int32_t send_to_task(sched::task* t, uint32_t sig) { // at its next kernel crossing, not only after leader teardown sched::thread_group* tg = t->group; tg->sig.shared_pending.fetch_or_acq_rel(sig_bit(SIGKILL)); + rc::strong_ref leader; + sync::irq_state irq = sync::spin_lock_irqsave(tg->lock); if (tg->leader && tg->leader != t) { - sched::force_wake_for_kill(tg->leader); + leader = sched::task_ref(tg->leader); } sync::spin_unlock_irqrestore(tg->lock, irq); + + if (leader) { + sched::force_wake_for_kill(leader.ptr()); + } + sched::force_wake_for_kill(t); return OK; } @@ -209,13 +216,20 @@ __PRIVILEGED_CODE int32_t send_to_group(sched::thread_group* tg, uint32_t sig) { if (sig == SIGKILL) { tg->sig.shared_pending.fetch_or_acq_rel(sig_bit(SIGKILL)); + rc::strong_ref leader; + sync::irq_state irq = sync::spin_lock_irqsave(tg->lock); if (tg->leader) { - sched::force_wake_for_kill(tg->leader); // leader exit reaps the group + leader = sched::task_ref(tg->leader); } sync::spin_unlock_irqrestore(tg->lock, irq); + + if (leader) { + sched::force_wake_for_kill(leader.ptr()); // leader exit reaps the group + } + return OK; } @@ -248,26 +262,28 @@ __PRIVILEGED_CODE int32_t send_to_group(sched::thread_group* tg, uint32_t sig) { tg->sig.shared_pending.fetch_or_acq_rel(bit); + rc::strong_ref target; if (verdict != send_verdict::IGNORABLE) { // Wake one thread the signal can act on now, leader preferred bool needs_handler = verdict == send_verdict::HANDLED; - sched::task* target = nullptr; if (tg->leader && wake_eligible(tg->leader, bit, needs_handler)) { - target = tg->leader; + target = sched::task_ref(tg->leader); } else { for (sched::task& thread : tg->threads) { if (wake_eligible(&thread, bit, needs_handler)) { - target = &thread; + target = sched::task_ref(&thread); break; } } } - if (target) { - wake_for_signal(target); - } } sync::spin_unlock_irqrestore(tg->lock, irq); + + if (target) { + wake_for_signal(target.ptr()); + } + return OK; } @@ -497,11 +513,16 @@ __PRIVILEGED_CODE void die_from_signal(uint32_t sig) { // A dying non-leader force-kills the leader, whose exit reaps // every remaining thread + rc::strong_ref leader; sync::irq_state irq = sync::spin_lock_irqsave(tg->lock); if (tg->leader && tg->leader != self) { - sched::force_wake_for_kill(tg->leader); + leader = sched::task_ref(tg->leader); } sync::spin_unlock_irqrestore(tg->lock, irq); + + if (leader) { + sched::force_wake_for_kill(leader.ptr()); + } } // The SIGKILL bit makes exit() encode a killed-by-signal wait status diff --git a/kernel/syscall/handlers/sys_task.cpp b/kernel/syscall/handlers/sys_task.cpp index 78caf59e..50ee6c2a 100644 --- a/kernel/syscall/handlers/sys_task.cpp +++ b/kernel/syscall/handlers/sys_task.cpp @@ -225,13 +225,19 @@ DEFINE_SYSCALL1(exit_group, status) { // A non leader forces the leader down, the leader's exit then // reaps every remaining thread including this one + rc::strong_ref leader; + sync::irq_state irq = sync::spin_lock_irqsave(tg->lock); if (tg->leader && tg->leader != self) { - sched::force_wake_for_kill(tg->leader); + leader = sched::task_ref(tg->leader); } sync::spin_unlock_irqrestore(tg->lock, irq); + + if (leader) { + sched::force_wake_for_kill(leader.ptr()); + } } sched::exit(static_cast(status)); From 4eab48aa2a3b2f44581be5a4c3104848fa6dc0b2 Mon Sep 17 00:00:00 2001 From: FlareCoding Date: Fri, 28 Aug 2026 09:02:12 -0700 Subject: [PATCH 06/12] fix(signals): kept fatal signals from killing tasks inside a syscall --- kernel/arch/aarch64/sched/sched.cpp | 10 ++++++++-- kernel/arch/x86_64/sched/sched.cpp | 10 ++++++++-- 2 files changed, 16 insertions(+), 4 deletions(-) diff --git a/kernel/arch/aarch64/sched/sched.cpp b/kernel/arch/aarch64/sched/sched.cpp index 8dc1c4cb..5ec3a610 100644 --- a/kernel/arch/aarch64/sched/sched.cpp +++ b/kernel/arch/aarch64/sched/sched.cpp @@ -191,7 +191,10 @@ __PRIVILEGED_CODE void on_yield(aarch64::trap_frame* tf) { save_cpu_context(tf, &prev->exec.cpu_ctx); prev->exec.tls_base = cpu::read_tls_base(); - if (!(prev->exec.flags & TASK_FLAG_KERNEL) && prev->state.load_relaxed() != TASK_STATE_DEAD) { + // A task inside a syscall still owns kernel state such as a linked wait + // node, so it dies at the syscall exit fatal check instead of here + if (!(prev->exec.flags & (TASK_FLAG_KERNEL | TASK_FLAG_IN_SYSCALL)) && + prev->state.load_relaxed() != TASK_STATE_DEAD) { uint32_t fsig = signals::fatal_pending(prev); if (fsig) { signals::die_from_signal(fsig); @@ -243,7 +246,10 @@ __PRIVILEGED_CODE void on_tick(aarch64::trap_frame* tf) { save_cpu_context(tf, &prev->exec.cpu_ctx); prev->exec.tls_base = cpu::read_tls_base(); - if (!(prev->exec.flags & TASK_FLAG_KERNEL) && prev->state.load_relaxed() != TASK_STATE_DEAD) { + // A task inside a syscall still owns kernel state such as a linked wait + // node, so it dies at the syscall exit fatal check instead of here + if (!(prev->exec.flags & (TASK_FLAG_KERNEL | TASK_FLAG_IN_SYSCALL)) && + prev->state.load_relaxed() != TASK_STATE_DEAD) { uint32_t fsig = signals::fatal_pending(prev); if (fsig) { signals::die_from_signal(fsig); diff --git a/kernel/arch/x86_64/sched/sched.cpp b/kernel/arch/x86_64/sched/sched.cpp index 3c17b070..f4aac94e 100644 --- a/kernel/arch/x86_64/sched/sched.cpp +++ b/kernel/arch/x86_64/sched/sched.cpp @@ -179,7 +179,10 @@ __PRIVILEGED_CODE void on_yield(x86::trap_frame* tf) { save_cpu_context(tf, &prev->exec.cpu_ctx); prev->exec.tls_base = cpu::read_tls_base(); - if (!(prev->exec.flags & TASK_FLAG_KERNEL) && prev->state.load_relaxed() != TASK_STATE_DEAD) { + // A task inside a syscall still owns kernel state such as a linked wait + // node, so it dies at the syscall exit fatal check instead of here + if (!(prev->exec.flags & (TASK_FLAG_KERNEL | TASK_FLAG_IN_SYSCALL)) && + prev->state.load_relaxed() != TASK_STATE_DEAD) { uint32_t fsig = signals::fatal_pending(prev); if (fsig) { signals::die_from_signal(fsig); @@ -230,7 +233,10 @@ __PRIVILEGED_CODE void on_tick(x86::trap_frame* tf) { save_cpu_context(tf, &prev->exec.cpu_ctx); prev->exec.tls_base = cpu::read_tls_base(); - if (!(prev->exec.flags & TASK_FLAG_KERNEL) && prev->state.load_relaxed() != TASK_STATE_DEAD) { + // A task inside a syscall still owns kernel state such as a linked wait + // node, so it dies at the syscall exit fatal check instead of here + if (!(prev->exec.flags & (TASK_FLAG_KERNEL | TASK_FLAG_IN_SYSCALL)) && + prev->state.load_relaxed() != TASK_STATE_DEAD) { uint32_t fsig = signals::fatal_pending(prev); if (fsig) { signals::die_from_signal(fsig); From 143c932cf6b15e8407411f289fc87f2267e2d35c Mon Sep 17 00:00:00 2001 From: FlareCoding Date: Fri, 28 Aug 2026 09:02:54 -0700 Subject: [PATCH 07/12] fix(signals): sent signals through counted task references --- kernel/sched/sched.cpp | 11 +++++++ kernel/sched/sched.h | 9 ++++++ kernel/signals/signal.cpp | 18 ++++++++--- kernel/syscall/handlers/sys_signal.cpp | 42 +++++++++++--------------- kernel/tests/sched/kill.test.cpp | 8 +++++ kernel/tests/sync/futex.test.cpp | 2 ++ kernel/tests/sync/poll.test.cpp | 2 ++ 7 files changed, 63 insertions(+), 29 deletions(-) diff --git a/kernel/sched/sched.cpp b/kernel/sched/sched.cpp index bab0ec27..56c26195 100644 --- a/kernel/sched/sched.cpp +++ b/kernel/sched/sched.cpp @@ -436,6 +436,17 @@ __PRIVILEGED_CODE rc::strong_ref task_ref(task* t) { return rc::strong_ref::try_from_raw(t); } +/** + * @note Privilege: **required** + */ +__PRIVILEGED_CODE rc::strong_ref task_ref_by_tid(uint32_t tid) { + sync::irq_state irq = g_task_registry.lock(); + rc::strong_ref ref = task_ref(g_task_registry.find_locked(tid)); + g_task_registry.unlock(irq); + + return ref; +} + /** * @note Privilege: **required** */ diff --git a/kernel/sched/sched.h b/kernel/sched/sched.h index 6302458c..e0a5229a 100644 --- a/kernel/sched/sched.h +++ b/kernel/sched/sched.h @@ -171,6 +171,15 @@ __PRIVILEGED_CODE void force_wake_for_kill(task* t); */ [[nodiscard]] __PRIVILEGED_CODE rc::strong_ref task_ref(task* t); +/** + * @brief Acquire a counted reference to the task with the given tid. + * Takes the registry lock internally, so no caller-side pin is needed. + * Returns a null reference if no task with that tid is registered or if + * the task is already tearing down. + * @note Privilege: **required** + */ +[[nodiscard]] __PRIVILEGED_CODE rc::strong_ref task_ref_by_tid(uint32_t tid); + /** * @brief Publish intent to block: moves the current task to BLOCKED. * Pair with block_task_interrupted before yielding. diff --git a/kernel/signals/signal.cpp b/kernel/signals/signal.cpp index f84378c0..28a53133 100644 --- a/kernel/signals/signal.cpp +++ b/kernel/signals/signal.cpp @@ -304,16 +304,26 @@ __PRIVILEGED_CODE int32_t send_to_group_id(uint32_t group_id, uint32_t sig) { return; } } + + found = true; if (seen_count < MAX_GROUP_SEND_GROUPS) { + tg->add_ref(); seen[seen_count++] = tg; } + }); + sched::g_task_registry.unlock(irq); - found = true; + // Sends run after the registry lock drops, pinned by the references + // above. Groups past the array bound are dropped. + for (uint32_t i = 0; i < seen_count; i++) { if (sig != 0) { - send_to_group(tg, sig); + send_to_group(seen[i], sig); } - }); - sched::g_task_registry.unlock(irq); + + if (seen[i]->release()) { + sched::thread_group::ref_destroy(seen[i]); + } + } return found ? OK : ERR_INVAL; } diff --git a/kernel/syscall/handlers/sys_signal.cpp b/kernel/syscall/handlers/sys_signal.cpp index 0a23f7a6..e1c35935 100644 --- a/kernel/syscall/handlers/sys_signal.cpp +++ b/kernel/syscall/handlers/sys_signal.cpp @@ -28,24 +28,20 @@ static int64_t kill_process_group(uint32_t group_id, uint32_t sig) { // Thread-directed send shared by tkill and tgkill, tgid 0 skips the pair check static int64_t send_to_thread(uint32_t tgid, uint32_t tid, uint32_t sig) { - int64_t result = syscall::ESRCH; - - sync::irq_state irq = sched::g_task_registry.lock(); - sched::task* t = sched::g_task_registry.find_locked(tid); - if (t && (tgid == 0 || (t->group && t->group->pid == tgid))) { - if (sig != 0) { - result = map_send_error(signals::send_to_task(t, sig)); - } else { - // The null probe reports the same permission gate a send would - bool denied = (t->exec.flags & - (sched::TASK_FLAG_KERNEL | sched::TASK_FLAG_IDLE)) || - !t->group; - result = denied ? syscall::EPERM : 0; - } + rc::strong_ref t = sched::task_ref_by_tid(tid); + if (!t || (tgid != 0 && (!t->group || t->group->pid != tgid))) { + return syscall::ESRCH; + } + + if (sig == 0) { + // The null probe reports the same permission gate a send would + bool denied = (t->exec.flags & + (sched::TASK_FLAG_KERNEL | sched::TASK_FLAG_IDLE)) || + !t->group; + return denied ? syscall::EPERM : 0; } - sched::g_task_registry.unlock(irq); - return result; + return map_send_error(signals::send_to_task(t.ptr(), sig)); } DEFINE_SYSCALL4(rt_sigaction, signum, u_act, u_oldact, sigsetsize) { @@ -172,17 +168,13 @@ DEFINE_SYSCALL2(kill, u_pid, u_sig) { // Any thread id resolves to its containing process (kill semantics // on Linux), and the signal is delivered process-wide - int64_t result = syscall::ESRCH; - sync::irq_state irq = sched::g_task_registry.lock(); - sched::task* t = - sched::g_task_registry.find_locked(static_cast(pid)); - if (t && t->group) { - result = sig ? map_send_error(signals::send_to_group(t->group, sig)) - : 0; + rc::strong_ref t = + sched::task_ref_by_tid(static_cast(pid)); + if (!t || !t->group) { + return syscall::ESRCH; } - sched::g_task_registry.unlock(irq); - return result; + return sig ? map_send_error(signals::send_to_group(t->group, sig)) : 0; } // pid 0 targets the caller's group, below -1 the group named by -pid diff --git a/kernel/tests/sched/kill.test.cpp b/kernel/tests/sched/kill.test.cpp index e6a06634..d87419fb 100644 --- a/kernel/tests/sched/kill.test.cpp +++ b/kernel/tests/sched/kill.test.cpp @@ -56,9 +56,11 @@ TEST(kill, force_wake_kills_sleeping_task) { g_sleep_kill_elapsed_ns.store_relaxed(0); sched::task* t = nullptr; + rc::strong_ref pin; RUN_ELEVATED({ t = sched::create_kernel_task(sleep_kill_fn, nullptr, "kill_sleep"); ASSERT_NOT_NULL(t); + pin = sched::task_ref(t); sched::enqueue(t); }); @@ -133,9 +135,11 @@ TEST(kill, force_wake_kills_blocked_on_wq) { g_wq_kill_was_pending.store_relaxed(0); sched::task* t = nullptr; + rc::strong_ref pin; RUN_ELEVATED({ t = sched::create_kernel_task(wq_kill_fn, nullptr, "kill_wq"); ASSERT_NOT_NULL(t); + pin = sched::task_ref(t); sched::enqueue(t); }); @@ -178,9 +182,11 @@ TEST(kill, self_removal_cleans_wq) { g_sr_done.store_relaxed(0); sched::task* t = nullptr; + rc::strong_ref pin; RUN_ELEVATED({ t = sched::create_kernel_task(sr_waiter_fn, nullptr, "kill_sr"); ASSERT_NOT_NULL(t); + pin = sched::task_ref(t); sched::enqueue(t); }); @@ -226,9 +232,11 @@ TEST(kill, double_kill_is_harmless) { g_double_kp.store_relaxed(0); sched::task* t = nullptr; + rc::strong_ref pin; RUN_ELEVATED({ t = sched::create_kernel_task(double_kill_fn, nullptr, "kill_dbl"); ASSERT_NOT_NULL(t); + pin = sched::task_ref(t); sched::enqueue(t); }); diff --git a/kernel/tests/sync/futex.test.cpp b/kernel/tests/sync/futex.test.cpp index cb46e007..e555b54f 100644 --- a/kernel/tests/sync/futex.test.cpp +++ b/kernel/tests/sync/futex.test.cpp @@ -195,10 +195,12 @@ TEST(futex, killed_thread_unblocks) { g_kill_entered.store_relaxed(0); g_kill_task = nullptr; + rc::strong_ref pin; RUN_ELEVATED({ g_kill_task = sched::create_kernel_task( kill_waiter_fn, nullptr, "ftx_kill"); ASSERT_NOT_NULL(g_kill_task); + pin = sched::task_ref(g_kill_task); sched::enqueue(g_kill_task); }); diff --git a/kernel/tests/sync/poll.test.cpp b/kernel/tests/sync/poll.test.cpp index 0f964773..90fee483 100644 --- a/kernel/tests/sync/poll.test.cpp +++ b/kernel/tests/sync/poll.test.cpp @@ -373,9 +373,11 @@ TEST(poll, kill_pending_wakes_poll) { g_kill_done.store_relaxed(0); sched::task* t = nullptr; + rc::strong_ref pin; RUN_ELEVATED({ t = sched::create_kernel_task(kill_poll_fn, nullptr, "poll_kill"); ASSERT_NOT_NULL(t); + pin = sched::task_ref(t); sched::enqueue(t); }); From 1bf69d134b5cbc5a82f2c8e578312c4428a0116f Mon Sep 17 00:00:00 2001 From: FlareCoding Date: Fri, 28 Aug 2026 09:09:29 -0700 Subject: [PATCH 08/12] docs(sched): documented the task state machine and reclamation mechanism --- kernel/sched/sched.cpp | 3 +++ kernel/sched/sched_internal.h | 3 ++- kernel/sched/task.h | 28 +++++++++++++++++++++++++++- 3 files changed, 32 insertions(+), 2 deletions(-) diff --git a/kernel/sched/sched.cpp b/kernel/sched/sched.cpp index 56c26195..4918b427 100644 --- a/kernel/sched/sched.cpp +++ b/kernel/sched/sched.cpp @@ -104,6 +104,9 @@ __PRIVILEGED_CODE static inline void assert_switch_privilege_state( #endif /** + * Runs only after the last counted reference dropped. Registry lookups can + * still find the task until the removal below, its poisoned refcount turns + * them away. The TLB sync wait covers the freed stack pages, nothing else. * @note Privilege: **required** */ __PRIVILEGED_CODE static rc::reaper::cleanup_result reap_task(sched::task* t) { diff --git a/kernel/sched/sched_internal.h b/kernel/sched/sched_internal.h index ffaa5596..2d64b91b 100644 --- a/kernel/sched/sched_internal.h +++ b/kernel/sched/sched_internal.h @@ -66,7 +66,8 @@ __PRIVILEGED_CODE void defer_off_cpu_finalize(task* prev); /** * Common: advances this CPU's TLB sync epoch. - * This marks a safe point that reaper can rely on before reclaiming stack pages. + * This marks a safe point that reaper can rely on before reclaiming stack + * pages. It says nothing about task pointers, which counted references pin. * @note Privilege: **required** */ __PRIVILEGED_CODE void advance_cpu_tlb_sync_epoch(); diff --git a/kernel/sched/task.h b/kernel/sched/task.h index bb81cc94..0200c527 100644 --- a/kernel/sched/task.h +++ b/kernel/sched/task.h @@ -18,6 +18,22 @@ namespace sched { constexpr size_t TASK_NAME_MAX = 256; +/** + * Task states and the legal transitions between them: + * + * CREATED -> READY enqueue / enqueue_on (CAS) + * CREATED -> DEAD unstarted task teardown claims it (CAS) + * READY -> RUNNING pick_next_and_switch + * RUNNING -> READY preemption re-enqueue + * RUNNING -> BLOCKED prepare_to_block_task, by the task itself + * RUNNING -> DEAD exit, by the task itself + * BLOCKED -> READY wake (CAS) + * BLOCKED -> RUNNING cancel_block_task, by the task itself (CAS) + * + * A task is BLOCKED yet still on-CPU between prepare_to_block_task and + * its yield. A tick in that window switches it out without re-enqueueing + * it, so it stays off the runqueues until a wake arrives. + */ constexpr uint32_t TASK_STATE_CREATED = 0; // exists but not on any queue constexpr uint32_t TASK_STATE_READY = 1; // on a runqueue constexpr uint32_t TASK_STATE_RUNNING = 2; // executing on a CPU @@ -26,6 +42,14 @@ constexpr uint32_t TASK_STATE_DEAD = 4; // terminated constexpr int32_t TASK_KILL_STATUS = 9; // wait-status for forcibly killed tasks +/** + * Reclamation ladder. exit() records EXIT_REQUESTED on the dying task, the + * scheduler advances to SCHEDULER_DETACHED when the task is switched out + * (unstarted teardown jumps there directly), and the reaper owns the last + * two stages: it snapshots every CPU's TLB sync epoch, waits for each CPU + * to move past its snapshot, then reclaims. The struct itself is freed only + * after the last counted reference has dropped and handed it to the reaper. + */ constexpr uint32_t TASK_CLEANUP_STAGE_ACTIVE = 0; constexpr uint32_t TASK_CLEANUP_STAGE_EXIT_REQUESTED = 1; constexpr uint32_t TASK_CLEANUP_STAGE_SCHEDULER_DETACHED = 2; @@ -36,7 +60,9 @@ constexpr uint32_t TASK_CLEANUP_STAGE_READY_TO_RECLAIM = 4; * Per-task TLB sync ticket used by reaper before reclaiming task stacks. * * The ticket snapshots each CPU's reclaim epoch and requires every CPU to - * advance past that snapshot before stack unmap/free can proceed. + * advance past that snapshot before stack unmap/free can proceed. It only + * retires stale TLB entries for the freed stacks, keeping the task struct + * itself alive is the job of its counted references. */ struct task_tlb_sync_ticket { uint64_t cpu_epoch_snapshot[MAX_CPUS]; From 9a02c813dd009f5e847f5601789f421437cd3bfd Mon Sep 17 00:00:00 2001 From: Albert Slepak Date: Fri, 28 Aug 2026 11:10:29 -0700 Subject: [PATCH 09/12] fix(signals): delivered group-id signals to every matching process group A group-id send collected matching groups into a fixed 64-entry batch and silently skipped the rest while still reporting success, so a kill of a large process group left some of its processes running. Each registry pass now sends to the group with the smallest leader pid above a cursor, visiting every matching group exactly once however many exist. Leader pids are allocated monotonically and never reused, which makes the cursor a stable iteration order. --- kernel/signals/signal.cpp | 54 ++++++++--------- kernel/tests/signals/signal_send.test.cpp | 71 +++++++++++++++++++++++ 2 files changed, 96 insertions(+), 29 deletions(-) diff --git a/kernel/signals/signal.cpp b/kernel/signals/signal.cpp index 28a53133..3b8d1134 100644 --- a/kernel/signals/signal.cpp +++ b/kernel/signals/signal.cpp @@ -15,10 +15,6 @@ enum class send_verdict : uint8_t { HANDLED, // a user handler is installed, wake the target to deliver }; -// Distinct groups remembered during one group-id send. Signals coalesce, -// so duplicate sends past this window are harmless extra wakes. -constexpr uint32_t MAX_GROUP_SEND_GROUPS = 64; - /** * Clear pending instances of sig from the shared set and every thread. * @note Privilege: **required** @@ -288,40 +284,40 @@ __PRIVILEGED_CODE int32_t send_to_group(sched::thread_group* tg, uint32_t sig) { } __PRIVILEGED_CODE int32_t send_to_group_id(uint32_t group_id, uint32_t sig) { - sched::thread_group* seen[MAX_GROUP_SEND_GROUPS]; - uint32_t seen_count = 0; bool found = false; + uint32_t cursor = 0; - sync::irq_state irq = sched::g_task_registry.lock(); - sched::g_task_registry.for_each_locked([&](sched::task& t) { - sched::thread_group* tg = t.group; - if (!tg || tg->group_id.load_acquire() != group_id) { - return; - } + // Each pass sends to the matching group with the smallest leader pid + // above the cursor, reaching every group however many match + for (;;) { + sched::thread_group* best = nullptr; - for (uint32_t i = 0; i < seen_count; i++) { - if (seen[i] == tg) { + sync::irq_state irq = sched::g_task_registry.lock(); + sched::g_task_registry.for_each_locked([&](sched::task& t) { + sched::thread_group* tg = t.group; + if (!tg || tg->group_id.load_acquire() != group_id) { return; } - } - found = true; - if (seen_count < MAX_GROUP_SEND_GROUPS) { - tg->add_ref(); - seen[seen_count++] = tg; - } - }); - sched::g_task_registry.unlock(irq); + if (tg->pid > cursor && (!best || tg->pid < best->pid)) { + best = tg; + } + }); - // Sends run after the registry lock drops, pinned by the references - // above. Groups past the array bound are dropped. - for (uint32_t i = 0; i < seen_count; i++) { - if (sig != 0) { - send_to_group(seen[i], sig); + // A registry member pins its group until reaped, so the reference + // is taken under the registry lock and the send runs after it drops + rc::strong_ref target = + rc::strong_ref::try_from_raw(best); + sched::g_task_registry.unlock(irq); + + if (!target) { + break; } - if (seen[i]->release()) { - sched::thread_group::ref_destroy(seen[i]); + found = true; + cursor = target->pid; + if (sig != 0) { + send_to_group(target.ptr(), sig); } } diff --git a/kernel/tests/signals/signal_send.test.cpp b/kernel/tests/signals/signal_send.test.cpp index 18f42b55..1c06ad53 100644 --- a/kernel/tests/signals/signal_send.test.cpp +++ b/kernel/tests/signals/signal_send.test.cpp @@ -3,6 +3,7 @@ #include "stlx_unit_test.h" #include "signals/signal.h" #include "sched/task.h" +#include "sched/task_registry.h" #include "mm/heap.h" #include "dynpriv/dynpriv.h" @@ -160,3 +161,73 @@ TEST(signal_send, stop_class_send_is_ignored) { EXPECT_EQ(rc, signals::OK); EXPECT_EQ(g_thread->sig.pending.load_relaxed(), 0ULL); } + +// A group-id send must reach every matching group, not only the ones +// that fit a fixed window. Sized past the 64-group batch an earlier +// implementation silently dropped. +constexpr uint32_t GID_GROUP_COUNT = 67; +constexpr uint32_t GID_TEST_GROUP = 0x7E577E57u; +constexpr uint32_t GID_TID_BASE = 0x40000000u; + +static sched::task* g_gid_leaders[GID_GROUP_COUNT]; +static sched::thread_group* g_gid_groups[GID_GROUP_COUNT]; + +TEST(signal_send, group_id_send_reaches_all_groups) { + bool built = true; + + // Single-task mock processes registered in the live task registry, + // all members of the same process group + RUN_ELEVATED({ + for (uint32_t i = 0; i < GID_GROUP_COUNT; i++) { + g_gid_leaders[i] = heap::kalloc_new(); + g_gid_groups[i] = heap::kalloc_new(); + if (!g_gid_leaders[i] || !g_gid_groups[i]) { + built = false; + break; + } + + sched::task* t = g_gid_leaders[i]; + sched::thread_group* tg = g_gid_groups[i]; + tg->lock = sync::SPINLOCK_INIT; + tg->leader = t; + tg->pid = GID_TID_BASE + i; + tg->threads.init(); + tg->group_id.store_relaxed(GID_TEST_GROUP); + + t->tid = GID_TID_BASE + i; + t->group = tg; + sched::g_task_registry.insert(t); + } + }); + + int32_t rc = -1; + if (built) { + RUN_ELEVATED({ + rc = signals::send_to_group_id(GID_TEST_GROUP, signals::SIGTERM); + }); + } + + EXPECT_TRUE(built); + EXPECT_EQ(rc, signals::OK); + + uint32_t reached = 0; + for (uint32_t i = 0; i < GID_GROUP_COUNT; i++) { + if (g_gid_groups[i] && (g_gid_groups[i]->sig.shared_pending.load_relaxed() + & signals::sig_bit(signals::SIGTERM))) { + reached++; + } + } + EXPECT_EQ(reached, GID_GROUP_COUNT); + + RUN_ELEVATED({ + for (uint32_t i = 0; i < GID_GROUP_COUNT; i++) { + if (g_gid_leaders[i] && g_gid_groups[i]) { + sched::g_task_registry.remove(*g_gid_leaders[i]); + } + if (g_gid_groups[i]) heap::kfree_delete(g_gid_groups[i]); + if (g_gid_leaders[i]) heap::kfree_delete(g_gid_leaders[i]); + g_gid_groups[i] = nullptr; + g_gid_leaders[i] = nullptr; + } + }); +} From cc32954c53fd009dea14efd2fbf91f0f83304d42 Mon Sep 17 00:00:00 2001 From: Albert Slepak Date: Fri, 28 Aug 2026 11:14:55 -0700 Subject: [PATCH 10/12] fix(proc): released the proc resource reference only after claiming an unstarted task Closing a process handle released the child's proc resource reference before claiming the task, while a concurrent group teardown claiming the same CREATED task released it again after winning the state CAS, dropping the reference twice. The CAS is the mutual exclusion for unstarted teardown, so the release now lives behind it and only the winning path touches the reference. Every other destroy_unstarted_task caller runs before a proc resource exists, so the move changes nothing for them. --- kernel/resource/providers/proc_provider.cpp | 14 +++++++++----- kernel/resource/providers/proc_provider.h | 3 ++- 2 files changed, 11 insertions(+), 6 deletions(-) diff --git a/kernel/resource/providers/proc_provider.cpp b/kernel/resource/providers/proc_provider.cpp index 4afc4d23..52823f6c 100644 --- a/kernel/resource/providers/proc_provider.cpp +++ b/kernel/resource/providers/proc_provider.cpp @@ -55,11 +55,6 @@ __PRIVILEGED_CODE static void proc_close(resource_object* obj) { sync::spin_unlock_irqrestore(pr->lock, irq); if (child && unstarted) { - if (child->proc_res) { - (void)child->proc_res->release(); - child->proc_res = nullptr; - } - destroy_unstarted_task(child.ptr()); } else if (child) { sched::force_wake_for_kill(child.ptr()); @@ -176,6 +171,15 @@ __PRIVILEGED_CODE void destroy_unstarted_task(sched::task* t) { return; } + // Winning the claim confers sole ownership of the task's proc + // resource reference, a losing teardown path must not touch it + if (t->proc_res) { + if (t->proc_res->release()) { + proc_resource::ref_destroy(t->proc_res); + } + t->proc_res = nullptr; + } + // Unlink from the group list here, reap_task releases the group // reference but never touches the thread list if (t->group && t->group->leader != t && t->group_link.is_linked()) { diff --git a/kernel/resource/providers/proc_provider.h b/kernel/resource/providers/proc_provider.h index 440592a5..34c7e562 100644 --- a/kernel/resource/providers/proc_provider.h +++ b/kernel/resource/providers/proc_provider.h @@ -47,7 +47,8 @@ __PRIVILEGED_CODE int32_t create_proc_resource( /** * @brief Destroy a task that was created but never started (TASK_STATE_CREATED). - * Frees mm_ctx, system stack, and the task struct. Does NOT release proc_res ref. + * Claims the task against concurrent group teardown, drops its proc + * resource reference, and defers reclamation to the reaper. * @note Privilege: **required** */ __PRIVILEGED_CODE void destroy_unstarted_task(sched::task* t); From 136bec424bfe7ccc49bd689a1a2b91f7807506b1 Mon Sep 17 00:00:00 2001 From: Albert Slepak Date: Fri, 28 Aug 2026 11:17:04 -0700 Subject: [PATCH 11/12] docs(timer): documented why the sleep-queue expiry walk may wake under its lock The wake pin contract forbids holding an irqsave spinlock across a wake because of the remote off-CPU spin, yet the timer expiry walk does exactly that and is correct. Sleepers always re-take the queue lock in cancel_sleep before they can exit, and a task sleeps on the CPU whose queue holds it, so an expiry wake never spins. Recording the argument keeps the walk from being either miscorrected or copied into a context where it does not hold. --- kernel/arch/aarch64/timer/timer.cpp | 3 +++ kernel/arch/x86_64/timer/timer.cpp | 3 +++ kernel/sched/sched.h | 3 ++- 3 files changed, 8 insertions(+), 1 deletion(-) diff --git a/kernel/arch/aarch64/timer/timer.cpp b/kernel/arch/aarch64/timer/timer.cpp index a7c14762..0843a28e 100644 --- a/kernel/arch/aarch64/timer/timer.cpp +++ b/kernel/arch/aarch64/timer/timer.cpp @@ -157,6 +157,9 @@ __PRIVILEGED_CODE bool on_interrupt() { return true; } + // Waking raw pointers under the queue lock satisfies the wake pin + // contract: a sleeper re-takes this lock in cancel_sleep before it + // can exit, and it slept on this CPU, so wake never spins off-CPU while (!state.sleep_queue.empty()) { sched::task* t = state.sleep_queue.front(); if (t->timer_deadline > now) break; diff --git a/kernel/arch/x86_64/timer/timer.cpp b/kernel/arch/x86_64/timer/timer.cpp index 2615ad71..0414e7a2 100644 --- a/kernel/arch/x86_64/timer/timer.cpp +++ b/kernel/arch/x86_64/timer/timer.cpp @@ -223,6 +223,9 @@ __PRIVILEGED_CODE bool on_interrupt() { return true; } + // Waking raw pointers under the queue lock satisfies the wake pin + // contract: a sleeper re-takes this lock in cancel_sleep before it + // can exit, and it slept on this CPU, so wake never spins off-CPU while (!state.sleep_queue.empty()) { sched::task* t = state.sleep_queue.front(); if (t->timer_deadline > now) break; diff --git a/kernel/sched/sched.h b/kernel/sched/sched.h index e0a5229a..9ccd2049 100644 --- a/kernel/sched/sched.h +++ b/kernel/sched/sched.h @@ -148,7 +148,8 @@ __PRIVILEGED_CODE void enqueue_on(task* t, uint32_t cpu_id); * The caller must pin t so the reaper cannot free it mid-call: hold a * counted reference (task_ref) or a lock t must take before it can exit. * A remote wake spins until t leaves its CPU, so never hold a spinlock - * with interrupts off across the call. + * with interrupts off across the call. Waking a task that last ran on + * the calling CPU never spins, so the timer expiry walk is exempt. * @note Privilege: **required** */ __PRIVILEGED_CODE void wake(task* t); From 499ab981f134d01365a1dca20515d93b6c4682a9 Mon Sep 17 00:00:00 2001 From: Albert Slepak Date: Fri, 28 Aug 2026 11:19:26 -0700 Subject: [PATCH 12/12] chore(sched): moved the teardown proc-resource batch onto counted references The leader teardown batch carried raw proc_resource pointers with hand-rolled release and destroy calls, the one remaining wake batch not expressed through strong_ref. Adopting the thread's reference into the batch lets scope handle the release and keeps every deferred-wake batch on the same ownership idiom. --- kernel/rc/strong_ref.h | 4 ++-- kernel/sched/sched.cpp | 7 ++----- 2 files changed, 4 insertions(+), 7 deletions(-) diff --git a/kernel/rc/strong_ref.h b/kernel/rc/strong_ref.h index 7222401c..15be76e8 100644 --- a/kernel/rc/strong_ref.h +++ b/kernel/rc/strong_ref.h @@ -79,8 +79,8 @@ class strong_ref { } /** - * Wrap a raw pointer whose refcount is already 1 (from allocation). - * Does NOT call add_ref. + * Wrap a raw pointer, taking ownership of one already-held reference, + * as when adopting a freshly allocated object. Does NOT call add_ref. */ [[nodiscard]] static strong_ref adopt(T* raw) noexcept { return strong_ref(raw, ADOPT_REF); diff --git a/kernel/sched/sched.cpp b/kernel/sched/sched.cpp index 4918b427..2e33d3e2 100644 --- a/kernel/sched/sched.cpp +++ b/kernel/sched/sched.cpp @@ -554,7 +554,7 @@ __PRIVILEGED_CODE void sleep_ms(uint64_t ms) { // it drops, keeping the off-CPU spin in wake outside the lock for (;;) { rc::strong_ref kill_batch[TEARDOWN_BATCH_SIZE]; - resource::proc_provider::proc_resource* pr_batch[TEARDOWN_BATCH_SIZE]; + rc::strong_ref pr_batch[TEARDOWN_BATCH_SIZE]; uint32_t kills = 0; uint32_t prs = 0; bool rescan = false; @@ -590,7 +590,7 @@ __PRIVILEGED_CODE void sleep_ms(uint64_t ms) { // The thread's resource reference moves to the // batch and is released after the deferred wake thread.proc_res = nullptr; - pr_batch[prs++] = pr; + pr_batch[prs++] = rc::strong_ref::adopt(pr); } store_cleanup_stage(&thread, TASK_CLEANUP_STAGE_SCHEDULER_DETACHED); @@ -615,9 +615,6 @@ __PRIVILEGED_CODE void sleep_ms(uint64_t ms) { for (uint32_t i = 0; i < prs; i++) { sync::wake_all(pr_batch[i]->wait_queue); - if (pr_batch[i]->release()) { - resource::proc_provider::proc_resource::ref_destroy(pr_batch[i]); - } } for (uint32_t i = 0; i < kills; i++) {