Re: [PATCH v2] rust: configfs: require thread-safe callback data

[Andreas Hindborg]

"Yilin Chen" <1479826151@xxxxxx> writes:

> The Rust configfs abstractions do not fully constrain callback data for
> cross-thread use. Add the missing `Send` and `Sync` requirements.
>
> Specifically, make the following changes:
>
> 1. Require `Data: Send` when implementing `Send` for `Subsystem<Data>`,
>    since the subsystem stores its data by value.
> 2. Make `GroupOperations` a `Sync` supertrait because `make_group` and
>    `drop_item` receive `&self` from foreign threads. Require `Child: Send`
>    because configfs may release child groups on an arbitrary thread.
> 3. Require `AttributeOperations::Data: Sync` because its callbacks receive
>    `&Data` from foreign threads.
> 4. Update the safety comments in FFI callbacks that call `get_group_data`
>    to cite these bounds as justification for sharing the returned
>    references with the callback thread.
> 5. Remove redundant `Child: 'static` bounds from `GroupOperationsVTable`
>    and `new_with_child_ctor`.
>

I would add the diff below and update the commit message. OK with you?

Best regards,
Andreas Hindborg




diff --git a/rust/kernel/configfs.rs b/rust/kernel/configfs.rs
index 0534b5054fc8..ec295ab965da 100644
--- a/rust/kernel/configfs.rs
+++ b/rust/kernel/configfs.rs
@@ -250,6 +250,13 @@ pub struct Group<Data> {
     data: Data,
 }
 
+// SAFETY: We do not provide any operations on `Group`.
+unsafe impl<Data> Sync for Group<Data> {}
+
+// SAFETY: Ownership of `Group` can safely be transferred to other threads if
+// its data can be transferred as well.
+unsafe impl<Data: Send> Send for Group<Data> {}
+
 impl<Data> Group<Data> {
     /// Create an initializer for a new group.
     ///
@@ -326,6 +333,9 @@ unsafe fn get_group_data<'a, Parent>(this: *mut bindings::config_group) -> &'a P
 impl<Parent, Child> GroupOperationsVTable<Parent, Child>
 where
     Parent: GroupOperations<Child = Child>,
+    // We transfer `Arc<Group<Data>>` across a thread boundary in `make_group`
+    // and `drop_item`.
+    Arc<Group<Child>>: Send,
 {
     /// # Safety
     ///
@@ -405,7 +415,9 @@ impl<Parent, Child> GroupOperationsVTable<Parent, Child>
 
         if Parent::HAS_DROP_ITEM {
             // SAFETY: We called `into_raw` to produce `r_child_group_ptr` in
-            // `make_group`.
+            // `make_group`. This function may be executing on a different
+            // thread than `into_raw`. As `Arc<Group<Child>>: Send` this
+            // ownership transfer is safe.
             let arc: Arc<Group<Child>> = unsafe { Arc::from_raw(r_child_group_ptr.cast_mut()) };
 
             Parent::drop_item(parent_data, arc.as_arc_borrow());
@@ -436,6 +448,8 @@ const fn vtable_ptr() -> *const bindings::configfs_group_operations {
 impl<Data> ItemOperationsVTable<Group<Data>, Data>
 where
     Data: 'static,
+    // We transfer `Arc<Group<Data>>` across a thread boundary in `release`.
+    Arc<Group<Data>>: Send,
 {
     /// # Safety
     ///
@@ -452,8 +466,9 @@ impl<Data> ItemOperationsVTable<Group<Data>, Data>
         // embedded within a `Group<Data>`.
         let r_group_ptr = unsafe { Group::<Data>::container_of(c_group_ptr) };
 
-        // SAFETY: We called `into_raw` on `r_group_ptr` in
-        // `make_group`.
+        // SAFETY: We called `into_raw` on `r_group_ptr` in `make_group`. This
+        // function may be running on a different thread than the thread that
+        // called `into_raw`. As `Arc<Group<Data>>: Send`, this is safe.
         let pin_self: Arc<Group<Data>> = unsafe { Arc::from_raw(r_group_ptr.cast_mut()) };
         drop(pin_self);
     }
@@ -755,6 +770,8 @@ pub const fn new_with_child_ctor<const N: usize, Child>(
             ) -> Self
             where
                 Data: GroupOperations<Child = Child>,
+                Arc<Group<Child>>: Send,
+                Arc<Group<Data>>: Send,
             {
                 Self {
                     item_type: Opaque::new(bindings::config_item_type {
@@ -772,7 +789,10 @@ pub const fn new_with_child_ctor<const N: usize, Child>(
             pub const fn new<const N: usize>(
                 owner: &'static ThisModule,
                 attributes: &'static AttributeList<N, Data>,
-            ) -> Self {
+            ) -> Self
+            where
+                Arc<Group<Data>>: Send,
+            {
                 Self {
                     item_type: Opaque::new(bindings::config_item_type {
                         ct_owner: owner.as_ptr(),
-- 
2.51.2