[RFC] Add Optional Safe-to-Use Flavours of Mutex/CondVar

drivers/android/context.rs

@@ -4,7 +4,8 @@ extern crate alloc;
 
 use alloc::sync::Arc;
 use core::pin::Pin;
-use kernel::{bindings, prelude::*, sync::Mutex, Error};
+use kernel::boxed_mutex;
+use kernel::{bindings, prelude::*, sync::BoxedMutex, Error};
 
 use crate::{
     node::NodeRef,
@@ -17,28 +18,23 @@ struct Manager {
 }
 
 pub(crate) struct Context {
-    manager: Mutex<Manager>,
+    manager: BoxedMutex<Manager>,
 }
 
 unsafe impl Send for Context {}
 unsafe impl Sync for Context {}
 
 impl Context {
     pub(crate) fn new() -> Result<Pin<Arc<Self>>> {
-        let mut ctx_ref = Arc::try_new(Self {
-            // SAFETY: Init is called below.
-            manager: unsafe {
-                Mutex::new(Manager {
+        let ctx_ref = Arc::try_new(Self {
+            manager: boxed_mutex!(
+                Manager {
                     node: None,
                     uid: None,
-                })
-            },
+                },
+                "Context::manager"
+            )?,
         })?;
-        let ctx = Arc::get_mut(&mut ctx_ref).unwrap();
-
-        // SAFETY: `manager` is also pinned when `ctx` is.
-        let manager = unsafe { Pin::new_unchecked(&ctx.manager) };
-        kernel::mutex_init!(manager, "Context::manager");
 
         // SAFETY: `ctx_ref` is pinned behind the `Arc` reference.
         Ok(unsafe { Pin::new_unchecked(ctx_ref) })

drivers/android/node.rs

@@ -9,7 +9,7 @@ use kernel::{
     io_buffer::IoBufferWriter,
     linked_list::{GetLinks, Links, List},
     prelude::*,
-    sync::{Guard, LockedBy, Mutex, Ref, SpinLock},
+    sync::{BoxedMutex, Guard, LockedBy, Ref, SpinLock},
     user_ptr::UserSlicePtrWriter,
 };
 
@@ -216,7 +216,7 @@ pub(crate) struct Node {
     cookie: usize,
     pub(crate) flags: u32,
     pub(crate) owner: Ref<Process>,
-    inner: LockedBy<NodeInner, Mutex<ProcessInner>>,
+    inner: LockedBy<NodeInner, BoxedMutex<ProcessInner>>,
     links: Links<dyn DeliverToRead>,
 }
 
@@ -248,12 +248,16 @@ impl Node {
 
     pub(crate) fn next_death(
         &self,
-        guard: &mut Guard<Mutex<ProcessInner>>,
+        guard: &mut Guard<BoxedMutex<ProcessInner>>,
     ) -> Option<Arc<NodeDeath>> {
         self.inner.access_mut(guard).death_list.pop_front()
     }
 
-    pub(crate) fn add_death(&self, death: Arc<NodeDeath>, guard: &mut Guard<Mutex<ProcessInner>>) {
+    pub(crate) fn add_death(
+        &self,
+        death: Arc<NodeDeath>,
+        guard: &mut Guard<BoxedMutex<ProcessInner>>,
+    ) {
         self.inner.access_mut(guard).death_list.push_back(death);
     }
 
@@ -306,7 +310,7 @@ impl Node {
     pub(crate) fn populate_counts(
         &self,
         out: &mut BinderNodeInfoForRef,
-        guard: &Guard<Mutex<ProcessInner>>,
+        guard: &Guard<BoxedMutex<ProcessInner>>,
     ) {
         let inner = self.inner.access(guard);
         out.strong_count = inner.strong.count as _;
@@ -316,7 +320,7 @@ impl Node {
     pub(crate) fn populate_debug_info(
         &self,
         out: &mut BinderNodeDebugInfo,
-        guard: &Guard<Mutex<ProcessInner>>,
+        guard: &Guard<BoxedMutex<ProcessInner>>,
     ) {
         out.ptr = self.ptr as _;
         out.cookie = self.cookie as _;
@@ -329,7 +333,7 @@ impl Node {
         }
     }
 
-    pub(crate) fn force_has_count(&self, guard: &mut Guard<Mutex<ProcessInner>>) {
+    pub(crate) fn force_has_count(&self, guard: &mut Guard<BoxedMutex<ProcessInner>>) {
         let inner = self.inner.access_mut(guard);
         inner.strong.has_count = true;
         inner.weak.has_count = true;

drivers/android/process.rs

@@ -7,14 +7,14 @@ use core::{
     pin::Pin,
 };
 use kernel::{
-    bindings, c_types,
+    bindings, boxed_mutex, c_types,
     file::File,
     file_operations::{FileOpener, FileOperations, IoctlCommand, IoctlHandler, PollTable},
     io_buffer::{IoBufferReader, IoBufferWriter},
     linked_list::List,
     pages::Pages,
     prelude::*,
-    sync::{Guard, Mutex, Ref, RefCount, RefCounted},
+    sync::{BoxedMutex, Guard, Ref, RefCount, RefCounted},
     user_ptr::{UserSlicePtr, UserSlicePtrReader},
     Error,
 };
@@ -281,33 +281,25 @@ pub(crate) struct Process {
     // holding the lock. We may want to split up the process state at some point to use a spin lock
     // for the other fields; we can also get rid of allocations in BTreeMap once we replace it.
     // TODO: Make this private again.
-    pub(crate) inner: Mutex<ProcessInner>,
+    pub(crate) inner: BoxedMutex<ProcessInner>,
 
     // References are in a different mutex to avoid recursive acquisition when
     // incrementing/decrementing a node in another process.
-    node_refs: Mutex<ProcessNodeRefs>,
+    node_refs: BoxedMutex<ProcessNodeRefs>,
 }
 
 unsafe impl Send for Process {}
 unsafe impl Sync for Process {}
 
 impl Process {
     fn new(ctx: Arc<Context>) -> Result<Ref<Self>> {
-        let mut proc_ref = Ref::try_new(Self {
+        let proc_ref = Ref::try_new(Self {
             ref_count: RefCount::new(),
             ctx,
-            // SAFETY: `inner` is initialised in the call to `mutex_init` below.
-            inner: unsafe { Mutex::new(ProcessInner::new()) },
+            inner: boxed_mutex!(ProcessInner::new(), "Process::inner")?,
             // SAFETY: `node_refs` is initialised in the call to `mutex_init` below.
-            node_refs: unsafe { Mutex::new(ProcessNodeRefs::new()) },
+            node_refs: boxed_mutex!(ProcessNodeRefs::new(), "Process::node_refs")?,
         })?;
-        let process = Ref::get_mut(&mut proc_ref).ok_or(Error::EINVAL)?;
-        // SAFETY: `inner` is pinned behind the `Arc` reference.
-        let pinned = unsafe { Pin::new_unchecked(&process.inner) };
-        kernel::mutex_init!(pinned, "Process::inner");
-        // SAFETY: `node_refs` is pinned behind the `Arc` reference.
-        let pinned = unsafe { Pin::new_unchecked(&process.node_refs) };
-        kernel::mutex_init!(pinned, "Process::node_refs");
         Ok(proc_ref)
     }
 
@@ -545,6 +537,9 @@ impl Process {
     pub(crate) fn buffer_get(&self, ptr: usize) -> Option<Allocation> {
         let mut inner = self.inner.lock();
         let mapping = inner.mapping.as_mut()?;
+        if ptr < mapping.address {
+            return None;
+        }
         let offset = ptr - mapping.address;
         let (size, odata) = mapping.alloc.reserve_existing(offset).ok()?;
         let mut alloc = Allocation::new(self, offset, size, ptr, mapping.pages.clone());
@@ -557,6 +552,9 @@ impl Process {
     pub(crate) fn buffer_raw_free(&self, ptr: usize) {
         let mut inner = self.inner.lock();
         if let Some(ref mut mapping) = &mut inner.mapping {
+            if ptr < mapping.address {
+                return;
+            }
             if mapping
                 .alloc
                 .reservation_abort(ptr - mapping.address)
@@ -931,7 +929,7 @@ impl<'a> Registration<'a> {
     fn new(
         process: &'a Process,
         thread: &'a Arc<Thread>,
-        guard: &mut Guard<Mutex<ProcessInner>>,
+        guard: &mut Guard<BoxedMutex<ProcessInner>>,
     ) -> Self {
         guard.ready_threads.push_back(thread.clone());
         Self { process, thread }

drivers/android/thread.rs

@@ -3,13 +3,13 @@
 use alloc::sync::Arc;
 use core::{alloc::AllocError, mem::size_of, pin::Pin};
 use kernel::{
-    bindings,
+    bindings, boxed_condvar,
     file::File,
     file_operations::PollTable,
     io_buffer::{IoBufferReader, IoBufferWriter},
     linked_list::{GetLinks, Links, List},
     prelude::*,
-    sync::{CondVar, Ref, SpinLock},
+    sync::{BoxedCondVar, Ref, SpinLock},
     user_ptr::{UserSlicePtr, UserSlicePtrWriter},
     Error,
 };
@@ -228,7 +228,7 @@ pub(crate) struct Thread {
     pub(crate) id: i32,
     pub(crate) process: Ref<Process>,
     inner: SpinLock<InnerThread>,
-    work_condvar: CondVar,
+    work_condvar: BoxedCondVar,
     links: Links<Thread>,
 }
 
@@ -241,15 +241,13 @@ impl Thread {
             process,
             // SAFETY: `inner` is initialised in the call to `spinlock_init` below.
             inner: unsafe { SpinLock::new(InnerThread::new()) },
-            // SAFETY: `work_condvar` is initalised in the call to `condvar_init` below.
-            work_condvar: unsafe { CondVar::new() },
+            work_condvar: boxed_condvar!("Thread::work_condvar")?,
             links: Links::new(),
         })?;
         let thread = Arc::get_mut(&mut arc).unwrap();
         // SAFETY: `inner` is pinned behind the `Arc` reference.
         let inner = unsafe { Pin::new_unchecked(&thread.inner) };
         kernel::spinlock_init!(inner, "Thread::inner");
-        kernel::condvar_init!(thread.pinned_condvar(), "Thread::work_condvar");
         {
             let mut inner = arc.inner.lock();
             inner.set_reply_work(reply_work);
@@ -258,8 +256,8 @@ impl Thread {
         Ok(arc)
     }
 
-    fn pinned_condvar(&self) -> Pin<&CondVar> {
-        unsafe { Pin::new_unchecked(&self.work_condvar) }
+    fn pinned_condvar(&self) -> &BoxedCondVar {
+        &self.work_condvar
     }
 
     pub(crate) fn set_current_transaction(&self, transaction: Arc<Transaction>) {
@@ -754,7 +752,7 @@ impl Thread {
 
     pub(crate) fn poll(&self, file: &File, table: &PollTable) -> (bool, u32) {
         // SAFETY: `free_waiters` is called on release.
-        unsafe { table.register_wait(file, &self.work_condvar) };
+        unsafe { table.register_wait2(file, &self.work_condvar) };
         let mut inner = self.inner.lock();
         (inner.should_use_process_work_queue(), inner.poll())
     }

rust/kernel/file_operations.rs

@@ -16,7 +16,7 @@ use crate::{
     from_kernel_result,
     io_buffer::{IoBufferReader, IoBufferWriter},
     iov_iter::IovIter,
-    sync::CondVar,
+    sync::{BoxedCondVar, CondVar},
     types::PointerWrapper,
     user_ptr::{UserSlicePtr, UserSlicePtrReader, UserSlicePtrWriter},
 };
@@ -63,6 +63,30 @@ impl PollTable {
             unsafe { proc(file.ptr as _, cv.wait_list.get(), self.ptr) }
         }
     }
+
+    /// Associates the given file and condition variable to this poll table. It means notifying the
+    /// condition variable will notify the poll table as well; additionally, the association
+    /// between the condition variable and the file will automatically be undone by the kernel when
+    /// the file is destructed. To unilaterally remove the association before then, one can call
+    /// [`CondVar::free_waiters`].
+    ///
+    /// # Safety
+    ///
+    /// If the condition variable is destroyed before the file, then [`CondVar::free_waiters`] must
+    /// be called to ensure that all waiters are flushed out.
+    pub unsafe fn register_wait2<'a>(&self, file: &'a File, cv: &'a BoxedCondVar) {
+        if self.ptr.is_null() {
+            return;
+        }
+
+        // SAFETY: `PollTable::ptr` is guaranteed to be valid by the type invariants and the null
+        // check above.
+        let table = unsafe { &*self.ptr };
+        if let Some(proc) = table._qproc {
+            // SAFETY: All pointers are known to be valid.
+            unsafe { proc(file.ptr as _, cv.wait_list.get(), self.ptr) }
+        }
+    }
 }
 
 /// Equivalent to [`std::io::SeekFrom`].