ANDROID: rust_binder: serialize oneway transactions

The binder driver guarantees that oneway transactions sent to the same
node are serialized, that is, userspace will not be given the next one
until it has finished processing the previous oneway transaction. This
is done to avoid the case where two oneway transactions arrive in
opposite order from the order in which they were sent. (E.g., they could
be delivered to two different threads, which could appear as-if they
were sent in opposite order.)

To fix that, we store pending oneway transactions in a separate list in
the node, and don't deliver the next oneway transaction until userspace
signals that it has finished processing the previous oneway transaction
by calling the BC_FREE_BUFFER ioctl.

Link: https://lore.kernel.org/rust-for-linux/20231101-rust-binder-v1-9-08ba9197f637@google.com/
Change-Id: I7fbe0716a2ecea36ea6a7594ed0e3ce03afe9362
Signed-off-by: Alice Ryhl <aliceryhl@google.com>
Bug: 278052745

drivers/android/binder/allocation.rs

@@ -6,13 +6,22 @@ use core::mem::size_of_val;
 
 use kernel::{page::Page, prelude::*, sync::Arc, uaccess::UserSliceReader};
 
-use crate::{node::NodeRef, process::Process};
+use crate::{
+    node::{Node, NodeRef},
+    process::Process,
+    DArc,
+};
 
 #[derive(Default)]
 pub(crate) struct AllocationInfo {
     /// The target node of the transaction this allocation is associated to.
     /// Not set for replies.
     pub(crate) target_node: Option<NodeRef>,
+    /// When this allocation is dropped, call `pending_oneway_finished` on the node.
+    ///
+    /// This is used to serialize oneway transaction on the same node. Binder guarantees that
+    /// oneway transactions to the same node are delivered sequentially in the order they are sent.
+    pub(crate) oneway_node: Option<DArc<Node>>,
     /// Zero the data in the buffer on free.
     pub(crate) clear_on_free: bool,
 }
@@ -115,6 +124,10 @@ impl Allocation {
         self.allocation_info.get_or_insert_with(Default::default)
     }
 
+    pub(crate) fn set_info_oneway_node(&mut self, oneway_node: DArc<Node>) {
+        self.get_or_init_info().oneway_node = Some(oneway_node);
+    }
+
     pub(crate) fn set_info_clear_on_drop(&mut self) {
         self.get_or_init_info().clear_on_free = true;
     }
