rust/kernel/configfs.rs | 35 ++++++++++++++++++++--------------- 1 file changed, 20 insertions(+), 15 deletions(-)
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"Yilin Chen" <1479826151@qq.com> 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
© 2016 - 2026 Red Hat, Inc.