From a9b62b10f7d75d007bacc1fb70dfba37fafd75f2 Mon Sep 17 00:00:00 2001 From: Ard Biesheuvel Date: Thu, 26 Oct 2023 11:26:55 +0200 Subject: [PATCH 1/2] Avoid spurious break-before-make errors on other maperror types The check_range_bbm() routine will return an Err() Result of type MapError::BreakBeforeMakeViolation on any errors returned by the call to walk_range(), including ones that are the result of other issues. Let's avoid this by only converting PteUpdateFault errors into BreakBeforeMakeViolation, and leaving other error types untouched. --- src/lib.rs | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/src/lib.rs b/src/lib.rs index a1ccd97..6ec1adb 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -274,7 +274,12 @@ impl Mapping { Ok(()) }, ) - .map_err(|_| MapError::BreakBeforeMakeViolation(range.clone()))?; + .map_err(|e| match e { + MapError::PteUpdateFault(_) => { + MapError::BreakBeforeMakeViolation(range.clone()) + } + e => e, + })?; Ok(()) } From 32dbf60b137a794cbc1db9e56a64fddb2eee0f9f Mon Sep 17 00:00:00 2001 From: Ard Biesheuvel Date: Thu, 26 Oct 2023 11:30:48 +0200 Subject: [PATCH 2/2] Return the subrange that violates break-before-make on BBM failure Passing the entire range back to the caller on a break-before-make (BBM) violation is not very helpful. We might add a violation type to the enum later but let's at least return the subrange that actually triggered the failure, which is expected to be far more useful to the caller. To allow the closure used by check_range_bbm() to return MapErrors directly, tweak the internal API and make the () -> MapError conversion an implementation detail of the public walk_range() API. --- src/idmap.rs | 6 +++--- src/lib.rs | 23 +++++++++-------------- src/paging.rs | 29 +++++++++++++++++------------ 3 files changed, 29 insertions(+), 29 deletions(-) diff --git a/src/idmap.rs b/src/idmap.rs index 8b25356..73641c9 100644 --- a/src/idmap.rs +++ b/src/idmap.rs @@ -434,7 +434,7 @@ mod tests { Attributes::NORMAL | Attributes::VALID | Attributes::READ_ONLY, ), Err(MapError::BreakBeforeMakeViolation(MemoryRegion::new( - 0, + BLOCK_SIZE, BLOCK_SIZE + PAGE_SIZE ))) ); @@ -453,7 +453,7 @@ mod tests { Attributes::DEVICE_NGNRE | Attributes::VALID | Attributes::NON_GLOBAL, ), Err(MapError::BreakBeforeMakeViolation(MemoryRegion::new( - 0, BLOCK_SIZE + 0, PAGE_SIZE ))) ); @@ -464,7 +464,7 @@ mod tests { Attributes::NORMAL, ), Err(MapError::BreakBeforeMakeViolation(MemoryRegion::new( - PAGE_SIZE, + BLOCK_SIZE, BLOCK_SIZE + PAGE_SIZE ))) ); diff --git a/src/lib.rs b/src/lib.rs index 6ec1adb..c47c9d8 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -232,21 +232,23 @@ impl Mapping { where F: Fn(&MemoryRegion, &mut Descriptor, usize) -> Result<(), ()> + ?Sized, { - self.walk_range( + self.root.visit_range( range, &mut |mr: &MemoryRegion, d: &Descriptor, level: usize| { if d.is_valid() { + let err = MapError::BreakBeforeMakeViolation(mr.clone()); + if !mr.is_block(level) { // Cannot split a live block mapping - return Err(()); + return Err(err); } // Get the new flags and output address for this descriptor by applying // the updater function to a copy let (flags, oa) = { let mut dd = *d; - updater(mr, &mut dd, level)?; - (dd.flags().ok_or(())?, dd.output_address()) + updater(mr, &mut dd, level).or(Err(err.clone()))?; + (dd.flags().ok_or(err.clone())?, dd.output_address()) }; if !flags.contains(Attributes::VALID) { @@ -256,31 +258,24 @@ impl Mapping { if oa != d.output_address() { // Cannot change output address on a live mapping - return Err(()); + return Err(err); } let desc_flags = d.flags().unwrap(); if (desc_flags ^ flags).intersects(Attributes::NORMAL) { // Cannot change memory type - return Err(()); + return Err(err); } if (desc_flags - flags).contains(Attributes::NON_GLOBAL) { // Cannot convert from non-global to global - return Err(()); + return Err(err); } } Ok(()) }, ) - .map_err(|e| match e { - MapError::PteUpdateFault(_) => { - MapError::BreakBeforeMakeViolation(range.clone()) - } - e => e, - })?; - Ok(()) } /// Maps the given range of virtual addresses to the corresponding range of physical addresses diff --git a/src/paging.rs b/src/paging.rs index 9dac27b..56d680b 100644 --- a/src/paging.rs +++ b/src/paging.rs @@ -366,9 +366,19 @@ impl RootTable { pub fn walk_range(&self, range: &MemoryRegion, f: &mut F) -> Result<(), MapError> where F: FnMut(&MemoryRegion, &Descriptor, usize) -> Result<(), ()>, + { + self.visit_range(range, &mut |mr, desc, level| { + f(mr, desc, level).map_err(|_| MapError::PteUpdateFault(*desc)) + }) + } + + // Private version of `walk_range` using a closure that returns MapError on error + pub(crate) fn visit_range(&self, range: &MemoryRegion, f: &mut F) -> Result<(), MapError> + where + F: FnMut(&MemoryRegion, &Descriptor, usize) -> Result<(), MapError>, { self.verify_region(range)?; - self.table.walk_range(&self.translation, range, f) + self.table.visit_range(&self.translation, range, f) } /// Returns the level of mapping used for the given virtual address: @@ -699,24 +709,19 @@ impl PageTableWithLevel { Ok(()) } - /// Walks a range of page table entries and passes each one to a caller provided function - /// If the range is not aligned to block boundaries, it will be expanded. - fn walk_range( - &self, - translation: &T, - range: &MemoryRegion, - f: &mut F, - ) -> Result<(), MapError> + /// Walks a range of page table entries and passes each one to a caller provided function. + /// If the function returns an error, the walk is terminated and the error value is passed on + fn visit_range(&self, translation: &T, range: &MemoryRegion, f: &mut F) -> Result<(), E> where - F: FnMut(&MemoryRegion, &Descriptor, usize) -> Result<(), ()>, + F: FnMut(&MemoryRegion, &Descriptor, usize) -> Result<(), E>, { let level = self.level; for chunk in range.split(level) { let entry = self.get_entry(chunk.0.start); if let Some(subtable) = entry.subtable(translation, level) { - subtable.walk_range(translation, &chunk, f)?; + subtable.visit_range(translation, &chunk, f)?; } else { - f(&chunk, entry, level).map_err(|_| MapError::PteUpdateFault(*entry))?; + f(&chunk, entry, level)?; } } Ok(())