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

[Andreas Hindborg]

Hi,

Thanks for the patch.

"Yilin Chen" <1479826151@xxxxxx> 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@xxxxxx>
> ---
>  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