* [PATCH 6.18.y 1/5] rust: devres: fix race condition due to nesting
2026-10-05 9:47 [PATCH 6.18.y 0/5] Four Rust Devres fixes Alice Ryhl
@ 2026-10-05 9:47 ` Alice Ryhl
2026-10-05 9:47 ` [PATCH 6.18.y 2/5] rust: devres: add 'static bound to Devres<T> Alice Ryhl
` (3 subsequent siblings)
4 siblings, 0 replies; 6+ messages in thread
From: Alice Ryhl @ 2026-10-05 9:47 UTC (permalink / raw)
To: stable, Greg Kroah-Hartman, Sasha Levin, Danilo Krummrich
Cc: Alexandre Courbot, Andreas Hindborg, Benno Lossin,
Björn Roy Baron, Boqun Feng, Boris Brezillon,
Daniel Almeida, Eliot Courtney, Gary Guo, Markus Probst,
Miguel Ojeda, Rafael J. Wysocki, Trevor Gross, rust-for-linux,
linux-kernel, Alice Ryhl
From: Danilo Krummrich <dakr@kernel.org>
commit ba268514ea14b44570030e8ed2aef92a38679e85 upstream.
Commit f5d3ef25d238 ("rust: devres: get rid of Devres' inner Arc") did
attempt to optimize away the internal reference count of Devres.
However, without an internal reference count, we can't support cases
where Devres is indirectly nested, resulting into a deadlock.
Such indirect nesting easily happens in the following way:
A registration object (which is guarded by devres) hold a reference
count of an object that holds a device resource guarded by devres
itself.
For instance a drm::Registration holds a reference of a drm::Device. The
drm::Device itself holds a device resource in its private data.
When the drm::Registration is dropped by devres, and it happens that it
did hold the last reference count of the drm::Device, it also drops the
device resource, which is guarded by devres itself.
Thus, resulting into a deadlock in the Devres destructor of the device
resource, as in the following backtrace.
sysrq: Show Blocked State
task:rmmod state:D stack:0 pid:1331 tgid:1331 ppid:1330 task_flags:0x400100 flags:0x00000010
Call trace:
__switch_to+0x190/0x294 (T)
__schedule+0x878/0xf10
schedule+0x4c/0xcc
schedule_timeout+0x44/0x118
wait_for_common+0xc0/0x18c
wait_for_completion+0x18/0x24
_RINvNtCs4gKlGRWyJ5S_4core3ptr13drop_in_placeINtNtNtCsgzhNYVB7wSz_6kernel4sync3arc3ArcINtNtBN_6devres6DevresmEEECsRdyc7Hyps3_15rust_driver_pci+0x68/0xe8 [rust_driver_pci]
_RINvNvNtCsgzhNYVB7wSz_6kernel6devres16register_foreign8callbackINtNtCs4gKlGRWyJ5S_4core3pin3PinINtNtNtB6_5alloc4kbox3BoxINtNtNtB6_4sync3arc3ArcINtB4_6DevresmEENtNtB1A_9allocator7KmallocEEECsRdyc7Hyps3_15rust_driver_pci+0x34/0xc8 [rust_driver_pci]
devm_action_release+0x14/0x20
devres_release_all+0xb8/0x118
device_release_driver_internal+0x1c4/0x28c
driver_detach+0x94/0xd4
bus_remove_driver+0xdc/0x11c
driver_unregister+0x34/0x58
pci_unregister_driver+0x20/0x80
__arm64_sys_delete_module+0x1d8/0x254
invoke_syscall+0x40/0xcc
el0_svc_common+0x8c/0xd8
do_el0_svc+0x1c/0x28
el0_svc+0x54/0x1d4
el0t_64_sync_handler+0x84/0x12c
el0t_64_sync+0x198/0x19c
In order to fix this, re-introduce the internal reference count.
Reported-by: Boris Brezillon <boris.brezillon@collabora.com>
Closes: https://rust-for-linux.zulipchat.com/#narrow/channel/288089-General/topic/.E2.9C.94.20Deadlock.20caused.20by.20nested.20Devres/with/571242651
Reported-by: Markus Probst <markus.probst@posteo.de>
Closes: https://rust-for-linux.zulipchat.com/#narrow/channel/288089-General/topic/.E2.9C.94.20Devres.20inside.20Devres.20stuck.20on.20cleanup/with/571239721
Reported-by: Alice Ryhl <aliceryhl@google.com>
Closes: https://gitlab.freedesktop.org/panfrost/linux/-/merge_requests/56#note_3282757
Fixes: f5d3ef25d238 ("rust: devres: get rid of Devres' inner Arc")
Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Reviewed-by: Alice Ryhl <aliceryhl@google.com>
Tested-by: Boris Brezillon <boris.brezillon@collabora.com>
Link: https://patch.msgid.link/20260205222529.91465-1-dakr@kernel.org
[ Call clone() prior to devm_add_action(). - Danilo ]
Signed-off-by: Danilo Krummrich <dakr@kernel.org>
Signed-off-by: Alice Ryhl <aliceryhl@google.com>
---
rust/kernel/devres.rs | 145 ++++++++++++++------------------------------------
1 file changed, 40 insertions(+), 105 deletions(-)
diff --git a/rust/kernel/devres.rs b/rust/kernel/devres.rs
index 835d9c11948e..562dd54fbe43 100644
--- a/rust/kernel/devres.rs
+++ b/rust/kernel/devres.rs
@@ -13,25 +13,10 @@
ffi::c_void,
prelude::*,
revocable::{Revocable, RevocableGuard},
- sync::{aref::ARef, rcu, Completion},
- types::{ForeignOwnable, Opaque, ScopeGuard},
+ sync::{aref::ARef, rcu, Arc},
+ types::ForeignOwnable,
};
-use pin_init::Wrapper;
-
-/// [`Devres`] inner data accessed from [`Devres::callback`].
-#[pin_data]
-struct Inner<T: Send> {
- #[pin]
- data: Revocable<T>,
- /// Tracks whether [`Devres::callback`] has been completed.
- #[pin]
- devm: Completion,
- /// Tracks whether revoking [`Self::data`] has been completed.
- #[pin]
- revoke: Completion,
-}
-
/// This abstraction is meant to be used by subsystems to containerize [`Device`] bound resources to
/// manage their lifetime.
///
@@ -105,18 +90,13 @@ struct Inner<T: Send> {
/// # fn no_run(dev: &Device<Bound>) -> Result<(), Error> {
/// // SAFETY: Invalid usage for example purposes.
/// let iomem = unsafe { IoMem::<{ core::mem::size_of::<u32>() }>::new(0xBAAAAAAD)? };
-/// let devres = KBox::pin_init(Devres::new(dev, iomem), GFP_KERNEL)?;
+/// let devres = Devres::new(dev, iomem)?;
///
/// let res = devres.try_access().ok_or(ENXIO)?;
/// res.write8(0x42, 0x0);
/// # Ok(())
/// # }
/// ```
-///
-/// # Invariants
-///
-/// `Self::inner` is guaranteed to be initialized and is always accessed read-only.
-#[pin_data(PinnedDrop)]
pub struct Devres<T: Send> {
dev: ARef<Device>,
/// Pointer to [`Self::devres_callback`].
@@ -124,14 +104,7 @@ pub struct Devres<T: Send> {
/// 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 `Devres::new` as well as the
- // explicit `Send` and `Sync' impls can be removed.
- #[pin]
- inner: Opaque<Inner<T>>,
- _add_action: (),
+ data: Arc<Revocable<T>>,
}
impl<T: Send> Devres<T> {
@@ -139,74 +112,48 @@ impl<T: Send> Devres<T> {
///
/// 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<Bound>,
- data: impl PinInit<T, E> + 'a,
- ) -> impl PinInit<Self, Error> + 'a
+ pub fn new<E>(dev: &Device<Bound>, data: impl PinInit<T, E>) -> Result<Self>
where
- T: 'a,
Error: From<E>,
{
- try_pin_init!(&this in Self {
- dev: dev.into(),
- callback: Self::devres_callback,
- // INVARIANT: `inner` is properly initialized.
- 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 };
+ let callback = Self::devres_callback;
+ let data = Arc::pin_init(Revocable::new(data), GFP_KERNEL)?;
+ let devres_data = data.clone();
+
+ // SAFETY:
+ // - `dev.as_raw()` is a pointer to a valid bound device.
+ // - `data` is guaranteed to be a valid for the duration of the lifetime of `Self`.
+ // - `devm_add_action()` is guaranteed not to call `callback` for the entire lifetime of
+ // `dev`.
+ to_result(unsafe {
+ bindings::devm_add_action(
+ dev.as_raw(),
+ Some(callback),
+ Arc::as_ptr(&data).cast_mut().cast(),
+ )
+ })?;
- // 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<Self, Error>`.
- to_result(unsafe {
- bindings::devm_add_action(dev.as_raw(), Some(*callback), inner.cast())
- }).inspect_err(|_| {
- let inner = Opaque::cast_into(inner);
+ // `devm_add_action()` was successful and has consumed the reference count.
+ core::mem::forget(devres_data);
- // SAFETY: `inner` is a valid pointer to an `Inner<T>` and valid for both reads
- // and writes.
- unsafe { core::ptr::drop_in_place(inner) };
- })?;
- },
+ Ok(Self {
+ dev: dev.into(),
+ callback,
+ data,
})
}
- fn inner(&self) -> &Inner<T> {
- // SAFETY: By the type invairants of `Self`, `inner` is properly initialized and always
- // accessed read-only.
- unsafe { &*self.inner.get() }
- }
-
fn data(&self) -> &Revocable<T> {
- &self.inner().data
+ &self.data
}
#[allow(clippy::missing_safety_doc)]
unsafe extern "C" fn devres_callback(ptr: *mut kernel::ffi::c_void) {
- // 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::<Inner<T>>() };
+ // SAFETY: In `Self::new` we've passed a valid pointer of `Revocable<T>` to
+ // `devm_add_action()`, hence `ptr` must be a valid pointer to `Revocable<T>`.
+ let data = unsafe { Arc::from_raw(ptr.cast::<Revocable<T>>()) };
- // 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
- // `data` for us. Hence we have to wait until `Devres::drop` signals that it
- // completed revoking `data`.
- inner.revoke.wait_for_completion();
- }
+ data.revoke();
}
fn remove_action(&self) -> bool {
@@ -218,7 +165,7 @@ fn remove_action(&self) -> bool {
bindings::devm_remove_action_nowarn(
self.dev.as_raw(),
Some(self.callback),
- core::ptr::from_ref(self.inner()).cast_mut().cast(),
+ core::ptr::from_ref(self.data()).cast_mut().cast(),
)
} == 0)
}
@@ -289,31 +236,19 @@ unsafe impl<T: Send> Send for Devres<T> {}
// SAFETY: `Devres` can be shared with any task, if `T: Sync`.
unsafe impl<T: Send + Sync> Sync for Devres<T> {}
-#[pinned_drop]
-impl<T: Send> PinnedDrop for Devres<T> {
- fn drop(self: Pin<&mut Self>) {
+impl<T: Send> Drop for Devres<T> {
+ 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.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.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();
+ if self.remove_action() {
+ // SAFETY: In `Self::new` we have taken an additional reference count of `self.data`
+ // for `devm_add_action()`. Since `remove_action()` was successful, we have to drop
+ // this additional reference count.
+ drop(unsafe { Arc::from_raw(Arc::as_ptr(&self.data)) });
}
- } 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()) };
}
}
--
2.56.0.360.g66cac248cb-goog
^ permalink raw reply [flat|nested] 6+ messages in thread* [PATCH 6.18.y 2/5] rust: devres: add 'static bound to Devres<T>
2026-10-05 9:47 [PATCH 6.18.y 0/5] Four Rust Devres fixes Alice Ryhl
2026-10-05 9:47 ` [PATCH 6.18.y 1/5] rust: devres: fix race condition due to nesting Alice Ryhl
@ 2026-10-05 9:47 ` Alice Ryhl
2026-10-05 9:47 ` [PATCH 6.18.y 3/5] rust: devres: fix race between concurrent revokers Alice Ryhl
` (2 subsequent siblings)
4 siblings, 0 replies; 6+ messages in thread
From: Alice Ryhl @ 2026-10-05 9:47 UTC (permalink / raw)
To: stable, Greg Kroah-Hartman, Sasha Levin, Danilo Krummrich
Cc: Alexandre Courbot, Andreas Hindborg, Benno Lossin,
Björn Roy Baron, Boqun Feng, Boris Brezillon,
Daniel Almeida, Eliot Courtney, Gary Guo, Markus Probst,
Miguel Ojeda, Rafael J. Wysocki, Trevor Gross, rust-for-linux,
linux-kernel, Alice Ryhl
From: Danilo Krummrich <dakr@kernel.org>
commit 016267b521b18529c977c9eca9597a1669c3d73c upstream.
Devres::new() registers a callback with the C devres subsystem via
devres_node_add(). If the Devres is leaked (e.g. via
core::mem::forget(), which is safe), its Drop impl never runs, and the
devres release callback will revoke the inner Revocable on device
unbind, which drops T in place. If T contains non-'static references,
those may be dangling by that point.
Add a 'static bound to prevent storing types with borrowed data in
Devres.
Fixes: 76c01ded724b ("rust: add devres abstraction")
Reviewed-by: Alexandre Courbot <acourbot@nvidia.com>
Reviewed-by: Eliot Courtney <ecourtney@nvidia.com>
Link: https://patch.msgid.link/20260526000447.350558-1-dakr@kernel.org
Signed-off-by: Danilo Krummrich <dakr@kernel.org>
Signed-off-by: Alice Ryhl <aliceryhl@google.com>
---
rust/kernel/devres.rs | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/rust/kernel/devres.rs b/rust/kernel/devres.rs
index 562dd54fbe43..0fea4f2844a5 100644
--- a/rust/kernel/devres.rs
+++ b/rust/kernel/devres.rs
@@ -97,7 +97,7 @@
/// # Ok(())
/// # }
/// ```
-pub struct Devres<T: Send> {
+pub struct Devres<T: Send + 'static> {
dev: ARef<Device>,
/// Pointer to [`Self::devres_callback`].
///
@@ -107,7 +107,7 @@ pub struct Devres<T: Send> {
data: Arc<Revocable<T>>,
}
-impl<T: Send> Devres<T> {
+impl<T: Send + 'static> Devres<T> {
/// Creates a new [`Devres`] instance of the given `data`.
///
/// The `data` encapsulated within the returned `Devres` instance' `data` will be
@@ -236,7 +236,7 @@ unsafe impl<T: Send> Send for Devres<T> {}
// SAFETY: `Devres` can be shared with any task, if `T: Sync`.
unsafe impl<T: Send + Sync> Sync for Devres<T> {}
-impl<T: Send> Drop for Devres<T> {
+impl<T: Send + 'static> Drop for Devres<T> {
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.
--
2.56.0.360.g66cac248cb-goog
^ permalink raw reply [flat|nested] 6+ messages in thread* [PATCH 6.18.y 3/5] rust: devres: fix race between concurrent revokers
2026-10-05 9:47 [PATCH 6.18.y 0/5] Four Rust Devres fixes Alice Ryhl
2026-10-05 9:47 ` [PATCH 6.18.y 1/5] rust: devres: fix race condition due to nesting Alice Ryhl
2026-10-05 9:47 ` [PATCH 6.18.y 2/5] rust: devres: add 'static bound to Devres<T> Alice Ryhl
@ 2026-10-05 9:47 ` Alice Ryhl
2026-10-05 9:47 ` [PATCH 6.18.y 4/5] rust: devres: ensure revocation is complete before device finishes unbinding Alice Ryhl
2026-10-05 9:47 ` [PATCH 6.18.y 5/5] rust: irq: pass RegistrationInner as cookie to request_irq Alice Ryhl
4 siblings, 0 replies; 6+ messages in thread
From: Alice Ryhl @ 2026-10-05 9:47 UTC (permalink / raw)
To: stable, Greg Kroah-Hartman, Sasha Levin, Danilo Krummrich
Cc: Alexandre Courbot, Andreas Hindborg, Benno Lossin,
Björn Roy Baron, Boqun Feng, Boris Brezillon,
Daniel Almeida, Eliot Courtney, Gary Guo, Markus Probst,
Miguel Ojeda, Rafael J. Wysocki, Trevor Gross, rust-for-linux,
linux-kernel, Alice Ryhl, Sashiko
From: Danilo Krummrich <dakr@kernel.org>
commit acc516dfa1972d31836b50abc0115216cd0fccc5 upstream.
There is a potential race condition when two paths try to revoke a
Devres concurrently.
The driver core's devres_release_all() calls Revocable::revoke() via the
release callback, while Devres::drop() calls revoke_nosync() on another
CPU.
The revoker that does not claim the is_available swap returns
immediately, but the revoker that did may still be executing
drop_in_place() on the inner data. This can cause a use-after-free when
the other revoker's caller proceeds to drop adjacent resources that
drop_in_place() still references (e.g., Devres<DmaMappedSgt> racing with
SGTable freeing the backing sg_table and pages).
Fix this by adding a Completion. The release callback signals the
Completion after revoke() finishes, and Devres::drop() waits for it when
it loses the is_available swap. This ensures the wrapped object is fully
torn down before Devres::drop() returns.
Cc: stable@vger.kernel.org
Reported-by: Sashiko <sashiko-bot@kernel.org>
Closes: https://lore.kernel.org/dri-devel/20260612202841.2577C1F000E9@smtp.kernel.org/
Fixes: 05aa6fb1c21d ("rust: scatterlist: Add abstraction for sg_table")
Reviewed-by: Gary Guo <gary@garyguo.net>
Reviewed-by: Alice Ryhl <aliceryhl@google.com>
Link: https://patch.msgid.link/20260628174451.2275679-1-dakr@kernel.org
Signed-off-by: Danilo Krummrich <dakr@kernel.org>
[ Re-introduce Inner<T> for Arc<Inner<T>> as commit 9aa64d2503c6 ("rust:
devres: embed struct devres_node directly") is not in 6.18. ]
Signed-off-by: Alice Ryhl <aliceryhl@google.com>
---
rust/kernel/devres.rs | 56 ++++++++++++++++++++++++++++++++++++---------------
1 file changed, 40 insertions(+), 16 deletions(-)
diff --git a/rust/kernel/devres.rs b/rust/kernel/devres.rs
index 0fea4f2844a5..02916f80db5a 100644
--- a/rust/kernel/devres.rs
+++ b/rust/kernel/devres.rs
@@ -13,10 +13,18 @@
ffi::c_void,
prelude::*,
revocable::{Revocable, RevocableGuard},
- sync::{aref::ARef, rcu, Arc},
+ sync::{aref::ARef, rcu, Arc, Completion},
types::ForeignOwnable,
};
+#[pin_data]
+struct Inner<T> {
+ #[pin]
+ data: Revocable<T>,
+ #[pin]
+ revocation: Completion,
+}
+
/// This abstraction is meant to be used by subsystems to containerize [`Device`] bound resources to
/// manage their lifetime.
///
@@ -31,6 +39,10 @@
/// After the [`Devres`] has been unbound it is not possible to access the encapsulated resource
/// anymore.
///
+/// When a [`Devres`] is dropped, it is guaranteed that `T` has been fully dropped by the time
+/// [`Devres::drop`] returns, even if a concurrent revocation through the release callback is in
+/// progress.
+///
/// [`Devres`] users should make sure to simply free the corresponding backing resource in `T`'s
/// [`Drop`] implementation.
///
@@ -104,7 +116,7 @@ pub struct Devres<T: Send + 'static> {
/// 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),
- data: Arc<Revocable<T>>,
+ inner: Arc<Inner<T>>,
}
impl<T: Send + 'static> Devres<T> {
@@ -117,43 +129,51 @@ pub fn new<E>(dev: &Device<Bound>, data: impl PinInit<T, E>) -> Result<Self>
Error: From<E>,
{
let callback = Self::devres_callback;
- let data = Arc::pin_init(Revocable::new(data), GFP_KERNEL)?;
- let devres_data = data.clone();
+ let inner = Arc::pin_init::<Error>(
+ try_pin_init!(Inner {
+ data <- Revocable::new(data),
+ revocation <- Completion::new(),
+ }),
+ GFP_KERNEL,
+ )?;
+ let devres_inner = inner.clone();
// SAFETY:
// - `dev.as_raw()` is a pointer to a valid bound device.
- // - `data` is guaranteed to be a valid for the duration of the lifetime of `Self`.
+ // - `inner` is guaranteed to be a valid for the duration of the lifetime of `Self`.
// - `devm_add_action()` is guaranteed not to call `callback` for the entire lifetime of
// `dev`.
to_result(unsafe {
bindings::devm_add_action(
dev.as_raw(),
Some(callback),
- Arc::as_ptr(&data).cast_mut().cast(),
+ Arc::as_ptr(&inner).cast_mut().cast(),
)
})?;
// `devm_add_action()` was successful and has consumed the reference count.
- core::mem::forget(devres_data);
+ core::mem::forget(devres_inner);
Ok(Self {
dev: dev.into(),
callback,
- data,
+ inner,
})
}
fn data(&self) -> &Revocable<T> {
- &self.data
+ &self.inner.data
}
#[allow(clippy::missing_safety_doc)]
unsafe extern "C" fn devres_callback(ptr: *mut kernel::ffi::c_void) {
- // SAFETY: In `Self::new` we've passed a valid pointer of `Revocable<T>` to
- // `devm_add_action()`, hence `ptr` must be a valid pointer to `Revocable<T>`.
- let data = unsafe { Arc::from_raw(ptr.cast::<Revocable<T>>()) };
+ // SAFETY: In `Self::new` we've passed a valid pointer of `Inner<T>` to
+ // `devm_add_action()`, hence `ptr` must be a valid pointer to `Inner<T>`.
+ let inner = unsafe { Arc::from_raw(ptr.cast::<Inner<T>>()) };
- data.revoke();
+ if inner.data.revoke() {
+ inner.revocation.complete_all();
+ }
}
fn remove_action(&self) -> bool {
@@ -165,7 +185,7 @@ fn remove_action(&self) -> bool {
bindings::devm_remove_action_nowarn(
self.dev.as_raw(),
Some(self.callback),
- core::ptr::from_ref(self.data()).cast_mut().cast(),
+ Arc::as_ptr(&self.inner).cast_mut().cast(),
)
} == 0)
}
@@ -243,11 +263,15 @@ fn drop(&mut self) {
if unsafe { self.data().revoke_nosync() } {
// We revoked `self.data` before the devres action did, hence try to remove it.
if self.remove_action() {
- // SAFETY: In `Self::new` we have taken an additional reference count of `self.data`
+ // SAFETY: In `Self::new` we have taken an additional reference count of `self.inner`
// for `devm_add_action()`. Since `remove_action()` was successful, we have to drop
// this additional reference count.
- drop(unsafe { Arc::from_raw(Arc::as_ptr(&self.data)) });
+ drop(unsafe { Arc::from_raw(Arc::as_ptr(&self.inner)) });
}
+ } else {
+ // The release callback is concurrently revoking; wait for it to finish
+ // `drop_in_place()` of the wrapped object before returning.
+ self.inner.revocation.wait_for_completion();
}
}
}
--
2.56.0.360.g66cac248cb-goog
^ permalink raw reply [flat|nested] 6+ messages in thread* [PATCH 6.18.y 4/5] rust: devres: ensure revocation is complete before device finishes unbinding
2026-10-05 9:47 [PATCH 6.18.y 0/5] Four Rust Devres fixes Alice Ryhl
` (2 preceding siblings ...)
2026-10-05 9:47 ` [PATCH 6.18.y 3/5] rust: devres: fix race between concurrent revokers Alice Ryhl
@ 2026-10-05 9:47 ` Alice Ryhl
2026-10-05 9:47 ` [PATCH 6.18.y 5/5] rust: irq: pass RegistrationInner as cookie to request_irq Alice Ryhl
4 siblings, 0 replies; 6+ messages in thread
From: Alice Ryhl @ 2026-10-05 9:47 UTC (permalink / raw)
To: stable, Greg Kroah-Hartman, Sasha Levin, Danilo Krummrich
Cc: Alexandre Courbot, Andreas Hindborg, Benno Lossin,
Björn Roy Baron, Boqun Feng, Boris Brezillon,
Daniel Almeida, Eliot Courtney, Gary Guo, Markus Probst,
Miguel Ojeda, Rafael J. Wysocki, Trevor Gross, rust-for-linux,
linux-kernel, Alice Ryhl
From: Danilo Krummrich <dakr@kernel.org>
commit a10639966fd72fff8f7fbf3c8e733307daabd38f upstream.
Now that the revocation Completion is in place, also address the
symmetric case. When Devres::drop() wins the is_available swap and the
devres callback loses, the callback returns to devres_release_all()
without waiting. This means device unbinding can complete while
Devres::drop() is still executing drop_in_place() on another CPU, which
is a problem if T's destructor accesses device state.
Make the synchronization bidirectional. Whichever side performs
drop_in_place() signals the Completion, and the other side waits.
This does not reintroduce the nested Devres deadlock fixed by commit
ba268514ea14 ("rust: devres: fix race condition due to nesting"),
because that deadlock was caused by drop waiting for the release
callback to return (the old 'devm' Completion). Here, both sides only
wait for drop_in_place() to finish, which completes within the current
call chain. The Arc<Inner<T>> keeps the Inner allocation alive
independently.
Cc: stable@vger.kernel.org
Fixes: ba268514ea14 ("rust: devres: fix race condition due to nesting")
Reviewed-by: Gary Guo <gary@garyguo.net>
Reviewed-by: Alice Ryhl <aliceryhl@google.com>
Link: https://patch.msgid.link/20260628200304.2365598-1-dakr@kernel.org
Signed-off-by: Danilo Krummrich <dakr@kernel.org>
Signed-off-by: Alice Ryhl <aliceryhl@google.com>
---
rust/kernel/devres.rs | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/rust/kernel/devres.rs b/rust/kernel/devres.rs
index 02916f80db5a..fc0d8b2cb7b2 100644
--- a/rust/kernel/devres.rs
+++ b/rust/kernel/devres.rs
@@ -173,6 +173,11 @@ fn data(&self) -> &Revocable<T> {
if inner.data.revoke() {
inner.revocation.complete_all();
+ } else {
+ // Devres::drop() is concurrently revoking; wait for it to finish `drop_in_place()`
+ // before returning to `devres_release_all()`, ensuring `T` is fully torn down before
+ // the device finishes unbinding.
+ inner.revocation.wait_for_completion();
}
}
@@ -261,6 +266,8 @@ 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.data().revoke_nosync() } {
+ self.inner.revocation.complete_all();
+
// We revoked `self.data` before the devres action did, hence try to remove it.
if self.remove_action() {
// SAFETY: In `Self::new` we have taken an additional reference count of `self.inner`
--
2.56.0.360.g66cac248cb-goog
^ permalink raw reply [flat|nested] 6+ messages in thread* [PATCH 6.18.y 5/5] rust: irq: pass RegistrationInner as cookie to request_irq
2026-10-05 9:47 [PATCH 6.18.y 0/5] Four Rust Devres fixes Alice Ryhl
` (3 preceding siblings ...)
2026-10-05 9:47 ` [PATCH 6.18.y 4/5] rust: devres: ensure revocation is complete before device finishes unbinding Alice Ryhl
@ 2026-10-05 9:47 ` Alice Ryhl
4 siblings, 0 replies; 6+ messages in thread
From: Alice Ryhl @ 2026-10-05 9:47 UTC (permalink / raw)
To: stable, Greg Kroah-Hartman, Sasha Levin, Danilo Krummrich
Cc: Alexandre Courbot, Andreas Hindborg, Benno Lossin,
Björn Roy Baron, Boqun Feng, Boris Brezillon,
Daniel Almeida, Eliot Courtney, Gary Guo, Markus Probst,
Miguel Ojeda, Rafael J. Wysocki, Trevor Gross, rust-for-linux,
linux-kernel, Alice Ryhl
With commit ba268514ea14 ("rust: devres: fix race condition due to
nesting"), Devres::new() returns Result<Self> by value instead of
initializing in-place via PinInit. Because request_irq() is called
inside Devres::new(), registration.inner is still uninitialized when the
IRQ is enabled, so accessing registration.inner.device() in the IRQ
callback can read uninitialized memory.
Fix this by storing the Device and handler pointer in RegistrationInner
and passing RegistrationInner as the IRQ cookie after its fields are
initialized.
This is not needed in mainline because commit 98c63ce4d760 ("rust: irq:
make Registration compatible with lifetime-bound drivers") removed
Devres from irq::Registration.
Fixes: 29e16fcd67ee ("rust: irq: add &Device<Bound> argument to irq callbacks")
Fixes: ba268514ea14 ("rust: devres: fix race condition due to nesting")
Signed-off-by: Alice Ryhl <aliceryhl@google.com>
---
rust/kernel/irq/request.rs | 89 ++++++++++++++++++++++++++++------------------
1 file changed, 54 insertions(+), 35 deletions(-)
diff --git a/rust/kernel/irq/request.rs b/rust/kernel/irq/request.rs
index 2ceeaeb0543a..70700f43ddd3 100644
--- a/rust/kernel/irq/request.rs
+++ b/rust/kernel/irq/request.rs
@@ -14,7 +14,7 @@
use crate::irq::flags::Flags;
use crate::prelude::*;
use crate::str::CStr;
-use crate::sync::Arc;
+use crate::sync::{aref::ARef, Arc};
/// The value that can be returned from a [`Handler`] or a [`ThreadedHandler`].
#[repr(u32)]
@@ -54,13 +54,18 @@ fn handle(&self, device: &Device<Bound>) -> IrqReturn {
/// # Invariants
///
/// - `self.irq` is the same as the one passed to `request_{threaded}_irq`.
-/// - `cookie` was passed to `request_{threaded}_irq` as the cookie. It is guaranteed to be unique
+/// - `&self` was passed to `request_{threaded}_irq` as the cookie. It is guaranteed to be unique
/// by the type system, since each call to `new` will return a different instance of
/// `Registration`.
+/// - `self.handler` points to a valid instance of the handler `T` that lives at least until
+/// `Self::drop` completes.
#[pin_data(PinnedDrop)]
struct RegistrationInner {
irq: u32,
- cookie: *mut c_void,
+ dev: ARef<Device>,
+ handler: *const c_void,
+ #[pin]
+ _pin: PhantomPinned,
}
impl RegistrationInner {
@@ -77,18 +82,22 @@ fn drop(self: Pin<&mut Self>) {
//
// Safe as per the invariants of `RegistrationInner` and:
//
- // - The containing struct is `!Unpin` and was initialized using
+ // - `RegistrationInner` is `!Unpin` and was initialized using
// pin-init, so it occupied the same memory location for the entirety of
// its lifetime.
//
// Notice that this will block until all handlers finish executing,
// i.e.: at no point will &self be invalid while the handler is running.
- unsafe { bindings::free_irq(self.irq, self.cookie) };
+ unsafe {
+ bindings::free_irq(
+ self.irq,
+ core::ptr::from_mut::<Self>(self.get_unchecked_mut()).cast::<c_void>(),
+ )
+ };
}
}
-// SAFETY: We only use `inner` on drop, which called at most once with no
-// concurrent access.
+// SAFETY: `RegistrationInner` has no interior mutability and `handler` points to a `Sync` handler.
unsafe impl Sync for RegistrationInner {}
// SAFETY: It is safe to send `RegistrationInner` across threads.
@@ -180,7 +189,7 @@ pub fn irq(&self) -> u32 {
///
/// # Invariants
///
-/// * We own an irq handler whose cookie is a pointer to `Self`.
+/// * We own an irq handler whose cookie is a pointer to `Self::inner`.
#[pin_data]
pub struct Registration<T: Handler + 'static> {
#[pin]
@@ -207,10 +216,13 @@ pub fn new<'a>(
handler <- handler,
inner <- Devres::new(
request.dev,
- try_pin_init!(RegistrationInner {
- // INVARIANT: `this` is a valid pointer to the `Registration` instance
- cookie: this.as_ptr().cast::<c_void>(),
- irq: {
+ try_pin_init!(&inner_this in RegistrationInner {
+ irq: request.irq,
+ dev: request.dev.into(),
+ // SAFETY: `this` is a valid pointer to the `Registration` instance.
+ handler: unsafe { &raw const (*this.as_ptr()).handler }.cast(),
+ _pin: PhantomPinned,
+ _: {
// SAFETY:
// - The callbacks are valid for use with request_irq.
// - If this succeeds, the slot is guaranteed to be valid until the
@@ -225,11 +237,10 @@ pub fn new<'a>(
Some(handle_irq_callback::<T>),
flags.into_inner(),
name.as_char_ptr(),
- this.as_ptr().cast::<c_void>(),
+ inner_this.as_ptr().cast::<c_void>(),
)
})?;
- request.irq
- }
+ },
})
),
_pin: PhantomPinned,
@@ -265,13 +276,15 @@ pub fn synchronize(&self, dev: &Device<Bound>) -> Result {
_irq: i32,
ptr: *mut c_void,
) -> c_uint {
- // SAFETY: `ptr` is a pointer to `Registration<T>` set in `Registration::new`
- let registration = unsafe { &*(ptr as *const Registration<T>) };
+ // SAFETY: `ptr` is a pointer to `RegistrationInner` set in `Registration::new`
+ let inner = unsafe { &*(ptr as *const RegistrationInner) };
+ // SAFETY: `inner.handler` is a pointer to `T` set in `Registration::new`
+ let handler = unsafe { &*inner.handler.cast::<T>() };
// SAFETY: The irq callback is removed before the device is unbound, so the fact that the irq
// callback is running implies that the device has not yet been unbound.
- let device = unsafe { registration.inner.device().as_bound() };
+ let device = unsafe { inner.dev.as_bound() };
- T::handle(®istration.handler, device) as c_uint
+ T::handle(handler, device) as c_uint
}
/// The value that can be returned from [`ThreadedHandler::handle`].
@@ -401,7 +414,7 @@ fn handle_threaded(&self, device: &Device<Bound>) -> IrqReturn {
///
/// # Invariants
///
-/// * We own an irq handler whose cookie is a pointer to `Self`.
+/// * We own an irq handler whose cookie is a pointer to `Self::inner`.
#[pin_data]
pub struct ThreadedRegistration<T: ThreadedHandler + 'static> {
#[pin]
@@ -428,10 +441,13 @@ pub fn new<'a>(
handler <- handler,
inner <- Devres::new(
request.dev,
- try_pin_init!(RegistrationInner {
- // INVARIANT: `this` is a valid pointer to the `ThreadedRegistration` instance.
- cookie: this.as_ptr().cast::<c_void>(),
- irq: {
+ try_pin_init!(&inner_this in RegistrationInner {
+ irq: request.irq,
+ dev: request.dev.into(),
+ // SAFETY: `this` is a valid pointer to the `ThreadedRegistration` instance.
+ handler: unsafe { &raw const (*this.as_ptr()).handler }.cast(),
+ _pin: PhantomPinned,
+ _: {
// SAFETY:
// - The callbacks are valid for use with request_threaded_irq.
// - If this succeeds, the slot is guaranteed to be valid until the
@@ -447,11 +463,10 @@ pub fn new<'a>(
Some(thread_fn_callback::<T>),
flags.into_inner(),
name.as_char_ptr(),
- this.as_ptr().cast::<c_void>(),
+ inner_this.as_ptr().cast::<c_void>(),
)
})?;
- request.irq
- }
+ },
})
),
_pin: PhantomPinned,
@@ -487,13 +502,15 @@ pub fn synchronize(&self, dev: &Device<Bound>) -> Result {
_irq: i32,
ptr: *mut c_void,
) -> c_uint {
- // SAFETY: `ptr` is a pointer to `ThreadedRegistration<T>` set in `ThreadedRegistration::new`
- let registration = unsafe { &*(ptr as *const ThreadedRegistration<T>) };
+ // SAFETY: `ptr` is a pointer to `RegistrationInner` set in `ThreadedRegistration::new`
+ let inner = unsafe { &*(ptr as *const RegistrationInner) };
+ // SAFETY: `inner.handler` is a pointer to `T` set in `ThreadedRegistration::new`
+ let handler = unsafe { &*inner.handler.cast::<T>() };
// SAFETY: The irq callback is removed before the device is unbound, so the fact that the irq
// callback is running implies that the device has not yet been unbound.
- let device = unsafe { registration.inner.device().as_bound() };
+ let device = unsafe { inner.dev.as_bound() };
- T::handle(®istration.handler, device) as c_uint
+ T::handle(handler, device) as c_uint
}
/// # Safety
@@ -503,11 +520,13 @@ pub fn synchronize(&self, dev: &Device<Bound>) -> Result {
_irq: i32,
ptr: *mut c_void,
) -> c_uint {
- // SAFETY: `ptr` is a pointer to `ThreadedRegistration<T>` set in `ThreadedRegistration::new`
- let registration = unsafe { &*(ptr as *const ThreadedRegistration<T>) };
+ // SAFETY: `ptr` is a pointer to `RegistrationInner` set in `ThreadedRegistration::new`
+ let inner = unsafe { &*(ptr as *const RegistrationInner) };
+ // SAFETY: `inner.handler` is a pointer to `T` set in `ThreadedRegistration::new`
+ let handler = unsafe { &*inner.handler.cast::<T>() };
// SAFETY: The irq callback is removed before the device is unbound, so the fact that the irq
// callback is running implies that the device has not yet been unbound.
- let device = unsafe { registration.inner.device().as_bound() };
+ let device = unsafe { inner.dev.as_bound() };
- T::handle_threaded(®istration.handler, device) as c_uint
+ T::handle_threaded(handler, device) as c_uint
}
--
2.56.0.360.g66cac248cb-goog
^ permalink raw reply [flat|nested] 6+ messages in thread