From 272f385cf68feb65936ecddd7b041a8c040ef20a Mon Sep 17 00:00:00 2001 From: Zhang Junyang Date: Sun, 23 Nov 2025 17:05:21 +0800 Subject: [PATCH] Panic if map on a mapped page --- kernel/src/vm/vmar/vm_mapping.rs | 2 + .../x86/iommu/dma_remapping/context_table.rs | 2 +- ostd/src/mm/kspace/kvirt_area.rs | 5 +- ostd/src/mm/kspace/mod.rs | 9 +- ostd/src/mm/kspace/test.rs | 3 +- ostd/src/mm/page_table/cursor/mod.rs | 36 ++------ ostd/src/mm/page_table/test.rs | 83 ++++--------------- ostd/src/mm/test.rs | 23 +---- ostd/src/mm/vm_space.rs | 68 ++------------- 9 files changed, 46 insertions(+), 185 deletions(-) diff --git a/kernel/src/vm/vmar/vm_mapping.rs b/kernel/src/vm/vmar/vm_mapping.rs index 631416fec..279379e1e 100644 --- a/kernel/src/vm/vmar/vm_mapping.rs +++ b/kernel/src/vm/vmar/vm_mapping.rs @@ -427,6 +427,8 @@ impl VmMapping { } else { let new_frame = duplicate_frame(&frame)?; prop.flags |= new_flags; + cursor.unmap(PAGE_SIZE); + cursor.jump(va.start).unwrap(); cursor.map(new_frame.into(), prop); rss_delta.add(self.rss_type(), 1); } diff --git a/ostd/src/arch/x86/iommu/dma_remapping/context_table.rs b/ostd/src/arch/x86/iommu/dma_remapping/context_table.rs index 6be444c89..61994f459 100644 --- a/ostd/src/arch/x86/iommu/dma_remapping/context_table.rs +++ b/ostd/src/arch/x86/iommu/dma_remapping/context_table.rs @@ -315,7 +315,7 @@ impl ContextTable { let mut cursor = pt.cursor_mut(&preempt_guard, &from).unwrap(); // SAFETY: The safety is upheld by the caller. - unsafe { cursor.map((paddr, 1, prop)).unwrap() }; + unsafe { cursor.map((paddr, 1, prop)) }; Ok(()) } diff --git a/ostd/src/mm/kspace/kvirt_area.rs b/ostd/src/mm/kspace/kvirt_area.rs index c73cd88c0..21f4944d9 100644 --- a/ostd/src/mm/kspace/kvirt_area.rs +++ b/ostd/src/mm/kspace/kvirt_area.rs @@ -120,8 +120,7 @@ impl KVirtArea { for frame in frames.into_iter() { // SAFETY: The constructor of the `KVirtArea` has already ensured // that this mapping does not affect kernel's memory safety. - unsafe { cursor.map(MappedItem::Tracked(Frame::from_unsized(frame), prop)) } - .expect("Failed to map frame in a new `KVirtArea`"); + unsafe { cursor.map(MappedItem::Tracked(Frame::from_unsized(frame), prop)) }; } Self { range } @@ -170,7 +169,7 @@ impl KVirtArea { for (pa, level) in largest_pages::(va_range.start, pa_range.start, len) { // SAFETY: The caller of `map_untracked_frames` has ensured the safety of this mapping. - let _ = unsafe { cursor.map(MappedItem::Untracked(pa, level, prop)) }; + unsafe { cursor.map(MappedItem::Untracked(pa, level, prop)) }; } } diff --git a/ostd/src/mm/kspace/mod.rs b/ostd/src/mm/kspace/mod.rs index 5af5a8339..13c4eeebf 100644 --- a/ostd/src/mm/kspace/mod.rs +++ b/ostd/src/mm/kspace/mod.rs @@ -242,8 +242,7 @@ pub fn init_kernel_page_table(meta_pages: Segment) { let mut cursor = kpt.cursor_mut(&preempt_guard, &from).unwrap(); for (pa, level) in largest_pages::(from.start, 0, max_paddr) { // SAFETY: we are doing the linear mapping for the kernel. - unsafe { cursor.map(MappedItem::Untracked(pa, level, prop)) } - .expect("Kernel linear address space is mapped twice"); + unsafe { cursor.map(MappedItem::Untracked(pa, level, prop)) }; } } @@ -265,8 +264,7 @@ pub fn init_kernel_page_table(meta_pages: Segment) { largest_pages::(from.start, pa_range.start, pa_range.len()) { // SAFETY: We are doing the metadata mappings for the kernel. - unsafe { cursor.map(MappedItem::Untracked(pa, level, prop)) } - .expect("Frame metadata address space is mapped twice"); + unsafe { cursor.map(MappedItem::Untracked(pa, level, prop)) }; } } @@ -290,8 +288,7 @@ pub fn init_kernel_page_table(meta_pages: Segment) { let mut cursor = kpt.cursor_mut(&preempt_guard, &from).unwrap(); for (pa, level) in largest_pages::(from.start, region.base(), from.len()) { // SAFETY: we are doing the kernel code mapping. - unsafe { cursor.map(MappedItem::Untracked(pa, level, prop)) } - .expect("Kernel code mapped twice"); + unsafe { cursor.map(MappedItem::Untracked(pa, level, prop)) }; } } diff --git a/ostd/src/mm/kspace/test.rs b/ostd/src/mm/kspace/test.rs index ddebf653e..cbaab5cd4 100644 --- a/ostd/src/mm/kspace/test.rs +++ b/ostd/src/mm/kspace/test.rs @@ -40,7 +40,8 @@ fn kvirt_area_tracked_map_pages() { panic!("Expected a tracked page"); }; assert_eq!(page.paddr(), paddr + (i * PAGE_SIZE)); - assert_eq!(prop, default_prop()); + assert_eq!(prop.flags, default_prop().flags); + assert_eq!(prop.cache, default_prop().cache); } } diff --git a/ostd/src/mm/page_table/cursor/mod.rs b/ostd/src/mm/page_table/cursor/mod.rs index c042910cc..1637c0dbe 100644 --- a/ostd/src/mm/page_table/cursor/mod.rs +++ b/ostd/src/mm/page_table/cursor/mod.rs @@ -95,19 +95,6 @@ pub(crate) enum PageTableFrag { }, } -#[cfg(ktest)] -impl PageTableFrag { - pub(crate) fn va_range(&self) -> Range { - match self { - PageTableFrag::Mapped { va, item } => { - let (_pa, level, _prop) = C::item_raw_info(item); - *va..*va + page_size::(level) - } - PageTableFrag::StrayPageTable { va, len, .. } => *va..*va + *len, - } - } -} - impl<'rcu, C: PageTableConfig> Cursor<'rcu, C> { /// Creates a cursor claiming exclusive access over the given range. /// @@ -406,13 +393,7 @@ impl<'rcu, C: PageTableConfig> CursorMut<'rcu, C> { /// Maps the item starting from the current address to a physical address range. /// - /// If the current address has already mapped pages, it will do a re-map, - /// taking out the old physical address and replacing it with the new one. - /// This function will return [`Err`] with a [`PageTableFrag`], the not - /// mapped item. The caller should drop it after TLB coherence. - /// - /// If there is no mapped pages in the specified virtual address range, - /// the function will return [`None`]. + /// The current virtual address should not be mapped. /// /// # Panics /// @@ -420,13 +401,14 @@ impl<'rcu, C: PageTableConfig> CursorMut<'rcu, C> { /// - the virtual address range to be mapped is out of the locked range; /// - the current virtual address is not aligned to the page size of the /// item to be mapped; + /// - the virtual address range contains mappings that conflicts with the item. /// /// # Safety /// /// The caller should ensure that /// - the range being mapped does not affect kernel's memory safety; /// - the physical address to be mapped is valid and safe to use. - pub unsafe fn map(&mut self, item: C::Item) -> Result<(), PageTableFrag> { + pub unsafe fn map(&mut self, item: C::Item) { assert!(self.0.va < self.0.barrier_va.end); let (_, level, _) = C::item_raw_info(&item); assert!(level <= C::HIGHEST_TRANSLATION_LEVEL); @@ -462,15 +444,13 @@ impl<'rcu, C: PageTableConfig> CursorMut<'rcu, C> { } } - let frag = self.replace_cur_entry(PteState::Mapped(RcuDrop::new(item))); + if !matches!(self.0.cur_entry().to_ref(), PteStateRef::Absent) { + panic!("Mapping over an already mapped page at {:#x}", self.0.va); + } + + let _ = self.replace_cur_entry(PteState::Mapped(RcuDrop::new(item))); self.0.move_forward(); - - if let Some(frag) = frag { - Err(frag) - } else { - Ok(()) - } } /// Finds and removes the first page table fragment in the following range. diff --git a/ostd/src/mm/page_table/test.rs b/ostd/src/mm/page_table/test.rs index 3d131f66a..9c8662e62 100644 --- a/ostd/src/mm/page_table/test.rs +++ b/ostd/src/mm/page_table/test.rs @@ -33,8 +33,7 @@ mod test_utils { .cursor_mut(&preempt_guard, &virt_range) .unwrap() .map(VmItem::new_tracked(frame.into(), page_property)) - } - .expect("First map found an unexpected item"); + }; page_table } @@ -50,7 +49,7 @@ mod test_utils { let preempt_guard = disable_preempt(); let mut cursor = pt.cursor_mut(&preempt_guard, &(va..va + pa.len())).unwrap(); for (paddr, level) in largest_pages::(va, pa.start, pa.len()) { - let _ = unsafe { cursor.map((paddr, level, prop)) }; + unsafe { cursor.map((paddr, level, prop)) }; } } @@ -340,7 +339,7 @@ mod page_properties { let preempt_guard = disable_preempt(); let virtual_range = PAGE_SIZE..(PAGE_SIZE * 2); let frame = FrameAllocOptions::new().alloc_frame().unwrap(); - let _ = unsafe { + unsafe { page_table .cursor_mut(&preempt_guard, &virtual_range) .unwrap() @@ -438,6 +437,7 @@ mod overlapping_mappings { use super::{test_utils::*, *}; #[ktest] + #[should_panic] fn overlapping_mappings() { let page_table = PageTable::::empty(); let vrange1 = PAGE_SIZE..(PAGE_SIZE * 2); @@ -452,28 +452,15 @@ mod overlapping_mappings { page_table .cursor_mut(&preempt_guard, &vrange1) .unwrap() - .map((prange1.start, 1, page_property)) - .expect("Mapping to empty range failed"); + .map((prange1.start, 1, page_property)); } // Maps the second range, overlapping with the first. - let res2 = unsafe { + unsafe { page_table .cursor_mut(&preempt_guard, &vrange2) .unwrap() .map((prange2.start, 1, page_property)) }; - let Err(frag) = res2 else { - panic!( - "Expected an error due to overlapping mapping, got {:#x?}", - res2 - ); - }; - assert_eq!(frag.va_range(), vrange1); - - // Verifies that the overlapping address maps to the latest physical address. - assert!(page_table.page_walk(vrange2.start + 10).is_some()); - let mapped_pa = page_table.page_walk(vrange2.start + 10).unwrap().0; - assert_eq!(mapped_pa, prange2.start + 10); } #[ktest] @@ -487,7 +474,7 @@ mod overlapping_mappings { // Attempts to map an unaligned virtual address range (expected to panic). unsafe { - let _ = page_table + page_table .cursor_mut(&preempt_guard, &virt_range) .unwrap() .map((phys_range.start, 1, page_property)); @@ -516,8 +503,7 @@ mod navigation { &(FIRST_MAP_ADDR..FIRST_MAP_ADDR + PAGE_SIZE), ) .unwrap() - .map((pa1, 1, page_property)) - .unwrap(); + .map((pa1, 1, page_property)); } unsafe { @@ -527,8 +513,7 @@ mod navigation { &(SECOND_MAP_ADDR..SECOND_MAP_ADDR + PAGE_SIZE), ) .unwrap() - .map((pa2, 1, page_property)) - .unwrap(); + .map((pa2, 1, page_property)); } (page_table, pa1, pa2) @@ -578,7 +563,7 @@ mod navigation { let mut cursor = page_table.cursor_mut(&preempt_guard, &virt_range).unwrap(); cursor.jump(map_va).unwrap(); - unsafe { cursor.map(map_item).unwrap() }; + unsafe { cursor.map(map_item) }; // Now the cursor is at the end of the range with level 2. assert!(cursor.query().is_err()); @@ -606,13 +591,11 @@ mod navigation { let mut cursor = page_table.cursor_mut(&preempt_guard, &virt_range).unwrap(); unsafe { - cursor - .map(( - 0, - 1, - PageProperty::new_user(PageFlags::RW, CachePolicy::Writeback), - )) - .unwrap() + cursor.map(( + 0, + 1, + PageProperty::new_user(PageFlags::RW, CachePolicy::Writeback), + )) }; assert_eq!(cursor.virt_addr(), virt_range.end); @@ -662,7 +645,7 @@ mod unmap { { let mut cursor = page_table.cursor_mut(&preempt_guard, &virt_range).unwrap(); unsafe { - cursor.map((phys_addr, 1, page_property)).unwrap(); + cursor.map((phys_addr, 1, page_property)); } } @@ -691,7 +674,7 @@ mod unmap { { let mut cursor = page_table.cursor_mut(&preempt_guard, &virt_range).unwrap(); unsafe { - cursor.map((PAGE_SIZE, 1, page_property)).unwrap(); + cursor.map((PAGE_SIZE, 1, page_property)); } } @@ -717,35 +700,6 @@ mod unmap { mod mapping { use super::{test_utils::*, *}; - use crate::mm::vm_space::UserPtConfig; - - #[ktest] - fn remap_yields_original() { - let pt = PageTable::::empty(); - let preempt_guard = disable_preempt(); - - let virt_range = PAGE_SIZE..(PAGE_SIZE * 2); - let page_property = PageProperty::new_user(PageFlags::RW, CachePolicy::Writeback); - - let frame = FrameAllocOptions::new().alloc_frame().unwrap(); - unsafe { - pt.cursor_mut(&preempt_guard, &virt_range) - .unwrap() - .map(VmItem::new_tracked(frame.into(), page_property)) - .unwrap() - } - - let frame2 = FrameAllocOptions::new().alloc_frame().unwrap(); - let Err(frag) = (unsafe { - pt.cursor_mut(&preempt_guard, &virt_range) - .unwrap() - .map(VmItem::new_tracked(frame2.into(), page_property)) - }) else { - panic!("Expected to get error on remapping, got `Ok`"); - }; - - assert_eq!(frag.va_range(), virt_range); - } #[ktest] fn mixed_granularity_map_unmap() { @@ -1013,8 +967,7 @@ mod protection_and_query { page_table .cursor_mut(&preempt_guard, &sub_range) .unwrap() - .map((frame_range.start, 1, prop)) - .unwrap(); + .map((frame_range.start, 1, prop)); } // Attempts to protect the larger range. `protect_next` should traverse. diff --git a/ostd/src/mm/test.rs b/ostd/src/mm/test.rs index 148b4bf38..0b5930fde 100644 --- a/ostd/src/mm/test.rs +++ b/ostd/src/mm/test.rs @@ -581,6 +581,7 @@ mod vmspace { /// Maps a page twice and unmaps twice using `CursorMut`. #[ktest] + #[should_panic = "Mapping over an already mapped page at 0x1000"] fn vmspace_map_twice() { let vmspace = VmSpace::default(); let range = 0x1000..0x2000; @@ -608,28 +609,6 @@ mod vmspace { .expect("Failed to create mutable cursor"); cursor_mut.map(frame.clone(), prop); } - - { - let mut cursor = vmspace - .cursor(&preempt_guard, &range) - .expect("Failed to create cursor"); - assert_matches_mapped!(cursor, range.clone(), frame, prop); - } - - { - let mut cursor_mut = vmspace - .cursor_mut(&preempt_guard, &range) - .expect("Failed to create mutable cursor"); - cursor_mut.unmap(range.start); - } - - let mut cursor = vmspace - .cursor(&preempt_guard, &range) - .expect("Failed to create cursor"); - assert!(matches!( - cursor.query().unwrap(), - (r, None) if r == range - )); } /// Unmaps twice using `CursorMut`. diff --git a/ostd/src/mm/vm_space.rs b/ostd/src/mm/vm_space.rs index 6def3b797..06acffb48 100644 --- a/ostd/src/mm/vm_space.rs +++ b/ostd/src/mm/vm_space.rs @@ -355,16 +355,15 @@ impl<'a> CursorMut<'a> { /// Maps a frame into the current slot. /// /// This method will bring the cursor to the next slot after the modification. + /// + /// # Panics + /// + /// Panics if the current virtual address is already mapped. pub fn map(&mut self, frame: UFrame, prop: PageProperty) { - let start_va = self.virt_addr(); let item = VmItem::new_tracked(frame, prop); // SAFETY: It is safe to map untyped memory into the userspace. - let Err(frag) = (unsafe { self.pt_cursor.map(item) }) else { - return; // No mapping exists at the current address. - }; - - self.handle_remapped_frag(frag, start_va); + unsafe { self.pt_cursor.map(item) }; } /// Maps a range of [`IoMem`] into the current slot. @@ -383,7 +382,9 @@ impl<'a> CursorMut<'a> { /// /// # Panics /// - /// Panics if `len` or `offset` is not aligned to the page size. + /// Panics if + /// - `len` or `offset` is not aligned to the page size; + /// - the current virtual address is already mapped. pub fn map_iomem(&mut self, io_mem: IoMem, prop: PageProperty, len: usize, offset: usize) { assert_eq!(len % PAGE_SIZE, 0); assert_eq!(offset % PAGE_SIZE, 0); @@ -400,21 +401,11 @@ impl<'a> CursorMut<'a> { }; for current_paddr in (paddr_begin..paddr_end).step_by(PAGE_SIZE) { - // Save the current virtual address before mapping, since map() will advance the cursor - let current_va = self.virt_addr(); - // SAFETY: It is safe to map I/O memory into the userspace. - let map_result = unsafe { + unsafe { self.pt_cursor .map(VmItem::new_untracked_io(current_paddr, prop)) }; - - let Err(frag) = map_result else { - // No mapping exists at the current address. - continue; - }; - - self.handle_remapped_frag(frag, current_va); } // If the `iomems` list in `VmSpace` does not contain the current I/O @@ -443,47 +434,6 @@ impl<'a> CursorMut<'a> { self.vmspace.find_iomem_by_paddr(paddr) } - /// Handles a page table fragment that was remapped. - /// - /// This method handles the TLB flushing and other cleanup when a mapping - /// operation results in a fragment being replaced. - fn handle_remapped_frag(&mut self, frag: PageTableFrag, start_va: Vaddr) { - match frag { - PageTableFrag::Mapped { va, item } => { - debug_assert_eq!(va, start_va); - // SAFETY: If the item is not a scalar (e.g., a frame - // pointer), we will drop it after the RCU grace period - // (see `issue_tlb_flush_with`). - let (item, panic_guard) = unsafe { RcuDrop::into_inner(item) }; - - match item.mapped_item { - MappedItem::TrackedFrame(old_frame) => { - let rcu_frame = RcuDrop::new(old_frame); - panic_guard.forget(); - let rcu_frame = Frame::rcu_from_unsized(rcu_frame); - self.flusher - .issue_tlb_flush_with(TlbFlushOp::for_single(start_va), rcu_frame); - } - MappedItem::UntrackedIoMem { .. } => { - panic_guard.forget(); - // Flush the TLB entry for the current address, but in - // the current design, we cannot drop the corresponding - // `IoMem`. This is because we manage the range of I/O - // as a whole, but the frames handled here might be one - // segment of it. - self.flusher - .issue_tlb_flush(TlbFlushOp::for_single(start_va)); - } - } - - self.flusher.dispatch_tlb_flush(); - } - PageTableFrag::StrayPageTable { .. } => { - panic!("`UFrame` is base page sized but re-mapping out a child PT"); - } - } - } - /// Clears the mapping starting from the current slot, /// and returns the number of unmapped pages. ///