From 3161a44c25aeae005e1f509becb653883b84268f Mon Sep 17 00:00:00 2001 From: Wildan M Date: Tue, 31 Mar 2026 14:08:23 +0700 Subject: [PATCH] Solve borrow rules for memory.rs --- src/context/memory.rs | 85 +++++++++++++++++++++++++----------------- src/scheme/user.rs | 8 ++-- src/sync/ordered.rs | 80 ++++++++++++++++++++++++++++++++++++++- src/sync/wait_queue.rs | 2 + 4 files changed, 136 insertions(+), 39 deletions(-) diff --git a/src/context/memory.rs b/src/context/memory.rs index ce4d87f70a..a7a448ca76 100644 --- a/src/context/memory.rs +++ b/src/context/memory.rs @@ -23,8 +23,8 @@ use crate::{ percpu::PercpuBlock, scheme::{self, KernelSchemes}, sync::{ - CleanLockToken, LockToken, RwLock, RwLockReadGuard, RwLockUpgradableGuard, - RwLockWriteGuard, L0, L2, L3, + ArcRwLockWriteGuard, CleanLockToken, LockToken, RwLock, RwLockReadGuard, + RwLockUpgradableGuard, RwLockWriteGuard, L0, L2, L3, }, }; @@ -115,6 +115,18 @@ impl AddrSpaceWrapper { ) -> RwLockWriteGuard<'a, L3, AddrSpace> { self.inner.write(lock_token) } + pub unsafe fn acquire_reupgradeable_read<'a>( + &'a self, + mut lock_token: LockToken<'a, L3>, + ) -> RwLockUpgradableGuard<'a, L3, AddrSpace> { + unsafe { self.inner.reupgradeable_read(lock_token) } + } + pub unsafe fn acquire_rewrite<'a>( + &'a self, + mut lock_token: LockToken<'a, L3>, + ) -> RwLockWriteGuard<'a, L3, AddrSpace> { + unsafe { self.inner.rewrite(lock_token) } + } pub fn into_drop(self, token: &mut CleanLockToken) { self.inner.into_inner().into_drop(token); } @@ -522,16 +534,16 @@ impl AddrSpaceWrapper { return Err(Error::new(EPERM)); } + let mut guard_lock = None; let frame = if let Some((f, fl)) = guard.table.utable.translate(page.start_address()) && fl.has_write() { Frame::containing(f) } else { - let (frame, flush, new_guard) = - correct_inner(self, guard, page, AccessMode::Write, 0, token) - .map_err(|_| Error::new(ENOMEM))?; + let (frame, flush, new_guard) = correct_inner(self, guard, page, AccessMode::Write, 0) + .map_err(|_| Error::new(ENOMEM))?; flush.flush(); - guard = new_guard; + guard_lock = Some(new_guard); frame }; @@ -547,7 +559,7 @@ impl AddrSpaceWrapper { "if it was CoW, it was read-only, but in that case we already called correct_inner" ), }; - drop(guard); + drop(guard_lock.unwrap()); frame } @@ -679,7 +691,7 @@ impl AddrSpace { notify_files_out: Option<&mut Vec>, map: impl FnOnce(Page, PageFlags, &mut PageMapper, &mut Flusher) -> Result, ) -> Result { - debug_assert_eq!(dst_lock.inner.as_mut_ptr(), self as *mut Self); + assert_eq!(dst_lock.inner.as_mut_ptr(), self as *mut Self); let selected_span = match requested_base_opt { // TODO: Rename MAP_FIXED+MAP_FIXED_NOREPLACE to MAP_FIXED and @@ -1404,13 +1416,13 @@ impl Grant { Some((phys, _)) => Frame::containing(phys), // TODO: ensure the correct context is hardblocked, if necessary None => { + let (_, token) = guard.token_split(); let (frame, _, new_guard) = correct_inner( src.addr_space_lock, guard, src_page, AccessMode::Read, 0, - token, ) .map_err(|_| Error::new(EIO))?; guard = new_guard; @@ -1435,7 +1447,6 @@ impl Grant { src_page, AccessMode::Read, 0, - token, ) .map_err(|_| Error::new(EIO))?; guard = new_guard; @@ -2433,31 +2444,27 @@ pub fn try_correcting_page_tables( let lock = &addr_space; let mut lock_token = token.token(); - let addr_space_lock = addr_space.acquire_write(lock_token.downgrade()); + let mut addr_space_lock = addr_space.acquire_write(lock_token.downgrade()); - let (_, flush, _) = correct_inner( - &addr_space, - addr_space_lock, - faulting_page, - access, - 0, - token, - )?; + let (_, flush, _) = correct_inner(&addr_space, addr_space_lock, faulting_page, access, 0)?; flush.flush(); Ok(()) } + +/// XXX: This require passing L3 addr_space_guard. +/// Caller must ensure there's no other lock being held at this point. +/// Caller also need to provide clean token for the new AddrSpace. fn correct_inner<'l>( addr_space_lock: &'l Arc, mut addr_space_guard: RwLockWriteGuard<'l, L3, AddrSpace>, faulting_page: Page, access: AccessMode, recursion_level: u32, - token: &'l mut CleanLockToken, ) -> Result<(Frame, PageFlush, RwLockWriteGuard<'l, L3, AddrSpace>), PfError> { - let mut addr_space = &mut *addr_space_guard; let mut flusher = Flusher::with_cpu_set(&addr_space_lock.used_by, &addr_space_lock.tlb_ack); + let (mut addr_space, mut lock_token) = addr_space_guard.token_split(); let Some((grant_base, grant_info)) = addr_space.grants.contains(faulting_page) else { debug!("Lacks grant"); @@ -2566,8 +2573,9 @@ fn correct_inner<'l>( return Err(PfError::NonfatalInternalError); } - let mut lock_token = token.token(); - let mut guard = foreign_address_space.acquire_upgradeable_read(lock_token.downgrade()); + /// XXX: This is cheating, but guaranteed from Arc::ptr_eq above we won't deadlock + let mut free_token = unsafe { CleanLockToken::new() }; + let mut guard = foreign_address_space.acquire_upgradeable_read(free_token.downgrade()); let src_page = src_base.next_by(pages_from_grant_start); match guard.grants.contains(src_page) { @@ -2584,33 +2592,37 @@ fn correct_inner<'l>( .filter(|new_lvl| *new_lvl < 16) .ok_or(PfError::RecursionLimitExceeded)?; - drop(guard); + let guard_token = guard.into_token(); + let addr_space_guard_token = addr_space_guard.into_token(); drop(flusher); - drop(addr_space_guard); // FIXME: Can this result in invalid address space state? let ext_addrspace = &foreign_address_space; + let mut free_token = unsafe { CleanLockToken::new() }; let (frame, _, _) = { - let g = ext_addrspace.acquire_write(lock_token.downgrade()); + let g = ext_addrspace.acquire_write(free_token.downgrade()); correct_inner( ext_addrspace, g, src_page, AccessMode::Read, new_recursion_level, - token, )? }; + // SAFETY: Caller guarantees addr_space_guard is coming from this addr_space_lock addr_space_guard = - addr_space_lock.acquire_write(lock_token.downgrade()); + unsafe { addr_space_lock.acquire_rewrite(addr_space_guard_token) }; addr_space = &mut *addr_space_guard; flusher = Flusher::with_cpu_set( &addr_space_lock.used_by, &addr_space_lock.tlb_ack, ); - guard = foreign_address_space - .acquire_upgradeable_read(lock_token.downgrade()); + + // SAFETY: We guarantee that guard is coming from foreign_address_space + guard = unsafe { + foreign_address_space.acquire_reupgradeable_read(guard_token) + }; frame } @@ -2673,7 +2685,10 @@ fn correct_inner<'l>( let file_ref = file_ref.clone(); let flags = map_flags(grant_info.flags()); drop(flusher); - drop(addr_space_guard); + let addr_space_guard_token = addr_space_guard.into_token(); + + /// XXX: This is cheating, but guaranteed we won't deadlock because we've dropped addr_space_guard + let mut token = unsafe { CleanLockToken::new() }; let (scheme_id, scheme_number) = { let desc = &file_ref.description.read(token.token()); @@ -2692,7 +2707,7 @@ fn correct_inner<'l>( let offset = file_ref.base_offset as u64 + (pages_from_grant_start * PAGE_SIZE) as u64; user_inner - .request_fmap(scheme_number, offset, 1, flags, token) + .request_fmap(scheme_number, offset, 1, flags, &mut token) .unwrap(); let context_lock = crate::context::current(); @@ -2700,7 +2715,7 @@ fn correct_inner<'l>( .write(token.token()) .hard_block(HardBlockedReason::AwaitingMmap { file_ref }); - super::switch(token); + super::switch(&mut token); let frame = context_lock .write(token.token()) @@ -2708,8 +2723,8 @@ fn correct_inner<'l>( .take() .ok_or(PfError::NonfatalInternalError)?; - let mut token = token.token(); - addr_space_guard = addr_space_lock.acquire_write(token.downgrade()); + // SAFETY: Caller guarantees addr_space_guard is coming from this addr_space_lock + addr_space_guard = unsafe { addr_space_lock.acquire_rewrite(addr_space_guard_token) }; addr_space = &mut *addr_space_guard; flusher = Flusher::with_cpu_set(&addr_space_lock.used_by, &addr_space_lock.tlb_ack); diff --git a/src/scheme/user.rs b/src/scheme/user.rs index 0a2df0fbf2..58e3e005e0 100644 --- a/src/scheme/user.rs +++ b/src/scheme/user.rs @@ -905,8 +905,8 @@ impl UserInner { let to_close: Vec; { - let mut states = self.states.lock(token.token()); - let (states, mut token) = states.token_split(); + let mut states_lock = self.states.lock(token.token()); + let (states, mut lock_token) = states_lock.token_split(); match states.get_mut(tag as usize) { Some(o) => match mem::replace(o, State::Placeholder) { // invalid state @@ -957,13 +957,15 @@ impl UserInner { match context.upgrade() { Some(context) => { *o = State::Responded(response); - context.write(token.token()).unblock(); + context.write(lock_token.token()).unblock(); } _ => { states.remove(tag as usize); } } + drop(states_lock); + let unpin = true; AddrSpace::current()?.munmap(callee_responsible, unpin, token)?; } diff --git a/src/sync/ordered.rs b/src/sync/ordered.rs index ce6ac083cf..dbd4335b35 100644 --- a/src/sync/ordered.rs +++ b/src/sync/ordered.rs @@ -498,6 +498,72 @@ impl RwLock { }) } + /// Arcquires the lock_token to replace older LockWriteGuard. + /// SAFETY: Caller must guarantee lock_token is coming from RwLockWriteGuard::into_token() from the same lock. + pub unsafe fn rewrite<'a>( + &'a self, + lock_token: LockToken<'a, L>, + ) -> RwLockWriteGuard<'a, L, T> { + let inner = { + #[cfg(feature = "busy_panic")] + let mut i = DEADLOCK_SPIN_CAP; + let my_percpu = PercpuBlock::current(); + loop { + match self.inner.try_write() { + Some(inner) => break inner, + None => { + my_percpu.maybe_handle_tlb_shootdown(); + core::hint::spin_loop(); + #[cfg(feature = "busy_panic")] + { + i -= 1; + if i == 0 { + panic!("Deadlock at write may have triggered") + } + } + } + } + } + }; + RwLockWriteGuard { + inner, + lock_token: lock_token, + } + } + + /// Arcquires the lock_token to replace older LockUpgradableGuard. + /// SAFETY: Caller must guarantee lock_token is coming from RwLockUpgradableGuard::into_token() from the same lock. + pub unsafe fn reupgradeable_read<'a>( + &'a self, + lock_token: LockToken<'a, L>, + ) -> RwLockUpgradableGuard<'a, L, T> { + let inner = { + #[cfg(feature = "busy_panic")] + let mut i = DEADLOCK_SPIN_CAP; + let my_percpu = PercpuBlock::current(); + loop { + match self.inner.try_upgradeable_read() { + Some(inner) => break inner, + None => { + my_percpu.maybe_handle_tlb_shootdown(); + core::hint::spin_loop(); + #[cfg(feature = "busy_panic")] + { + i -= 1; + if i == 0 { + panic!("Deadlock at reupgradeable_read may have triggered") + } + } + } + } + } + }; + RwLockUpgradableGuard { + inner, + lock_token: lock_token, + } + } + // Unsafe due to not using token, currently required by context::switch pub unsafe fn write_arc(self: &Arc) -> ArcRwLockWriteGuard { core::mem::forget(self.inner.write()); @@ -513,12 +579,18 @@ pub struct RwLockWriteGuard<'a, L: Level, T> { lock_token: LockToken<'a, L>, } -impl RwLockWriteGuard<'_, L, T> { +impl<'a, L: Level, T> RwLockWriteGuard<'_, L, T> { /// Split the guard into two parts, the first a mutable reference to the held content /// the second a [`LockToken`] that can be used for further locking pub fn token_split(&mut self) -> (&mut T, LockToken<'_, L>) { (&mut self.inner, self.lock_token.token()) } + + /// Drop this Guard and extract the token to be reused for another write lock with rewrite() + pub fn into_token(self) -> LockToken<'a, L> { + drop(self.inner); + self.lock_token + } } impl core::ops::Deref for RwLockWriteGuard<'_, L, T> { @@ -577,6 +649,12 @@ impl<'a, L: Level, T> RwLockUpgradableGuard<'a, L, T> { lock_token: self.lock_token, } } + + /// Drop this Guard and extract the token to be reused for another write lock with reupgradeable_read() + pub fn into_token(self) -> LockToken<'a, L> { + drop(self.inner); + self.lock_token + } } impl core::ops::Deref for RwLockUpgradableGuard<'_, L, T> { diff --git a/src/sync/wait_queue.rs b/src/sync/wait_queue.rs index 3c0411a129..3071bab004 100644 --- a/src/sync/wait_queue.rs +++ b/src/sync/wait_queue.rs @@ -38,6 +38,8 @@ impl WaitQueue { if inner.is_empty() { if block { + // SAFETY: Uses wait_inner because this inner is L2. It's guaranteed there's no other + // lock held at this point because clean token is provided from caller. if !self.condition.wait_inner(inner, reason, &mut token) { return Err(Error::new(EINTR)); }