kernel: shared-fate review fixes — lock the shm/dma walks, contract edges

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

@@ -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
 

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 —
+    // Frame allocation and cursor reservation under the lock — the pmm has no
-    // sibling threads must hand out disjoint windows of the one shared arena.
+    // 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); // arena exhausted
-            return fail(state);
         }
+        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
+    // One locked section builds the whole named object — cursor reservation,
-    // hand out disjoint windows), so a mapping failure needs no rollback.
+    // 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 OUTSIDE the lock (the frames are private until
-    // Zero through the physmap (the frames aren't mapped in the caller yet).
+    // 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);
-
+    for (0..pages) |i| {
-    const shared_memory = ipc.createSharedMemory(phys, pages) orelse {
+        const lock_flags = sync.enter();
-        for (0..pages) |i| pmm.free(phys + i * page_size);
+        architecture.mapUserSharedInto(t.address_space, base_v + i * page_size, phys + i * page_size, page_size);
-        return fail(state);
+        sync.leave(lock_flags);
-    };
-    // 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);
     }
-    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);
+    const tid = spawnThreadSupervised(t.address_space, entry, stack_top, arg, t.priority, t.id, exit_endpoint, t.leader);
-    architecture.setSystemCallResult(state, tid);
+    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
+    // Whole-group kill (docs/shared-fate-plan.md). The caller is never a member
-    // already true: accepted and irrevocable either way, return 0. The caller is
+    // (the leader's supervisor predates the group and cannot be inside it), so
-    // never a member (the leader's supervisor predates the group and cannot be
+    // this always returns.
-    // inside it), so this always returns.
-    if (scheduler.groupDyingLocked(target.address_space)) return 0;
     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

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.

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.
+/// shared-fate case below. "Exactly one": after a settling sleep, a second
-fn checkGroupDead(me: u32, leader: u32, badge: u64, reason: abi.ExitReason) void {
+/// (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();
 }