Merge #84 #89

84: Make Mapper trait object safe by adding `Self: Sized` bounds on generic functions r=phil-opp a=phil-opp

See #80 for more information

I'm not quite sure whether this is a **breaking change**, but I think it is.

89: Add new UnsafePhysFrame type and use it in Mapper::map_to r=phil-opp a=phil-opp

Fixes #88 

This pull request adds a new `UnsafePhysFrame` type that wraps a `PhysFrame`. The type is unsafe to construct and the caller must guarantee that the frame is not mapped already. This type allows to make the `map_to` and `identity_map` methods of the `Mapper` trait safe. This PR also adjust the `FrameAllocator` to use the new type.

This is a **breaking change**.



Co-authored-by: Philipp Oppermann <dev@phil-opp.com>

src/structures/paging/frame_alloc.rs

@@ -1,18 +1,51 @@
 //! Traits for abstracting away frame allocation and deallocation.
 
-use crate::structures::paging::{PageSize, PhysFrame};
+use crate::structures::paging::{PageSize, PhysFrame, Size4KiB};
+use core::ops::{Deref, DerefMut};
 
 /// A trait for types that can allocate a frame of memory.
 ///
 /// This trait is unsafe to implement because the implementer must guarantee that
 /// the `allocate_frame` method returns only unique unused frames.
 pub unsafe trait FrameAllocator<S: PageSize> {
     /// Allocate a frame of the appropriate size and return it if possible.
-    fn allocate_frame(&mut self) -> Option<PhysFrame<S>>;
+    fn allocate_frame(&mut self) -> Option<UnusedPhysFrame<S>>;
 }
 
 /// A trait for types that can deallocate a frame of memory.
 pub trait FrameDeallocator<S: PageSize> {
     /// Deallocate the given frame of memory.
-    fn deallocate_frame(&mut self, frame: PhysFrame<S>);
+    fn deallocate_frame(&mut self, frame: UnusedPhysFrame<S>);
+}
+
+/// Represents a physical frame that is not used for any mapping.
+#[derive(Debug)]
+pub struct UnusedPhysFrame<S: PageSize = Size4KiB>(PhysFrame<S>);
+
+impl<S: PageSize> UnusedPhysFrame<S> {
+    /// Creates a new UnusedPhysFrame from the given frame.
+    ///
+    /// This method is unsafe because the caller must guarantee
+    /// that the given frame is unused.
+    pub unsafe fn new(frame: PhysFrame<S>) -> Self {
+        Self(frame)
+    }
+
+    pub fn frame(self) -> PhysFrame<S> {
+        self.0
+    }
+}
+
+impl<S: PageSize> Deref for UnusedPhysFrame<S> {
+    type Target = PhysFrame<S>;
+
+    fn deref(&self) -> &Self::Target {
+        &self.0
+    }
+}
+
+impl<S: PageSize> DerefMut for UnusedPhysFrame<S> {
+    fn deref_mut(&mut self) -> &mut Self::Target {
+        &mut self.0
+    }
 }

src/structures/paging/mapper/mapped_page_table.rs

