From f744201c6159fc7323c40936fd079525f7063598 Mon Sep 17 00:00:00 2001 From: Danilo Krummrich Date: Thu, 12 Jun 2025 14:17:15 +0200 Subject: rust: devres: fix race in Devres::drop() In Devres::drop() we first remove the devres action and then drop the wrapped device resource. The design goal is to give the owner of a Devres object control over when the device resource is dropped, but limit the overall scope to the corresponding device being bound to a driver. However, there's a race that was introduced with commit 8ff656643d30 ("rust: devres: remove action in `Devres::drop`"), but also has been (partially) present from the initial version on. In Devres::drop(), the devres action is removed successfully and subsequently the destructor of the wrapped device resource runs. However, there is no guarantee that the destructor of the wrapped device resource completes before the driver core is done unbinding the corresponding device. If in Devres::drop(), the devres action can't be removed, it means that the devres callback has been executed already, or is still running concurrently. In case of the latter, either Devres::drop() wins revoking the Revocable or the devres callback wins revoking the Revocable. If Devres::drop() wins, we (again) have no guarantee that the destructor of the wrapped device resource completes before the driver core is done unbinding the corresponding device. CPU0 CPU1 ------------------------------------------------------------------------ Devres::drop() { Devres::devres_callback() { self.data.revoke() { this.data.revoke() { is_available.swap() == true is_available.swap == false } } // [...] // device fully unbound drop_in_place() { // release device resource } } } Depending on the specific device resource, this can potentially lead to user-after-free bugs. In order to fix this, implement the following logic. In the devres callback, we're always good when we get to revoke the device resource ourselves, i.e. Revocable::revoke() returns true. If Revocable::revoke() returns false, it means that Devres::drop(), concurrently, already drops the device resource and we have to wait for Devres::drop() to signal that it finished dropping the device resource. Note that if we hit the case where we need to wait for the completion of Devres::drop() in the devres callback, it means that we're actually racing with a concurrent Devres::drop() call, which already started revoking the device resource for us. This is rather unlikely and means that the concurrent Devres::drop() already started doing our work and we just need to wait for it to complete it for us. Hence, there should not be any additional overhead from that. (Actually, for now it's even better if Devres::drop() does the work for us, since it can bypass the synchronize_rcu() call implied by Revocable::revoke(), but this goes away anyways once I get to implement the split devres callback approach, which allows us to first flip the atomics of all registered Devres objects of a certain device, execute a single synchronize_rcu() and then drop all revocable objects.) In Devres::drop() we try to revoke the device resource. If that is *not* successful, it means that the devres callback already did and we're good. Otherwise, we try to remove the devres action, which, if successful, means that we're good, since the device resource has just been revoked by us *before* we removed the devres action successfully. If the devres action could not be removed, it means that the devres callback must be running concurrently, hence we signal that the device resource has been revoked by us, using the completion. This makes it safe to drop a Devres object from any task and at any point of time, which is one of the design goals. Fixes: 76c01ded724b ("rust: add devres abstraction") Reported-by: Alice Ryhl Closes: https://lore.kernel.org/lkml/aD64YNuqbPPZHAa5@google.com/ Reviewed-by: Benno Lossin Link: https://lore.kernel.org/r/20250612121817.1621-4-dakr@kernel.org Signed-off-by: Danilo Krummrich --- rust/kernel/devres.rs | 37 +++++++++++++++++++++++++++++-------- 1 file changed, 29 insertions(+), 8 deletions(-) (limited to 'rust/kernel/devres.rs') diff --git a/rust/kernel/devres.rs b/rust/kernel/devres.rs index 0f79a2ec9474..2f74bce5ed9d 100644 --- a/rust/kernel/devres.rs +++ b/rust/kernel/devres.rs @@ -13,7 +13,7 @@ use crate::{ ffi::c_void, prelude::*, revocable::Revocable, - sync::Arc, + sync::{Arc, Completion}, types::ARef, }; @@ -25,13 +25,17 @@ struct DevresInner { callback: unsafe extern "C" fn(*mut c_void), #[pin] data: Revocable, + #[pin] + revoke: Completion, } /// This abstraction is meant to be used by subsystems to containerize [`Device`] bound resources to /// manage their lifetime. /// /// [`Device`] bound resources should be freed when either the resource goes out of scope or the -/// [`Device`] is unbound respectively, depending on what happens first. +/// [`Device`] is unbound respectively, depending on what happens first. In any case, it is always +/// guaranteed that revoking the device resource is completed before the corresponding [`Device`] +/// is unbound. /// /// To achieve that [`Devres`] registers a devres callback on creation, which is called once the /// [`Device`] is unbound, revoking access to the encapsulated resource (see also [`Revocable`]). @@ -102,6 +106,7 @@ impl DevresInner { dev: dev.into(), callback: Self::devres_callback, data <- Revocable::new(data), + revoke <- Completion::new(), }), flags, )?; @@ -130,26 +135,28 @@ impl DevresInner { self as _ } - fn remove_action(this: &Arc) { + fn remove_action(this: &Arc) -> bool { // SAFETY: // - `self.inner.dev` is a valid `Device`, // - the `action` and `data` pointers are the exact same ones as given to devm_add_action() // previously, // - `self` is always valid, even if the action has been released already. - let ret = unsafe { + let success = unsafe { bindings::devm_remove_action_nowarn( this.dev.as_raw(), Some(this.callback), this.as_ptr() as _, ) - }; + } == 0; - if ret == 0 { + if success { // SAFETY: We leaked an `Arc` reference to devm_add_action() in `DevresInner::new`; if // devm_remove_action_nowarn() was successful we can (and have to) claim back ownership // of this reference. let _ = unsafe { Arc::from_raw(this.as_ptr()) }; } + + success } #[allow(clippy::missing_safety_doc)] @@ -161,7 +168,12 @@ impl DevresInner { // `DevresInner::new`. let inner = unsafe { Arc::from_raw(ptr) }; - inner.data.revoke(); + if !inner.data.revoke() { + // If `revoke()` returns false, it means that `Devres::drop` already started revoking + // `inner.data` for us. Hence we have to wait until `Devres::drop()` signals that it + // completed revoking `inner.data`. + inner.revoke.wait_for_completion(); + } } } @@ -232,6 +244,15 @@ impl Deref for Devres { impl Drop for Devres { fn drop(&mut self) { - DevresInner::remove_action(&self.0); + // SAFETY: When `drop` runs, it is guaranteed that nobody is accessing the revocable data + // anymore, hence it is safe not to wait for the grace period to finish. + if unsafe { self.revoke_nosync() } { + // We revoked `self.0.data` before the devres action did, hence try to remove it. + if !DevresInner::remove_action(&self.0) { + // We could not remove the devres action, which means that it now runs concurrently, + // hence signal that `self.0.data` has been revoked successfully. + self.0.revoke.complete_all(); + } + } } } -- cgit From 20c96ed278e362ae4e324ed7d8c69fb48c508d3c Mon Sep 17 00:00:00 2001 From: Danilo Krummrich Date: Wed, 11 Jun 2025 19:48:25 +0200 Subject: rust: devres: do not dereference to the internal Revocable We can't expose direct access to the internal Revocable, since this allows users to directly revoke the internal Revocable without Devres having the chance to synchronize with the devres callback -- we have to guarantee that the internal Revocable has been fully revoked before the device is fully unbound. Hence, remove the corresponding Deref implementation and, instead, provide indirect accessors for the internal Revocable. Note that we can still support Devres::revoke() by implementing the required synchronization (which would be almost identical to the synchronization in Devres::drop()). Fixes: 76c01ded724b ("rust: add devres abstraction") Reviewed-by: Benno Lossin Link: https://lore.kernel.org/r/20250611174827.380555-1-dakr@kernel.org Signed-off-by: Danilo Krummrich --- rust/kernel/devres.rs | 27 ++++++++++++++++----------- 1 file changed, 16 insertions(+), 11 deletions(-) (limited to 'rust/kernel/devres.rs') diff --git a/rust/kernel/devres.rs b/rust/kernel/devres.rs index 2f74bce5ed9d..57502534d985 100644 --- a/rust/kernel/devres.rs +++ b/rust/kernel/devres.rs @@ -12,13 +12,11 @@ use crate::{ error::{Error, Result}, ffi::c_void, prelude::*, - revocable::Revocable, - sync::{Arc, Completion}, + revocable::{Revocable, RevocableGuard}, + sync::{rcu, Arc, Completion}, types::ARef, }; -use core::ops::Deref; - #[pin_data] struct DevresInner { dev: ARef, @@ -230,15 +228,22 @@ impl Devres { // SAFETY: `dev` being the same device as the device this `Devres` has been created for // proves that `self.0.data` hasn't been revoked and is guaranteed to not be revoked as // long as `dev` lives; `dev` lives at least as long as `self`. - Ok(unsafe { self.deref().access() }) + Ok(unsafe { self.0.data.access() }) } -} -impl Deref for Devres { - type Target = Revocable; + /// [`Devres`] accessor for [`Revocable::try_access`]. + pub fn try_access(&self) -> Option> { + self.0.data.try_access() + } + + /// [`Devres`] accessor for [`Revocable::try_access_with`]. + pub fn try_access_with R>(&self, f: F) -> Option { + self.0.data.try_access_with(f) + } - fn deref(&self) -> &Self::Target { - &self.0.data + /// [`Devres`] accessor for [`Revocable::try_access_with_guard`]. + pub fn try_access_with_guard<'a>(&'a self, guard: &'a rcu::Guard) -> Option<&'a T> { + self.0.data.try_access_with_guard(guard) } } @@ -246,7 +251,7 @@ impl Drop for Devres { fn drop(&mut self) { // SAFETY: When `drop` runs, it is guaranteed that nobody is accessing the revocable data // anymore, hence it is safe not to wait for the grace period to finish. - if unsafe { self.revoke_nosync() } { + if unsafe { self.0.data.revoke_nosync() } { // We revoked `self.0.data` before the devres action did, hence try to remove it. if !DevresInner::remove_action(&self.0) { // We could not remove the devres action, which means that it now runs concurrently, -- cgit From fcad9bbf9e1a7de6c53908954ba1b1a1ab11ef1e Mon Sep 17 00:00:00 2001 From: Tamir Duberstein Date: Sun, 15 Jun 2025 16:55:05 -0400 Subject: rust: enable `clippy::ptr_as_ptr` lint In Rust 1.51.0, Clippy introduced the `ptr_as_ptr` lint [1]: > Though `as` casts between raw pointers are not terrible, > `pointer::cast` is safer because it cannot accidentally change the > pointer's mutability, nor cast the pointer to other types like `usize`. There are a few classes of changes required: - Modules generated by bindgen are marked `#[allow(clippy::ptr_as_ptr)]`. - Inferred casts (` as _`) are replaced with `.cast()`. - Ascribed casts (` as *... T`) are replaced with `.cast::()`. - Multistep casts from references (` as *const _ as *const T`) are replaced with `core::ptr::from_ref(&x).cast()` with or without `::` according to the previous rules. The `core::ptr::from_ref` call is required because `(x as *const _).cast::()` results in inference failure. - Native literal C strings are replaced with `c_str!().as_char_ptr()`. - `*mut *mut T as _` is replaced with `let *mut *const T = (*mut *mut T)`.cast();` since pointer to pointer can be confusing. Apply these changes and enable the lint -- no functional change intended. Link: https://rust-lang.github.io/rust-clippy/master/index.html#ptr_as_ptr [1] Reviewed-by: Benno Lossin Reviewed-by: Boqun Feng Signed-off-by: Tamir Duberstein Acked-by: Viresh Kumar Acked-by: Greg Kroah-Hartman Acked-by: Tejun Heo Acked-by: Danilo Krummrich Link: https://lore.kernel.org/r/20250615-ptr-as-ptr-v12-1-f43b024581e8@gmail.com [ Added `.cast()` for `opp`. - Miguel ] 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 57502534d985..b8ba5417337b 100644 --- a/rust/kernel/devres.rs +++ b/rust/kernel/devres.rs @@ -159,7 +159,7 @@ impl DevresInner { #[allow(clippy::missing_safety_doc)] unsafe extern "C" fn devres_callback(ptr: *mut kernel::ffi::c_void) { - let ptr = ptr as *mut DevresInner; + let ptr = ptr.cast::>(); // Devres owned this memory; now that we received the callback, drop the `Arc` and hence the // reference. // SAFETY: Safe, since we leaked an `Arc` reference to devm_add_action() in -- cgit From 23773bd8da719b83013e66795e990036c4bfe014 Mon Sep 17 00:00:00 2001 From: Tamir Duberstein Date: Sun, 15 Jun 2025 16:55:07 -0400 Subject: rust: enable `clippy::as_ptr_cast_mut` lint MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit In Rust 1.66.0, Clippy introduced the `as_ptr_cast_mut` lint [1]: > Since `as_ptr` takes a `&self`, the pointer won’t have write > permissions unless interior mutability is used, making it unlikely > that having it as a mutable pointer is correct. There is only one affected callsite, and the change amounts to replacing `as _` with `.cast_mut().cast()`. This doesn't change the semantics, but is more descriptive of what's going on. Apply this change and enable the lint -- no functional change intended. Link: https://rust-lang.github.io/rust-clippy/master/index.html#as_ptr_cast_mut [1] Reviewed-by: Benno Lossin Reviewed-by: Boqun Feng Signed-off-by: Tamir Duberstein Acked-by: Greg Kroah-Hartman Acked-by: Danilo Krummrich Link: https://lore.kernel.org/r/20250615-ptr-as-ptr-v12-3-f43b024581e8@gmail.com 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 b8ba5417337b..b418cfc6f90d 100644 --- a/rust/kernel/devres.rs +++ b/rust/kernel/devres.rs @@ -143,7 +143,7 @@ impl DevresInner { bindings::devm_remove_action_nowarn( this.dev.as_raw(), Some(this.callback), - this.as_ptr() as _, + this.as_ptr().cast_mut().cast(), ) } == 0; -- cgit From 5e30550558b1eace5fa4af4e2257216fa8a7c90f Mon Sep 17 00:00:00 2001 From: Tamir Duberstein Date: Sun, 15 Jun 2025 16:55:08 -0400 Subject: rust: enable `clippy::as_underscore` lint In Rust 1.63.0, Clippy introduced the `as_underscore` lint [1]: > The conversion might include lossy conversion or a dangerous cast that > might go undetected due to the type being inferred. > > The lint is allowed by default as using `_` is less wordy than always > specifying the type. Always specifying the type is especially helpful in function call contexts where the inferred type may change at a distance. Specifying the type also allows Clippy to spot more cases of `useless_conversion`. The primary downside is the need to specify the type in trivial getters. There are 4 such functions: 3 have become slightly less ergonomic, 1 was revealed to be a `useless_conversion`. While this doesn't eliminate unchecked `as` conversions, it makes such conversions easier to scrutinize. It also has the slight benefit of removing a degree of freedom on which to bikeshed. Thus apply the changes and enable the lint -- no functional change intended. Link: https://rust-lang.github.io/rust-clippy/master/index.html#as_underscore [1] Reviewed-by: Benno Lossin Reviewed-by: Boqun Feng Signed-off-by: Tamir Duberstein Acked-by: Greg Kroah-Hartman Acked-by: Danilo Krummrich Link: https://lore.kernel.org/r/20250615-ptr-as-ptr-v12-4-f43b024581e8@gmail.com [ Changed `isize` to `c_long`. - Miguel ] Signed-off-by: Miguel Ojeda --- rust/kernel/devres.rs | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) (limited to 'rust/kernel/devres.rs') diff --git a/rust/kernel/devres.rs b/rust/kernel/devres.rs index b418cfc6f90d..8dfbc5b21dc1 100644 --- a/rust/kernel/devres.rs +++ b/rust/kernel/devres.rs @@ -61,19 +61,19 @@ struct DevresInner { /// unsafe fn new(paddr: usize) -> Result{ /// // SAFETY: By the safety requirements of this function [`paddr`, `paddr` + `SIZE`) is /// // valid for `ioremap`. -/// let addr = unsafe { bindings::ioremap(paddr as _, SIZE as _) }; +/// let addr = unsafe { bindings::ioremap(paddr as bindings::phys_addr_t, SIZE) }; /// if addr.is_null() { /// return Err(ENOMEM); /// } /// -/// Ok(IoMem(IoRaw::new(addr as _, SIZE)?)) +/// Ok(IoMem(IoRaw::new(addr as usize, SIZE)?)) /// } /// } /// /// impl Drop for IoMem { /// fn drop(&mut self) { /// // SAFETY: `self.0.addr()` is guaranteed to be properly mapped by `Self::new`. -/// unsafe { bindings::iounmap(self.0.addr() as _); }; +/// unsafe { bindings::iounmap(self.0.addr() as *mut c_void); }; /// } /// } /// @@ -115,8 +115,9 @@ impl DevresInner { // SAFETY: `devm_add_action` guarantees to call `Self::devres_callback` once `dev` is // detached. - let ret = - unsafe { bindings::devm_add_action(dev.as_raw(), Some(inner.callback), data as _) }; + let ret = unsafe { + bindings::devm_add_action(dev.as_raw(), Some(inner.callback), data.cast_mut().cast()) + }; if ret != 0 { // SAFETY: We just created another reference to `inner` in order to pass it to @@ -130,7 +131,7 @@ impl DevresInner { } fn as_ptr(&self) -> *const Self { - self as _ + self } fn remove_action(this: &Arc) -> bool { -- cgit From b6985083be1deb1f5fa14d160265f57d9ccb42a1 Mon Sep 17 00:00:00 2001 From: Viresh Kumar Date: Tue, 10 Jun 2025 14:33:00 +0530 Subject: rust: Use consistent "# Examples" heading style in rustdoc Use a consistent `# Examples` heading in rustdoc across the codebase. Some modules previously used `## Examples` (even when they should be available as top-level headers), while others used `# Example`, which deviates from the preferred `# Examples` style. Suggested-by: Miguel Ojeda Signed-off-by: Viresh Kumar Acked-by: Benno Lossin Link: https://lore.kernel.org/r/ddd5ce0ac20c99a72a4f1e4322d3de3911056922.1749545815.git.viresh.kumar@linaro.org Signed-off-by: Miguel Ojeda --- rust/kernel/devres.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) (limited to 'rust/kernel/devres.rs') diff --git a/rust/kernel/devres.rs b/rust/kernel/devres.rs index 8dfbc5b21dc1..d0e6c6e162c2 100644 --- a/rust/kernel/devres.rs +++ b/rust/kernel/devres.rs @@ -44,7 +44,7 @@ struct DevresInner { /// [`Devres`] users should make sure to simply free the corresponding backing resource in `T`'s /// [`Drop`] implementation. /// -/// # Example +/// # Examples /// /// ```no_run /// # use kernel::{bindings, c_str, device::{Bound, Device}, devres::Devres, io::{Io, IoRaw}}; @@ -203,7 +203,7 @@ impl Devres { /// An error is returned if `dev` does not match the same [`Device`] this [`Devres`] instance /// has been created with. /// - /// # Example + /// # Examples /// /// ```no_run /// # #![cfg(CONFIG_PCI)] -- cgit From 0dab138d0f4c0b3ce7f835d577e52a2b5ebdd536 Mon Sep 17 00:00:00 2001 From: Danilo Krummrich Date: Thu, 26 Jun 2025 15:24:46 +0200 Subject: rust: devres: require T: Send for Devres Due to calling Revocable::revoke() from Devres::devres_callback() T may be dropped from Devres::devres_callback() and hence must be Send. Fix this by adding the corresponding bound to Devres and DevresInner. Reported-by: Boqun Feng Closes: https://lore.kernel.org/lkml/aFzI5L__OcB9hqdG@Mac.home/ Fixes: 76c01ded724b ("rust: add devres abstraction") Reviewed-by: Boqun Feng Reviewed-by: Benno Lossin Link: https://lore.kernel.org/r/20250626132544.72866-1-dakr@kernel.org Signed-off-by: Danilo Krummrich --- rust/kernel/devres.rs | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) (limited to 'rust/kernel/devres.rs') diff --git a/rust/kernel/devres.rs b/rust/kernel/devres.rs index 57502534d985..8ede607414fd 100644 --- a/rust/kernel/devres.rs +++ b/rust/kernel/devres.rs @@ -18,7 +18,7 @@ use crate::{ }; #[pin_data] -struct DevresInner { +struct DevresInner { dev: ARef, callback: unsafe extern "C" fn(*mut c_void), #[pin] @@ -95,9 +95,9 @@ struct DevresInner { /// # Ok(()) /// # } /// ``` -pub struct Devres(Arc>); +pub struct Devres(Arc>); -impl DevresInner { +impl DevresInner { fn new(dev: &Device, data: T, flags: Flags) -> Result>> { let inner = Arc::pin_init( pin_init!( DevresInner { @@ -175,7 +175,7 @@ impl DevresInner { } } -impl Devres { +impl Devres { /// Creates a new [`Devres`] instance of the given `data`. The `data` encapsulated within the /// returned `Devres` instance' `data` will be revoked once the device is detached. pub fn new(dev: &Device, data: T, flags: Flags) -> Result { @@ -247,7 +247,7 @@ impl Devres { } } -impl Drop for Devres { +impl Drop for Devres { fn drop(&mut self) { // SAFETY: When `drop` runs, it is guaranteed that nobody is accessing the revocable data // anymore, hence it is safe not to wait for the grace period to finish. -- cgit From ce7c22b2e1fb1db467d33bd050546941ce82f21f Mon Sep 17 00:00:00 2001 From: Danilo Krummrich Date: Thu, 26 Jun 2025 22:00:39 +0200 Subject: rust: revocable: support fallible PinInit types Currently, Revocable::new() only supports infallible PinInit implementations, i.e. impl PinInit. This has been sufficient so far, since users such as Devres do not support fallibility. Since this is about to change, make Revocable::new() generic over the error type E. Reviewed-by: Benno Lossin Reviewed-by: Alice Ryhl Acked-by: Miguel Ojeda Link: https://lore.kernel.org/r/20250626200054.243480-2-dakr@kernel.org Signed-off-by: Danilo Krummrich --- 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 8ede607414fd..fd8b75aa03bc 100644 --- a/rust/kernel/devres.rs +++ b/rust/kernel/devres.rs @@ -100,7 +100,7 @@ pub struct Devres(Arc>); impl DevresInner { fn new(dev: &Device, data: T, flags: Flags) -> Result>> { let inner = Arc::pin_init( - pin_init!( DevresInner { + try_pin_init!( DevresInner { dev: dev.into(), callback: Self::devres_callback, data <- Revocable::new(data), -- cgit From 46ae8fd7386abf809355d1857abac5cf2d7c3f62 Mon Sep 17 00:00:00 2001 From: Danilo Krummrich Date: Thu, 26 Jun 2025 22:00:40 +0200 Subject: rust: devres: replace Devres::new_foreign_owned() Replace Devres::new_foreign_owned() with devres::register(). The current implementation of Devres::new_foreign_owned() creates a full Devres container instance, including the internal Revocable and completion. However, none of that is necessary for the intended use of giving full ownership of an object to devres and getting it dropped once the given device is unbound. Hence, implement devres::register(), which is limited to consume the given data, wrap it in a KBox and drop the KBox once the given device is unbound, without any other synchronization. Cc: Dave Airlie Cc: Simona Vetter Cc: Viresh Kumar Acked-by: Viresh Kumar Reviewed-by: Benno Lossin Reviewed-by: Alice Ryhl Link: https://lore.kernel.org/r/20250626200054.243480-3-dakr@kernel.org Signed-off-by: Danilo Krummrich --- rust/kernel/devres.rs | 73 ++++++++++++++++++++++++++++++++++++++++++++------- 1 file changed, 63 insertions(+), 10 deletions(-) (limited to 'rust/kernel/devres.rs') diff --git a/rust/kernel/devres.rs b/rust/kernel/devres.rs index fd8b75aa03bc..64458ca3d69f 100644 --- a/rust/kernel/devres.rs +++ b/rust/kernel/devres.rs @@ -9,12 +9,12 @@ use crate::{ alloc::Flags, bindings, device::{Bound, Device}, - error::{Error, Result}, + error::{to_result, Error, Result}, ffi::c_void, prelude::*, revocable::{Revocable, RevocableGuard}, sync::{rcu, Arc, Completion}, - types::ARef, + types::{ARef, ForeignOwnable}, }; #[pin_data] @@ -184,14 +184,6 @@ impl Devres { Ok(Devres(inner)) } - /// Same as [`Devres::new`], but does not return a `Devres` instance. Instead the given `data` - /// is owned by devres and will be revoked / dropped, once the device is detached. - pub fn new_foreign_owned(dev: &Device, data: T, flags: Flags) -> Result { - let _ = DevresInner::new(dev, data, flags)?; - - Ok(()) - } - /// Obtain `&'a T`, bypassing the [`Revocable`]. /// /// This method allows to directly obtain a `&'a T`, bypassing the [`Revocable`], by presenting @@ -261,3 +253,64 @@ impl Drop for Devres { } } } + +/// Consume `data` and [`Drop::drop`] `data` once `dev` is unbound. +fn register_foreign

(dev: &Device, data: P) -> Result +where + P: ForeignOwnable + Send + 'static, +{ + let ptr = data.into_foreign(); + + #[allow(clippy::missing_safety_doc)] + unsafe extern "C" fn callback(ptr: *mut kernel::ffi::c_void) { + // SAFETY: `ptr` is the pointer to the `ForeignOwnable` leaked above and hence valid. + drop(unsafe { P::from_foreign(ptr.cast()) }); + } + + // SAFETY: + // - `dev.as_raw()` is a pointer to a valid and bound device. + // - `ptr` is a valid pointer the `ForeignOwnable` devres takes ownership of. + to_result(unsafe { + // `devm_add_action_or_reset()` also calls `callback` on failure, such that the + // `ForeignOwnable` is released eventually. + bindings::devm_add_action_or_reset(dev.as_raw(), Some(callback::

), ptr.cast()) + }) +} + +/// Encapsulate `data` in a [`KBox`] and [`Drop::drop`] `data` once `dev` is unbound. +/// +/// # Examples +/// +/// ```no_run +/// use kernel::{device::{Bound, Device}, devres}; +/// +/// /// Registration of e.g. a class device, IRQ, etc. +/// struct Registration; +/// +/// impl Registration { +/// fn new() -> Self { +/// // register +/// +/// Self +/// } +/// } +/// +/// impl Drop for Registration { +/// fn drop(&mut self) { +/// // unregister +/// } +/// } +/// +/// fn from_bound_context(dev: &Device) -> Result { +/// devres::register(dev, Registration::new(), GFP_KERNEL) +/// } +/// ``` +pub fn register(dev: &Device, data: impl PinInit, flags: Flags) -> Result +where + T: Send + 'static, + Error: From, +{ + let data = KBox::pin_init(data, flags)?; + + register_foreign(dev, data) +} -- cgit From f5d3ef25d238901a76fe0277787afa44f7714739 Mon Sep 17 00:00:00 2001 From: Danilo Krummrich Date: Thu, 26 Jun 2025 22:00:41 +0200 Subject: rust: devres: get rid of Devres' inner Arc So far Devres uses an inner memory allocation and reference count, i.e. an inner Arc, in order to ensure that the devres callback can't run into a use-after-free in case where the Devres object is dropped while the devres callback runs concurrently. Instead, use a completion in order to avoid a potential UAF: In Devres::drop(), if we detect that we can't remove the devres action anymore, we wait for the completion that is completed from the devres callback. If, in turn, we were able to successfully remove the devres action, we can just go ahead. This, again, allows us to get rid of the internal Arc, and instead let Devres consume an `impl PinInit` in order to return an `impl PinInit, E>`, which enables us to get away with less memory allocations. Additionally, having the resulting explicit synchronization in Devres::drop() prevents potential subtle undesired side effects of the devres callback dropping the final Arc reference asynchronously within the devres callback. Reviewed-by: Benno Lossin Reviewed-by: Boqun Feng Link: https://lore.kernel.org/r/20250626200054.243480-4-dakr@kernel.org [ Move '# Invariants' below '# Examples'. - Danilo ] Signed-off-by: Danilo Krummrich --- rust/kernel/devres.rs | 213 +++++++++++++++++++++++++++++--------------------- 1 file changed, 126 insertions(+), 87 deletions(-) (limited to 'rust/kernel/devres.rs') diff --git a/rust/kernel/devres.rs b/rust/kernel/devres.rs index 64458ca3d69f..7c5c5de8bcb6 100644 --- a/rust/kernel/devres.rs +++ b/rust/kernel/devres.rs @@ -13,16 +13,21 @@ use crate::{ ffi::c_void, prelude::*, revocable::{Revocable, RevocableGuard}, - sync::{rcu, Arc, Completion}, - types::{ARef, ForeignOwnable}, + sync::{rcu, Completion}, + types::{ARef, ForeignOwnable, Opaque, ScopeGuard}, }; +use pin_init::Wrapper; + +/// [`Devres`] inner data accessed from [`Devres::callback`]. #[pin_data] -struct DevresInner { - dev: ARef, - callback: unsafe extern "C" fn(*mut c_void), +struct Inner { #[pin] data: Revocable, + /// Tracks whether [`Devres::callback`] has been completed. + #[pin] + devm: Completion, + /// Tracks whether revoking [`Self::data`] has been completed. #[pin] revoke: Completion, } @@ -88,100 +93,115 @@ struct DevresInner { /// # fn no_run(dev: &Device) -> Result<(), Error> { /// // SAFETY: Invalid usage for example purposes. /// let iomem = unsafe { IoMem::<{ core::mem::size_of::() }>::new(0xBAAAAAAD)? }; -/// let devres = Devres::new(dev, iomem, GFP_KERNEL)?; +/// let devres = KBox::pin_init(Devres::new(dev, iomem), GFP_KERNEL)?; /// /// let res = devres.try_access().ok_or(ENXIO)?; /// res.write8(0x42, 0x0); /// # Ok(()) /// # } /// ``` -pub struct Devres(Arc>); +/// +/// # Invariants +/// +/// [`Self::inner`] is guaranteed to be initialized and is always accessed read-only. +#[pin_data(PinnedDrop)] +pub struct Devres { + dev: ARef, + /// Pointer to [`Self::devres_callback`]. + /// + /// Has to be stored, since Rust does not guarantee to always return the same address for a + /// function. However, the C API uses the address as a key. + callback: unsafe extern "C" fn(*mut c_void), + /// 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. + #[pin] + inner: Opaque>, +} + +impl Devres { + /// Creates a new [`Devres`] instance of the given `data`. + /// + /// The `data` encapsulated within the returned `Devres` instance' `data` will be + /// (revoked)[`Revocable`] once the device is detached. + pub fn new<'a, E>( + dev: &'a Device, + data: impl PinInit + 'a, + ) -> impl PinInit + 'a + where + T: 'a, + Error: From, + { + let callback = Self::devres_callback; -impl DevresInner { - fn new(dev: &Device, data: T, flags: Flags) -> Result>> { - let inner = Arc::pin_init( - try_pin_init!( DevresInner { - dev: dev.into(), - callback: Self::devres_callback, + try_pin_init!(&this in Self { + // INVARIANT: `inner` is properly initialized. + inner <- Opaque::pin_init(try_pin_init!(Inner { data <- Revocable::new(data), + devm <- Completion::new(), revoke <- Completion::new(), - }), - flags, - )?; - - // Convert `Arc` into a raw pointer and make devres own this reference until - // `Self::devres_callback` is called. - let data = inner.clone().into_raw(); + })), + callback, + dev: { + // SAFETY: `this` is a valid pointer to uninitialized memory. + let inner = unsafe { &raw mut (*this.as_ptr()).inner }; - // SAFETY: `devm_add_action` guarantees to call `Self::devres_callback` once `dev` is - // detached. - let ret = - unsafe { bindings::devm_add_action(dev.as_raw(), Some(inner.callback), data as _) }; - - if ret != 0 { - // SAFETY: We just created another reference to `inner` in order to pass it to - // `bindings::devm_add_action`. If `bindings::devm_add_action` fails, we have to drop - // this reference accordingly. - let _ = unsafe { Arc::from_raw(data) }; - return Err(Error::from_errno(ret)); - } + // SAFETY: + // - `dev.as_raw()` is a pointer to a valid bound device. + // - `inner` is guaranteed to be a valid for the duration of the lifetime of `Self`. + // - `devm_add_action()` is guaranteed not to call `callback` until `this` has been + // 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()) + })?; - Ok(inner) + dev.into() + }, + }) } - fn as_ptr(&self) -> *const Self { - self as _ + fn inner(&self) -> &Inner { + // SAFETY: By the type invairants of `Self`, `inner` is properly initialized and always + // accessed read-only. + unsafe { &*self.inner.get() } } - fn remove_action(this: &Arc) -> bool { - // SAFETY: - // - `self.inner.dev` is a valid `Device`, - // - the `action` and `data` pointers are the exact same ones as given to devm_add_action() - // previously, - // - `self` is always valid, even if the action has been released already. - let success = unsafe { - bindings::devm_remove_action_nowarn( - this.dev.as_raw(), - Some(this.callback), - this.as_ptr() as _, - ) - } == 0; - - if success { - // SAFETY: We leaked an `Arc` reference to devm_add_action() in `DevresInner::new`; if - // devm_remove_action_nowarn() was successful we can (and have to) claim back ownership - // of this reference. - let _ = unsafe { Arc::from_raw(this.as_ptr()) }; - } - - success + fn data(&self) -> &Revocable { + &self.inner().data } #[allow(clippy::missing_safety_doc)] unsafe extern "C" fn devres_callback(ptr: *mut kernel::ffi::c_void) { - let ptr = ptr as *mut DevresInner; - // Devres owned this memory; now that we received the callback, drop the `Arc` and hence the - // reference. - // SAFETY: Safe, since we leaked an `Arc` reference to devm_add_action() in - // `DevresInner::new`. - let inner = unsafe { Arc::from_raw(ptr) }; + // SAFETY: In `Self::new` we've passed a valid pointer to `Inner` to `devm_add_action()`, + // hence `ptr` must be a valid pointer to `Inner`. + let inner = unsafe { &*ptr.cast::>() }; + + // Ensure that `inner` can't be used anymore after we signal completion of this callback. + let inner = ScopeGuard::new_with_data(inner, |inner| inner.devm.complete_all()); if !inner.data.revoke() { // If `revoke()` returns false, it means that `Devres::drop` already started revoking - // `inner.data` for us. Hence we have to wait until `Devres::drop()` signals that it - // completed revoking `inner.data`. + // `data` for us. Hence we have to wait until `Devres::drop` signals that it + // completed revoking `data`. inner.revoke.wait_for_completion(); } } -} -impl Devres { - /// Creates a new [`Devres`] instance of the given `data`. The `data` encapsulated within the - /// returned `Devres` instance' `data` will be revoked once the device is detached. - pub fn new(dev: &Device, data: T, flags: Flags) -> Result { - let inner = DevresInner::new(dev, data, flags)?; - - Ok(Devres(inner)) + fn remove_action(&self) -> bool { + // SAFETY: + // - `self.dev` is a valid `Device`, + // - the `action` and `data` pointers are the exact same ones as given to + // `devm_add_action()` previously, + (unsafe { + bindings::devm_remove_action_nowarn( + self.dev.as_raw(), + Some(self.callback), + core::ptr::from_ref(self.inner()).cast_mut().cast(), + ) + } == 0) } /// Obtain `&'a T`, bypassing the [`Revocable`]. @@ -213,44 +233,63 @@ impl Devres { /// } /// ``` pub fn access<'a>(&'a self, dev: &'a Device) -> Result<&'a T> { - if self.0.dev.as_raw() != dev.as_raw() { + if self.dev.as_raw() != dev.as_raw() { return Err(EINVAL); } // SAFETY: `dev` being the same device as the device this `Devres` has been created for - // proves that `self.0.data` hasn't been revoked and is guaranteed to not be revoked as - // long as `dev` lives; `dev` lives at least as long as `self`. - Ok(unsafe { self.0.data.access() }) + // proves that `self.data` hasn't been revoked and is guaranteed to not be revoked as long + // as `dev` lives; `dev` lives at least as long as `self`. + Ok(unsafe { self.data().access() }) } /// [`Devres`] accessor for [`Revocable::try_access`]. pub fn try_access(&self) -> Option> { - self.0.data.try_access() + self.data().try_access() } /// [`Devres`] accessor for [`Revocable::try_access_with`]. pub fn try_access_with R>(&self, f: F) -> Option { - self.0.data.try_access_with(f) + self.data().try_access_with(f) } /// [`Devres`] accessor for [`Revocable::try_access_with_guard`]. pub fn try_access_with_guard<'a>(&'a self, guard: &'a rcu::Guard) -> Option<&'a T> { - self.0.data.try_access_with_guard(guard) + self.data().try_access_with_guard(guard) } } -impl Drop for Devres { - fn drop(&mut self) { +// SAFETY: `Devres` can be send to any task, if `T: Send`. +unsafe impl Send for Devres {} + +// SAFETY: `Devres` can be shared with any task, if `T: Sync`. +unsafe impl Sync for Devres {} + +#[pinned_drop] +impl PinnedDrop for Devres { + fn drop(self: Pin<&mut Self>) { // SAFETY: When `drop` runs, it is guaranteed that nobody is accessing the revocable data // anymore, hence it is safe not to wait for the grace period to finish. - if unsafe { self.0.data.revoke_nosync() } { - // We revoked `self.0.data` before the devres action did, hence try to remove it. - if !DevresInner::remove_action(&self.0) { + if unsafe { self.data().revoke_nosync() } { + // We revoked `self.data` before the devres action did, hence try to remove it. + if !self.remove_action() { // We could not remove the devres action, which means that it now runs concurrently, - // hence signal that `self.0.data` has been revoked successfully. - self.0.revoke.complete_all(); + // hence signal that `self.data` has been revoked by us successfully. + self.inner().revoke.complete_all(); + + // Wait for `Self::devres_callback` to be done using this object. + self.inner().devm.wait_for_completion(); } + } else { + // `Self::devres_callback` revokes `self.data` for us, hence wait for it to be done + // using this object. + self.inner().devm.wait_for_completion(); } + + // INVARIANT: At this point it is guaranteed that `inner` can't be accessed any more. + // + // SAFETY: `inner` is valid for dropping. + unsafe { core::ptr::drop_in_place(self.inner.get()) }; } } -- cgit From 6d16cd5769bbb5eb62974e8eddb97fca830b49fd Mon Sep 17 00:00:00 2001 From: Tamir Duberstein Date: Fri, 4 Jul 2025 16:11:02 -0400 Subject: rust: devres: remove unused import As far as I can tell, `c_str` was never used, hence remove it. Signed-off-by: Tamir Duberstein Link: https://lore.kernel.org/r/20250704-cstr-include-devres-v1-1-4ee9e56fca09@gmail.com Signed-off-by: Danilo Krummrich --- 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 7c5c5de8bcb6..f43de3d77d61 100644 --- a/rust/kernel/devres.rs +++ b/rust/kernel/devres.rs @@ -52,7 +52,7 @@ struct Inner { /// # Example /// /// ```no_run -/// # use kernel::{bindings, c_str, device::{Bound, Device}, devres::Devres, io::{Io, IoRaw}}; +/// # use kernel::{bindings, device::{Bound, Device}, devres::Devres, io::{Io, IoRaw}}; /// # use core::ops::Deref; /// /// // See also [`pci::Bar`] for a real example. -- cgit From 91ae26b06aab476bdd8b56cd99e127f029490330 Mon Sep 17 00:00:00 2001 From: Danilo Krummrich Date: Mon, 14 Jul 2025 13:32:35 +0200 Subject: rust: devres: initialize Devres::inner::data last Users may want to access the Devres object from callbacks registered through the initialization of Devres::inner::data. For those accesses to be valid, Devres::inner::data must be initialized last [1]. Credit to Boqun for spotting this [2]. Link: https://lore.kernel.org/lkml/DBBPHO26CPBS.2OVI1OERCB2J5@kernel.org/ [1] Link: https://lore.kernel.org/lkml/aHSmxWeIy3L-AKIV@Mac.home/ [2] Reviewed-by: Alice Ryhl Link: https://lore.kernel.org/r/20250714113712.22158-1-dakr@kernel.org Signed-off-by: Danilo Krummrich --- rust/kernel/devres.rs | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) (limited to 'rust/kernel/devres.rs') diff --git a/rust/kernel/devres.rs b/rust/kernel/devres.rs index f43de3d77d61..ac5e5d94ee96 100644 --- a/rust/kernel/devres.rs +++ b/rust/kernel/devres.rs @@ -137,14 +137,10 @@ impl Devres { let callback = Self::devres_callback; try_pin_init!(&this in Self { - // INVARIANT: `inner` is properly initialized. - inner <- Opaque::pin_init(try_pin_init!(Inner { - data <- Revocable::new(data), - devm <- Completion::new(), - revoke <- Completion::new(), - })), + dev: dev.into(), callback, - dev: { + // INVARIANT: `inner` is properly initialized. + inner <- { // SAFETY: `this` is a valid pointer to uninitialized memory. let inner = unsafe { &raw mut (*this.as_ptr()).inner }; @@ -158,7 +154,11 @@ impl Devres { bindings::devm_add_action(dev.as_raw(), Some(callback), inner.cast()) })?; - dev.into() + Opaque::pin_init(try_pin_init!(Inner { + devm <- Completion::new(), + revoke <- Completion::new(), + data <- Revocable::new(data), + })) }, }) } -- cgit From 85aa5b16fef7040213581df9ff093dae27bf8675 Mon Sep 17 00:00:00 2001 From: Danilo Krummrich Date: Sun, 13 Jul 2025 20:26:53 +0200 Subject: rust: devres: provide an accessor for the device Provide an accessor for the Device a Devres instance has been created with. For instance, this is useful when registrations want to provide a &Device for a scope that is protected by Devres. Suggested-by: Benno Lossin Acked-by: Greg Kroah-Hartman Reviewed-by: Benno Lossin Link: https://lore.kernel.org/r/20250713182737.64448-1-dakr@kernel.org Signed-off-by: Danilo Krummrich --- rust/kernel/devres.rs | 5 +++++ 1 file changed, 5 insertions(+) (limited to 'rust/kernel/devres.rs') diff --git a/rust/kernel/devres.rs b/rust/kernel/devres.rs index ac5e5d94ee96..152a89b78943 100644 --- a/rust/kernel/devres.rs +++ b/rust/kernel/devres.rs @@ -204,6 +204,11 @@ impl Devres { } == 0) } + /// Return a reference of the [`Device`] this [`Devres`] instance has been created with. + pub fn device(&self) -> &Device { + &self.dev + } + /// Obtain `&'a T`, bypassing the [`Revocable`]. /// /// This method allows to directly obtain a `&'a T`, bypassing the [`Revocable`], by presenting -- cgit