From f7bffacef05301757cd9ca4b5f5c1898442bb55d Mon Sep 17 00:00:00 2001
From: 4lDO2 <4lDO2@protonmail.com>
Date: Mon, 26 Jun 2023 19:58:19 +0200
Subject: [PATCH] Implement Drop for AddrSpace.
---
src/context/memory.rs | 12 ++++++++++
src/scheme/proc.rs | 21 ++----------------
src/syscall/process.rs | 50 +++---------------------------------------
3 files changed, 17 insertions(+), 66 deletions(-)
diff --git a/src/context/memory.rs b/src/context/memory.rs
index b3f3820f..c34d20f5 100644
--- a/src/context/memory.rs
+++ b/src/context/memory.rs
@@ -915,6 +915,18 @@ pub struct Table {
pub utable: PageMapper,
}
+impl Drop for AddrSpace {
+ fn drop(&mut self) {
+ for grant in core::mem::take(&mut self.grants).into_iter() {
+ // TODO: Optimize away clearing the actual page tables? Since this address space is no
+ // longer arc-rwlock wrapped, it cannot be referenced `External`ly by borrowing grants,
+ // so it should suffice to iterate over PageInfos and decrement and maybe deallocate
+ // the underlying pages (and send some funmaps possibly).
+ grant.unmap(&mut self.table.utable, ());
+ }
+ }
+}
+
impl Drop for Table {
fn drop(&mut self) {
if self.utable.is_current() {
diff --git a/src/scheme/proc.rs b/src/scheme/proc.rs
index 52c3fba4..6dbcf111 100644
--- a/src/scheme/proc.rs
+++ b/src/scheme/proc.rs
@@ -616,17 +616,13 @@ impl Scheme for ProcScheme {
context.clone_entry = Some([new_ip, new_sp]);
}
- let prev_addr_space = context.set_addr_space(new);
-
- if let Some(prev_addr_space) = prev_addr_space {
- maybe_cleanup_addr_space(prev_addr_space);
- }
+ let _prev_addr_space = context.set_addr_space(new);
Ok(())
})?;
let _ = ptrace::send_event(crate::syscall::ptrace_event!(PTRACE_EVENT_ADDRSPACE_SWITCH, 0));
}
- Operation::AddrSpace { addrspace } | Operation::MmapMinAddr(addrspace) => maybe_cleanup_addr_space(addrspace),
+ Operation::AddrSpace { addrspace } | Operation::MmapMinAddr(addrspace) => drop(addrspace),
Operation::AwaitingFiletableChange(new) => with_context_mut(handle.info.pid, |context: &mut Context| {
context.files = new;
@@ -1248,16 +1244,3 @@ fn extract_scheme_number(fd: usize) -> Result<(Arc<dyn KernelScheme>, usize)> {
Ok((scheme, number))
}
-fn maybe_cleanup_addr_space(addr_space: Arc<RwLock<AddrSpace>>) {
- if let Ok(mut space) = Arc::try_unwrap(addr_space).map(RwLock::into_inner) {
- // We are the last reference to the address space; therefore it must be
- // unmapped.
-
- // TODO: Optimize away clearing of page tables? In that case, what about memory
- // deallocation?
- for grant in space.grants.into_iter() {
- grant.unmap(&mut space.table.utable, ());
- }
- }
-
-}
diff --git a/src/syscall/process.rs b/src/syscall/process.rs
index 76b2f7b8..1284d317 100644
--- a/src/syscall/process.rs
+++ b/src/syscall/process.rs
@@ -25,44 +25,6 @@ use crate::syscall::ptrace_event;
use super::usercopy::{UserSliceWo, UserSliceRo};
-fn empty<'lock>(context_lock: &'lock RwLock<Context>, mut context: RwLockWriteGuard<'lock, Context>, reaping: bool) -> RwLockWriteGuard<'lock, Context> {
- // NOTE: If we do not replace the grants `Arc`, then a strange situation can appear where the
- // main thread and another thread exit simultaneously before either one is reaped. If that
- // happens, then the last context that runs exit will think that there is still are still
- // remaining references to the grants, where there are in fact none. However, if either one is
- // reaped before, then that reference will disappear, and no leak will occur.
- //
- // By removing the reference to the address space when the context will no longer be used, this
- // problem will never occur.
- let addr_space_arc = match context.addr_space.take() {
- Some(a) => a,
- None => return context,
- };
-
- if let Ok(mut addr_space) = Arc::try_unwrap(addr_space_arc).map(RwLock::into_inner) {
- let mapper = &mut addr_space.table.utable;
-
- for grant in addr_space.grants.into_iter() {
- let unmap_result = if reaping {
- log::error!("{}: {}: Grant should not exist: {:?}", context.id.into(), context.name, grant);
-
- grant.unmap(mapper, &mut InactiveFlusher::new())
- } else {
- grant.unmap(mapper, PageFlushAll::new())
- };
-
- if unmap_result.file_desc.is_some() {
- drop(context);
-
- drop(unmap_result);
-
- context = context_lock.write();
- }
- }
- }
- context
-}
-
pub fn exit(status: usize) -> ! {
ptrace::breakpoint_callback(PTRACE_STOP_EXIT, Some(ptrace_event!(PTRACE_STOP_EXIT, status)));
@@ -119,8 +81,6 @@ pub fn exit(status: usize) -> ! {
let children = {
let mut context = context_lock.write();
- context = empty(&context_lock, context, false);
-
context.status = context::Status::Exited(status);
context.waitpid.receive_all()
@@ -407,6 +367,7 @@ fn reap(pid: ContextId) -> Result<ContextId> {
// Spin until not running
let mut running = true;
while running {
+ // TODO: exit WaitCondition?
{
let contexts = context::contexts();
let context_lock = contexts.get(pid).ok_or(Error::new(ESRCH))?;
@@ -417,13 +378,8 @@ fn reap(pid: ContextId) -> Result<ContextId> {
interrupt::pause();
}
- let mut contexts = context::contexts_mut();
- let context_lock = contexts.remove(pid).ok_or(Error::new(ESRCH))?;
- {
- let context = context_lock.write();
- empty(&context_lock, context, true);
- }
- drop(context_lock);
+ let _ = context::contexts_mut()
+ .remove(pid).ok_or(Error::new(ESRCH))?;
Ok(pid)
}
--
GitLab