From 3f1bf99b2ade568a09e3a483ec8bf0f75ca36608 Mon Sep 17 00:00:00 2001 From: Ruihan Li Date: Sun, 22 Jun 2025 16:12:50 +0800 Subject: [PATCH] Make `mremap` behavior consistent with Linux --- kernel/src/syscall/mremap.rs | 8 ++-- kernel/src/vm/vmar/mod.rs | 64 +++++++++++++++++++------------- test/apps/mmap/mmap_and_mremap.c | 61 +++++++++++++++++++++++++++--- 3 files changed, 96 insertions(+), 37 deletions(-) diff --git a/kernel/src/syscall/mremap.rs b/kernel/src/syscall/mremap.rs index 48dbae710..d2214577b 100644 --- a/kernel/src/syscall/mremap.rs +++ b/kernel/src/syscall/mremap.rs @@ -51,11 +51,9 @@ fn do_sys_mremap( let root_vmar = user_space.root_vmar(); if !flags.contains(MremapFlags::MREMAP_FIXED) && new_size <= old_size { - if new_size < old_size { - // We can shrink a old range which spans multiple mappings. See - // . - root_vmar.resize_mapping(old_addr, old_size, new_size, false)?; - } + // We can shrink a old range which spans multiple mappings. See + // . + root_vmar.resize_mapping(old_addr, old_size, new_size, false)?; return Ok(old_addr); } diff --git a/kernel/src/vm/vmar/mod.rs b/kernel/src/vm/vmar/mod.rs index 53127999f..f586dd5de 100644 --- a/kernel/src/vm/vmar/mod.rs +++ b/kernel/src/vm/vmar/mod.rs @@ -197,7 +197,7 @@ impl VmarInner { { Ok(vm_mapping.map_to_addr()) } else { - return_errno_with_message!(Errno::EFAULT, "The range must lie in a single mapping"); + return_errno_with_message!(Errno::EFAULT, "the range must lie in a single mapping"); } } @@ -377,22 +377,18 @@ impl VmarInner { debug_assert_eq!(old_size % PAGE_SIZE, 0); debug_assert_eq!(new_size % PAGE_SIZE, 0); - // FIXME: We should check whether all existing ranges in - // `map_addr..map_addr + old_size` have a mapping. If not, - // we should return a `Err`. - if new_size == 0 { - return_errno_with_message!(Errno::EINVAL, "can not resize a mapping to 0 size"); + return_errno_with_message!(Errno::EINVAL, "cannot resize a mapping to 0 size"); } if new_size == old_size { return Ok(()); } - let old_map_end = map_addr + old_size; + let old_map_end = map_addr.checked_add(old_size).ok_or(Errno::EINVAL)?; let new_map_end = map_addr.checked_add(new_size).ok_or(Errno::EINVAL)?; if !is_userspace_vaddr(new_map_end - 1) { - return_errno_with_message!(Errno::EINVAL, "resize to a invalid new size"); + return_errno_with_message!(Errno::EINVAL, "resize to an invalid new size"); } if new_size < old_size { @@ -572,13 +568,19 @@ impl Vmar_ { new_size: usize, check_single_mapping: bool, ) -> Result<()> { - let mut rss_delta = RssDelta::new(self); let mut inner = self.inner.write(); + let mut rss_delta = RssDelta::new(self); + if check_single_mapping { inner.check_lies_in_single_mapping(map_addr, old_size)?; + } else if inner.vm_mappings.find_one(&map_addr).is_none() { + return_errno_with_message!(Errno::EFAULT, "there is no mapping at the old address") } - inner.resize_mapping(&self.vm_space, map_addr, old_size, new_size, &mut rss_delta)?; - Ok(()) + // FIXME: We should check whether all existing ranges in + // `map_addr..map_addr + old_size` have a mapping. If not, + // we should return an `Err`. + + inner.resize_mapping(&self.vm_space, map_addr, old_size, new_size, &mut rss_delta) } fn remap( @@ -593,12 +595,29 @@ impl Vmar_ { debug_assert_eq!(new_size % PAGE_SIZE, 0); let mut inner = self.inner.write(); - let old_mapping_addr = inner.check_lies_in_single_mapping(old_addr, old_size)?; - - let mut old_range = old_addr..old_addr + old_size; - let mut old_size = old_size; let mut rss_delta = RssDelta::new(self); + if inner.vm_mappings.find_one(&old_addr).is_none() { + return_errno_with_message!( + Errno::EFAULT, + "remap: there is no mapping at the old address" + ) + } + + // Shrink the old mapping first. + old_addr.checked_add(old_size).ok_or(Errno::EINVAL)?; + let (old_size, old_range) = if new_size < old_size { + inner.alloc_free_region_exact_truncate( + &self.vm_space, + old_addr + new_size, + old_size - new_size, + &mut rss_delta, + )?; + (new_size, old_addr..old_addr + new_size) + } else { + (old_size, old_addr..old_addr + old_size) + }; + // Allocate a new free region that does not overlap with the old range. let new_range = if let Some(new_addr) = new_addr { let new_range = new_addr..new_addr.checked_add(new_size).ok_or(Errno::EINVAL)?; @@ -606,7 +625,7 @@ impl Vmar_ { || !is_userspace_vaddr(new_addr) || !is_userspace_vaddr(new_range.end - 1) { - return_errno_with_message!(Errno::EINVAL, "remap: invalid fixed new addr"); + return_errno_with_message!(Errno::EINVAL, "remap: invalid fixed new address"); } if is_intersected(&old_range, &new_range) { return_errno_with_message!( @@ -626,6 +645,7 @@ impl Vmar_ { // Create a new `VmMapping`. let old_mapping = { + let old_mapping_addr = inner.check_lies_in_single_mapping(old_addr, old_size)?; let vm_mapping = inner.remove(&old_mapping_addr).unwrap(); let (left, old_mapping, right) = vm_mapping.split_range(&old_range); if let Some(left) = left { @@ -634,17 +654,9 @@ impl Vmar_ { if let Some(right) = right { inner.insert_without_try_merge(right); } - if new_size < old_size { - let (old_mapping, taken) = old_mapping.split(old_range.start + new_size).unwrap(); - rss_delta.add(taken.rss_type(), -(taken.unmap(&self.vm_space) as isize)); - old_size = new_size; - old_range = old_range.start..(old_range.start + old_size); - old_mapping - } else { - old_mapping - } + old_mapping }; - // Now we can ensure that `new_size >= old_size`. + // Note that we have ensured that `new_size >= old_size` at the beginning. let new_mapping = old_mapping.clone_for_remap_at(new_range.start).unwrap(); inner.insert_try_merge(new_mapping.enlarge(new_size - old_size)); diff --git a/test/apps/mmap/mmap_and_mremap.c b/test/apps/mmap/mmap_and_mremap.c index 2f678d2dd..08fdad65d 100644 --- a/test/apps/mmap/mmap_and_mremap.c +++ b/test/apps/mmap/mmap_and_mremap.c @@ -2,18 +2,18 @@ #define _GNU_SOURCE +#include #include #include #include -#include -#include + #include "../test.h" #define PAGE_SIZE 4096 FN_TEST(mremap) { - char *addr = TEST_SUCC(mmap(NULL, 3 * PAGE_SIZE, PROT_READ | PROT_WRITE, + char *addr = TEST_SUCC(mmap(NULL, 5 * PAGE_SIZE, PROT_READ | PROT_WRITE, MAP_PRIVATE | MAP_ANONYMOUS, -1, 0)); TEST_SUCC(munmap(addr + 2 * PAGE_SIZE, PAGE_SIZE)); @@ -45,7 +45,17 @@ FN_TEST(mremap) // FIXME: Asterinas returns EACCESS here, which is not a correct error code. // TEST_ERRNO(mremap(addr, PAGE_SIZE, 2 * PAGE_SIZE, 0), ENOMEM); - TEST_SUCC(munmap(addr, 2 * PAGE_SIZE)); + char *hole = addr + 2 * PAGE_SIZE; + + // There is no mapping at the old address. + TEST_ERRNO(mremap(hole, 2 * PAGE_SIZE, PAGE_SIZE, 0), EFAULT); + TEST_ERRNO(mremap(hole, 2 * PAGE_SIZE, PAGE_SIZE, MREMAP_MAYMOVE), + EFAULT); + TEST_ERRNO(mremap(hole, 2 * PAGE_SIZE, PAGE_SIZE, + MREMAP_MAYMOVE | MREMAP_FIXED, addr), + EFAULT); + + TEST_SUCC(munmap(addr, 5 * PAGE_SIZE)); } END_TEST() @@ -70,6 +80,8 @@ FN_TEST(mmap_and_mremap) char *new_addr = CHECK_MM( mremap(addr, PAGE_SIZE, 3 * PAGE_SIZE, MREMAP_MAYMOVE)); + // Ensure that the mapping at the old address does not exist any more. + TEST_ERRNO(mremap(addr, PAGE_SIZE, PAGE_SIZE, 0), EFAULT); // The following operation (if uncommented) would cause a segmentation fault. // strcpy(addr, "Writing to old address"); @@ -154,7 +166,44 @@ FN_TEST(mmap_and_mremap_auto_merge_file) TEST_RES(strcmp(new_addr, content), _ret == 0); TEST_SUCC(munmap(new_addr, 3 * PAGE_SIZE)); - close(fd); - unlink(filename); + TEST_SUCC(close(fd)); + TEST_SUCC(unlink(filename)); +} +END_TEST() + +FN_TEST(mremap_may_fail_after_unmap) +{ + int fd = TEST_SUCC(open("/bin/sh", O_RDONLY)); + char *addr = TEST_SUCC( + mmap(NULL, 5 * PAGE_SIZE, PROT_READ, MAP_SHARED, fd, 0)); + TEST_SUCC(munmap(addr + PAGE_SIZE, 4 * PAGE_SIZE)); + TEST_RES(mmap(addr + PAGE_SIZE, 4 * PAGE_SIZE, PROT_READ | PROT_WRITE, + MAP_PRIVATE | MAP_ANONYMOUS | MAP_FIXED, -1, 0), + _ret == addr + PAGE_SIZE); + TEST_SUCC(close(fd)); + + // Shared, Anonymous, Anonymous, Anonymous, Anonymous + // [ Page 1-3: source ] [ Page 4-5: target ] + // + // This `mremap()` will fail because Page 1 and Page 2 are of different + // types. However, it still shrinks the source mapping and clears the + // target mapping, so Page 3, 4, and 5 will be unmapped. + + TEST_ERRNO(mremap(addr, 3 * PAGE_SIZE, 2 * PAGE_SIZE, + MREMAP_MAYMOVE | MREMAP_FIXED, addr + 3 * PAGE_SIZE), + EFAULT); + // Page 1 and 2 still exists. + TEST_RES(mremap(addr, PAGE_SIZE, PAGE_SIZE, 0), _ret == addr); + TEST_RES(mremap(addr + PAGE_SIZE, PAGE_SIZE, PAGE_SIZE, 0), + _ret == addr + PAGE_SIZE); + // Page 3, 4, and 5 do not exist anymore. + TEST_ERRNO(mremap(addr + 2 * PAGE_SIZE, PAGE_SIZE, PAGE_SIZE, 0), + EFAULT); + TEST_ERRNO(mremap(addr + 3 * PAGE_SIZE, PAGE_SIZE, PAGE_SIZE, 0), + EFAULT); + TEST_ERRNO(mremap(addr + 4 * PAGE_SIZE, PAGE_SIZE, PAGE_SIZE, 0), + EFAULT); + + TEST_SUCC(munmap(addr, 5 * PAGE_SIZE)); } END_TEST()