From 75a7b151e808355a1fdf972e85da137612b8f2ae Mon Sep 17 00:00:00 2001 From: Danilo Krummrich Date: Tue, 12 Aug 2025 15:09:06 +0200 Subject: rust: devres: fix leaking call to devm_add_action() When the data argument of Devres::new() is Err(), we leak the preceding call to devm_add_action(). In order to fix this, call devm_add_action() in a unit type initializer in try_pin_init!() after the initializers of all other fields. Fixes: f5d3ef25d238 ("rust: devres: get rid of Devres' inner Arc") Reviewed-by: Alice Ryhl Reviewed-by: Benno Lossin Link: https://lore.kernel.org/r/20250812130928.11075-1-dakr@kernel.org Signed-off-by: Danilo Krummrich --- rust/kernel/devres.rs | 27 ++++++++++++++++++--------- 1 file changed, 18 insertions(+), 9 deletions(-) (limited to 'rust/kernel/devres.rs') diff --git a/rust/kernel/devres.rs b/rust/kernel/devres.rs index da18091143a6..d04e3fcebafb 100644 --- a/rust/kernel/devres.rs +++ b/rust/kernel/devres.rs @@ -115,10 +115,11 @@ pub struct Devres { /// Contains all the fields shared with [`Self::callback`]. // TODO: Replace with `UnsafePinned`, once available. // - // Subsequently, the `drop_in_place()` in `Devres::drop` and the explicit `Send` and `Sync' - // impls can be removed. + // Subsequently, the `drop_in_place()` in `Devres::drop` and `Devres::new` as well as the + // explicit `Send` and `Sync' impls can be removed. #[pin] inner: Opaque>, + _add_action: (), } impl Devres { @@ -140,7 +141,15 @@ impl Devres { dev: dev.into(), callback, // INVARIANT: `inner` is properly initialized. - inner <- { + inner <- Opaque::pin_init(try_pin_init!(Inner { + devm <- Completion::new(), + revoke <- Completion::new(), + data <- Revocable::new(data), + })), + // TODO: Replace with "initializer code blocks" [1] once available. + // + // [1] https://github.com/Rust-for-Linux/pin-init/pull/69 + _add_action: { // SAFETY: `this` is a valid pointer to uninitialized memory. let inner = unsafe { &raw mut (*this.as_ptr()).inner }; @@ -152,13 +161,13 @@ impl Devres { // live at least as long as the returned `impl PinInit`. to_result(unsafe { bindings::devm_add_action(dev.as_raw(), Some(callback), inner.cast()) - })?; + }).inspect_err(|_| { + let inner = Opaque::cast_into(inner); - Opaque::pin_init(try_pin_init!(Inner { - devm <- Completion::new(), - revoke <- Completion::new(), - data <- Revocable::new(data), - })) + // SAFETY: `inner` is a valid pointer to an `Inner` and valid for both reads + // and writes. + unsafe { core::ptr::drop_in_place(inner) }; + })?; }, }) } -- cgit v1.2.3 From 1e180614b3608e1cb0f81753b2172af253d58a52 Mon Sep 17 00:00:00 2001 From: Shankari Anand Date: Thu, 14 Aug 2025 16:16:15 +0530 Subject: rust: driver-core: Update ARef and AlwaysRefCounted imports from sync::aref Update call sites in the driver-core files and its related samples to import `ARef` and `AlwaysRefCounted` from `sync::aref` instead of `types`. This aligns with the ongoing effort to move `ARef` and `AlwaysRefCounted` to sync. Suggested-by: Benno Lossin Link: https://github.com/Rust-for-Linux/linux/issues/1173 Signed-off-by: Shankari Anand Link: https://lore.kernel.org/r/20250814104615.355106-1-shankari.ak0208@gmail.com Signed-off-by: Danilo Krummrich --- rust/kernel/auxiliary.rs | 2 +- rust/kernel/device.rs | 7 ++++--- rust/kernel/devres.rs | 4 ++-- rust/kernel/pci.rs | 5 +++-- rust/kernel/platform.rs | 2 +- samples/rust/rust_driver_pci.rs | 2 +- samples/rust/rust_driver_platform.rs | 2 +- 7 files changed, 13 insertions(+), 11 deletions(-) (limited to 'rust/kernel/devres.rs') diff --git a/rust/kernel/auxiliary.rs b/rust/kernel/auxiliary.rs index f73c460665ec..8dc7490a79a4 100644 --- a/rust/kernel/auxiliary.rs +++ b/rust/kernel/auxiliary.rs @@ -245,7 +245,7 @@ kernel::impl_device_context_deref!(unsafe { Device }); kernel::impl_device_context_into_aref!(Device); // SAFETY: Instances of `Device` are always reference-counted. -unsafe impl crate::types::AlwaysRefCounted for Device { +unsafe impl crate::sync::aref::AlwaysRefCounted for Device { fn inc_ref(&self) { // SAFETY: The existence of a shared reference guarantees that the refcount is non-zero. unsafe { bindings::get_device(self.as_ref().as_raw()) }; diff --git a/rust/kernel/device.rs b/rust/kernel/device.rs index b8613289de8e..b860ab389f18 100644 --- a/rust/kernel/device.rs +++ b/rust/kernel/device.rs @@ -6,7 +6,8 @@ use crate::{ bindings, - types::{ARef, ForeignOwnable, Opaque}, + sync::aref::ARef, + types::{ForeignOwnable, Opaque}, }; use core::{fmt, marker::PhantomData, ptr}; @@ -292,7 +293,7 @@ kernel::impl_device_context_deref!(unsafe { Device }); kernel::impl_device_context_into_aref!(Device); // SAFETY: Instances of `Device` are always reference-counted. -unsafe impl crate::types::AlwaysRefCounted for Device { +unsafe impl crate::sync::aref::AlwaysRefCounted for Device { fn inc_ref(&self) { // SAFETY: The existence of a shared reference guarantees that the refcount is non-zero. unsafe { bindings::get_device(self.as_raw()) }; @@ -411,7 +412,7 @@ macro_rules! impl_device_context_deref { #[macro_export] macro_rules! __impl_device_context_into_aref { ($src:ty, $device:tt) => { - impl ::core::convert::From<&$device<$src>> for $crate::types::ARef<$device> { + impl ::core::convert::From<&$device<$src>> for $crate::sync::aref::ARef<$device> { fn from(dev: &$device<$src>) -> Self { (&**dev).into() } diff --git a/rust/kernel/devres.rs b/rust/kernel/devres.rs index da18091143a6..99b7520019f0 100644 --- a/rust/kernel/devres.rs +++ b/rust/kernel/devres.rs @@ -13,8 +13,8 @@ use crate::{ ffi::c_void, prelude::*, revocable::{Revocable, RevocableGuard}, - sync::{rcu, Completion}, - types::{ARef, ForeignOwnable, Opaque, ScopeGuard}, + sync::{aref::ARef, rcu, Completion}, + types::{ForeignOwnable, Opaque, ScopeGuard}, }; use pin_init::Wrapper; diff --git a/rust/kernel/pci.rs b/rust/kernel/pci.rs index 08fb69e5eb97..cae4e274f776 100644 --- a/rust/kernel/pci.rs +++ b/rust/kernel/pci.rs @@ -13,7 +13,8 @@ use crate::{ io::{Io, IoRaw}, irq::{self, IrqRequest}, str::CStr, - types::{ARef, Opaque}, + sync::aref::ARef, + types::Opaque, ThisModule, }; use core::{ @@ -544,7 +545,7 @@ kernel::impl_device_context_into_aref!(Device); impl crate::dma::Device for Device {} // SAFETY: Instances of `Device` are always reference-counted. -unsafe impl crate::types::AlwaysRefCounted for Device { +unsafe impl crate::sync::aref::AlwaysRefCounted for Device { fn inc_ref(&self) { // SAFETY: The existence of a shared reference guarantees that the refcount is non-zero. unsafe { bindings::pci_dev_get(self.as_raw()) }; diff --git a/rust/kernel/platform.rs b/rust/kernel/platform.rs index ce2bb4d4e882..7205fe3416d3 100644 --- a/rust/kernel/platform.rs +++ b/rust/kernel/platform.rs @@ -468,7 +468,7 @@ kernel::impl_device_context_into_aref!(Device); impl crate::dma::Device for Device {} // SAFETY: Instances of `Device` are always reference-counted. -unsafe impl crate::types::AlwaysRefCounted for Device { +unsafe impl crate::sync::aref::AlwaysRefCounted for Device { fn inc_ref(&self) { // SAFETY: The existence of a shared reference guarantees that the refcount is non-zero. unsafe { bindings::get_device(self.as_ref().as_raw()) }; diff --git a/samples/rust/rust_driver_pci.rs b/samples/rust/rust_driver_pci.rs index 606946ff4d7f..0798019014cd 100644 --- a/samples/rust/rust_driver_pci.rs +++ b/samples/rust/rust_driver_pci.rs @@ -4,7 +4,7 @@ //! //! To make this driver probe, QEMU must be run with `-device pci-testdev`. -use kernel::{bindings, c_str, device::Core, devres::Devres, pci, prelude::*, types::ARef}; +use kernel::{bindings, c_str, device::Core, devres::Devres, pci, prelude::*, sync::aref::ARef}; struct Regs; diff --git a/samples/rust/rust_driver_platform.rs b/samples/rust/rust_driver_platform.rs index 69ed55b7b0fa..6473baf4f120 100644 --- a/samples/rust/rust_driver_platform.rs +++ b/samples/rust/rust_driver_platform.rs @@ -72,7 +72,7 @@ use kernel::{ of, platform, prelude::*, str::CString, - types::ARef, + sync::aref::ARef, }; struct SampleDriver { -- cgit v1.2.3 From 42415d163e5df6db799c7de6262d707e402c2c7e Mon Sep 17 00:00:00 2001 From: Benno Lossin Date: Fri, 5 Sep 2025 16:00:46 +0200 Subject: rust: pin-init: add references to previously initialized fields After initializing a field in an initializer macro, create a variable holding a reference that points at that field. The type is either `Pin<&mut T>` or `&mut T` depending on the field's structural pinning kind. [ Applied fixes to devres and rust_driver_pci sample - Benno] Reviewed-by: Danilo Krummrich Signed-off-by: Benno Lossin --- rust/kernel/devres.rs | 6 +- rust/pin-init/src/macros.rs | 149 +++++++++++++++++++++++++++++++--------- samples/rust/rust_driver_pci.rs | 2 +- 3 files changed, 118 insertions(+), 39 deletions(-) (limited to 'rust/kernel/devres.rs') diff --git a/rust/kernel/devres.rs b/rust/kernel/devres.rs index da18091143a6..91dbf3f4b166 100644 --- a/rust/kernel/devres.rs +++ b/rust/kernel/devres.rs @@ -134,11 +134,9 @@ impl Devres { T: 'a, Error: From, { - let callback = Self::devres_callback; - try_pin_init!(&this in Self { dev: dev.into(), - callback, + callback: Self::devres_callback, // INVARIANT: `inner` is properly initialized. inner <- { // SAFETY: `this` is a valid pointer to uninitialized memory. @@ -151,7 +149,7 @@ impl Devres { // properly initialized, because we require `dev` (i.e. the *bound* device) to // live at least as long as the returned `impl PinInit`. to_result(unsafe { - bindings::devm_add_action(dev.as_raw(), Some(callback), inner.cast()) + bindings::devm_add_action(dev.as_raw(), Some(*callback), inner.cast()) })?; Opaque::pin_init(try_pin_init!(Inner { diff --git a/rust/pin-init/src/macros.rs b/rust/pin-init/src/macros.rs index c3462080b7b6..d6acf2cd291e 100644 --- a/rust/pin-init/src/macros.rs +++ b/rust/pin-init/src/macros.rs @@ -1049,38 +1049,56 @@ macro_rules! __pin_data { @pinned($($(#[$($p_attr:tt)*])* $pvis:vis $p_field:ident : $p_type:ty),* $(,)?), @not_pinned($($(#[$($attr:tt)*])* $fvis:vis $field:ident : $type:ty),* $(,)?), ) => { - // For every field, we create a projection function according to its projection type. If a - // field is structurally pinned, then it must be initialized via `PinInit`, if it is not - // structurally pinned, then it can be initialized via `Init`. - // - // The functions are `unsafe` to prevent accidentally calling them. - #[allow(dead_code)] - #[expect(clippy::missing_safety_doc)] - impl<$($impl_generics)*> $pin_data<$($ty_generics)*> - where $($whr)* - { - $( - $(#[$($p_attr)*])* - $pvis unsafe fn $p_field( - self, - slot: *mut $p_type, - init: impl $crate::PinInit<$p_type, E>, - ) -> ::core::result::Result<(), E> { - // SAFETY: TODO. - unsafe { $crate::PinInit::__pinned_init(init, slot) } - } - )* - $( - $(#[$($attr)*])* - $fvis unsafe fn $field( - self, - slot: *mut $type, - init: impl $crate::Init<$type, E>, - ) -> ::core::result::Result<(), E> { - // SAFETY: TODO. - unsafe { $crate::Init::__init(init, slot) } - } - )* + $crate::macros::paste! { + // For every field, we create a projection function according to its projection type. If a + // field is structurally pinned, then it must be initialized via `PinInit`, if it is not + // structurally pinned, then it can be initialized via `Init`. + // + // The functions are `unsafe` to prevent accidentally calling them. + #[allow(dead_code)] + #[expect(clippy::missing_safety_doc)] + impl<$($impl_generics)*> $pin_data<$($ty_generics)*> + where $($whr)* + { + $( + $(#[$($p_attr)*])* + $pvis unsafe fn $p_field( + self, + slot: *mut $p_type, + init: impl $crate::PinInit<$p_type, E>, + ) -> ::core::result::Result<(), E> { + // SAFETY: TODO. + unsafe { $crate::PinInit::__pinned_init(init, slot) } + } + + $(#[$($p_attr)*])* + $pvis unsafe fn [<__project_ $p_field>]<'__slot>( + self, + slot: &'__slot mut $p_type, + ) -> ::core::pin::Pin<&'__slot mut $p_type> { + ::core::pin::Pin::new_unchecked(slot) + } + )* + $( + $(#[$($attr)*])* + $fvis unsafe fn $field( + self, + slot: *mut $type, + init: impl $crate::Init<$type, E>, + ) -> ::core::result::Result<(), E> { + // SAFETY: TODO. + unsafe { $crate::Init::__init(init, slot) } + } + + $(#[$($attr)*])* + $fvis unsafe fn [<__project_ $field>]<'__slot>( + self, + slot: &'__slot mut $type, + ) -> &'__slot mut $type { + slot + } + )* + } } }; } @@ -1292,6 +1310,13 @@ macro_rules! __init_internal { // return when an error/panic occurs. // We also use the `data` to require the correct trait (`Init` or `PinInit`) for `$field`. unsafe { $data.$field(::core::ptr::addr_of_mut!((*$slot).$field), init)? }; + // SAFETY: + // - the project function does the correct field projection, + // - the field has been initialized, + // - the reference is only valid until the end of the initializer. + #[allow(unused_variables)] + let $field = $crate::macros::paste!(unsafe { $data.[< __project_ $field >](&mut (*$slot).$field) }); + // Create the drop guard: // // We rely on macro hygiene to make it impossible for users to access this local variable. @@ -1323,6 +1348,14 @@ macro_rules! __init_internal { // SAFETY: `slot` is valid, because we are inside of an initializer closure, we // return when an error/panic occurs. unsafe { $crate::Init::__init(init, ::core::ptr::addr_of_mut!((*$slot).$field))? }; + + // SAFETY: + // - the field is not structurally pinned, since the line above must compile, + // - the field has been initialized, + // - the reference is only valid until the end of the initializer. + #[allow(unused_variables)] + let $field = unsafe { &mut (*$slot).$field }; + // Create the drop guard: // // We rely on macro hygiene to make it impossible for users to access this local variable. @@ -1341,7 +1374,48 @@ macro_rules! __init_internal { ); } }; - (init_slot($($use_data:ident)?): + (init_slot(): // No `use_data`, so all fields are not structurally pinned + @data($data:ident), + @slot($slot:ident), + @guards($($guards:ident,)*), + // Init by-value. + @munch_fields($field:ident $(: $val:expr)?, $($rest:tt)*), + ) => { + { + $(let $field = $val;)? + // Initialize the field. + // + // SAFETY: The memory at `slot` is uninitialized. + unsafe { ::core::ptr::write(::core::ptr::addr_of_mut!((*$slot).$field), $field) }; + } + + #[allow(unused_variables)] + // SAFETY: + // - the field is not structurally pinned, since no `use_data` was required to create this + // initializer, + // - the field has been initialized, + // - the reference is only valid until the end of the initializer. + let $field = unsafe { &mut (*$slot).$field }; + + // Create the drop guard: + // + // We rely on macro hygiene to make it impossible for users to access this local variable. + // We use `paste!` to create new hygiene for `$field`. + $crate::macros::paste! { + // SAFETY: We forget the guard later when initialization has succeeded. + let [< __ $field _guard >] = unsafe { + $crate::__internal::DropGuard::new(::core::ptr::addr_of_mut!((*$slot).$field)) + }; + + $crate::__init_internal!(init_slot(): + @data($data), + @slot($slot), + @guards([< __ $field _guard >], $($guards,)*), + @munch_fields($($rest)*), + ); + } + }; + (init_slot($use_data:ident): @data($data:ident), @slot($slot:ident), @guards($($guards:ident,)*), @@ -1355,6 +1429,13 @@ macro_rules! __init_internal { // SAFETY: The memory at `slot` is uninitialized. unsafe { ::core::ptr::write(::core::ptr::addr_of_mut!((*$slot).$field), $field) }; } + // SAFETY: + // - the project function does the correct field projection, + // - the field has been initialized, + // - the reference is only valid until the end of the initializer. + #[allow(unused_variables)] + let $field = $crate::macros::paste!(unsafe { $data.[< __project_ $field >](&mut (*$slot).$field) }); + // Create the drop guard: // // We rely on macro hygiene to make it impossible for users to access this local variable. @@ -1365,7 +1446,7 @@ macro_rules! __init_internal { $crate::__internal::DropGuard::new(::core::ptr::addr_of_mut!((*$slot).$field)) }; - $crate::__init_internal!(init_slot($($use_data)?): + $crate::__init_internal!(init_slot($use_data): @data($data), @slot($slot), @guards([< __ $field _guard >], $($guards,)*), diff --git a/samples/rust/rust_driver_pci.rs b/samples/rust/rust_driver_pci.rs index 606946ff4d7f..1ac0b06fa3b3 100644 --- a/samples/rust/rust_driver_pci.rs +++ b/samples/rust/rust_driver_pci.rs @@ -78,8 +78,8 @@ impl pci::Driver for SampleDriver { let drvdata = KBox::pin_init( try_pin_init!(Self { - pdev: pdev.into(), bar <- pdev.iomap_region_sized::<{ Regs::END }>(0, c_str!("rust_driver_pci")), + pdev: pdev.into(), index: *info, }), GFP_KERNEL, -- cgit v1.2.3 From ff4d2ef3874773c9c6173b0f099372bf62252aaf Mon Sep 17 00:00:00 2001 From: Miguel Ojeda Date: Wed, 29 Oct 2025 08:14:06 +0100 Subject: rust: devres: fix private intra-doc link The future move of pin-init to `syn` uncovers the following private intra-doc link: error: public documentation for `Devres` links to private item `Self::inner` --> rust/kernel/devres.rs:106:7 | 106 | /// [`Self::inner`] is guaranteed to be initialized and is always accessed read-only. | ^^^^^^^^^^^ this item is private | = note: this link will resolve properly if you pass `--document-private-items` = note: `-D rustdoc::private-intra-doc-links` implied by `-D warnings` = help: to override `-D warnings` add `#[allow(rustdoc::private_intra_doc_links)]` Currently, when rendered, the link points to "nowhere" (an inexistent anchor for a "method"). Thus fix it. Cc: stable@vger.kernel.org Fixes: f5d3ef25d238 ("rust: devres: get rid of Devres' inner Arc") Acked-by: Danilo Krummrich Link: https://patch.msgid.link/20251029071406.324511-1-ojeda@kernel.org Signed-off-by: Miguel Ojeda --- rust/kernel/devres.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'rust/kernel/devres.rs') diff --git a/rust/kernel/devres.rs b/rust/kernel/devres.rs index 10a6a1789854..2392c281459e 100644 --- a/rust/kernel/devres.rs +++ b/rust/kernel/devres.rs @@ -103,7 +103,7 @@ struct Inner { /// /// # Invariants /// -/// [`Self::inner`] is guaranteed to be initialized and is always accessed read-only. +/// `Self::inner` is guaranteed to be initialized and is always accessed read-only. #[pin_data(PinnedDrop)] pub struct Devres { dev: ARef, -- cgit v1.2.3