[PATCH] rust: configfs: require Send data for Subsystem

[PATCH] rust: configfs: require Send data for Subsystem

[Yilin Chen]

Subsystem stores its data by value, but its blanket Send implementation
did not require the data to be Send. This allowed a configfs subsystem
containing a non-Send value to be transferred across threads.

Link: https://rust-for-linux.zulipchat.com/#narrow/channel/288089-General/topic/Should.20add.20Data.3A.20Send.2FSync.20bounds.20in.20configfs.3A.3ASubsystem.3F/with/623719979

Fixes: 446cafc295bf ("rust: configfs: introduce rust support for configfs")

Assisted-by: Gpt-5.6 Sol

Signed-off-by: Yilin Chen <1479826151@qq.com>
---
 rust/kernel/configfs.rs | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/rust/kernel/configfs.rs b/rust/kernel/configfs.rs
index cd082b83e9e7..f8ca5fc03bf1 100644
--- a/rust/kernel/configfs.rs
+++ b/rust/kernel/configfs.rs
@@ -135,8 +135,9 @@ pub struct Subsystem<Data> {
 // SAFETY: We do not provide any operations on `Subsystem`.
 unsafe impl<Data> Sync for Subsystem<Data> {}
 
-// SAFETY: Ownership of `Subsystem` can safely be transferred to other threads.
-unsafe impl<Data> Send for Subsystem<Data> {}
+// SAFETY: Ownership of `Subsystem` can safely be transferred to other threads
+// if its data can be transferred as well.
+unsafe impl<Data: Send> Send for Subsystem<Data> {}
 
 impl<Data> Subsystem<Data> {
     /// Create an initializer for a [`Subsystem`].

base-commit: 08df884136f1c1197bab2a27814404fd329d9aac

Re: [PATCH] rust: configfs: require Send data for Subsystem

[Andreas Hindborg]

Hi,

Thanks for the patch.

"Yilin Chen" <1479826151@qq.com> writes:

> Subsystem stores its data by value, but its blanket Send implementation
> did not require the data to be Send. This allowed a configfs subsystem
> containing a non-Send value to be transferred across threads.
>
> Link: https://rust-for-linux.zulipchat.com/#narrow/channel/288089-General/topic/Should.20add.20Data.3A.20Send.2FSync.20bounds.20in.20configfs.3A.3ASubsystem.3F/with/623719979
>
> Fixes: 446cafc295bf ("rust: configfs: introduce rust support for configfs")
>
> Assisted-by: Gpt-5.6 Sol
>
> Signed-off-by: Yilin Chen <1479826151@qq.com>
> ---
>  rust/kernel/configfs.rs | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/rust/kernel/configfs.rs b/rust/kernel/configfs.rs
> index cd082b83e9e7..f8ca5fc03bf1 100644
> --- a/rust/kernel/configfs.rs
> +++ b/rust/kernel/configfs.rs
> @@ -135,8 +135,9 @@ pub struct Subsystem<Data> {
>  // SAFETY: We do not provide any operations on `Subsystem`.
>  unsafe impl<Data> Sync for Subsystem<Data> {}
>
> -// SAFETY: Ownership of `Subsystem` can safely be transferred to other threads.
> -unsafe impl<Data> Send for Subsystem<Data> {}
> +// SAFETY: Ownership of `Subsystem` can safely be transferred to other threads
> +// if its data can be transferred as well.
> +unsafe impl<Data: Send> Send for Subsystem<Data> {}
>
>  impl<Data> Subsystem<Data> {
>      /// Create an initializer for a [`Subsystem`].
>
> base-commit: 08df884136f1c1197bab2a27814404fd329d9aac

Re our discussion on zulip [1], I think your observation is correct, but
there are a few more issues we should fix:

- AttributeOperations: require AttributeOperations::Data: Sync. This is the
  point where the user implements a method that receives &Data from a
  foreign thread, so the requirement is visible close to the use site.

- GroupOperations: add Sync as a supertrait, since make_group and
  drop_item receive &self the same way.

- Change type GroupOperations::Child: 'static; to
  GroupOperations::Child: 'static + Send;, because release drops the
  child group on an arbitrary thread. The child's own Sync needs are
  already covered by its own AttributeOperations and GroupOperations
  impls.

- Update the SAFETY comments on the FFI callbacks that call
  get_group_data to cite these bounds as the justification for handing
  out &Data on this thread.

An alternative is to put Data: Sync on Subsystem::new and Data: Send +
Sync on Group::new. That is simpler but less precise, and it does not
document the requirement next to the trait methods that receive the
reference. I prefer the trait-level bounds.

Can you send a new version with these fixes?

Best regards,
Andreas

[1] https://rust-for-linux.zulipchat.com/#narrow/channel/288089-General/topic/Should.20add.20Data.3A.20Send.2FSync.20bounds.20in.20configfs.3A.3ASubsystem.3F/with/623192434

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

[Yilin Chen]

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`.

Fixes: 446cafc295bf ("rust: configfs: introduce rust support for configfs")
Link: https://rust-for-linux.zulipchat.com/#narrow/channel/288089-General/topic/Should.20add.20Data.3A.20Send.2FSync.20bounds.20in.20configfs.3A.3ASubsystem.3F/with/623719979
Assisted-by: Gpt-5.6 Sol
Signed-off-by: Yilin Chen <1479826151@qq.com>
---
Changes in v2:
- Require attribute data and group operation implementers to be `Sync`.
- Require child data to be `Send` for arbitrary-thread release.
- Document the bounds that make shared references safe in FFI callbacks.
- Remove redundant `Child: 'static` bounds from `GroupOperationsVTable`
  and `new_with_child_ctor`.

 rust/kernel/configfs.rs | 35 ++++++++++++++++++++---------------
 1 file changed, 20 insertions(+), 15 deletions(-)

diff --git a/rust/kernel/configfs.rs b/rust/kernel/configfs.rs
index cd082b83e9e7..de9306a5e527 100644
--- a/rust/kernel/configfs.rs
+++ b/rust/kernel/configfs.rs
@@ -135,8 +135,9 @@ pub struct Subsystem<Data> {
 // SAFETY: We do not provide any operations on `Subsystem`.
 unsafe impl<Data> Sync for Subsystem<Data> {}
 
-// SAFETY: Ownership of `Subsystem` can safely be transferred to other threads.
-unsafe impl<Data> Send for Subsystem<Data> {}
+// SAFETY: Ownership of `Subsystem` can safely be transferred to other threads
+// if its data can be transferred as well.
+unsafe impl<Data: Send> Send for Subsystem<Data> {}
 
 impl<Data> Subsystem<Data> {
     /// Create an initializer for a [`Subsystem`].
@@ -325,7 +326,6 @@ 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>,
-    Child: 'static,
 {
     /// # Safety
     ///
@@ -344,8 +344,9 @@ impl<Parent, Child> GroupOperationsVTable<Parent, Child>
         this: *mut bindings::config_group,
         name: *const kernel::ffi::c_char,
     ) -> *mut bindings::config_group {
-        // SAFETY: By function safety requirements of this function, this call
-        // is safe.
+        // SAFETY: By function safety requirements, `this` points to a configfs
+        // group containing `Parent`. The `GroupOperations` bound guarantees
+        // that `Parent: Sync`, so it is safe to share it with this thread.
         let parent_data = unsafe { get_group_data(this) };
 
         let group_init = match Parent::make_group(
@@ -390,8 +391,9 @@ impl<Parent, Child> GroupOperationsVTable<Parent, Child>
         this: *mut bindings::config_group,
         item: *mut bindings::config_item,
     ) {
-        // SAFETY: By function safety requirements of this function, this call
-        // is safe.
+        // SAFETY: By function safety requirements, `this` points to a configfs
+        // group containing `Parent`. The `GroupOperations` bound guarantees
+        // that `Parent: Sync`, so it is safe to share it with this thread.
         let parent_data = unsafe { get_group_data(this) };
 
         // SAFETY: By function safety requirements, `item` is embedded in a
@@ -483,12 +485,12 @@ const fn vtable_ptr() -> *const bindings::configfs_item_operations {
 ///
 /// Implement this trait on structs that embed a [`Subsystem`] or a [`Group`].
 #[vtable]
-pub trait GroupOperations {
+pub trait GroupOperations: Sync {
     /// The child data object type.
     ///
     /// This group will create subgroups (subdirectories) backed by this kind of
     /// object.
-    type Child: 'static;
+    type Child: 'static + Send;
 
     /// Creates a new subgroup.
     ///
@@ -555,8 +557,10 @@ impl<const ID: u64, O, Data> Attribute<ID, O, Data>
             // `config_group`.
             unsafe { container_of!(item, bindings::config_group, cg_item) };
 
-        // SAFETY: The function safety requirements for this function satisfy
-        // the conditions for this call.
+        // SAFETY: By function safety requirements, `c_group` points to a
+        // configfs group containing `Data`. The `AttributeOperations` bound
+        // guarantees that `Data: Sync`, so it is safe to share it with this
+        // thread.
         let data: &Data = unsafe { get_group_data(c_group) };
 
         // SAFETY: By function safety requirements, `page` is writable for `PAGE_SIZE`.
@@ -589,8 +593,10 @@ impl<const ID: u64, O, Data> Attribute<ID, O, Data>
         // `config_group`.
             unsafe { container_of!(item, bindings::config_group, cg_item) };
 
-        // SAFETY: The function safety requirements for this function satisfy
-        // the conditions for this call.
+        // SAFETY: By function safety requirements, `c_group` points to a
+        // configfs group containing `Data`. The `AttributeOperations` bound
+        // guarantees that `Data: Sync`, so it is safe to share it with this
+        // thread.
         let data: &Data = unsafe { get_group_data(c_group) };
 
         let ret = O::store(
@@ -643,7 +649,7 @@ pub const fn new(name: &'static CStr) -> Self {
 pub trait AttributeOperations<const ID: u64 = 0> {
     /// The type of the object that contains the field that is backing the
     /// attribute for this operation.
-    type Data;
+    type Data: Sync;
 
     /// Renders the value of an attribute.
     ///
@@ -749,7 +755,6 @@ pub const fn new_with_child_ctor<const N: usize, Child>(
             ) -> Self
             where
                 Data: GroupOperations<Child = Child>,
-                Child: 'static,
             {
                 Self {
                     item_type: Opaque::new(bindings::config_item_type {

base-commit: 08df884136f1c1197bab2a27814404fd329d9aac