From 17ca0bc06bb246d9488556f3f16ad6f37a4fc5ad Mon Sep 17 00:00:00 2001 From: Yilin Chen <1479826151@qq.com> Date: Wed, 8 Jul 2026 07:21:06 +0000 Subject: [PATCH 1/3] rust: configfs: fix release safety documentation ItemOperationsVTable, Data>::release() receives a pointer to a config_item, not a config_group. The old Safety docs described the pointer as a config_group embedded in Group. Update them to state that the pointer must be the cg_item field of a config_group embedded in a valid Group. Assisted-by: Codex:GPT-5 Signed-off-by: Yilin Chen <1479826151@qq.com> Link: https://msgid.link/tencent_9FAFC60382D724C608F0F6BFABFC63BB6707@qq.com Signed-off-by: Andreas Hindborg --- rust/kernel/configfs.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/rust/kernel/configfs.rs b/rust/kernel/configfs.rs index cd082b83e9e7..7722035ebc02 100644 --- a/rust/kernel/configfs.rs +++ b/rust/kernel/configfs.rs @@ -437,8 +437,8 @@ impl ItemOperationsVTable, Data> { /// # Safety /// - /// `this` must be a pointer to a `bindings::config_group` embedded in a - /// `Group`. + /// `this` must be a pointer to the `cg_item` field of a `bindings::config_group` embedded in a + /// valid `Group`. /// /// This function will destroy the pointee of `this`. The pointee of `this` /// must not be accessed after the function returns. From ec27f3b700010d2ebae1cde2df0246197689d159 Mon Sep 17 00:00:00 2001 From: Yilin Chen <1479826151@qq.com> Date: Mon, 14 Sep 2026 14:09:00 +0000 Subject: [PATCH 2/3] rust: configfs: require thread-safe callback data 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`, 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> Link: https://msgid.link/tencent_5449081195EDDF5726C709CF648DD4044A08@qq.com [ Andreas: Add Send/Sync bounds for `Group`, add `Send` bounds to `GroupOperationsVTable`, `ItemOperationsVTable` and `ItemType` macro implementation. ] Signed-off-by: Andreas Hindborg --- rust/kernel/configfs.rs | 63 ++++++++++++++++++++++++++++------------- 1 file changed, 44 insertions(+), 19 deletions(-) diff --git a/rust/kernel/configfs.rs b/rust/kernel/configfs.rs index 7722035ebc02..ec295ab965da 100644 --- a/rust/kernel/configfs.rs +++ b/rust/kernel/configfs.rs @@ -135,8 +135,9 @@ pub struct Subsystem { // SAFETY: We do not provide any operations on `Subsystem`. unsafe impl Sync for Subsystem {} -// SAFETY: Ownership of `Subsystem` can safely be transferred to other threads. -unsafe impl Send for Subsystem {} +// SAFETY: Ownership of `Subsystem` can safely be transferred to other threads +// if its data can be transferred as well. +unsafe impl Send for Subsystem {} impl Subsystem { /// Create an initializer for a [`Subsystem`]. @@ -249,6 +250,13 @@ pub struct Group { data: Data, } +// SAFETY: We do not provide any operations on `Group`. +unsafe impl Sync for Group {} + +// SAFETY: Ownership of `Group` can safely be transferred to other threads if +// its data can be transferred as well. +unsafe impl Send for Group {} + impl Group { /// Create an initializer for a new group. /// @@ -325,7 +333,9 @@ unsafe fn get_group_data<'a, Parent>(this: *mut bindings::config_group) -> &'a P impl GroupOperationsVTable where Parent: GroupOperations, - Child: 'static, + // We transfer `Arc>` across a thread boundary in `make_group` + // and `drop_item`. + Arc>: Send, { /// # Safety /// @@ -344,8 +354,9 @@ impl GroupOperationsVTable 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 +401,9 @@ impl GroupOperationsVTable 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 @@ -403,7 +415,9 @@ impl GroupOperationsVTable 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>: Send` this + // ownership transfer is safe. let arc: Arc> = unsafe { Arc::from_raw(r_child_group_ptr.cast_mut()) }; Parent::drop_item(parent_data, arc.as_arc_borrow()); @@ -434,6 +448,8 @@ const fn vtable_ptr() -> *const bindings::configfs_group_operations { impl ItemOperationsVTable, Data> where Data: 'static, + // We transfer `Arc>` across a thread boundary in `release`. + Arc>: Send, { /// # Safety /// @@ -450,8 +466,9 @@ impl ItemOperationsVTable, Data> // embedded within a `Group`. let r_group_ptr = unsafe { Group::::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>: Send`, this is safe. let pin_self: Arc> = unsafe { Arc::from_raw(r_group_ptr.cast_mut()) }; drop(pin_self); } @@ -483,12 +500,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 +572,10 @@ impl Attribute // `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 +608,10 @@ impl Attribute // `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 +664,7 @@ pub const fn new(name: &'static CStr) -> Self { pub trait AttributeOperations { /// 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 +770,8 @@ pub const fn new_with_child_ctor( ) -> Self where Data: GroupOperations, - Child: 'static, + Arc>: Send, + Arc>: Send, { Self { item_type: Opaque::new(bindings::config_item_type { @@ -767,7 +789,10 @@ pub const fn new_with_child_ctor( pub const fn new( owner: &'static ThisModule, attributes: &'static AttributeList, - ) -> Self { + ) -> Self + where + Arc>: Send, + { Self { item_type: Opaque::new(bindings::config_item_type { ct_owner: owner.as_ptr(), From e84c9b64df615e658e47561b835c1364dc48f0c0 Mon Sep 17 00:00:00 2001 From: Andreas Hindborg Date: Mon, 8 Jun 2026 14:32:00 +0200 Subject: [PATCH 3/3] rust: configfs: fix data offset calculation for subsystem callbacks `get_group_data()` chooses between `Group::::container_of()` and `Subsystem::::container_of()` based on whether `this` represents the root group of a configfs subsystem. It detects this by checking `(*this).cg_subsys.is_null()`, but `link_group()` in `fs/configfs/dir.c` unconditionally sets `cg_subsys` for every `config_group` attached anywhere in a registered subsystem, including the subsystem's own `su_group`. The only `config_group` with a NULL `cg_subsys` is the configfs root, on which userspace cannot trigger callbacks. The check is therefore always false at runtime, and the `Subsystem` branch is dead. Subsystem-level callbacks reach the `Group` branch and may read `data` at the wrong offset. Thus change the `is_root` check to correctly identify whether a group is a root group by comparing `this` to `subsys.su_group`. Cc: stable@vger.kernel.org Fixes: 446cafc295bf ("rust: configfs: introduce rust support for configfs") Link: https://msgid.link/20260608-configfs-fix-offset-v1-1-7f01b8fb9e5f@kernel.org [ Andreas: Fix clippy warning on 1.85.0 by moving safety comment. ] Signed-off-by: Andreas Hindborg --- rust/kernel/configfs.rs | 43 +++++++++++++++++++++++++++-------------- 1 file changed, 28 insertions(+), 15 deletions(-) diff --git a/rust/kernel/configfs.rs b/rust/kernel/configfs.rs index ec295ab965da..0f506d3a3cf5 100644 --- a/rust/kernel/configfs.rs +++ b/rust/kernel/configfs.rs @@ -305,26 +305,39 @@ unsafe fn container_of(group: *const bindings::config_group) -> *const Self { /// /// `this` must be a valid pointer. /// -/// If `this` does not represent the root group of a configfs subsystem, -/// `this` must be a pointer to a `bindings::config_group` embedded in a -/// `Group`. +/// If `this` is the `su_group` field of a `bindings::configfs_subsystem`, that +/// `configfs_subsystem` must be embedded in a `Subsystem`. /// -/// Otherwise, `this` must be a pointer to a `bindings::config_group` that -/// is embedded in a `bindings::configfs_subsystem` that is embedded in a -/// `Subsystem`. +/// Otherwise, `this` must be a pointer to a `bindings::config_group` embedded +/// in a `Group`. unsafe fn get_group_data<'a, Parent>(this: *mut bindings::config_group) -> &'a Parent { // SAFETY: `this` is a valid pointer. - let is_root = unsafe { (*this).cg_subsys.is_null() }; + let subsys = unsafe { (*this).cg_subsys }; - if !is_root { - // SAFETY: By C API contact,`this` was returned from a call to - // `make_group`. The pointer is known to be embedded within a - // `Group`. - unsafe { &(*Group::::container_of(this)).data } - } else { - // SAFETY: By C API contract, `this` is a pointer to the - // `bindings::config_group` field within a `Subsystem`. + // `link_group()` in `fs/configfs/dir.c` assigns `cg_subsys` for every + // `config_group` attached anywhere in a registered subsystem, including + // the subsystem's own `su_group` (which gets a pointer to itself). The + // only `config_group` with a NULL `cg_subsys` is the configfs root, and + // userspace cannot trigger callbacks on it. The group is therefore the + // subsystem's `su_group` iff it equals `&cg_subsys->su_group`. + let is_root = !subsys.is_null() + && core::ptr::eq( + this.cast_const(), + // SAFETY: For every `config_group` the configfs core dispatches a + // callback on, `cg_subsys` was set by `link_group()` at registration time + // and points to a valid `configfs_subsystem` that outlives the callback. + unsafe { &raw const (*subsys).su_group }, + ); + + if is_root { + // SAFETY: By the above, `this` is the `su_group` field of a + // `configfs_subsystem` that, by function safety requirements, is + // embedded in a `Subsystem`. unsafe { &(*Subsystem::container_of(this)).data } + } else { + // SAFETY: By function safety requirements, `this` is a + // `config_group` embedded in a `Group`. + unsafe { &(*Group::::container_of(this)).data } } }