From c8191570e18c65269f8b6b2b196ac6af74bdd391 Mon Sep 17 00:00:00 2001 From: Daniel Samson <12231216+daniel-samson@users.noreply.github.com> Date: Wed, 22 Jul 2026 11:23:32 +0100 Subject: [PATCH] =?UTF-8?q?kernel:=20shared-fate=20review=20fixes=20?= =?UTF-8?q?=E2=80=94=20lock=20the=20shm/dma=20walks,=20contract=20edges?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The adversarial review of the branch confirmed the big one: the shm/DMA page-table walks and their pmm/heap calls ran outside the big kernel lock — pre-existing, but fatal once the per-space cursors invited sibling threads to race them (two concurrent creates could orphan a page table: one thread's region silently unmapped, the frame leaked — a plausible root for the long-standing intermittent AP ring-3 fault at the shm base). All three paths now follow the mmap discipline: allocation, object build, record, and handle under one lock hold with full rollback; the map itself per-page under brief holds; dma_free's translate/unmap/free per-page likewise. Contract edges from the same review: thread_spawn into a dying group returns -ESRCH (was generic -1); process_kill during the condemned window answers from the latch's stashed supervisor (0 or -EPERM, was -ESRCH once the leader slot was reaped); exit derives the group reason from its own argument rather than the racy exit_code global; a worker's thread_exit no longer overwrites a concurrent group-kill stamp; checkGroupDead now asserts exactly-one notification via the drained ring. Full suite: 100/100. --- docs/shared-fate-plan.md | 17 +++- system/kernel/process.zig | 157 ++++++++++++++++++++++++------------ system/kernel/scheduler.zig | 28 +++++++ system/kernel/tests.zig | 19 +++-- 4 files changed, 160 insertions(+), 61 deletions(-) diff --git a/docs/shared-fate-plan.md b/docs/shared-fate-plan.md index 564e1c1..c8631f7 100644 --- a/docs/shared-fate-plan.md +++ b/docs/shared-fate-plan.md @@ -251,7 +251,22 @@ refcount, and no group-kill special case is needed at all. - **Per-task DMA/shm cursors** — *fixed during M4 after all*: the `shm-mapping-ref` test tripped the overlap (the sibling's churn regions mapped over the worker's region), so both cursors moved to the `AddressSpaceRef` - like the mmap/MMIO cursors before them. + like the mmap/MMIO cursors before them. The post-implementation review then + found the other half: the shm/DMA page-table walks and their pmm/heap calls + ran *outside* the big kernel lock — pre-existing, but fatal once siblings + were invited to race them (and a plausible root for the long-standing + intermittent AP ring-3 fault at the shm base). All three paths now follow + the mmap discipline: metadata and allocation under one hold, the map itself + per-page under brief holds. +- **Mapping-record slots are never recycled**: 16 per space, one per + `shared_memory_create`/`map`, freed only at space destruction (there is no + shm unmap). A long-lived compositor that churns surfaces will hit the cap; + the failure is a clean refused create, and slot recycling can ride whatever + adds `shared_memory_unmap`. +- **Two properties lack direct tests**: the spawn gate (an in-flight + `thread_spawn` racing the fan-out — inherently nondeterministic to arrange; + covered by code inspection and the `-ESRCH` path) and the `process_signal` + leader re-key (exercised only implicitly by the signals case). ## Milestones diff --git a/system/kernel/process.zig b/system/kernel/process.zig index d627e6b..bff36a2 100644 --- a/system/kernel/process.zig +++ b/system/kernel/process.zig @@ -201,7 +201,10 @@ fn system_call(state: *architecture.CpuState) void { // restart those, unlike a clean `.exited` ("nothing for me here"), // which they let lie. if (scheduler.currentIsUserProcess()) { - exitGroupCurrent(if (exit_code == 0) .exited else .aborted); + // The reason derives from this call's own argument (a local), not + // the shared exit_code global — two racing exits must not decide + // each other's group reason. + exitGroupCurrent(if (architecture.systemCallArg(state, 0) == 0) .exited else .aborted); } else architecture.userExit(); }, .yield => { @@ -268,7 +271,10 @@ fn system_call(state: *architecture.CpuState) void { const dying = scheduler.current(); if (dying.id == dying.leader) return failErr(state, ipc.EPERM); _ = sync.enter(); // handed off through the exit switch - dying.exit_reason = .exited; + // A group kill may have stamped us .killed while we raced to + // this dispatch (past the entry check, spinning on the lock) — + // keep that stamp; the fan-out's story wins. + if (!dying.kill_pending.load(.monotonic)) dying.exit_reason = .exited; terminateCurrentLocked(); } else architecture.userExit(); }, @@ -499,33 +505,44 @@ fn systemDmaAlloc(state: *architecture.CpuState) void { const pages: usize = @intCast((len + page_size - 1) / page_size); const max_phys: u64 = if (flags & abi.dma_below_4g != 0) (@as(u64, 4) << 30) else ~@as(u64, 0); - const phys = pmm.allocContiguous(pages, max_phys) orelse return fail(state); - // Reserve arena virtual space from the per-SPACE cursor, under the lock — - // sibling threads must hand out disjoint windows of the one shared arena. + // Frame allocation and cursor reservation under the lock — the pmm has no + // lock of its own, and sibling threads must hand out disjoint windows of + // the one shared arena. var base_v: u64 = 0; + var phys: u64 = 0; { const lock_flags = sync.enter(); const cursor = scheduler.addressSpaceDmaNextPtr(t.address_space) orelse { sync.leave(lock_flags); - for (0..pages) |i| pmm.free(phys + i * page_size); return fail(state); }; if (cursor.* == 0) cursor.* = dma_arena_base; base_v = cursor.*; if (base_v + pages * page_size > dma_arena_end) { sync.leave(lock_flags); - for (0..pages) |i| pmm.free(phys + i * page_size); // arena exhausted; give the frames back - return fail(state); + return fail(state); // arena exhausted } + phys = pmm.allocContiguous(pages, max_phys) orelse { + sync.leave(lock_flags); + return fail(state); + }; cursor.* = base_v + pages * page_size; sync.leave(lock_flags); } - // Zero through the physmap (the frames aren't mapped in the caller yet), then map. + // Zero through the physmap OUTSIDE the lock (the frames are still private to + // this call), then map page-by-page, each under a brief lock hold — the lock + // serializes the shared page-table walk (siblings on other cores walk the + // same tables), and per-page holds keep the mmap discipline: never pin every + // core's ticks across a multi-MiB operation. const kernel_view: [*]u8 = @ptrFromInt(boot_handoff.physicalToVirtual(phys)); @memset(kernel_view[0 .. pages * page_size], 0); - architecture.mapUserDmaInto(t.address_space, base_v, phys, pages * page_size); + for (0..pages) |i| { + const lock_flags = sync.enter(); + architecture.mapUserDmaInto(t.address_space, base_v + i * page_size, phys + i * page_size, page_size); + sync.leave(lock_flags); + } architecture.setSystemCallResult(state, base_v); // virtual address for the CPU architecture.setSystemCallResult2(state, phys); // physical address for the device } @@ -545,10 +562,14 @@ fn systemDmaFree(state: *architecture.CpuState) void { for (0..pages) |i| { const va = base_v + i * page_size; + // Per-page lock hold: the translate/unmap walks the shared page tables + // and pmm.free mutates the unlocked frame bitmap. + const lock_flags = sync.enter(); if (architecture.translate(t.address_space, va)) |phys| { architecture.unmapUserPageInto(t.address_space, va); pmm.free(phys); } + sync.leave(lock_flags); } architecture.setSystemCallResult(state, 0); } @@ -569,9 +590,15 @@ fn systemSharedMemoryCreate(state: *architecture.CpuState) void { const pages: usize = @intCast((len + page_size - 1) / page_size); if (pages == 0 or pages > maximum_shared_memory_pages) return fail(state); - // Reserve arena virtual space up front (per-SPACE cursor: sibling threads - // hand out disjoint windows), so a mapping failure needs no rollback. + // One locked section builds the whole named object — cursor reservation, + // frames, the refcounted object, the space's mapping record (M3: frames must + // outlive every MAPPING, not just every handle), and the handle — with full + // rollback, so no failure path ever touches the unlocked pmm/heap and no + // half-built region is ever reachable. var base_v: u64 = 0; + var phys: u64 = 0; + var handle: i64 = -1; + var shared_memory: *ipc.SharedMemoryObject = undefined; { const lock_flags = sync.enter(); const cursor = scheduler.addressSpaceSharedMemoryNextPtr(t.address_space) orelse { @@ -584,43 +611,46 @@ fn systemSharedMemoryCreate(state: *architecture.CpuState) void { sync.leave(lock_flags); return fail(state); // arena exhausted } + phys = pmm.allocContiguous(pages, ~@as(u64, 0)) orelse { + sync.leave(lock_flags); + return fail(state); + }; + shared_memory = ipc.createSharedMemory(phys, pages) orelse { + for (0..pages) |i| pmm.free(phys + i * page_size); + sync.leave(lock_flags); + return fail(state); + }; + if (!scheduler.recordSpaceMappingLocked(t.address_space, @ptrCast(shared_memory))) { + ipc.dropSharedMemoryReference(shared_memory); // last ref: frees the object AND its frames + sync.leave(lock_flags); + return fail(state); + } + ipc.retainSharedMemory(shared_memory); // the mapping's own reference + handle = ipc.installSharedMemoryHandle(t, shared_memory); + if (handle < 0) { + // Full rollback: unrecord the mapping, then drop both references — + // the second drop frees the object and its frames, all under the lock. + scheduler.removeSpaceMappingLocked(t.address_space, @ptrCast(shared_memory)); + ipc.dropSharedMemoryReference(shared_memory); + ipc.dropSharedMemoryReference(shared_memory); + sync.leave(lock_flags); + return fail(state); + } cursor.* = base_v + pages * page_size; sync.leave(lock_flags); } - const phys = pmm.allocContiguous(pages, ~@as(u64, 0)) orelse return fail(state); - // Zero through the physmap (the frames aren't mapped in the caller yet). + // Zero through the physmap OUTSIDE the lock (the frames are private until + // the handle is shared and the pages mapped), then map page-by-page under + // brief lock holds — the lock serializes the shared page-table walk against + // sibling threads, per-page so no core's tick starves (the mmap discipline). const kernel_view: [*]u8 = @ptrFromInt(boot_handoff.physicalToVirtual(phys)); @memset(kernel_view[0 .. pages * page_size], 0); - - const shared_memory = ipc.createSharedMemory(phys, pages) orelse { - for (0..pages) |i| pmm.free(phys + i * page_size); - return fail(state); - }; - // The creator's mapping holds its own reference, recorded on the space - // (docs/shared-fate-plan.md M3): frames must outlive every MAPPING, not just - // every handle — a sibling thread keeps using the region after the - // handle-holding thread dies. - { - const flags = sync.enter(); - if (!scheduler.recordSpaceMappingLocked(t.address_space, @ptrCast(shared_memory))) { - sync.leave(flags); - ipc.dropSharedMemoryReference(shared_memory); // last ref: frees the object AND its frames - return fail(state); - } - ipc.retainSharedMemory(shared_memory); - sync.leave(flags); + for (0..pages) |i| { + const lock_flags = sync.enter(); + architecture.mapUserSharedInto(t.address_space, base_v + i * page_size, phys + i * page_size, page_size); + sync.leave(lock_flags); } - const handle = ipc.installSharedMemoryHandle(t, shared_memory); - if (handle < 0) { - // The mapping record keeps one reference; drop only the creator's. The - // object (and frames) now live until this space is destroyed — the - // region was never named, so nothing else can reach it. - ipc.dropSharedMemoryReference(shared_memory); - return fail(state); - } - - architecture.mapUserSharedInto(t.address_space, base_v, phys, pages * page_size); architecture.setSystemCallResult(state, base_v); // virtual_address for the CPU architecture.setSystemCallResult2(state, @intCast(handle)); // capability handle to pass on } @@ -662,7 +692,14 @@ fn systemSharedMemoryMap(state: *architecture.CpuState) void { cursor.* = base_v + size; sync.leave(flags); } - architecture.mapUserSharedInto(t.address_space, base_v, shared_memory.phys, size); + // Map page-by-page under brief lock holds — the shared page-table walk must + // be serialized against sibling threads (the mmap discipline). + var page_index: usize = 0; + while (page_index * page_size < size) : (page_index += 1) { + const lock_flags = sync.enter(); + architecture.mapUserSharedInto(t.address_space, base_v + page_index * page_size, shared_memory.phys + page_index * page_size, page_size); + sync.leave(lock_flags); + } architecture.setSystemCallResult(state, base_v); } @@ -786,17 +823,24 @@ fn systemThreadSpawn(state: *architecture.CpuState) void { null else ipc.resolveHandle(t, exit_handle) orelse return failErr(state, ipc.EBADF); - const tid = spawnThreadSupervised(t.address_space, entry, stack_top, arg, t.priority, t.id, exit_endpoint, t.leader) orelse return fail(state); - architecture.setSystemCallResult(state, tid); + const tid = spawnThreadSupervised(t.address_space, entry, stack_top, arg, t.priority, t.id, exit_endpoint, t.leader); + if (tid == -ipc.ESRCH) return failErr(state, ipc.ESRCH); // dying group admits no member + if (tid < 0) return fail(state); + architecture.setSystemCallResult(state, @intCast(tid)); } /// Spawn a thread sharing `address_space`, taking the exit-endpoint reference under the **same** /// lock as the spawn (as `spawnProcessSupervised` does), so the thread cannot die before /// its reference exists. Returns the new thread id, or null on resource exhaustion. -fn spawnThreadSupervised(address_space: u64, entry: u64, stack_top: u64, arg: u64, priority: scheduler.Priority, supervisor: u32, exit_endpoint: ?*ipc.Endpoint, leader: u32) ?u32 { +fn spawnThreadSupervised(address_space: u64, entry: u64, stack_top: u64, arg: u64, priority: scheduler.Priority, supervisor: u32, exit_endpoint: ?*ipc.Endpoint, leader: u32) i64 { const flags = sync.enter(); defer sync.leave(flags); - const tid = scheduler.spawnUserLocked(address_space, entry, stack_top, arg, priority, "thread", supervisor, if (exit_endpoint) |e| @ptrCast(e) else null, leader) orelse return null; + // The spawn gate (docs/shared-fate-plan.md): a dying group admits no new + // member — checked under the same lock that would create it, and reported + // as -ESRCH (the process is as good as gone). retainAddressSpace inside + // spawnUserLocked backstops the same refusal. + if (scheduler.groupDyingLocked(address_space)) return -ipc.ESRCH; + const tid = scheduler.spawnUserLocked(address_space, entry, stack_top, arg, priority, "thread", supervisor, if (exit_endpoint) |e| @ptrCast(e) else null, leader) orelse return -1; if (exit_endpoint) |endpoint| endpoint.refcount += 1; // the thread holds it birth-to-death return tid; } @@ -1059,16 +1103,21 @@ pub fn killProcess(caller_id: u32, target_id: u32) i64 { // `supervisor` (the task that spawned it) grants nothing here // (docs/shared-fate-plan.md M1). Kernel tasks were -ESRCH'd above, so a // leader of 0 is unreachable. + // A group already dying answers from the latch's stash — the leader's slot + // may already be reaped while a condemned member is still enumerable. Same + // authority gate, then 0: the kill is already true, accepted and + // irrevocable (docs/shared-fate-plan.md). + if (scheduler.groupSupervisorLocked(target.address_space)) |group_supervisor| { + return if (group_supervisor == caller_id) 0 else -ipc.EPERM; + } const leader = if (target.leader == target.id) target else scheduler.taskByIdLocked(target.leader) orelse return -ipc.ESRCH; if (leader.supervisor != caller_id) return -ipc.EPERM; - // Whole-group kill (docs/shared-fate-plan.md). Already dying → the kill is - // already true: accepted and irrevocable either way, return 0. The caller is - // never a member (the leader's supervisor predates the group and cannot be - // inside it), so this always returns. - if (scheduler.groupDyingLocked(target.address_space)) return 0; + // Whole-group kill (docs/shared-fate-plan.md). The caller is never a member + // (the leader's supervisor predates the group and cannot be inside it), so + // this always returns. killGroupLocked(leader, .killed, null); return 0; } @@ -1130,6 +1179,9 @@ fn exitGroupCurrent(reason: abi.ExitReason) noreturn { const t = scheduler.current(); _ = sync.enter(); if (scheduler.groupDyingLocked(t.address_space)) terminateCurrentLocked(); + // The orelse fallback is unreachable today — a leader outlives its members + // (its thread_exit is refused; any leader death IS group death). If a future + // change ever made it reachable, it degrades to single-task semantics. const leader_task = if (t.leader == t.id) t else scheduler.taskByIdLocked(t.leader) orelse t; killGroupLocked(leader_task, reason, t); unreachable; // killGroupLocked never returns for an in-group trigger @@ -1149,6 +1201,7 @@ pub fn killCurrentProcess(reason: abi.ExitReason) noreturn { // (docs/shared-fate-plan.md). `fault_kill_count` is per faulting GROUP. if (scheduler.groupDyingLocked(t.address_space)) terminateCurrentLocked(); fault_kill_count += 1; + // orelse fallback unreachable today: a leader outlives its members (see exitGroupCurrent). const leader_task = if (t.leader == t.id) t else scheduler.taskByIdLocked(t.leader) orelse t; killGroupLocked(leader_task, reason, t); unreachable; // killGroupLocked never returns for an in-group trigger diff --git a/system/kernel/scheduler.zig b/system/kernel/scheduler.zig index 27b99c0..6be85f9 100644 --- a/system/kernel/scheduler.zig +++ b/system/kernel/scheduler.zig @@ -359,6 +359,34 @@ pub fn groupDyingLocked(root: u64) bool { return false; } +/// The stashed supervisor of `root`'s DYING group, or null if the group is not +/// dying. Answers the process_kill authority question during the condemned +/// window, when the leader's task slot may already be reaped. Lock held. +pub fn groupSupervisorLocked(root: u64) ?u32 { + for (&address_space_refs) |*entry| { + if (entry.count != 0 and entry.root == root) { + return if (entry.dying) entry.group_supervisor else null; + } + } + return null; +} + +/// Undo a `recordSpaceMappingLocked` — the rollback half for a caller whose +/// later step failed. Removes one matching slot; the caller drops the reference +/// it had transferred. Lock held. +pub fn removeSpaceMappingLocked(root: u64, object: *anyopaque) void { + for (&address_space_refs) |*entry| { + if (entry.count == 0 or entry.root != root) continue; + for (&entry.mappings) |*slot| { + if (slot.* == object) { + slot.* = null; + return; + } + } + return; + } +} + /// The whole static task pool, for process.zig's group fan-out — which must scan /// members under the lock it already holds. Slots may be `.free`/`.reaping`; /// callers filter by state and must not hold pointers past the lock. diff --git a/system/kernel/tests.zig b/system/kernel/tests.zig index 25488cc..9ef99ae 100644 --- a/system/kernel/tests.zig +++ b/system/kernel/tests.zig @@ -3233,11 +3233,14 @@ fn awaitExitBadge(endpoint: *ipcsync.Endpoint) u64 { /// A group death is one notification, badged with the LEADER, arriving only /// after every member (and the address space) is gone — asserted by every -/// shared-fate case below. -fn checkGroupDead(me: u32, leader: u32, badge: u64, reason: abi.ExitReason) void { +/// shared-fate case below. "Exactly one": after a settling sleep, a second +/// (buggy, double-posted) badge would still sit in the endpoint's notify ring. +fn checkGroupDead(me: u32, leader: u32, badge: u64, reason: abi.ExitReason, endpoint: *ipcsync.Endpoint) void { check("one exit notification, badged with the leader", badge == abi.notify_badge_bit | abi.notify_exit_bit | leader); check("the leader's recorded reason is the group reason", process.exitReasonOf(me, leader) == @intFromEnum(reason)); check("no group member is listed after the death", !groupListed(leader)); + scheduler.sleep(200); + check("exactly one exit notification (ring drained)", endpoint.notify_head == endpoint.notify_tail); } fn threadFaultGroupTest(boot_information: *const BootInformation) void { @@ -3258,7 +3261,7 @@ fn threadFaultGroupTest(boot_information: *const BootInformation) void { return; } const badge = awaitExitBadge(endpoint); - checkGroupDead(me, child, badge, .segmentation_fault); + checkGroupDead(me, child, badge, .segmentation_fault, endpoint); check("one fault kill for the whole group", process.fault_kill_count == 1); const deadline = architecture.millis() + 5000; while (scheduler.liveStackBytes() > stacks_base and architecture.millis() < deadline) scheduler.yield(); @@ -3293,7 +3296,7 @@ fn killThreadedGroupTest(boot_information: *const BootInformation) void { scheduler.sleep(100); // let the worker really be running on another core check("the supervisor's kill is accepted", process.killProcess(me, child) == 0); const badge = awaitExitBadge(endpoint); - checkGroupDead(me, child, badge, .killed); + checkGroupDead(me, child, badge, .killed, endpoint); check("the leader's claim was released before the notification", devices_broker.ownerOf(0) == null); check("the worker's claim was released before the notification", devices_broker.ownerOf(1) == null); check("a dead group stays dead (-ESRCH)", process.killProcess(me, child) == -ipcsync.ESRCH); @@ -3319,7 +3322,7 @@ fn killViaWorkerTidTest(boot_information: *const BootInformation) void { check("a non-supervisor aiming at the worker is refused (-EPERM)", process.killProcess(me + 12345, worker) == -ipcsync.EPERM); check("the supervisor's kill aimed at the WORKER id is accepted", process.killProcess(me, worker) == 0); const badge = awaitExitBadge(endpoint); - checkGroupDead(me, child, badge, .killed); + checkGroupDead(me, child, badge, .killed, endpoint); result(); } @@ -3339,7 +3342,7 @@ fn racingTriggersTest(boot_information: *const BootInformation) void { return; } const badge = awaitExitBadge(endpoint); - checkGroupDead(me, child, badge, .segmentation_fault); + checkGroupDead(me, child, badge, .segmentation_fault, endpoint); check("two racing faults counted as ONE group kill", process.fault_kill_count == 1); result(); } @@ -3359,7 +3362,7 @@ fn exitGroupTest(boot_information: *const BootInformation) void { return; } const badge = awaitExitBadge(endpoint); - checkGroupDead(me, child, badge, .aborted); + checkGroupDead(me, child, badge, .aborted, endpoint); result(); } @@ -3381,7 +3384,7 @@ fn threadTestMarkerCase(boot_information: *const BootInformation, case_name: []c return; } const badge = awaitExitBadge(endpoint); - checkGroupDead(me, child, badge, .exited); + checkGroupDead(me, child, badge, .exited, endpoint); result(); }