diff --git a/issues/a-region-keeps-the-address-space-of-every-process-that-ended-with-it-mapped.md b/issues/a-region-keeps-the-address-space-of-every-process-that-ended-with-it-mapped.md new file mode 100644 index 00000000000..41b360d430a --- /dev/null +++ b/issues/a-region-keeps-the-address-space-of-every-process-that-ended-with-it-mapped.md @@ -0,0 +1,45 @@ +--- +status: open +kind: defect +opened: 2026-10-08 +--- + +# A region keeps the address space of every process that ended with it mapped + +`SharedMemObject::map_into` (`kernel/src/object/shm.rs`) records each mapping +as `(Pid, PageTables, UserAddr)`, and `PageTables` is the `Arc` of the +process's address space. An entry leaves the list in two places only: the +region's zero-handle hook, when the last handle to it goes anywhere, and +`unmap_from`, which nothing but an inbox's teardown calls +(`kernel/src/inbox/mod.rs`). A process's own teardown +(`teardown_resources`, `kernel/src/process.rs`) closes its handles and +touches no region's list. + +So a process that maps a region somebody else also holds, and ends, leaves +its entry behind, and the entry keeps that process's `AddressSpace` alive +until the region's last holder lets go: its page-table pages, whatever its +`pages` map still owns, and its user PCID, which returns to the pool only +when the space drops (`PcidGuard`, `kernel/src/arch/x86_64/paging.rs`). The +list grows by one entry per such process and nothing bounds it. A region +handle carries `DUP` and `TRANSFER`, so one process can keep a region and +have child after child map it and end; the pool holds +`kernel::pcid::MAX_USER_PCID` tags, and with every one held +`AddressSpace::new_user` answers `None` and every spawn on the machine is +refused `ResourceExhausted` (`kernel/src/loader/mod.rs`). + +**Read from the code, not run.** Pids are never reissued +(`kernel/pure/proclife/pids.rs`), so a later process is never answered a dead +one's address by the list's `find`. + +**Exit condition**: a process's teardown leaves no entry of it in any +region's list — and a test in which a child maps a region its parent keeps +and ends sees the child's address space freed, its PCID back in the pool, +while the parent still holds the handle. + +**Owner**: `kernel::object::shm`'s handle-driven mapping teardown, with +`issues/the-compositor-keeps-a-committed-regions-mapping-for-as-long-as-the-client-does.md`, +whose exit ends a mapping at the close of its process's last handle and so +reaches this one wherever the process closed that handle, before it ended or +in its teardown. It does not reach a process that mapped the region and moved +its handle on by `SYS_HANDLE_SEND` (`sys_handle_send`, +`kernel/src/syscall/ipc.rs`), which is no close. diff --git a/kernel/src/object/mod.rs b/kernel/src/object/mod.rs index 205029806b9..da60f23bcb1 100644 --- a/kernel/src/object/mod.rs +++ b/kernel/src/object/mod.rs @@ -37,16 +37,26 @@ impl Held { Self(Lock::new(Some(value))) } + /// What is held, for a release that is more than a drop; `None` after the first. + pub(crate) fn take(&self) -> Option { + self.0.lock().take() + } + /// Drops what is held, outside the lock; `T`'s destructor must not re-enter it. pub(crate) fn release(&self) { - let taken = self.0.lock().take(); - drop(taken); + drop(self.take()); } /// `f` over what is held, under the lock, or `None` after the release. pub(crate) fn with(&self, f: impl FnOnce(&T) -> R) -> Option { self.0.lock().as_ref().map(f) } + + /// [`with`](Self::with) for an `f` that changes what is held: the change and the release + /// are one lock's, so nothing is added to what a release already took. + pub(crate) fn with_mut(&self, f: impl FnOnce(&mut T) -> R) -> Option { + self.0.lock().as_mut().map(f) + } } impl Held { diff --git a/kernel/src/object/shm.rs b/kernel/src/object/shm.rs index e19eda42b91..9d84684c4ca 100644 --- a/kernel/src/object/shm.rs +++ b/kernel/src/object/shm.rs @@ -1,8 +1,12 @@ //! A region of memory more than one process can see. //! //! Holding a handle allows mapping it; giving one away is `SYS_HANDLE_SEND`. -//! Mappings are torn down when the last handle goes; pages are freed when -//! the last `Arc` goes, always later since a handle holds an `Arc`. +//! The handle count owns the mappings and the `Arc` count owns the pages: the +//! last handle takes the list of mappings away for good and tears each one +//! down, and the pages are freed when the last `Arc` goes, always later since +//! a handle holds an `Arc`. An `Arc` a syscall cloned out of its table can +//! outlive the last handle, so a map through one finds no list and is refused +//! under the lock the teardown took it with. use alloc::sync::Arc; use alloc::vec::Vec; @@ -12,10 +16,9 @@ use toyos_abi::syscall::SyscallError; use crate::mm::policy::{CachePolicy, Prot}; use crate::mm::{align_2m_checked, pmm, Unmapped, PAGE_2M}; use crate::process::{PageTables, Pid}; -use crate::sync::Lock; use crate::{DirectMap, UserAddr}; -use super::{KObjectVariant, ObjectCore, ZeroHandles}; +use super::{Held, KObjectVariant, ObjectCore, ZeroHandles}; /// Physical pages a region keeps alive; behind an `Arc` since one page set /// can back several objects. @@ -52,8 +55,8 @@ impl Region { pub struct SharedMemObject { pub(super) core: ObjectCore, region: Region, - /// Where this region is mapped, per process; the zero-handle hook empties it. - mapped_in: Lock>, + /// Where this region is mapped, per process; released by the zero-handle hook. + mapped_in: Held>, } impl SharedMemObject { @@ -68,7 +71,7 @@ impl SharedMemObject { Arc::new(Self { core: Self::new_core(), region, - mapped_in: Lock::new(Vec::new()), + mapped_in: Held::new(Vec::new()), }) } @@ -119,7 +122,7 @@ impl SharedMemObject { /// could otherwise write the same bytes while the kernel is still initialising. pub fn phys_before_mapping(&self) -> DirectMap { assert!( - self.mapped_in.lock().is_empty(), + self.mapped_nowhere(), "shm koid {}: the region is mapped into a process already, so a kernel \ write through the direct map is not exclusive", self.core.koid().raw(), @@ -127,50 +130,58 @@ impl SharedMemObject { self.region.phys } + fn mapped_nowhere(&self) -> bool { + self.mapped_in.with(|mapped| mapped.is_empty()).unwrap_or(true) + } + /// Map into `pt`, or answer the address it is already mapped at; - /// idempotent per process. + /// idempotent per process. `Gone` once the last handle has gone. pub fn map_into(&self, pid: Pid, pt: &PageTables) -> Result { - let mut mapped = self.mapped_in.lock(); - if let Some((_, _, vaddr)) = mapped.iter().find(|(p, _, _)| *p == pid) { - return Ok(vaddr.raw()); - } - let (addr, _) = pt - .lock() - .alloc_and_map(self.region.phys.phys(), self.region.size, Prot::ReadWrite, self.region.cache) - .ok_or(SyscallError::ResourceExhausted)?; - // Logged only for a non-default policy: this process is the one - // paying for it. Read back the installed policy, not the request, - // so the line describes the mapping. - if self.region.cache != CachePolicy::Normal { - let installed = pt.lock().user_policy(addr).expect("shm: just mapped"); - crate::log!( - "shm: {:#x} mapped {:?} into pid {}", - self.region.phys.phys(), - installed, - pid - ); - } - mapped.push((pid, Arc::clone(pt), addr)); - Ok(addr.raw()) + let map = |mapped: &mut Vec<(Pid, PageTables, UserAddr)>| { + if let Some((_, _, vaddr)) = mapped.iter().find(|(p, _, _)| *p == pid) { + return Ok(vaddr.raw()); + } + let (addr, _) = pt + .lock() + .alloc_and_map(self.region.phys.phys(), self.region.size, Prot::ReadWrite, self.region.cache) + .ok_or(SyscallError::ResourceExhausted)?; + // Logged only for a non-default policy: this process is the one + // paying for it. Read back the installed policy, not the request, + // so the line describes the mapping. + if self.region.cache != CachePolicy::Normal { + let installed = pt.lock().user_policy(addr).expect("shm: just mapped"); + crate::log!( + "shm: {:#x} mapped {:?} into pid {}", + self.region.phys.phys(), + installed, + pid + ); + } + mapped.push((pid, Arc::clone(pt), addr)); + Ok(addr.raw()) + }; + self.mapped_in.with_mut(map).unwrap_or(Err(SyscallError::Gone)) } /// Take this process's mapping away, if it has one; the caller owes a /// shootdown before the freed address can be reissued. #[must_use = "the caller owes a shootdown before the address can be reissued"] pub fn unmap_from(&self, pid: Pid) -> Option> { - let mut mapped = self.mapped_in.lock(); - let pos = mapped.iter().position(|(p, _, _)| *p == pid)?; - let (_, pt, vaddr) = mapped.swap_remove(pos); - pt.lock().free_and_unmap(vaddr); - Some(Unmapped::new(())) + self.mapped_in.with_mut(|mapped| { + let pos = mapped.iter().position(|(p, _, _)| *p == pid)?; + let (_, pt, vaddr) = mapped.swap_remove(pos); + pt.lock().free_and_unmap(vaddr); + Some(Unmapped::new(())) + })? } } -/// Every mapping goes, flushed here; the pages do not, since a handle holds -/// an `Arc` and `Region::pages` frees them only when the last one drops. +/// Every mapping goes, flushed here, and with the list gone no later map can +/// add one; the pages do not, since a handle holds an `Arc` and +/// `Region::pages` frees them only when the last one drops. impl ZeroHandles for SharedMemObject { fn on_zero_handles(&self) { - let mapped = core::mem::take(&mut *self.mapped_in.lock()); + let mapped = self.mapped_in.take().expect("the zero-handle hook runs once"); if mapped.is_empty() { return; } @@ -184,9 +195,9 @@ impl ZeroHandles for SharedMemObject { impl Drop for SharedMemObject { fn drop(&mut self) { debug_assert!( - self.mapped_in.lock().is_empty(), - "shm koid {} freed with a live mapping: the zero-handle hook did \ - not run, so the pages go back to the PMM under somebody's window", + self.mapped_nowhere(), + "shm koid {} freed with a live mapping, so the pages go back to the \ + PMM under somebody's window", self.core.koid().raw(), ); } diff --git a/toyos-abi/src/syscall.rs b/toyos-abi/src/syscall.rs index 3e3260d780e..c1c39f00255 100644 --- a/toyos-abi/src/syscall.rs +++ b/toyos-abi/src/syscall.rs @@ -1981,7 +1981,8 @@ pub fn shm_create(size: usize) -> Result { /// Map the region `shm` names into this process. Needs [`Rights::MAP`]. /// -/// Idempotent: a second call answers the first call's address. +/// Idempotent: a second call answers the first call's address. [`SyscallError::Gone`] +/// when the region's last handle was closed under the call: nothing is mapped. /// /// [`Rights::MAP`]: crate::handle::Rights::MAP ///