@@ -39,7 +39,7 @@ impl<'a, P: PhysToVirt> MappedPageTable<'a, P> {
     fn map_to_1gib<A>(
         &mut self,
         page: Page<Size1GiB>,
-        frame: PhysFrame<Size1GiB>,
+        frame: UnusedPhysFrame<Size1GiB>,
         flags: PageTableFlags,
         allocator: &mut A,
     ) -> Result<MapperFlush<Size1GiB>, MapToError>
@@ -64,7 +64,7 @@ impl<'a, P: PhysToVirt> MappedPageTable<'a, P> {
     fn map_to_2mib<A>(
         &mut self,
         page: Page<Size2MiB>,
-        frame: PhysFrame<Size2MiB>,
+        frame: UnusedPhysFrame<Size2MiB>,
         flags: PageTableFlags,
         allocator: &mut A,
     ) -> Result<MapperFlush<Size2MiB>, MapToError>
@@ -92,7 +92,7 @@ impl<'a, P: PhysToVirt> MappedPageTable<'a, P> {
     fn map_to_4kib<A>(
         &mut self,
         page: Page<Size4KiB>,
-        frame: PhysFrame<Size4KiB>,
+        frame: UnusedPhysFrame<Size4KiB>,
         flags: PageTableFlags,
         allocator: &mut A,
     ) -> Result<MapperFlush<Size4KiB>, MapToError>
@@ -113,17 +113,17 @@ impl<'a, P: PhysToVirt> MappedPageTable<'a, P> {
         if !p1[page.p1_index()].is_unused() {
             return Err(MapToError::PageAlreadyMapped);
         }
-        p1[page.p1_index()].set_frame(frame, flags);
+        p1[page.p1_index()].set_frame(frame.frame(), flags);
 
         Ok(MapperFlush::new(page))
     }
 }
 
 impl<'a, P: PhysToVirt> Mapper<Size1GiB> for MappedPageTable<'a, P> {
-    unsafe fn map_to<A>(
+    fn map_to<A>(
         &mut self,
         page: Page<Size1GiB>,
-        frame: PhysFrame<Size1GiB>,
+        frame: UnusedPhysFrame<Size1GiB>,
         flags: PageTableFlags,
         allocator: &mut A,
     ) -> Result<MapperFlush<Size1GiB>, MapToError>
@@ -193,10 +193,10 @@ impl<'a, P: PhysToVirt> Mapper<Size1GiB> for MappedPageTable<'a, P> {
 }
 
 impl<'a, P: PhysToVirt> Mapper<Size2MiB> for MappedPageTable<'a, P> {
-    unsafe fn map_to<A>(
+    fn map_to<A>(
         &mut self,
         page: Page<Size2MiB>,
-        frame: PhysFrame<Size2MiB>,
+        frame: UnusedPhysFrame<Size2MiB>,
         flags: PageTableFlags,
         allocator: &mut A,
     ) -> Result<MapperFlush<Size2MiB>, MapToError>
@@ -274,10 +274,10 @@ impl<'a, P: PhysToVirt> Mapper<Size2MiB> for MappedPageTable<'a, P> {
 }
 
 impl<'a, P: PhysToVirt> Mapper<Size4KiB> for MappedPageTable<'a, P> {
-    unsafe fn map_to<A>(
+    fn map_to<A>(
         &mut self,
         page: Page<Size4KiB>,
-        frame: PhysFrame<Size4KiB>,
+        frame: UnusedPhysFrame<Size4KiB>,
         flags: PageTableFlags,
         allocator: &mut A,
     ) -> Result<MapperFlush<Size4KiB>, MapToError>
@@ -460,7 +460,10 @@ impl<P: PhysToVirt> PageTableWalker<P> {
 
         if entry.is_unused() {
             if let Some(frame) = allocator.allocate_frame() {
-                entry.set_frame(frame, PageTableFlags::PRESENT | PageTableFlags::WRITABLE);
+                entry.set_frame(
+                    frame.frame(),
+                    PageTableFlags::PRESENT | PageTableFlags::WRITABLE,
+                );
                 created = true;
             } else {
                 return Err(PageTableCreateError::FrameAllocationFailed);

src/structures/paging/mapper/mod.rs

@@ -5,8 +5,9 @@ pub use self::mapped_page_table::{MappedPageTable, PhysToVirt};
 pub use self::{offset_page_table::OffsetPageTable, recursive_page_table::RecursivePageTable};
 
 use crate::structures::paging::{
-    frame_alloc::FrameAllocator, page_table::PageTableFlags, Page, PageSize, PhysFrame, Size1GiB,
-    Size2MiB, Size4KiB,
+    frame_alloc::{FrameAllocator, UnusedPhysFrame},
+    page_table::PageTableFlags,
+    Page, PageSize, PhysFrame, Size1GiB, Size2MiB, Size4KiB,
 };
 use crate::{PhysAddr, VirtAddr};
 
@@ -81,17 +82,15 @@ pub trait Mapper<S: PageSize> {
     ///
     /// This function might need additional physical frames to create new page tables. These
     /// frames are allocated from the `allocator` argument. At most three frames are required.
-    ///
-    /// This function is unsafe because the caller must guarantee that passed `frame` is
-    /// unused, i.e. not used for any other mappings.
-    unsafe fn map_to<A>(
+    fn map_to<A>(
         &mut self,
         page: Page<S>,
-        frame: PhysFrame<S>,
+        frame: UnusedPhysFrame<S>,
         flags: PageTableFlags,
         frame_allocator: &mut A,
     ) -> Result<MapperFlush<S>, MapToError>
     where
+        Self: Sized,
         A: FrameAllocator<Size4KiB>;
 
     /// Removes a mapping from the page table and returns the frame that used to be mapped.
@@ -113,16 +112,14 @@ pub trait Mapper<S: PageSize> {
     fn translate_page(&self, page: Page<S>) -> Result<PhysFrame<S>, TranslateError>;
 
     /// Maps the given frame to the virtual page with the same address.
-    ///
-    /// This function is unsafe because the caller must guarantee that the passed `frame` is
-    /// unused, i.e. not used for any other mappings.
     unsafe fn identity_map<A>(
         &mut self,
-        frame: PhysFrame<S>,
+        frame: UnusedPhysFrame<S>,
         flags: PageTableFlags,
         frame_allocator: &mut A,
     ) -> Result<MapperFlush<S>, MapToError>
     where
+        Self: Sized,
         A: FrameAllocator<Size4KiB>,
         S: PageSize,
         Self: Mapper<S>,
@@ -203,3 +200,5 @@ pub enum TranslateError {
     /// The page table entry for the given page points to an invalid physical address.
     InvalidFrameAddress(PhysAddr),
 }
+
+static _ASSERT_OBJECT_SAFE: Option<&(dyn MapperAllSizes + Sync)> = None;

src/structures/paging/mapper/offset_page_table.rs

@@ -49,10 +49,10 @@ impl PhysToVirt for PhysOffset {
 // delegate all trait implementations to inner
 
 impl<'a> Mapper<Size1GiB> for OffsetPageTable<'a> {
-    unsafe fn map_to<A>(
+    fn map_to<A>(
         &mut self,
         page: Page<Size1GiB>,
-        frame: PhysFrame<Size1GiB>,
+        frame: UnusedPhysFrame<Size1GiB>,
         flags: PageTableFlags,
         allocator: &mut A,
     ) -> Result<MapperFlush<Size1GiB>, MapToError>
@@ -83,10 +83,10 @@ impl<'a> Mapper<Size1GiB> for OffsetPageTable<'a> {
 }
 
 impl<'a> Mapper<Size2MiB> for OffsetPageTable<'a> {
-    unsafe fn map_to<A>(
+    fn map_to<A>(
         &mut self,
         page: Page<Size2MiB>,
-        frame: PhysFrame<Size2MiB>,
+        frame: UnusedPhysFrame<Size2MiB>,
         flags: PageTableFlags,
         allocator: &mut A,
     ) -> Result<MapperFlush<Size2MiB>, MapToError>
@@ -117,10 +117,10 @@ impl<'a> Mapper<Size2MiB> for OffsetPageTable<'a> {
 }
 
 impl<'a> Mapper<Size4KiB> for OffsetPageTable<'a> {
-    unsafe fn map_to<A>(
+    fn map_to<A>(
         &mut self,
         page: Page<Size4KiB>,
-        frame: PhysFrame<Size4KiB>,
+        frame: UnusedPhysFrame<Size4KiB>,
         flags: PageTableFlags,
         allocator: &mut A,
     ) -> Result<MapperFlush<Size4KiB>, MapToError>

src/structures/paging/mapper/recursive_page_table.rs

@@ -110,7 +110,7 @@ impl<'a> RecursivePageTable<'a> {
 
             if entry.is_unused() {
                 if let Some(frame) = allocator.allocate_frame() {
-                    entry.set_frame(frame, Flags::PRESENT | Flags::WRITABLE);
+                    entry.set_frame(frame.frame(), Flags::PRESENT | Flags::WRITABLE);
                     created = true;
                 } else {
                     return Err(MapToError::FrameAllocationFailed);
@@ -138,7 +138,7 @@ impl<'a> RecursivePageTable<'a> {
     fn map_to_1gib<A>(
         &mut self,
         page: Page<Size1GiB>,
-        frame: PhysFrame<Size1GiB>,
+        frame: UnusedPhysFrame<Size1GiB>,
         flags: PageTableFlags,
         allocator: &mut A,
     ) -> Result<MapperFlush<Size1GiB>, MapToError>
@@ -164,7 +164,7 @@ impl<'a> RecursivePageTable<'a> {
     fn map_to_2mib<A>(
         &mut self,
         page: Page<Size2MiB>,
-        frame: PhysFrame<Size2MiB>,
+        frame: UnusedPhysFrame<Size2MiB>,
         flags: PageTableFlags,
         allocator: &mut A,
     ) -> Result<MapperFlush<Size2MiB>, MapToError>
@@ -193,7 +193,7 @@ impl<'a> RecursivePageTable<'a> {
     fn map_to_4kib<A>(
         &mut self,
         page: Page<Size4KiB>,
-        frame: PhysFrame<Size4KiB>,
+        frame: UnusedPhysFrame<Size4KiB>,
         flags: PageTableFlags,
         allocator: &mut A,
     ) -> Result<MapperFlush<Size4KiB>, MapToError>
@@ -214,17 +214,17 @@ impl<'a> RecursivePageTable<'a> {
         if !p1[page.p1_index()].is_unused() {
             return Err(MapToError::PageAlreadyMapped);
         }
-        p1[page.p1_index()].set_frame(frame, flags);
+        p1[page.p1_index()].set_frame(frame.frame(), flags);
 
         Ok(MapperFlush::new(page))
     }
 }
 
 impl<'a> Mapper<Size1GiB> for RecursivePageTable<'a> {
-    unsafe fn map_to<A>(
+    fn map_to<A>(
         &mut self,
         page: Page<Size1GiB>,
-        frame: PhysFrame<Size1GiB>,
+        frame: UnusedPhysFrame<Size1GiB>,
         flags: PageTableFlags,
         allocator: &mut A,
     ) -> Result<MapperFlush<Size1GiB>, MapToError>
@@ -306,10 +306,10 @@ impl<'a> Mapper<Size1GiB> for RecursivePageTable<'a> {
 }
 
 impl<'a> Mapper<Size2MiB> for RecursivePageTable<'a> {
-    unsafe fn map_to<A>(
+    fn map_to<A>(
         &mut self,
         page: Page<Size2MiB>,
-        frame: PhysFrame<Size2MiB>,
+        frame: UnusedPhysFrame<Size2MiB>,
         flags: PageTableFlags,
         allocator: &mut A,
     ) -> Result<MapperFlush<Size2MiB>, MapToError>
@@ -411,10 +411,10 @@ impl<'a> Mapper<Size2MiB> for RecursivePageTable<'a> {
 }
 
 impl<'a> Mapper<Size4KiB> for RecursivePageTable<'a> {
-    unsafe fn map_to<A>(
+    fn map_to<A>(
         &mut self,
         page: Page<Size4KiB>,
-        frame: PhysFrame<Size4KiB>,
+        frame: UnusedPhysFrame<Size4KiB>,
         flags: PageTableFlags,
         allocator: &mut A,
     ) -> Result<MapperFlush<Size4KiB>, MapToError>