@@ -131,6 +144,10 @@ impl Drop for Allocation {
         }
 
         if let Some(mut info) = self.allocation_info.take() {
+            if let Some(oneway_node) = info.oneway_node.as_ref() {
+                oneway_node.pending_oneway_finished();
+            }
+
             info.target_node = None;
 
             if info.clear_on_free {

drivers/android/binder/node.rs

@@ -3,7 +3,10 @@
 // Copyright (C) 2024 Google LLC.
 
 use kernel::{
-    list::{AtomicListArcTracker, List, ListArc, ListArcSafe, TryNewListArc},
+    list::{
+        AtomicListArcTracker, HasListLinks, List, ListArc, ListArcSafe, ListItem, ListLinks,
+        TryNewListArc,
+    },
     prelude::*,
     sync::lock::{spinlock::SpinLockBackend, Guard},
     sync::{Arc, LockedBy},
@@ -12,9 +15,11 @@ use kernel::{
 
 use crate::{
     defs::*,
+    error::BinderError,
     process::{NodeRefInfo, Process, ProcessInner},
     thread::Thread,
-    DArc, DeliverToRead,
+    transaction::Transaction,
+    DArc, DLArc, DTRWrap, DeliverToRead,
 };
 
 struct CountState {
@@ -39,6 +44,22 @@ struct NodeInner {
     strong: CountState,
     /// Weak refcounts held on this node by `NodeRef` objects.
     weak: CountState,
+    /// The binder driver guarantees that oneway transactions sent to the same node are serialized,
+    /// that is, userspace will not be given the next one until it has finished processing the
+    /// previous oneway transaction. This is done to avoid the case where two oneway transactions
+    /// arrive in opposite order from the order in which they were sent. (E.g., they could be
+    /// delivered to two different threads, which could appear as-if they were sent in opposite
+    /// order.)
+    ///
+    /// To fix that, we store pending oneway transactions in a separate list in the node, and don't
+    /// deliver the next oneway transaction until userspace signals that it has finished processing
+    /// the previous oneway transaction by calling the `BC_FREE_BUFFER` ioctl.
+    oneway_todo: List<DTRWrap<Transaction>>,
+    /// Keeps track of whether this node has a pending oneway transaction.
+    ///
+    /// When this is true, incoming oneway transactions are stored in `oneway_todo`, instead of
+    /// being delivered directly to the process.
+    has_oneway_transaction: bool,
     /// The number of active BR_INCREFS or BR_ACQUIRE operations. (should be maximum two)
     ///
     /// If this is non-zero, then we postpone any BR_RELEASE or BR_DECREFS notifications until the
@@ -66,6 +87,16 @@ kernel::list::impl_list_arc_safe! {
     }
 }
 
+// These make `oneway_todo` work.
+kernel::list::impl_has_list_links! {
+    impl HasListLinks<0> for DTRWrap<Transaction> { self.links.inner }
+}
+kernel::list::impl_list_item! {
+    impl ListItem<0> for DTRWrap<Transaction> {
+        using ListLinks;
+    }
+}
+
 impl Node {
     pub(crate) fn new(
         ptr: u64,
@@ -79,6 +110,8 @@ impl Node {
                 NodeInner {
                     strong: CountState::new(),
                     weak: CountState::new(),
+                    oneway_todo: List::new(),
+                    has_oneway_transaction: false,
                     active_inc_refs: 0,
                     refs: List::new(),
                 },
@@ -243,6 +276,63 @@ impl Node {
         writer.write(&self.cookie)?;
         Ok(())
     }
+
+    pub(crate) fn submit_oneway(
+        &self,
+        transaction: DLArc<Transaction>,
+        guard: &mut Guard<'_, ProcessInner, SpinLockBackend>,
+    ) -> Result<(), (BinderError, DLArc<dyn DeliverToRead>)> {
+        if guard.is_dead {
+            return Err((BinderError::new_dead(), transaction));
+        }
+
+        let inner = self.inner.access_mut(guard);
+        if inner.has_oneway_transaction {
+            inner.oneway_todo.push_back(transaction);
+        } else {
+            inner.has_oneway_transaction = true;
+            guard.push_work(transaction)?;
+        }
+        Ok(())
+    }
+
+    pub(crate) fn release(&self, guard: &mut Guard<'_, ProcessInner, SpinLockBackend>) {
+        // Move every pending oneshot message to the process todolist. The process
+        // will cancel it later.
+        //
+        // New items can't be pushed after this call, since `submit_oneway` fails when the process
+        // is dead, which is set before `Node::release` is called.
+        //
+        // TODO: Give our linked list implementation the ability to move everything in one go.
+        while let Some(work) = self.inner.access_mut(guard).oneway_todo.pop_front() {
+            guard.push_work_for_release(work);
+        }
+    }
+
+    pub(crate) fn pending_oneway_finished(&self) {
+        let mut guard = self.owner.inner.lock();
+        if guard.is_dead {
+            // Cleanup will happen in `Process::deferred_release`.
+            return;
+        }
+
+        let inner = self.inner.access_mut(&mut guard);
+
+        let transaction = inner.oneway_todo.pop_front();
+        inner.has_oneway_transaction = transaction.is_some();
+        if let Some(transaction) = transaction {
+            match guard.push_work(transaction) {
+                Ok(()) => {}
+                Err((_err, work)) => {
+                    // Process is dead.
+                    // This shouldn't happen due to the `is_dead` check, but if it does, just drop
+                    // the transaction and return.
+                    drop(guard);
+                    drop(work);
+                }
+            }
+        }
+    }
 }
 
 impl DeliverToRead for Node {

drivers/android/binder/process.rs

@@ -150,6 +150,11 @@ impl ProcessInner {
         }
     }
 
+    /// Push work to be cancelled. Only used during process teardown.
+    pub(crate) fn push_work_for_release(&mut self, work: DLArc<dyn DeliverToRead>) {
+        self.work.push_back(work);
+    }
+
     pub(crate) fn remove_node(&mut self, ptr: u64) {
         self.nodes.remove(&ptr);
     }
@@ -837,6 +842,21 @@ impl Process {
 
         self.ctx.deregister_process(&self);
 
+        // Move oneway_todo into the process todolist.
+        {
+            let mut inner = self.inner.lock();
+            let nodes = take(&mut inner.nodes);
+            for node in nodes.values() {
+                node.release(&mut inner);
+            }
+            inner.nodes = nodes;
+        }
+
+        // Cancel all pending work items.
+        while let Some(work) = self.get_work() {
+            work.into_arc().cancel();
+        }
+
         // Move the threads out of `inner` so that we can iterate over them without holding the
         // lock.
         let mut inner = self.inner.lock();
@@ -848,11 +868,6 @@ impl Process {
             thread.release();
         }
 
-        // Cancel all pending work items.
-        while let Some(work) = self.get_work() {
-            work.into_arc().cancel();
-        }
-
         // Free any resources kept alive by allocated buffers.
         let omapping = self.inner.lock().mapping.take();
         if let Some(mut mapping) = omapping {

drivers/android/binder/thread.rs

@@ -668,9 +668,7 @@ impl Thread {
                 BC_TRANSACTION => {
                     let tr = reader.read::<BinderTransactionData>()?.with_buffers_size(0);
                     if tr.transaction_data.flags & TF_ONE_WAY != 0 {
-                        // TODO: Allow sending oneway transactions when they are serialized.
+                        self.transaction(&tr, Self::oneway_transaction_inner);
-                        //self.transaction(&tr, Self::oneway_transaction_inner);
-                        return Err(EINVAL);
                     } else {
                         self.transaction(&tr, Self::transaction_inner);
                     }
@@ -678,9 +676,7 @@ impl Thread {
                 BC_TRANSACTION_SG => {
                     let tr = reader.read::<BinderTransactionDataSg>()?;
                     if tr.transaction_data.flags & TF_ONE_WAY != 0 {
-                        // TODO: Allow sending oneway transactions when they are serialized.
+                        self.transaction(&tr, Self::oneway_transaction_inner);
-                        //self.transaction(&tr, Self::oneway_transaction_inner);
-                        return Err(EINVAL);
                     } else {
                         self.transaction(&tr, Self::transaction_inner);
                     }

drivers/android/binder/transaction.rs

@@ -63,10 +63,13 @@ impl Transaction {
                     return Err(err);
                 }
             };
-        if trd.flags & TF_ONE_WAY != 0 && from_parent.is_some() {
+        if trd.flags & TF_ONE_WAY != 0 {
+            if from_parent.is_some() {
                 pr_warn!("Oneway transaction should not be in a transaction stack.");
                 return Err(EINVAL.into());
             }
+            alloc.set_info_oneway_node(node_ref.node.clone());
+        }
         if trd.flags & TF_CLEAR_BUF != 0 {
             alloc.set_info_clear_on_drop();
         }
@@ -166,9 +169,26 @@ impl Transaction {
     ///
     /// Not used for replies.
     pub(crate) fn submit(self: DLArc<Self>) -> BinderResult {
+        let oneway = self.flags & TF_ONE_WAY != 0;
         let process = self.to.clone();
         let mut process_inner = process.inner.lock();
 
+        if oneway {
+            if let Some(target_node) = self.target_node.clone() {
+                match target_node.submit_oneway(self, &mut process_inner) {
+                    Ok(()) => return Ok(()),
+                    Err((err, work)) => {
+                        drop(process_inner);
+                        // Drop work after releasing process lock.
+                        drop(work);
+                        return Err(err);
+                    }
+                }
+            } else {
+                pr_err!("Failed to submit oneway transaction to node.");
+            }
+        }
+
         let res = if let Some(thread) = self.find_target_thread() {
             match thread.push_work(self) {
                 PushWorkRes::Ok => Ok(()),