Repository navigation
munmap and a FIXED mmap take only the length the mapping was asked for - #765
Merged
Merged
Conversation
`sys_munmap(addr, _size)` freed the mapping starting at `addr` whatever size it was passed: a caller unmapping a prefix was answered 0 and lost the part it kept, and a size past the mapping was answered 0 too. The size is now made a `PageSpan` by `vma::window().span`, the rounding `sys_mmap` gives its request, before any lock, and is refused with `InvalidArgument` unless it is the mapping's recorded size. So the length a caller mapped still unmaps it (4096 mapped, 4096 unmapped), a prefix, an overrun, zero and a length no window holds are refused having unmapped nothing, and an address that starts no mapping stays `NotFound`. Partial unmap is not given semantics: a mapping is one region and one `PageAlloc`, and the 2 MiB track's stage 4 ruled the refusal. libc's `munmap` answers every refusal with `EINVAL`, the one error POSIX defines for it; `NotFound` used to reach a C caller as `ENOENT`. No caller in the tree passed a size other than the one it mapped, and dlmalloc under std and libc never unmaps (`can_release_part` is false). `mmap_stress` reads each refusal and then both spans of the mapping back. Closes issues/munmap-ignores-the-size-it-is-passed.md. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RvnWQFcMuGqTHYhvSnTe8A
Collaborator
Author
|
Control patches for the QEMU measurement in the body, both against
diff --git a/tests/toyos.rs b/tests/toyos.rs
index 996ed86de..fa3fc0bd0 100644
--- a/tests/toyos.rs
+++ b/tests/toyos.rs
@@ -1666,7 +1666,7 @@ fn virt_job(profile: qemu::Profile, job: &str, said: &str) -> Result<(), String>
smp: 1,
kernel_features: toyos_build::build::TEST_KERNEL,
ready_marker: "control registers: SCTLR_EL1=",
- extra_root_files: vec![suite_bin(profile.arch(), VIRT_COPYOUT), suite_bin(profile.arch(), VIRT_RING0_TIMER)],
+ extra_root_files: vec![suite_bin(profile.arch(), VIRT_COPYOUT), suite_bin(profile.arch(), VIRT_RING0_TIMER), suite_bin(profile.arch(), "mmap_stress")],
..Default::default()
},
);
@@ -2407,7 +2407,7 @@ fn run_screen_test(name: &str, profile: qemu::Profile, test_config: &Path) -> Re
"virt_timer_preempts" => virt_job(profile, "preempt", "preempt: the counting thread was preempted twice"),
"virt_fp_isolation" => virt_job(profile, "fp_isolation", "fp_isolation: v0-v31, FPCR and FPSR survived"),
"virt_first_entry" => virt_job(profile, "first_entry", "first_entry: x1-x30 were zero"),
- "virt_unmap_touch" => virt_job(profile, "unmap_touch", UNMAP_TOUCH_SAID),
+ "virt_unmap_touch" => virt_job(profile, "test_rs_mmap_stress", "all mmap stress tests passed"),
"virt_debug_refused" => virt_job(
profile,
"debug_refused",
diff --git a/tests/virtjobcase/system.toml b/tests/virtjobcase/system.toml
index bc852a436..6c45482ac 100644
--- a/tests/virtjobcase/system.toml
+++ b/tests/virtjobcase/system.toml
@@ -14,7 +14,7 @@ syscap = ["logread"]
receives = ["power"]
# Four minutes from boot rather than one: every job here runs on an emulated
# CPU, and a job past the bound reboots the guest with the job named.
-args = ["--bound-ms=240000", "preempt", "fp_isolation", "first_entry", "unmap_touch", "debug_refused", "test_rs_ring0_timer_in_syscall", "test_rs_abuse_readonly_copyout"]
+args = ["--bound-ms=240000", "test_rs_mmap_stress", "preempt", "fp_isolation", "first_entry", "unmap_touch", "debug_refused", "test_rs_ring0_timer_in_syscall", "test_rs_abuse_readonly_copyout"]
[programs.kernelprobe]
diff --git a/kernel/src/syscall/vm.rs b/kernel/src/syscall/vm.rs
index 729f0af9e..63e30f7e7 100644
--- a/kernel/src/syscall/vm.rs
+++ b/kernel/src/syscall/vm.rs
@@ -175,26 +175,39 @@ pub(super) fn sys_mmap(req_addr: u64, size: u64, prot: MmapProt, flags: MmapFlag
}
}
-/// Frees an anonymous mapping and shoots down every sibling thread's
-/// translation for its range.
-pub(super) fn sys_munmap(addr: u64, _size: u64) -> u64 {
+/// Frees the anonymous mapping that starts at `addr`, whole, and shoots down
+/// every sibling thread's translation for its range. `size` names it as the
+/// `mmap` that made it did: one that does not round to the mapping's recorded
+/// span is `InvalidArgument` and unmaps nothing, a mapping being one region
+/// with no part to take away.
+pub(super) fn sys_munmap(addr: u64, size: u64) -> u64 {
+ let Some(span) = crate::vma::window().span(size) else {
+ return SyscallError::InvalidArgument.to_u64();
+ };
let pt = process::current_address_space();
let taken = process::with_process_data(|data| {
- let idx = data.mmap_regions.iter().position(|r| r.addr.raw() == addr)?;
+ let Some(idx) = data.mmap_regions.iter().position(|r| r.addr.raw() == addr) else {
+ return Err(SyscallError::NotFound);
+ };
+ if data.mmap_regions[idx].size as u64 != span.bytes() {
+ return Err(SyscallError::InvalidArgument);
+ }
let region = data.mmap_regions.swap_remove(idx);
data.free_count += 1;
pt.lock()
.free_and_unmap(region.addr)
.expect("an mmap region is registered in the address space it was placed in");
- Some(crate::mm::Unmapped::new(region))
+ Ok(crate::mm::Unmapped::new(region))
});
- let Some(unmapped) = taken else {
- return SyscallError::NotFound.to_u64();
- };
// Dropped here, outside the closure: the drop shoots down and waits, and a
// sibling can be spinning on the process-data lock.
- drop(unmapped);
- 0
+ match taken {
+ Ok(unmapped) => {
+ drop(unmapped);
+ 0
+ }
+ Err(e) => e.to_u64(),
+ }
}
/// The first `len` bytes of the shared memory object `handle` names, as a
diff --git a/toyos-abi/src/syscall.rs b/toyos-abi/src/syscall.rs
index 3e3260d78..82bebfb22 100644
--- a/toyos-abi/src/syscall.rs
+++ b/toyos-abi/src/syscall.rs
@@ -2276,10 +2276,15 @@ pub unsafe fn mmap(addr: *mut u8, size: usize, prot: MmapProt, flags: MmapFlags)
}
}
-/// Unmap a previously mapped region.
+/// Unmap the whole mapping `mmap` returned at `addr`.
+///
+/// `size` names it as the `mmap` that made it did: any length that rounds to
+/// the same 2 MiB span. No mapping starting at `addr` is `NotFound`; a `size`
+/// that rounds to another span — a prefix, more than the mapping, zero — is
+/// `InvalidArgument` and unmaps nothing. A mapping has no part to give back.
///
/// # Safety
-/// `addr` and `size` must describe a region previously returned by `mmap`.
+/// Nothing reaches the mapping at `addr` after this answers `Ok`.
pub unsafe fn munmap(addr: *mut u8, size: usize) -> Result<(), SyscallError> {
check_unit(syscall(SYS_MUNMAP, addr as u64, size as u64, 0, 0))
}
diff --git a/userland/libc/src/posix_io.rs b/userland/libc/src/posix_io.rs
index a08d3e581..cb57f3815 100644
--- a/userland/libc/src/posix_io.rs
+++ b/userland/libc/src/posix_io.rs
@@ -555,12 +555,18 @@ pub unsafe extern "C" fn madvise(addr: *mut u8, _len: usize, advice: i32) -> i32
}
}
+/// Unmaps the whole mapping at `addr` or nothing: the kernel refuses a `len`
+/// that names part of one, which POSIX would unmap. `EINVAL` is the one error
+/// POSIX gives `munmap`, so it is every refusal's.
#[no_mangle]
pub unsafe extern "C" fn munmap(addr: *mut u8, len: usize) -> i32 {
- // SAFETY: caller is responsible for addr/len matching a previous mmap
+ // SAFETY: the caller's, as C's `munmap` leaves it.
match unsafe { syscall::munmap(addr, len) } {
Ok(()) => 0,
- Err(e) => set_errno(e),
+ Err(_) => {
+ crate::errno::set(EINVAL);
+ -1
+ }
}
}
|
Collaborator
Author
|
T14 at head
|
Collaborator
Author
|
Review round 1 of Net: 5 files, +76 −53. Production +38 −14 ( BLOCKER
NOTE
Held, no finding
The landing rests on SEND BACK |
Review round 1 found the loss the closed issue named still there: the kernel compared the size it was passed after rounding it to a 2 MiB span, so through libc, whose page is 4096 bytes, `mmap(NULL, 8192, ...)` then `munmap(p, 4096)` answered 0 and took the page the caller kept. The `FIXED` arm of `mmap` had the same loss by another road: it replaced a mapping of "the same 2 MiB-rounded size", so a 4096-byte `MAP_FIXED` request at `p` replaced both pages with zeros and answered `p`. The kernel now records the length each `mmap` was asked for beside the rounded size (`MmapRegion.len`), and both `SYS_MUNMAP` and the `FIXED` replacement compare the two words as given; any other length is `InvalidArgument` with nothing unmapped or replaced. No rounding decides which lengths name a mapping, so the accepted set does not move when the 2 MiB track's stages 3 and 4 change the page size. Every caller in the tree passes the length it mapped, and every `FIXED` replacement in it (`abuse_mmap_regions`, `mmap_stress`, `tlb_shootdown_waits`) asks for the length of what it replaces. POSIX counts in pages, and that lives in libc: `mmap` asks the kernel for `memreq::whole_pages(len)` and `munmap` names the mapping by the same function of its own `len`. A `len` that ends in the mapping's last 4096-byte page is the whole mapping, as POSIX has it; one that ends short of it names a part and is refused with EINVAL, nothing unmapped. Tests: `whole_pages` on the host (`toyos-libc-copies`); `206_libc_refusals` maps two pages and reads the answer, errno and both pages for a fixed mapping of the first and for each partial `munmap`; `mmap_stress` adds the two lengths that round to its mapping's spans and are not its length, and for the first 4096 bytes of 8192 the `munmap` and the `FIXED` request. Filed: libc's `munmap` still refuses a part of a mapping that POSIX unmaps (`issues/libc-munmap-refuses-part-of-a-mapping.md`). The 2 MiB track's stage 4 says the refusal is in and what it still owes. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RvnWQFcMuGqTHYhvSnTe8A
Japabu
changed the title
Collaborator
Author
|
Fix round 1, head
diff --git a/tests/toyos.rs b/tests/toyos.rs
index b0ebf22de..6a3fe6304 100644
--- a/tests/toyos.rs
+++ b/tests/toyos.rs
@@ -2786,12 +2786,81 @@ fn scanout_wc(console: &str) -> Result<(), String> {
Ok(())
}
+/// THROWAWAY, never committed: `206_libc_refusals` and `mmap_stress` on one
+/// x86-64 QEMU boot of `tests/testcases`, judged under `nested_nmi_is_loud`'s
+/// name. Neither is a guest test; both ride the T14's boots.
+fn throwaway_munmap(test_config: &Path) -> Result<(), String> {
+ let case = "206_libc_refusals".to_string();
+ let c_bins = compile_c_tests(std::slice::from_ref(&case));
+ let rust_bins = vec![(
+ "mmap_stress".to_string(),
+ qemu::build_toyos_bin(
+ qemu::SUITE_ARCH,
+ &compile::repo_root().join("tests/toyos-rust-tests"),
+ "mmap_stress",
+ ),
+ )];
+ let mut guest = QemuInstance::boot_with_options(test_config, &c_bins, &rust_bins, BootOptions::default());
+ let c = guest.run_test("test_c_206_libc_refusals", Duration::from_secs(120));
+ eprintln!(
+ "[throwaway] 206_libc_refusals: exit={:?} error={:?}\n[throwaway] its stdout:\n{}[throwaway] end of its stdout",
+ c.exit_code,
+ c.error.as_ref().map(|e| e.to_string()),
+ c.stdout
+ );
+ let rs = guest.run_test("test_rs_mmap_stress", Duration::from_secs(120));
+ eprintln!(
+ "[throwaway] mmap_stress: exit={:?} error={:?}\n[throwaway] its stdout:\n{}[throwaway] end of its stdout",
+ rs.exit_code,
+ rs.error.as_ref().map(|e| e.to_string()),
+ rs.stdout
+ );
+ let mut wrong = Vec::new();
+ if c.exit_code != Some(0) {
+ wrong.push(format!("206_libc_refusals ended with {:?}", c.exit_code));
+ }
+ for line in [
+ "mmap the first of two pages fixed: -1, ENOMEM",
+ "munmap the first of two pages: -1, EINVAL",
+ "pages after it: f s",
+ "munmap the second of two pages: -1, EINVAL",
+ "munmap no bytes: -1, EINVAL",
+ "munmap a byte past two pages: -1, EINVAL",
+ "pages after them: f s",
+ "munmap two pages by a length into the second: 0, no errno",
+ "munmap them again: -1, EINVAL",
+ "limit after getrlimit: 0 0",
+ ] {
+ if !c.stdout.lines().any(|said| said.ends_with(line)) {
+ wrong.push(format!("206_libc_refusals did not say {line:?}"));
+ }
+ }
+ if rs.exit_code != Some(0) {
+ wrong.push(format!("mmap_stress ended with {:?}", rs.exit_code));
+ }
+ if !rs.stdout.contains("all mmap stress tests passed") {
+ wrong.push("mmap_stress did not say it passed".to_string());
+ }
+ for line in &wrong {
+ eprintln!("[throwaway] RED: {line}");
+ }
+ if wrong.is_empty() {
+ eprintln!("[throwaway] GREEN: both said every line");
+ Ok(())
+ } else {
+ Err(wrong.join("; "))
+ }
+}
+
/// Run the machine-shape test, which owns its QEMU: the machine shape *is* the
/// test.
fn run_machine_test(name: &str, test_config: &Path) -> Result<(), String> {
match name {
"iommu_virtio_platform" => common::iommu::iommu_virtio_platform(test_config),
- "nested_nmi_is_loud" => faults::nested_nmi_is_loud(test_config),
+ "nested_nmi_is_loud" => {
+ let _ = faults::nested_nmi_is_loud;
+ throwaway_munmap(test_config)
+ }
"machine_shutdown" => power::machine_shutdown(test_config),
"acpi_power_button" => power::acpi_power_button(test_config),
"acpi_mediated_access" => acpi_mediated_access(),
diff --git a/kernel/src/syscall/vm.rs b/kernel/src/syscall/vm.rs
index 9d9e34126..e80ce313e 100644
--- a/kernel/src/syscall/vm.rs
+++ b/kernel/src/syscall/vm.rs
@@ -196,7 +196,9 @@ pub(super) fn sys_munmap(addr: u64, size: u64) -> u64 {
let Some(idx) = data.mmap_regions.iter().position(|r| r.addr.raw() == addr) else {
return Err(SyscallError::NotFound);
};
- if data.mmap_regions[idx].len != size {
+ // CONTROL, never committed: the rounded compare review round 1 refused.
+ let _ = data.mmap_regions[idx].len;
+ if crate::vma::window().span(size).map(|span| span.bytes()) != Some(data.mmap_regions[idx].size as u64) {
return Err(SyscallError::InvalidArgument);
}
let region = data.mmap_regions.swap_remove(idx);
diff --git a/kernel/src/syscall/vm.rs b/kernel/src/syscall/vm.rs
index 9d9e34126..00fcd7961 100644
--- a/kernel/src/syscall/vm.rs
+++ b/kernel/src/syscall/vm.rs
@@ -107,7 +107,7 @@ pub(super) fn sys_mmap(req_addr: u64, size: u64, prot: MmapProt, flags: MmapFlag
let mine = data
.mmap_regions
.iter()
- .position(|r| r.addr == start && r.len == size);
+ .position(|r| r.addr == start && r.size == aligned); // CONTROL, never committed: the rounded compare.
match mine {
Some(idx) => Some(idx),
None => return Err(SyscallError::InvalidArgument),Exits, as the request script recorded them: In-tree |
Collaborator
Author
|
T14 at head
Twenty-seven rows over seven boots, none failed; 29 |
Japabu
marked this pull request as ready for review
October 8, 2026 11:34
Collaborator
Author
|
Review round 2 of Net: 12 files, +252 −66. Production +73 −23 ( Round 1's BLOCKERs
The ruling asked for: sections that hold every caller of BLOCKER
NOTE
Held, no finding
SEND BACK |
…its pages', and the issue names the MAP_FIXED half Review round 2 of #765 found no test that fails when libc's `mmap` passes the kernel `len` unrounded: `206_libc_refusals` mapped 4096 and 8192 alone. With the kernel comparing lengths to the byte and `munmap` rounding its own side, that mutation makes `mmap(NULL, 5000)` then `munmap(p, 5000)` answer `EINVAL`. The case maps 5000 bytes twice and unmaps once by 5000 and once by 8192; each reads 0. `issues/libc-munmap-refuses-part-of-a-mapping.md` gains the `MAP_FIXED` half of the gap (POSIX replaces whatever pages a fixed request overlaps; libc answers `MAP_FAILED`, `ENOMEM` over part of a mapping) with an exit that reads the `.expect` line pinning it. Its Owner and the 2 MiB track's stage 4 line said stage 4 is where a region that splits would be built; the track's stage 4 rules the refusal and no stage builds one, so both now say that. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RvnWQFcMuGqTHYhvSnTe8A
Collaborator
Author
|
Fix round 2, head
diff --git a/userland/libc/src/posix_io.rs b/userland/libc/src/posix_io.rs
index 47dfd48e7..d544cfd2e 100644
--- a/userland/libc/src/posix_io.rs
+++ b/userland/libc/src/posix_io.rs
@@ -524,8 +524,8 @@ pub unsafe extern "C" fn mmap(
// Asked for in whole pages, the length `munmap` names it by; more pages
// than a `usize` counts is the null the kernel answers a size it cannot map.
- let ptr = memreq::whole_pages(len)
- .map_or(core::ptr::null_mut(), |len| unsafe { syscall::mmap(addr, len, mp, mf) });
+ // MUTATION, never committed: the length as given, not in whole pages.
+ let ptr = unsafe { syscall::mmap(addr, len, mp, mf) };
if ptr.is_null() {
// The kernel's refusal does not say which it is: ENOMEM is POSIX's
// for a place or size the address space does not allow.
diff --git a/tests/toyos.rs b/tests/toyos.rs
index b0ebf22de..a7b29d3e8 100644
--- a/tests/toyos.rs
+++ b/tests/toyos.rs
@@ -2786,12 +2786,83 @@ fn scanout_wc(console: &str) -> Result<(), String> {
Ok(())
}
+/// THROWAWAY, never committed: `206_libc_refusals` and `mmap_stress` on one
+/// x86-64 QEMU boot of `tests/testcases`, judged under `nested_nmi_is_loud`'s
+/// name. Neither is a guest test; both ride the T14's boots.
+fn throwaway_munmap(test_config: &Path) -> Result<(), String> {
+ let case = "206_libc_refusals".to_string();
+ let c_bins = compile_c_tests(std::slice::from_ref(&case));
+ let rust_bins = vec![(
+ "mmap_stress".to_string(),
+ qemu::build_toyos_bin(
+ qemu::SUITE_ARCH,
+ &compile::repo_root().join("tests/toyos-rust-tests"),
+ "mmap_stress",
+ ),
+ )];
+ let mut guest = QemuInstance::boot_with_options(test_config, &c_bins, &rust_bins, BootOptions::default());
+ let c = guest.run_test("test_c_206_libc_refusals", Duration::from_secs(120));
+ eprintln!(
+ "[throwaway] 206_libc_refusals: exit={:?} error={:?}\n[throwaway] its stdout:\n{}[throwaway] end of its stdout",
+ c.exit_code,
+ c.error.as_ref().map(|e| e.to_string()),
+ c.stdout
+ );
+ let rs = guest.run_test("test_rs_mmap_stress", Duration::from_secs(120));
+ eprintln!(
+ "[throwaway] mmap_stress: exit={:?} error={:?}\n[throwaway] its stdout:\n{}[throwaway] end of its stdout",
+ rs.exit_code,
+ rs.error.as_ref().map(|e| e.to_string()),
+ rs.stdout
+ );
+ let mut wrong = Vec::new();
+ if c.exit_code != Some(0) {
+ wrong.push(format!("206_libc_refusals ended with {:?}", c.exit_code));
+ }
+ for line in [
+ "mmap the first of two pages fixed: -1, ENOMEM",
+ "munmap the first of two pages: -1, EINVAL",
+ "pages after it: f s",
+ "munmap the second of two pages: -1, EINVAL",
+ "munmap no bytes: -1, EINVAL",
+ "munmap a byte past two pages: -1, EINVAL",
+ "pages after them: f s",
+ "munmap two pages by a length into the second: 0, no errno",
+ "munmap them again: -1, EINVAL",
+ "munmap 5000 bytes by 5000: 0, no errno",
+ "munmap 5000 bytes by 8192: 0, no errno",
+ "limit after getrlimit: 0 0",
+ ] {
+ if !c.stdout.lines().any(|said| said.ends_with(line)) {
+ wrong.push(format!("206_libc_refusals did not say {line:?}"));
+ }
+ }
+ if rs.exit_code != Some(0) {
+ wrong.push(format!("mmap_stress ended with {:?}", rs.exit_code));
+ }
+ if !rs.stdout.contains("all mmap stress tests passed") {
+ wrong.push("mmap_stress did not say it passed".to_string());
+ }
+ for line in &wrong {
+ eprintln!("[throwaway] RED: {line}");
+ }
+ if wrong.is_empty() {
+ eprintln!("[throwaway] GREEN: both said every line");
+ Ok(())
+ } else {
+ Err(wrong.join("; "))
+ }
+}
+
/// Run the machine-shape test, which owns its QEMU: the machine shape *is* the
/// test.
fn run_machine_test(name: &str, test_config: &Path) -> Result<(), String> {
match name {
"iommu_virtio_platform" => common::iommu::iommu_virtio_platform(test_config),
- "nested_nmi_is_loud" => faults::nested_nmi_is_loud(test_config),
+ "nested_nmi_is_loud" => {
+ let _ = faults::nested_nmi_is_loud;
+ throwaway_munmap(test_config)
+ }
"machine_shutdown" => power::machine_shutdown(test_config),
"acpi_power_button" => power::acpi_power_button(test_config),
"acpi_mediated_access" => acpi_mediated_access(),Exits, as the request script recorded them: |
Collaborator
Author
|
T14 at head
The five other sections were read at |
Collaborator
Author
|
Review round 3 of Net: 12 files, +271 −66. Production +73 −23, unchanged since round 2 and accepted there. Tests +128 −1 (this round +8). Issues +70 −42. Round 2's BLOCKERs
Round 2's NOTEs: the BLOCKER
NOTE
Held, no finding
SEND BACK |
Collaborator
Author
|
Closing review of Net ( Round 3's BLOCKER
Rounds 1 and 2's stay closed: nothing moved. The body
The merge
BLOCKERNone. NOTE
ArmingIt may be armed as it stands. LAND |
Japabu
added a commit
that referenced
this pull request
Oct 8, 2026
Clean. #767's commits were already here (921b485), so the merge brings #765's files alone. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RvnWQFcMuGqTHYhvSnTe8A
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes
issues/munmap-ignores-the-size-it-is-passed.md. Head3b942f1aa:a5c566a72(merged withorigin/main1084ddc9a) plus one commit that touches a C test, its.expectand two issue files.git diff --quiet a5c566a72 3b942f1aa -- kernel/ toyos-abi/exits 0, and so does the same overuserland/ toyos/ tests/toyos-rust-tests/: no kernel, ABI, libc, SDK or Rust guest program moved between the two heads.origin/mainhas moved since (#756, #761, #763) and is not merged in again; the merge queue tests the merged tree.What changed, per decision
The kernel records the length each
mmapwas asked for, andSYS_MUNMAPtakes only that length.sys_munmap(addr, _size)freed the mapping ataddrwhatever size came with it. The first round compared the size after rounding it to a 2 MiB span, which review round 1 showed keeps the loss for any partial length inside one span: through libc,mmap(NULL, 8192, …)thenmunmap(p, 4096)answered 0 and took the page the caller kept. NowMmapRegion.lenholds thesizewordmmapwas passed, andsys_munmapcompares it with thesizeword it is passed, as given, with no arithmetic on either.The edges of "the same length", at the kernel:
usize::MAX, and a length that only rounds to the same pages (8191 or 4096 of 8192;2 MiB + 1of4 MiB):InvalidArgument, nothing unmapped;NotFound, whatever the length.No rounding decides which lengths name a mapping, so the accepted set does not move when stages 3 and 4 of the 2 MiB track change the page size.
POSIX counts in pages, and that is libc's.
userland/libc'smmapasks the kernel formemreq::whole_pages(len)(the length rounded up to the 4096-byte pagesysconf(_SC_PAGESIZE)answers) andmunmapnames the mapping by the same function of its ownlen. So alenthat ends anywhere in the mapping's last page is the whole mapping, as POSIX has it, and one that ends short of that page names a part and is refused. Why this split and not page-rounding in the kernel: the kernel would need the page size libc tells its callers, and its accepted set would then depend on it. What callers pass: every in-tree caller oftoyos_abi::syscall::munmappasses the length it mapped (read call by call, list in the round-1 comment); memmap2 on other Unixes passes the length it mapped, not a rounded one, and on ToyOS maps nothing (toyos.rsis a heap buffer); a C program that mapslenand unmaps a page-roundedlen, or the reverse, is correct POSIX and is accepted by the libc rule. That last claim is now measured: see the 5000-byte case below.The
FIXEDarm ofmmaphad the same loss by another road, and takes the same rule (the orchestrator's direction mid-round). It replaced a mapping of "the same 2 MiB-rounded size", so a 4096-byteMAP_FIXEDrequest at the address of an 8192-byte mapping replaced both pages with zeros and answered the address. It now replaces a mapping only when the length asked for isMmapRegion.len; any other isInvalidArgument, nothing replaced. EveryFIXEDreplacement in the tree (abuse_mmap_regionsat the region bound,mmap_stress,tlb_shootdown_waits) asks for the length of what it replaces.libc's
munmapanswers every refusal withEINVAL, the only error POSIX defines for it.Why not the other two designs (unchanged from round 0, and held by the review): partial unmap needs
vma::Regionand itsPageAllocto split, and the 2 MiB track's stage 4 rules the refusal instead; removing the argument leaves libc unable to refuse a partiallenwithout a ledger of its own.Known gap against POSIX, filed
issues/libc-munmap-refuses-part-of-a-mapping.md, in two halves:munmap: POSIX unmaps any whole pages of a mapping; libc's unmaps one whole mapping or nothing. A prefix, suffix, interior range, a range over two mappings and a range holding no mapping are all-1/EINVALwith nothing unmapped.mmapwithMAP_FIXED(a paragraph new at this head, review round 2's NOTE): POSIX replaces whatever pages a fixed request overlaps; libc's places one over free pages or over one whole mapping by its page count, and over some of a mapping's pages answersMAP_FAILED/ENOMEMwith nothing replaced.206_libc_refusals.expect:41pins that answer, and the issue's exit now reads that line as well as themunmapone.After this change nothing a caller kept is taken; what remains is the refusal.
Stage 4, where the body and the issue disagreed. Round 2 found this body saying the track's stage 4 rules partial unmap out, and the issue's Owner (with a sentence this branch had added to the track's stage 4 line) saying stage 4 is where a region that splits would be built. The track's text decides it: stage 4 on
mainreads "munmaprefuses a size that is not the whole mapping, by the orchestrator's ruling", and no stage of the track names a region that splits. The issue and the added sentence were wrong. At this head the issue's Owner says the orchestrator ruled the refusal in stage 4 and that no stage of the track builds a region that splits, and the track's stage 4 line says the refusal is in and names the issue as what it costs a C program, owing nothing more.Callers outside the tree: what was searched and what it establishes
~/.cargo/git/checkouts: files namingmunmapare memmap2'sunix.rsandadvice.rs, stacker'smmap_stack_restore_guard.rsand softbuffer'sx11.rs. None is on a ToyOS path: memmap2 routes ToyOS totoyos.rs, stacker groups ToyOS with the targets that map nothing, and the X11 backend is not built.rust/library:std/src/sys/pal/unix/stack_overflow.rsand backtrace'smmap_unix.rs, neither compiled for ToyOS.clang,libcxx,libcxxabi), so a working-tree search of it sees none of the rest; the first round's search and the reviewer's were of that.git grep munmapat the pinned commitceaf0fbb8finds no file underlibcxx,libcxxabiorlibunwind, the three runtimes ToyOS builds and links into C++ programs. It finds three files underllvm/lib/Support:Unix/Memory.incandUnix/Path.inc, each passing the length it mapped (read), andrpmalloc/rpmalloc.c, not read. Support builds for ToyOS hosts at that commit; nothing was run to show which of these a ToyOS program reaches.can_release_part(release_unused_segments, false here); the otherfreeis for a chunk flagged mmapped, which the crate never makes. Neither runs.Checks (high-risk: memory management, the ABI)
Host.
tests/libc-arch/src/memory_refusals.rs::a_length_names_the_pages_it_reaches_intoholdswhole_pages: zero, the page boundaries, the top ofusize, and which lengths name an 8192-byte mapping. The kernel's decision is two word equalities; no host test re-tests==. The host test callswhole_pagesand nevermmap, so it cannot see whethermmapuses it; the 5000-byte case is what does.Running kernel, native.
mmap_stressmaps two 2 MiB spans and unmaps: the first span, one byte, one byte past, one byte short, the first span and a byte, zero,usize::MAX— each must beInvalidArgument, with both spans read back before the answer is judged. Then 8192 mapped:munmapof 4096 refused and the second page's byte read back; aFIXEDrequest for 4096 refused and the byte read back; aFIXEDrequest for 8192 replaces.Running kernel, through libc.
tests/testcases/tinycc/206_libc_refusals.cmaps two 4096-byte pages and prints the answer,errnoand both pages for: a fixed mapping of the first page;munmapof the first page, the second page, no bytes, a byte past both; thenmunmapby a length into the second page (0), and again (EINVAL). New at this head, for review round 2's second BLOCKER: it maps 5000 bytes and unmaps them by 5000, maps 5000 again and unmaps by 8192. Read under QEMU at3b942f1aa(green.log):Neither program is a QEMU guest test and this branch makes neither one: a host test reaches
whole_pages, and their verdict of record is the T14's. No guest test is added or changed;206_libc_refusalsis changed, not added, sotests/testcases/LICENSE's count does not move.Negative controls and the mutation, taken under a throwaway QEMU registration. An uncommitted patch ran both programs on one x86-64 QEMU boot of
tests/testcases, inside the test harness, and judged them by the lines above undernested_nmi_is_loud's name; it was reverted each time and the tree left clean. It is a throwaway, not a guest test:measure.patchata5c566a72, andmeasure-r2.patchat3b942f1aa, which is the same with the two 5000-byte lines added to what it requires. All patches are in the pull request's comments.3b942f1aacargo test --test toyos-build -- nested_nmi_is_loud --nocapture===TEST_END test_c_206_libc_refusals exit=0===,===TEST_END test_rs_mmap_stress exit=0===mutation-libc-mmap.patch(libc'smmappasseslento the kernel unrounded; review round 2's mutation)3b942f1aamunmap 5000 bytes by 5000: -1, EINVAL,munmap 5000 bytes by 8192: -1, EINVAL;[throwaway] RED: 206_libc_refusals did not say "munmap 5000 bytes by 5000: 0, no errno"and the same forby 8192;mmap_stressexit=0a5c566a72TEST_END … exit=0control-munmap.patch(sys_munmaprounds)a5c566a72206_libc_refusals:SEGFAULT tid=0: read unmapped address at 0xfffe800000,exit=-1;mmap_stress:SEGFAULT … at 0xfffea00000,exit=-1control-fixed.patch(theFIXEDarm rounds)a5c566a72mmap_stress:panicked at src/bin/mmap_stress.rs:101:5 … a FIXED request for the first 4096 bytes of 8192 replaced the second, left: 0 right: 119,exit=101;206_libc_refusals:SEGFAULT … at 0xfffe800000,exit=-1The two kernel controls were not run again at
3b942f1aa. They stand on the empty diff this body opens with: each reverts one compare inkernel/src/syscall/vm.rs, which is byte-identical at both heads, as are libc andmmap_stress; the commit between adds two calls at the end of the C program'smunmapblock, after every line the controls red on.Under the mutation the guest runner's own
TEST_ENDfor the C program isexit=0: the throwaway boot does not judge against the.expect, and the program returns 0 whatever it printed, so the red there is the throwaway's line check. On the T14's C corpus boot the program is judged in the guest against the committed.expect, which holds both lines.In both kernel control arms the C program's buffered output died with it, so its log shows the fault and not the line. Under
control-munmapthe fault is the read of the first page aftermunmap(pair, 4096). Undercontrol-fixedit is inferred, not printed: the replacement leaves a 4096-byte mapping, which the nextmunmap(pair, 4096)then names exactly and takes.Oracle. POSIX, for libc: which lengths name a whole mapping (pages containing the range), and
EINVALasmunmap's error. For partial unmap and for a fixed mapping over part of one, POSIX says the opposite of what this does; that is the filed gap.Gates
cargo metadata --locked --format-version 13b942f1aacargo test -p toyos-libc-copies3b942f1aacargo run -- --build-only3b942f1aa--metal --metal-readback <dir> <filter>:206_libc_refusals,munmap_reissues)3b942f1aacargo test --test toyos-build -- virt_unmap_toucha5c566a72cargo test --test toyos-build -- virt_readonly_copyouta5c566a72cargo test --test toyos-build -- virt_smpa5c566a72cargo run -- --ci host3b942f1aa:hostsuccess (job 113295620913),[ci] Host: 78 step(s), all greenguest / suite, aftertoolchain / build)toolchain / buildsuccess (113295621156),guest / suitesuccess (113296851511):PASS virt_unmap_touch,PASS virt_readonly_copyout,test result: ok. 32 passed, 32 total. It runs declared tests only, so it reachessys_munmapthrough those and the SMP rows and executes none of the new assertions, which are the T14'sThe three guest tests were not run again at
3b942f1aa: nothing underkernel/,toyos-abi/,userland/,toyos/ortests/toyos-rust-tests/moved (the empty diffs above).The T14
Read at
a5c566a72(the orchestrator's run, PR comment 6058950679; seven images staged from the clean tree at that head, every boot exit 0, every judge exit 0):mmapshared,cacaefe0…259c884dmmap_stress,mmap_prot,std_mmap,memmap_exec_refusedabuse_shared,a3230871…e241ad8d;shared-debug,2b3a35da…104e131cfutex_wake_countsshared,ca98c21e…7950ee7cprocess_bound_regionstestcases-bounds,e8d5b63a…85677115test_rs_abuse_mmap_regionsexit 0)tlb_shootdown_waitstestcases-debug,940d4d3b…9f3e8918206_libc_refusalsccorpus,3a9499d5…8f38bff03===TEST_END 206_libc_refusals exit=0===)Twenty-seven rows over seven boots, none failed; 29
===TEST_ENDlines in the readbacks, eachexit=0.The first five sections' readings carry to
3b942f1aaand are not taken again: every program they boot, the kernel and the ABI are byte-identical at both heads (the two empty diffs). The sixth does not carry: its program changed.A correction to what this body said before. It listed
munmap_reissues_read_windowandmunmap_reissues_second_read_windowin themmapsection. They were never in it: the filtermmapdoes not containmunmap_reissues(the section's stage log reads0 registration(s) and 4 shared member(s)), so the two were booted at no head, on no machine and under no QEMU. Both callmunmap, the second through a 4 MiB reservation released and twoFIXEDplacements over it. The lengths they pass were read; that is not a measurement.Read at
3b942f1aaby the orchestrator (comment 6059243360): the C corpus boot,206_libc_refusalsexit 0;munmap_reissues, both members exit 0:206_libc_refusalsccorpus,22044348430d0862b91da4547e65a1f15fb7e59898a4c950fa63f09ca28567fd206_libc_refusals, judged in the guest against the.expectwith the two 5000-byte linesmunmap_reissuesshared,21daf70ec210ebceb0d58f305016f8861b370e6b6d640b150a2242ff1d883307test_rs_munmap_reissues_read_window,test_rs_munmap_reissues_second_read_window(stage log:0 registration(s) and 2 shared member(s) over 1 boot(s))With those two, the sections hold every program in the tree that calls
munmapor aFIXEDmmap. Notsharedwhole: the suite takes one substring filter, and none selects a whole shared boot without the whole metal table. Members ofsharedthat call neither are not booted at either head.Unsure of
hostandguest / suiteagain on the merged tree.rpmalloc.cin LLVM Support was not read, and no LLVM tool was run on ToyOS.EINVALorENOMEM. None is in the tree, read acrossuserland/and the C corpus.Net: 12 files, +271 −66. Production +73 −23 (
kernel/src/syscall/vm.rs+35 −15,kernel/src/process.rs+2,toyos-abi/src/syscall.rs+8 −3,userland/libc/src/posix_io.rs+18 −5,userland/libc/src/memreq.rs+10). Tests +128 −1. Issues +70 −42.🤖 Generated with Claude Code
https://claude.ai/code/session_01RvnWQFcMuGqTHYhvSnTe8A