diff --git a/issues/build/kernel-loom-without-loom-is-linted-by-no-clippy-run.md b/issues/build/kernel-loom-without-loom-is-linted-by-no-clippy-run.md index cd5a02559ec..cb7a19d8686 100644 --- a/issues/build/kernel-loom-without-loom-is-linted-by-no-clippy-run.md +++ b/issues/build/kernel-loom-without-loom-is-linted-by-no-clippy-run.md @@ -7,15 +7,14 @@ opened: 2026-09-29 # `kernel-loom` without `loom` is linted by no clippy run Both host shapes in `src/clippy.rs` build `kernel-loom` with its default -`loom`, so its other arm — the three `cfg(not(feature = "loom"))` sites in -`kernel-loom/src/lib.rs`, `tests/log_body_words.rs` and -`tests/log_zeroed_init.rs` — is compiled by no shape. A `mem::forget` planted in -the non-`loom` `percpu_fetch_add` leaves `cargo run -- --clippy` green. +`loom`, so its other arm — the `cfg(not(feature = "loom"))` sites in +`kernel-loom/src/lib.rs` and the test files gated that way — is compiled by no +shape. A `mem::forget` planted in the non-`loom` `percpu_fetch_add` leaves +`cargo run -- --clippy` green. `cargo clippy -p kernel-loom --no-default-features --all-targets -- -D warnings` -exits 101 with twelve findings: `missing_safety_doc` on that `percpu_fetch_add`, -and `new_without_default` on eleven kernel types' non-`loom` `const fn new`, -which `kernel-loom` exports `pub` and the kernel binary does not. Their `loom` -arms carry `#[allow(clippy::new_without_default)]`; these do not. +exits 101: `missing_safety_doc` on that `percpu_fetch_add`, and +`new_without_default` on each kernel type's non-`loom` `const fn new` that +`kernel-loom` exports `pub` and the kernel binary does not. Exit: a shape builds `kernel-loom` without `loom`, and it is clean. diff --git a/kernel-loom/src/lib.rs b/kernel-loom/src/lib.rs index 4f2c2cdb1bf..fecb8fd876e 100644 --- a/kernel-loom/src/lib.rs +++ b/kernel-loom/src/lib.rs @@ -186,6 +186,33 @@ pub use log_shard as shard; #[path = "../../kernel/src/log/registry.rs"] pub mod log_registry; +/// The walks `SYS_LOG_READ` and the console drain take, driven by +/// `tests/log_cursor.rs` over real shards; without `loom` alone, as is the +/// [`shards`] shim it names. +#[cfg(not(feature = "loom"))] +#[path = "../../kernel/src/log/read.rs"] +pub mod log_read; + +// The shards `read.rs` walks: in the kernel cpu0's and every AP's published +// one, here whichever the calling test thread installed. +#[cfg(not(feature = "loom"))] +std::thread_local! { + static SHARDS: core::cell::Cell<[Option<&'static shard::Shard>; toyos_abi::log::MAX_LOG_SHARDS]> = + const { core::cell::Cell::new([None; toyos_abi::log::MAX_LOG_SHARDS]) }; +} + +/// What `read.rs` names as `super::shards()`. +#[cfg(not(feature = "loom"))] +pub fn shards() -> [Option<&'static shard::Shard>; toyos_abi::log::MAX_LOG_SHARDS] { + SHARDS.with(core::cell::Cell::get) +} + +/// Install the shards this thread's walks see. No kernel counterpart. +#[cfg(not(feature = "loom"))] +pub fn install_shards(shards: [Option<&'static shard::Shard>; toyos_abi::log::MAX_LOG_SHARDS]) { + SHARDS.with(|cell| cell.set(shards)); +} + #[path = "../../kernel/src/sched/reap_gate.rs"] pub mod reap_gate; diff --git a/kernel-loom/tests/log_cursor.rs b/kernel-loom/tests/log_cursor.rs new file mode 100644 index 00000000000..839da661d42 --- /dev/null +++ b/kernel-loom/tests/log_cursor.rs @@ -0,0 +1,99 @@ +//! Host-fast regression for what `SYS_LOG_READ` takes from a caller's cursor. +//! +//! `kernel/src/log/read.rs` is the walk the syscall copies a caller's cursor +//! into, compiled here over real shards. The cursor crossed the syscall +//! boundary, so its loss count is never read and a position no shard has +//! issued is refused. +//! +//! `--no-default-features`, for [`log_zeroed_init`]'s reason: the shards are the +//! real ones, built the way the kernel builds an AP's. +//! +//! [`log_zeroed_init`]: ../log_zeroed_init.rs + +#![cfg(not(feature = "loom"))] + +use std::alloc::{alloc_zeroed, Layout}; + +use kernel_loom::arch::IrqGuard; +use kernel_loom::log_read::{drain_ordered, Cursor, RecordSink}; +use kernel_loom::log_shard::{Shard, FIRST_SEQ, SHARD_RECORDS}; +use toyos_abi::log::{LogCursor, LogRecord, MAX_LOG_SHARDS}; + +/// A shard for the life of the test binary, which a walk names as `'static`. +fn shard() -> &'static Shard { + // SAFETY: a `Shard` is not zero-sized; the block is never freed. + let ptr = unsafe { alloc_zeroed(Layout::new::()) }.cast::(); + assert!(!ptr.is_null()); + // SAFETY: fresh, zeroed, aligned and private to this test until it returns. + unsafe { + Shard::initialize_zeroed(ptr); + &*ptr + } +} + +/// Commits `count` records to `shard`, each stamped with its own number. +fn emit(shard: &Shard, count: u64) { + let guard = IrqGuard::close(); + for _ in 0..count { + // SAFETY: this thread is the shard's only producer, and each number is + // committed once under the guard it was reserved with. + unsafe { + let seq = shard.reserve(&guard); + let mut record = LogRecord::EMPTY; + record.seq = seq; + record.at_ns = seq; + shard.commit(seq, &record, &guard); + } + } +} + +/// A sink that takes nothing, so a walk only positions its cursor. +struct Full; + +impl RecordSink for Full { + fn put(&mut self, _: &LogRecord) -> bool { + false + } +} + +/// A cursor at every shard's start claiming `u64::MAX` records already lost, +/// behind a shard that has overwritten some: the answer is the overwritten +/// count, not the claim plus it. +#[test] +fn a_cursors_loss_claim_is_not_read() { + const OVERWRITTEN: u64 = 3; + let lapped = shard(); + emit(lapped, SHARD_RECORDS as u64 + OVERWRITTEN); + let mut shards = [None; MAX_LOG_SHARDS]; + shards[0] = Some(lapped); + kernel_loom::install_shards(shards); + + let mut cursor = LogCursor { lost: u64::MAX, ..LogCursor::new() }; + let mut walk = Cursor::from_reader(&cursor).expect("a cursor at every shard's start is taken"); + drain_ordered(&mut walk, &mut Full); + walk.write_into(&mut cursor); + assert_eq!(cursor.lost, OVERWRITTEN); +} + +/// A position past the number a shard issues next is refused, and one at it +/// taken: a published shard's head, and an unpublished shard's first number. +#[test] +fn a_cursor_ahead_of_a_shard_is_refused() { + const RECORDS: u64 = 5; + let published = shard(); + emit(published, RECORDS); + let mut shards = [None; MAX_LOG_SHARDS]; + shards[0] = Some(published); + kernel_loom::install_shards(shards); + + let taken = |shard: usize, next: u64| { + let mut cursor = LogCursor::new(); + cursor.next[shard] = next; + Cursor::from_reader(&cursor).is_some() + }; + let head = FIRST_SEQ + RECORDS; + assert!(taken(0, head), "a cursor caught up with a published shard"); + assert!(!taken(0, head + 1), "a cursor past a published shard's head"); + assert!(taken(1, FIRST_SEQ), "a cursor at an unpublished shard's first number"); + assert!(!taken(1, FIRST_SEQ + 1), "a cursor past an unpublished shard's first number"); +} diff --git a/kernel/src/log/read.rs b/kernel/src/log/read.rs index ecaaa883122..ad3e658760d 100644 --- a/kernel/src/log/read.rs +++ b/kernel/src/log/read.rs @@ -65,9 +65,13 @@ impl Cursor { Self { next: [FIRST_SEQ; MAX_LOG_SHARDS], lost: 0 } } - /// A caller's raw `LogCursor`, unvalidated: every field is clamped where it is used instead. - pub fn from_reader(cursor: &toyos_abi::log::LogCursor) -> Self { - Self { next: cursor.next, lost: cursor.lost } + /// A caller's `LogCursor` as a walk, or `None` for a position past the number its shard issues + /// next; its `lost` is never read, so the walk counts this read's loss alone. + pub fn from_reader(cursor: &toyos_abi::log::LogCursor) -> Option { + // An unpublished shard is held to the head it is published with, so no cursor is ahead of one that appears mid-walk. + let issued = |shard: Option<&'static Shard>| shard.map_or(FIRST_SEQ, Shard::head); + let ahead = cursor.next.iter().zip(super::shards()).any(|(&next, shard)| next > issued(shard)); + (!ahead).then_some(Self { next: cursor.next, lost: 0 }) } /// Writes the walk state back into the caller's `LogCursor`. @@ -82,6 +86,7 @@ impl Cursor { let oldest = shard.oldest_readable(); // Clamped to `FIRST_SEQ`: a zeroed cursor from the syscall boundary must not read as having missed everything. let want = self.next.get(i).copied().unwrap_or(FIRST_SEQ).max(FIRST_SEQ); + // Cannot overflow: `lost` starts at zero in every cursor and grows only as far as the clamp below moves `next[i]`, which never passes a head. self.lost += oldest.saturating_sub(want); let want = want.max(oldest); *self.next.get_mut(i)? = want; diff --git a/kernel/src/log/user.rs b/kernel/src/log/user.rs index 0d0c0a6ac5f..dfdd429ecda 100644 --- a/kernel/src/log/user.rs +++ b/kernel/src/log/user.rs @@ -1,6 +1,6 @@ //! Kernel side of `SYS_LOG_READ` and its readiness source. //! -//! No per-reader state in a read: a cursor is the caller's own sequence numbers and loss count, copied in, walked, and copied back; readers coexist uncoordinated. Requires [`Rights::LOG`] on a `SysCap`, not ambient. +//! No per-reader state in a read: a cursor is the caller's own sequence numbers, copied in, refused if ahead of a shard, walked, and copied back with this read's loss; readers coexist uncoordinated. Requires [`Rights::LOG`] on a `SysCap`, not ambient. //! //! [`Rights::LOG`]: toyos_abi::handle::Rights::LOG @@ -52,7 +52,9 @@ pub fn read( return Err(SyscallError::InvalidArgument); } - let mut walk = Cursor::from_reader(cursor); + let Some(mut walk) = Cursor::from_reader(cursor) else { + return Err(SyscallError::InvalidArgument); + }; let mut sink = UserRecords { out, written: 0, capacity }; drain_ordered(&mut walk, &mut sink); let written = sink.written; diff --git a/src/ci.rs b/src/ci.rs index 938da3b5e3b..cbffbcdc617 100644 --- a/src/ci.rs +++ b/src/ci.rs @@ -545,8 +545,6 @@ fn host(root: &Path) -> Vec { Err(failed.join("; ")) } })); - // `log_zeroed_init` and `log_body_words` are gated `cfg(not(feature = - // "loom"))`, so the default invocation runs nothing from either. steps.push(step("kernel-loom without loom", || { cargo(root, &[ "test", @@ -557,6 +555,8 @@ fn host(root: &Path) -> Vec { "log_zeroed_init", "--test", "log_body_words", + "--test", + "log_cursor", ]) })); for control in CONTROLS { diff --git a/toyos-abi/src/log.rs b/toyos-abi/src/log.rs index e7c7326d83e..e6aed28d488 100644 --- a/toyos-abi/src/log.rs +++ b/toyos-abi/src/log.rs @@ -257,15 +257,17 @@ pub struct LogCursor { /// the first time and reads it back. pub shards: u32, pub _pad: u32, - /// In/out: cumulative records this cursor never saw because they were - /// overwritten. + /// Out: records this read skipped because they were overwritten. The + /// kernel never reads it, so a reader's total is the reader's own sum. /// /// **Derived, never counted by a producer.** The kernel computes it from /// `head` and `next`, which both have to be right anyway, so no counter can /// drift from the ring. It lives here so a reader that ignores loss has to /// actively ignore a field it is already passing. pub lost: u64, - /// In/out: the next sequence number wanted from each shard. + /// In/out: the next sequence number wanted from each shard. A number past + /// the one the shard issues next is refused, and a shard not yet published + /// issues 1 next. pub next: [u64; MAX_LOG_SHARDS], } diff --git a/toyos-abi/src/syscall.rs b/toyos-abi/src/syscall.rs index e970422f24e..71b5593784d 100644 --- a/toyos-abi/src/syscall.rs +++ b/toyos-abi/src/syscall.rs @@ -914,7 +914,8 @@ pub fn process_kill(proc: RawHandle) -> Result<(), SyscallError> { /// indexes by shift and the kernel does no length arithmetic. A buffer that /// cannot hold one record, or that cannot hold what the machine's shard count /// requires, is `InvalidArgument` — untrusted input that cannot be satisfied is -/// refused, never truncated to fit. +/// refused, never truncated to fit. So is a `cursor` ahead of a shard +/// ([`crate::log::LogCursor::next`]). pub fn log_read( syscap: RawHandle, cursor: &mut crate::log::LogCursor, diff --git a/toyos/src/log/mod.rs b/toyos/src/log/mod.rs index 3e6ae8904bf..00a502422ca 100644 --- a/toyos/src/log/mod.rs +++ b/toyos/src/log/mod.rs @@ -69,6 +69,8 @@ use crate::AsHandle; /// line. pub struct LogTail { cursor: LogCursor, + /// Every read's `cursor.lost`, summed: the kernel answers each read's alone. + lost: u64, } impl Default for LogTail { @@ -79,16 +81,12 @@ impl Default for LogTail { impl LogTail { pub const fn new() -> Self { - Self { cursor: LogCursor::new() } + Self { cursor: LogCursor::new(), lost: 0 } } /// Records this cursor never saw because a producer overwrote them. - /// - /// Cumulative and exact: the kernel derives it from the two numbers that - /// have to be right anyway, so it cannot drift from the ring the way a - /// producer-side counter would. pub fn lost(&self) -> u64 { - self.cursor.lost + self.lost } /// Shards the machine has, once a read has answered. Zero before that. @@ -106,6 +104,7 @@ impl LogTail { out: &'a mut [LogRecord], ) -> Result<&'a [LogRecord], SyscallError> { let count = syscall::log_read(cap.as_handle(), &mut self.cursor, out)?; + self.lost += self.cursor.lost; Ok(&out[..count]) } } diff --git a/userland/logd/src/main.rs b/userland/logd/src/main.rs index 31efeb7f217..68acb55a12d 100644 --- a/userland/logd/src/main.rs +++ b/userland/logd/src/main.rs @@ -479,7 +479,10 @@ impl Log { // The one call this program is built around. A refusal is not // survivable by retrying — the buffer and the rights are the // same every time — so it ends loudly. - Err(e) => panic!("logd: SYS_LOG_READ refused a {BATCH}-record buffer ({e:?})"), + Err(e) => panic!( + "logd: SYS_LOG_READ refused a {BATCH}-record buffer or a cursor ahead of a \ + shard ({e:?})" + ), }; let short = batch.len() < BATCH; out.extend(