From c4b497ee8b35fcdc9fd4b9284a4dbc0b5499a3f6 Mon Sep 17 00:00:00 2001 From: Adam Reichold Date: Sat, 23 Sep 2023 22:30:02 +0200 Subject: [PATCH] Change madvise API to differentiate between safe and unsafe advice options. --- src/advice.rs | 127 ++++++++++++++++++++++++++++++++++++++------------ src/lib.rs | 53 +++++++++++++-------- src/unix.rs | 2 +- 3 files changed, 133 insertions(+), 49 deletions(-) diff --git a/src/advice.rs b/src/advice.rs index a53a7f5c..9b25e4ff 100644 --- a/src/advice.rs +++ b/src/advice.rs @@ -1,35 +1,40 @@ -// The use statement is needed for the `cargo docs` -#[allow(unused_imports)] -use crate::{Mmap, MmapMut}; - -/// Values supported by [`Mmap::advise`] and [`MmapMut::advise`] functions. +/// Values supported by [`Mmap::advise`][crate::Mmap::advise] and [`MmapMut::advise`][crate::MmapMut::advise] functions. /// See [madvise()](https://man7.org/linux/man-pages/man2/madvise.2.html) map page. -#[repr(i32)] -#[derive(Clone, Copy, Debug, Eq, PartialEq, Hash)] -pub enum Advice { +#[derive(Debug, Eq, PartialEq, Hash)] +pub struct Advice(pub(crate) libc::c_int); + +impl Advice { /// **MADV_NORMAL** /// /// No special treatment. This is the default. - Normal = libc::MADV_NORMAL, + pub fn normal() -> Self { + Self(libc::MADV_NORMAL) + } /// **MADV_RANDOM** /// /// Expect page references in random order. (Hence, read /// ahead may be less useful than normally.) - Random = libc::MADV_RANDOM, + pub fn random() -> Self { + Self(libc::MADV_RANDOM) + } /// **MADV_SEQUENTIAL** /// /// Expect page references in sequential order. (Hence, pages /// in the given range can be aggressively read ahead, and may /// be freed soon after they are accessed.) - Sequential = libc::MADV_SEQUENTIAL, + pub fn sequential() -> Self { + Self(libc::MADV_SEQUENTIAL) + } /// **MADV_WILLNEED** /// /// Expect access in the near future. (Hence, it might be a /// good idea to read some pages ahead.) - WillNeed = libc::MADV_WILLNEED, + pub fn will_need() -> Self { + Self(libc::MADV_WILLNEED) + } /// **MADV_DONTNEED** /// @@ -60,7 +65,15 @@ pub enum Advice { /// not managed by the virtual memory subsystem. Such pages /// are typically created by device drivers that map the pages /// into user space.) - DontNeed = libc::MADV_DONTNEED, + /// + /// # Safety + /// + /// Using the returned value with conceptually write to the + /// mapped pages, i.e. borrowing the mapping when the pages + /// are freed results in undefined behaviour. + pub unsafe fn dont_need() -> Self { + Self(libc::MADV_DONTNEED) + } // // The rest are Linux-specific @@ -88,8 +101,16 @@ pub enum Advice { /// 4.12, when freeing pages on a swapless system, the pages /// in the given range are freed instantly, regardless of /// memory pressure. + /// + /// # Safety + /// + /// Using the returned value with conceptually write to the + /// mapped pages, i.e. borrowing the mapping while the pages + /// are still being freed results in undefined behaviour. #[cfg(any(target_os = "linux", target_os = "macos", target_os = "ios"))] - Free = libc::MADV_FREE, + pub unsafe fn free() -> Self { + Self(libc::MADV_FREE) + } /// **MADV_REMOVE** - Linux only (since Linux 2.6.16) /// @@ -109,8 +130,16 @@ pub enum Advice { /// supports MADV_REMOVE. Hugetlbfs fails with the error /// EINVAL and other filesystems fail with the error /// EOPNOTSUPP. + /// + /// # Safety + /// + /// Using the returned value with conceptually write to the + /// mapped pages, i.e. borrowing the mapping when the pages + /// are freed results in undefined behaviour. #[cfg(target_os = "linux")] - Remove = libc::MADV_REMOVE, + pub unsafe fn remove() -> Self { + Self(libc::MADV_REMOVE) + } /// **MADV_DONTFORK** - Linux only (since Linux 2.6.16) /// @@ -121,14 +150,18 @@ pub enum Advice { /// relocations cause problems for hardware that DMAs into the /// page.) #[cfg(target_os = "linux")] - DontFork = libc::MADV_DONTFORK, + pub fn dont_fork() -> Self { + Self(libc::MADV_DONTFORK) + } /// **MADV_DOFORK** - Linux only (since Linux 2.6.16) /// /// Undo the effect of MADV_DONTFORK, restoring the default /// behavior, whereby a mapping is inherited across fork(2). #[cfg(target_os = "linux")] - DoFork = libc::MADV_DOFORK, + pub fn do_fork() -> Self { + Self(libc::MADV_DOFORK) + } /// **MADV_MERGEABLE** - Linux only (since Linux 2.6.32) /// @@ -151,7 +184,9 @@ pub enum Advice { /// available only if the kernel was configured with /// CONFIG_KSM. #[cfg(target_os = "linux")] - Mergeable = libc::MADV_MERGEABLE, + pub fn mergeable() -> Self { + Self(libc::MADV_MERGEABLE) + } /// **MADV_UNMERGEABLE** - Linux only (since Linux 2.6.32) /// @@ -160,7 +195,9 @@ pub enum Advice { /// it had merged in the address range specified by addr and /// length. #[cfg(target_os = "linux")] - Unmergeable = libc::MADV_UNMERGEABLE, + pub fn unmergeable() -> Self { + Self(libc::MADV_UNMERGEABLE) + } /// **MADV_HUGEPAGE** - Linux only (since Linux 2.6.38) /// @@ -199,14 +236,18 @@ pub enum Advice { /// available only if the kernel was configured with /// CONFIG_TRANSPARENT_HUGEPAGE. #[cfg(target_os = "linux")] - HugePage = libc::MADV_HUGEPAGE, + pub fn huge_page() -> Self { + Self(libc::MADV_HUGEPAGE) + } /// **MADV_NOHUGEPAGE** - Linux only (since Linux 2.6.38) /// /// Ensures that memory in the address range specified by addr /// and length will not be backed by transparent hugepages. #[cfg(target_os = "linux")] - NoHugePage = libc::MADV_NOHUGEPAGE, + pub fn no_huge_page() -> Self { + Self(libc::MADV_NOHUGEPAGE) + } /// **MADV_DONTDUMP** - Linux only (since Linux 3.4) /// @@ -218,13 +259,17 @@ pub enum Advice { /// set via the `/proc/[pid]/coredump_filter` file (see /// core(5)). #[cfg(target_os = "linux")] - DontDump = libc::MADV_DONTDUMP, + pub fn dont_dump() -> Self { + Self(libc::MADV_DONTDUMP) + } /// **MADV_DODUMP** - Linux only (since Linux 3.4) /// /// Undo the effect of an earlier MADV_DONTDUMP. #[cfg(target_os = "linux")] - DoDump = libc::MADV_DODUMP, + pub fn do_dump() -> Self { + Self(libc::MADV_DODUMP) + } /// **MADV_HWPOISON** - Linux only (since Linux 2.6.32) /// @@ -239,7 +284,9 @@ pub enum Advice { /// handling code; it is available only if the kernel was /// configured with CONFIG_MEMORY_FAILURE. #[cfg(target_os = "linux")] - HwPoison = libc::MADV_HWPOISON, + pub fn hw_poison() -> Self { + Self(libc::MADV_HWPOISON) + } /// **MADV_POPULATE_READ** - Linux only (since Linux 5.14) /// @@ -274,7 +321,9 @@ pub enum Advice { /// Note that with MADV_POPULATE_READ, the process can be killed /// at any moment when the system runs out of memory. #[cfg(target_os = "linux")] - PopulateRead = libc::MADV_POPULATE_READ, + pub fn populate_read() -> Self { + Self(libc::MADV_POPULATE_READ) + } /// **MADV_POPULATE_WRITE** - Linux only (since Linux 5.14) /// @@ -306,7 +355,9 @@ pub enum Advice { /// Note that with MADV_POPULATE_WRITE, the process can be killed /// at any moment when the system runs out of memory. #[cfg(target_os = "linux")] - PopulateWrite = libc::MADV_POPULATE_WRITE, + pub fn populate_write() -> Self { + Self(libc::MADV_POPULATE_WRITE) + } /// **MADV_ZERO_WIRED_PAGES** - Darwin only /// @@ -315,21 +366,39 @@ pub enum Advice { /// a munmap(2) without a preceding munlock(2) or the application quits). This is used /// with madvise() system call. #[cfg(any(target_os = "macos", target_os = "ios"))] - ZeroWiredPages = libc::MADV_ZERO_WIRED_PAGES, + pub fn zero_wired_pages() -> Self { + Self(libc::MADV_ZERO_WIRED_PAGES) + } /// **MADV_FREE_REUSABLE** - Darwin only /// /// Behaves like **MADV_FREE**, but the freed pages are accounted for in the RSS of the process. + /// + /// # Safety + /// + /// Using the returned value with conceptually write to the + /// mapped pages, i.e. borrowing the mapping while the pages + /// are still being freed results in undefined behaviour. #[cfg(any(target_os = "macos", target_os = "ios"))] - FreeReusable = libc::MADV_FREE_REUSABLE, + pub unsafe fn free_reusable() -> Self { + Self(libc::MADV_FREE_REUSABLE) + } /// **MADV_FREE_REUSE** - Darwin only /// /// Marks a memory region previously freed by **MADV_FREE_REUSABLE** as non-reusable, accounts /// for the pages in the RSS of the process. Pages that have been freed will be replaced by /// zero-filled pages on demand, other pages will be left as is. + /// + /// # Safety + /// + /// Using the returned value with conceptually write to the + /// mapped pages, i.e. borrowing the mapping while the pages + /// are still being freed results in undefined behaviour. #[cfg(any(target_os = "macos", target_os = "ios"))] - FreeReuse = libc::MADV_FREE_REUSE, + pub unsafe fn free_reuse() -> Self { + Self(libc::MADV_FREE_REUSE) + } } // Future expansion: diff --git a/src/lib.rs b/src/lib.rs index dab8c2f6..61239be4 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -531,7 +531,7 @@ impl MmapOptions { /// Dereferencing and accessing the bytes of the buffer may result in page faults (e.g. swapping /// the mapped pages into physical memory) though the details of this are platform specific. /// -/// `Mmap` is [`Sync`](std::marker::Sync) and [`Send`](std::marker::Send). +/// `Mmap` is [`Sync`] and [`Send`]. /// /// ## Safety /// @@ -948,7 +948,7 @@ impl From for MmapRaw { /// Dereferencing and accessing the bytes of the buffer may result in page faults (e.g. swapping /// the mapped pages into physical memory) though the details of this are platform specific. /// -/// `Mmap` is [`Sync`](std::marker::Sync) and [`Send`](std::marker::Send). +/// `Mmap` is [`Sync`] and [`Send`]. /// /// See [`Mmap`] for the immutable version. /// @@ -1317,7 +1317,7 @@ mod test { .read(true) .write(true) .create(true) - .open(&path) + .open(path) .unwrap(); file.set_len(expected_len as u64).unwrap(); @@ -1350,7 +1350,7 @@ mod test { .read(true) .write(true) .create(true) - .open(&path) + .open(path) .unwrap(); file.set_len(expected_len as u64).unwrap(); @@ -1382,7 +1382,7 @@ mod test { .read(true) .write(true) .create(true) - .open(&path) + .open(path) .unwrap(); let mmap = unsafe { Mmap::map(&file).unwrap() }; assert!(mmap.is_empty()); @@ -1437,7 +1437,7 @@ mod test { .read(true) .write(true) .create(true) - .open(&path) + .open(path) .unwrap(); file.set_len(128).unwrap(); @@ -1461,7 +1461,7 @@ mod test { .read(true) .write(true) .create(true) - .open(&path) + .open(path) .unwrap(); file.set_len(128).unwrap(); let write = b"abc123"; @@ -1487,7 +1487,7 @@ mod test { .read(true) .write(true) .create(true) - .open(&path) + .open(path) .unwrap(); file.set_len(128).unwrap(); @@ -1523,7 +1523,7 @@ mod test { .read(true) .write(true) .create(true) - .open(&path) + .open(path) .unwrap(); file.set_len(128).unwrap(); @@ -1548,7 +1548,7 @@ mod test { .read(true) .write(true) .create(true) - .open(&path) + .open(path) .unwrap(); let offset = u32::MAX as u64 + 2; @@ -1635,7 +1635,7 @@ mod test { .read(true) .write(true) .create(true) - .open(&tempdir.path().join("jit_x86")) + .open(tempdir.path().join("jit_x86")) .expect("open"); file.set_len(4096).expect("set_len"); @@ -1655,7 +1655,7 @@ mod test { .read(true) .write(true) .create(true) - .open(&path) + .open(path) .expect("open"); file.set_len(256_u64).expect("set_len"); @@ -1701,7 +1701,7 @@ mod test { .read(true) .write(true) .create(true) - .open(&path) + .open(path) .expect("open"); file.set_len(256_u64).expect("set_len"); @@ -1755,7 +1755,7 @@ mod test { .read(true) .write(true) .create(true) - .open(&path) + .open(path) .expect("open"); file.write_all(b"abc123").unwrap(); let mmap = MmapOptions::new().map_raw(&file).unwrap(); @@ -1809,14 +1809,14 @@ mod test { .read(true) .write(true) .create(true) - .open(&path) + .open(path) .unwrap(); file.set_len(expected_len as u64).unwrap(); // Test MmapMut::advise let mut mmap = unsafe { MmapMut::map_mut(&file).unwrap() }; - mmap.advise(Advice::Random) + mmap.advise(Advice::random()) .expect("mmap advising should be supported on unix"); let len = mmap.len(); @@ -1828,7 +1828,7 @@ mod test { // check that the mmap is empty assert_eq!(&zeros[..], &mmap[..]); - mmap.advise_range(Advice::Sequential, 0, mmap.len()) + mmap.advise_range(Advice::sequential(), 0, mmap.len()) .expect("mmap advising should be supported on unix"); // write values into the mmap @@ -1840,13 +1840,28 @@ mod test { // Set advice and Read from the read-only map let mmap = unsafe { Mmap::map(&file).unwrap() }; - mmap.advise(Advice::Random) + mmap.advise(Advice::random()) .expect("mmap advising should be supported on unix"); // read values back assert_eq!(&incr[..], &mmap[..]); } + #[test] + #[cfg(target_os = "linux")] + fn advise_writes_unsafely() { + let mut mmap = MmapMut::map_anon(4096).unwrap(); + mmap.as_mut().fill(255); + let mmap = mmap.make_read_only().unwrap(); + + let a = mmap.as_ref()[0]; + mmap.advise(unsafe { Advice::dont_need() }).unwrap(); + let b = mmap.as_ref()[0]; + + assert_eq!(a, 255); + assert_eq!(b, 0); + } + /// Returns true if a non-zero amount of memory is locked. #[cfg(target_os = "linux")] fn is_locked() -> bool { @@ -1871,7 +1886,7 @@ mod test { .read(true) .write(true) .create(true) - .open(&path) + .open(path) .unwrap(); file.set_len(128).unwrap(); diff --git a/src/unix.rs b/src/unix.rs index 78bddfde..3761b27c 100644 --- a/src/unix.rs +++ b/src/unix.rs @@ -347,7 +347,7 @@ impl MmapInner { let offset = offset as isize - alignment as isize; let len = len + alignment; unsafe { - if libc::madvise(self.ptr.offset(offset), len, advice as i32) != 0 { + if libc::madvise(self.ptr.offset(offset), len, advice.0) != 0 { Err(io::Error::last_os_error()) } else { Ok(())