[PATCH v2 0/4] Improvements for Devres

[PATCH v2 0/4] Improvements for Devres

[Danilo Krummrich]

This patch series provides some optimizations for Devres:

  1) Provide a more lightweight replacement for Devres::new_foreign_owned().

  2) Get rid of Devres' inner Arc and instead consume and provide an
     impl PinInit instead.

     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.

  3) An optimization for when we never need to access the resource or release
     it manually.

Thanks to Alice and Benno for some great offline discussions on this topic.

This patch series depends on the Opaque patch in [1] and the pin-init patch in
[2], which Benno will provide a signed tag for. A branch containing the patches
can be found in [3].

[1] https://lore.kernel.org/lkml/20250610-b4-rust_miscdevice_registrationdata-v6-1-b03f5dfce998@gmail.com/
[2] https://lore.kernel.org/rust-for-linux/20250529081027.297648-2-lossin@kernel.org/
[3] https://git.kernel.org/pub/scm/linux/kernel/git/dakr/linux.git/log/?h=rust/devres

Changes in v2:
  - Revocable:
    - remove Error: From<E> bound
  - devres::register:
    - rename devres::register_foreign_boxed() to just devres::register()
    - move T: 'static bound to the function rather than the impl block
  - Devres:
    - Fix aliasing issue by using an Opaque<Inner>; should be
      UnsafePinned<Inner> once available.
    - Add doc-comments for a couple of private fields.
    - Link Revocable on 'revoke' in Devres::new().
  - devres::register_release():
    - expand documentation of Release
    - rename devres::register_foreign_release() for devres::register_release()

Danilo Krummrich (4):
  rust: revocable: support fallible PinInit types
  rust: devres: replace Devres::new_foreign_owned()
  rust: devres: get rid of Devres' inner Arc
  rust: devres: implement register_release()

 drivers/gpu/nova-core/driver.rs |   7 +-
 drivers/gpu/nova-core/gpu.rs    |   6 +-
 rust/helpers/device.c           |   7 +
 rust/kernel/cpufreq.rs          |  11 +-
 rust/kernel/devres.rs           | 362 +++++++++++++++++++++++---------
 rust/kernel/drm/driver.rs       |  14 +-
 rust/kernel/pci.rs              |  20 +-
 rust/kernel/revocable.rs        |   6 +-
 samples/rust/rust_driver_pci.rs |  19 +-
 9 files changed, 318 insertions(+), 134 deletions(-)


base-commit: 946d082536c43ed7d394aaba927f3d85eccfc03a
-- 
2.49.0

[PATCH v2 1/4] rust: revocable: support fallible PinInit types

[Danilo Krummrich]

Currently, Revocable::new() only supports infallible PinInit
implementations, i.e. impl PinInit<T, Infallible>.

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.

Signed-off-by: Danilo Krummrich <dakr@kernel.org>
---
 rust/kernel/devres.rs    | 2 +-
 rust/kernel/revocable.rs | 6 +++---
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/rust/kernel/devres.rs b/rust/kernel/devres.rs
index 57502534d985..544e50efab43 100644
--- a/rust/kernel/devres.rs
+++ b/rust/kernel/devres.rs
@@ -100,7 +100,7 @@ struct DevresInner<T> {
 impl<T> DevresInner<T> {
     fn new(dev: &Device<Bound>, data: T, flags: Flags) -> Result<Arc<DevresInner<T>>> {
         let inner = Arc::pin_init(
-            pin_init!( DevresInner {
+            try_pin_init!( DevresInner {
                 dev: dev.into(),
                 callback: Self::devres_callback,
                 data <- Revocable::new(data),
diff --git a/rust/kernel/revocable.rs b/rust/kernel/revocable.rs
index fa1fd70efa27..46768b374656 100644
--- a/rust/kernel/revocable.rs
+++ b/rust/kernel/revocable.rs
@@ -82,11 +82,11 @@ unsafe impl<T: Sync + Send> Sync for Revocable<T> {}
 
 impl<T> Revocable<T> {
     /// Creates a new revocable instance of the given data.
-    pub fn new(data: impl PinInit<T>) -> impl PinInit<Self> {
-        pin_init!(Self {
+    pub fn new<E>(data: impl PinInit<T, E>) -> impl PinInit<Self, E> {
+        try_pin_init!(Self {
             is_available: AtomicBool::new(true),
             data <- Opaque::pin_init(data),
-        })
+        }? E)
     }
 
     /// Tries to access the revocable wrapped object.
-- 
2.49.0

[PATCH v2 2/4] rust: devres: replace Devres::new_foreign_owned()

[Danilo Krummrich]

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 <airlied@redhat.com>
Cc: Simona Vetter <simona.vetter@ffwll.ch>
Cc: Viresh Kumar <viresh.kumar@linaro.org>
Acked-by: Viresh Kumar <viresh.kumar@linaro.org>
Signed-off-by: Danilo Krummrich <dakr@kernel.org>
---
 rust/helpers/device.c     |  7 ++++
 rust/kernel/cpufreq.rs    | 11 +++---
 rust/kernel/devres.rs     | 70 +++++++++++++++++++++++++++++++++------
 rust/kernel/drm/driver.rs | 14 ++++----
 4 files changed, 82 insertions(+), 20 deletions(-)

diff --git a/rust/helpers/device.c b/rust/helpers/device.c
index b2135c6686b0..502fef7e9ae8 100644
--- a/rust/helpers/device.c
+++ b/rust/helpers/device.c
@@ -8,3 +8,10 @@ int rust_helper_devm_add_action(struct device *dev,
 {
 	return devm_add_action(dev, action, data);
 }
+
+int rust_helper_devm_add_action_or_reset(struct device *dev,
+					 void (*action)(void *),
+					 void *data)
+{
+	return devm_add_action_or_reset(dev, action, data);
+}
diff --git a/rust/kernel/cpufreq.rs b/rust/kernel/cpufreq.rs
index 11b03e9d7e89..dd84e2b4d7ae 100644
--- a/rust/kernel/cpufreq.rs
+++ b/rust/kernel/cpufreq.rs
@@ -13,7 +13,7 @@
     cpu::CpuId,
     cpumask,
     device::{Bound, Device},
-    devres::Devres,
+    devres,
     error::{code::*, from_err_ptr, from_result, to_result, Result, VTABLE_DEFAULT_ERROR},
     ffi::{c_char, c_ulong},
     prelude::*,
@@ -1046,10 +1046,13 @@ pub fn new() -> Result<Self> {
 
     /// Same as [`Registration::new`], but does not return a [`Registration`] instance.
     ///
-    /// Instead the [`Registration`] is owned by [`Devres`] and will be revoked / dropped, once the
+    /// Instead the [`Registration`] is owned by [`devres::register`] and will be dropped, once the
     /// device is detached.
-    pub fn new_foreign_owned(dev: &Device<Bound>) -> Result {
-        Devres::new_foreign_owned(dev, Self::new()?, GFP_KERNEL)
+    pub fn new_foreign_owned(dev: &Device<Bound>) -> Result
+    where
+        T: 'static,
+    {
+        devres::register(dev, Self::new()?, GFP_KERNEL)
     }
 }
 
diff --git a/rust/kernel/devres.rs b/rust/kernel/devres.rs
index 544e50efab43..250073749279 100644
--- a/rust/kernel/devres.rs
+++ b/rust/kernel/devres.rs
@@ -9,12 +9,12 @@
     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 @@ pub fn new(dev: &Device<Bound>, data: T, flags: Flags) -> Result<Self> {
         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<Bound>, 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,61 @@ fn drop(&mut self) {
         }
     }
 }
+
+/// Consume `data` and [`Drop::drop`] `data` once `dev` is unbound.
+fn register_foreign<P: ForeignOwnable>(dev: &Device<Bound>, data: P) -> Result {
+    let ptr = data.into_foreign();
+
+    #[allow(clippy::missing_safety_doc)]
+    unsafe extern "C" fn callback<P: ForeignOwnable>(ptr: *mut kernel::ffi::c_void) {
+        // SAFETY: `ptr` is the pointer to the `ForeignOwnable` leaked above and hence valid.
+        let _ = 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::<P>), 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<Bound>) -> Result {
+///     devres::register(dev, Registration::new(), GFP_KERNEL)
+/// }
+/// ```
+pub fn register<T, E>(dev: &Device<Bound>, data: impl PinInit<T, E>, flags: Flags) -> Result
+where
+    T: 'static,
+    Error: From<E>,
+{
+    let data = KBox::pin_init(data, flags)?;
+
+    register_foreign(dev, data)
+}
diff --git a/rust/kernel/drm/driver.rs b/rust/kernel/drm/driver.rs
index acb638086131..f63addaf7235 100644
--- a/rust/kernel/drm/driver.rs
+++ b/rust/kernel/drm/driver.rs
@@ -5,9 +5,7 @@
 //! C header: [`include/linux/drm/drm_drv.h`](srctree/include/linux/drm/drm_drv.h)
 
 use crate::{
-    bindings, device,
-    devres::Devres,
-    drm,
+    bindings, device, devres, drm,
     error::{to_result, Result},
     prelude::*,
     str::CStr,
@@ -130,18 +128,22 @@ fn new(drm: &drm::Device<T>, flags: usize) -> Result<Self> {
     }
 
     /// Same as [`Registration::new`}, but transfers ownership of the [`Registration`] to
-    /// [`Devres`].
+    /// [`devres::register`].
     pub fn new_foreign_owned(
         drm: &drm::Device<T>,
         dev: &device::Device<device::Bound>,
         flags: usize,
-    ) -> Result {
+    ) -> Result
+    where
+        T: 'static,
+    {
         if drm.as_ref().as_raw() != dev.as_raw() {
             return Err(EINVAL);
         }
 
         let reg = Registration::<T>::new(drm, flags)?;
-        Devres::new_foreign_owned(dev, reg, GFP_KERNEL)
+
+        devres::register(dev, reg, GFP_KERNEL)
     }
 
     /// Returns a reference to the `Device` instance for this registration.
-- 
2.49.0

[PATCH v2 3/4] rust: devres: get rid of Devres' inner Arc

[Danilo Krummrich]

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<T, E>` in order to return an
`impl PinInit<Devres<T>, 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.

Signed-off-by: Danilo Krummrich <dakr@kernel.org>
---
 drivers/gpu/nova-core/driver.rs |   7 +-
 drivers/gpu/nova-core/gpu.rs    |   6 +-
 rust/kernel/devres.rs           | 208 +++++++++++++++++++-------------
 rust/kernel/pci.rs              |  20 +--
 samples/rust/rust_driver_pci.rs |  19 +--
 5 files changed, 149 insertions(+), 111 deletions(-)

diff --git a/drivers/gpu/nova-core/driver.rs b/drivers/gpu/nova-core/driver.rs
index 8c86101c26cb..110f2b355db4 100644
--- a/drivers/gpu/nova-core/driver.rs
+++ b/drivers/gpu/nova-core/driver.rs
@@ -1,6 +1,6 @@
 // SPDX-License-Identifier: GPL-2.0
 
-use kernel::{auxiliary, bindings, c_str, device::Core, pci, prelude::*};
+use kernel::{auxiliary, bindings, c_str, device::Core, pci, prelude::*, sync::Arc};
 
 use crate::gpu::Gpu;
 
@@ -34,7 +34,10 @@ fn probe(pdev: &pci::Device<Core>, _info: &Self::IdInfo) -> Result<Pin<KBox<Self
         pdev.enable_device_mem()?;
         pdev.set_master();
 
-        let bar = pdev.iomap_region_sized::<BAR0_SIZE>(0, c_str!("nova-core/bar0"))?;
+        let bar = Arc::pin_init(
+            pdev.iomap_region_sized::<BAR0_SIZE>(0, c_str!("nova-core/bar0")),
+            GFP_KERNEL,
+        )?;
 
         let this = KBox::pin_init(
             try_pin_init!(Self {
diff --git a/drivers/gpu/nova-core/gpu.rs b/drivers/gpu/nova-core/gpu.rs
index 60b86f370284..47653c14838b 100644
--- a/drivers/gpu/nova-core/gpu.rs
+++ b/drivers/gpu/nova-core/gpu.rs
@@ -1,6 +1,6 @@
 // SPDX-License-Identifier: GPL-2.0
 
-use kernel::{device, devres::Devres, error::code::*, pci, prelude::*};
+use kernel::{device, devres::Devres, error::code::*, pci, prelude::*, sync::Arc};
 
 use crate::driver::Bar0;
 use crate::firmware::{Firmware, FIRMWARE_VERSION};
@@ -161,14 +161,14 @@ fn new(bar: &Bar0) -> Result<Spec> {
 pub(crate) struct Gpu {
     spec: Spec,
     /// MMIO mapping of PCI BAR 0
-    bar: Devres<Bar0>,
+    bar: Arc<Devres<Bar0>>,
     fw: Firmware,
 }
 
 impl Gpu {
     pub(crate) fn new(
         pdev: &pci::Device<device::Bound>,
-        devres_bar: Devres<Bar0>,
+        devres_bar: Arc<Devres<Bar0>>,
     ) -> Result<impl PinInit<Self>> {
         let bar = devres_bar.access(pdev.as_ref())?;
         let spec = Spec::new(bar)?;
diff --git a/rust/kernel/devres.rs b/rust/kernel/devres.rs
index 250073749279..15a0a94e992b 100644
--- a/rust/kernel/devres.rs
+++ b/rust/kernel/devres.rs
@@ -13,20 +13,31 @@
     ffi::c_void,
     prelude::*,
     revocable::{Revocable, RevocableGuard},
-    sync::{rcu, Arc, Completion},
-    types::{ARef, ForeignOwnable},
+    sync::{rcu, Completion},
+    types::{ARef, ForeignOwnable, Opaque},
 };
 
+use pin_init::Wrapper;
+
+/// [`Devres`] inner data accessed from [`Devres::callback`].
 #[pin_data]
-struct DevresInner<T> {
-    dev: ARef<Device>,
-    callback: unsafe extern "C" fn(*mut c_void),
+struct Inner<T> {
     #[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,
 }
 
+impl<T> Inner<T> {
+    fn as_ptr(&self) -> *const Self {
+        self
+    }
+}
+
 /// This abstraction is meant to be used by subsystems to containerize [`Device`] bound resources to
 /// manage their lifetime.
 ///
@@ -88,100 +99,106 @@ struct DevresInner<T> {
 /// # 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 = 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<T>(Arc<DevresInner<T>>);
+#[pin_data(PinnedDrop)]
+pub struct Devres<T> {
+    dev: ARef<Device>,
+    /// 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.
+    #[pin]
+    inner: Opaque<Inner<T>>,
+}
 
-impl<T> DevresInner<T> {
-    fn new(dev: &Device<Bound>, data: T, flags: Flags) -> Result<Arc<DevresInner<T>>> {
-        let inner = Arc::pin_init(
-            try_pin_init!( DevresInner {
-                dev: dev.into(),
-                callback: Self::devres_callback,
+impl<T> Devres<T> {
+    /// 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<Bound>,
+        data: impl PinInit<T, E> + 'a,
+    ) -> impl PinInit<Self, Error> + 'a
+    where
+        T: 'a,
+        Error: From<E>,
+    {
+        let callback = Self::devres_callback;
+
+        try_pin_init!(&this in Self {
+            inner <- Opaque::pin_init(try_pin_init!(Inner {
                 data <- Revocable::new(data),
+                devm <- Completion::new(),
                 revoke <- Completion::new(),
-            }),
-            flags,
-        )?;
-
-        // Convert `Arc<DevresInner>` 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: It is valid to dereference `this` to find the address of `inner`.
+                let inner = unsafe { core::ptr::addr_of_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<Self, Error>`.
+                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<T> {
+        // SAFETY: `inner` is valid and properly initialized.
+        unsafe { &*self.inner.get() }
     }
 
-    fn remove_action(this: &Arc<Self>) -> 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<T> {
+        &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<T>;
-        // 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::<Inner<T>>() };
 
         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<T> Devres<T> {
-    /// 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<Bound>, data: T, flags: Flags) -> Result<Self> {
-        let inner = DevresInner::new(dev, data, flags)?;
+        // Signal that we're done using `inner`.
+        inner.devm.complete_all();
+    }
 
-        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),
+                self.inner().as_ptr().cast_mut().cast(),
+            )
+        } == 0)
     }
 
     /// Obtain `&'a T`, bypassing the [`Revocable`].
@@ -213,44 +230,61 @@ pub fn new(dev: &Device<Bound>, data: T, flags: Flags) -> Result<Self> {
     /// }
     /// ```
     pub fn access<'a>(&'a self, dev: &'a Device<Bound>) -> 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<RevocableGuard<'_, T>> {
-        self.0.data.try_access()
+        self.data().try_access()
     }
 
     /// [`Devres`] accessor for [`Revocable::try_access_with`].
     pub fn try_access_with<R, F: FnOnce(&T) -> R>(&self, f: F) -> Option<R> {
-        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<T> Drop for Devres<T> {
-    fn drop(&mut self) {
+// SAFETY: `Devres` can be send to any task, if `T: Send`.
+unsafe impl<T: Send> Send for Devres<T> {}
+
+// SAFETY: `Devres` can be shared with any task, if `T: Sync`.
+unsafe impl<T: Sync> Sync for Devres<T> {}
+
+#[pinned_drop]
+impl<T> PinnedDrop for Devres<T> {
+    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();
         }
+
+        // SAFETY: `inner` is valid for dropping.
+        unsafe { core::ptr::drop_in_place(self.inner.get()) };
     }
 }
 
diff --git a/rust/kernel/pci.rs b/rust/kernel/pci.rs
index 8435f8132e38..db0eb7eaf9b1 100644
--- a/rust/kernel/pci.rs
+++ b/rust/kernel/pci.rs
@@ -5,7 +5,6 @@
 //! C header: [`include/linux/pci.h`](srctree/include/linux/pci.h)
 
 use crate::{
-    alloc::flags::*,
     bindings, container_of, device,
     device_id::RawDeviceId,
     devres::Devres,
@@ -398,19 +397,20 @@ pub fn resource_len(&self, bar: u32) -> Result<bindings::resource_size_t> {
 impl Device<device::Bound> {
     /// Mapps an entire PCI-BAR after performing a region-request on it. I/O operation bound checks
     /// can be performed on compile time for offsets (plus the requested type size) < SIZE.
-    pub fn iomap_region_sized<const SIZE: usize>(
-        &self,
+    pub fn iomap_region_sized<'a, const SIZE: usize>(
+        &'a self,
         bar: u32,
-        name: &CStr,
-    ) -> Result<Devres<Bar<SIZE>>> {
-        let bar = Bar::<SIZE>::new(self, bar, name)?;
-        let devres = Devres::new(self.as_ref(), bar, GFP_KERNEL)?;
-
-        Ok(devres)
+        name: &'a CStr,
+    ) -> impl PinInit<Devres<Bar<SIZE>>, Error> + 'a {
+        Devres::new(self.as_ref(), Bar::<SIZE>::new(self, bar, name))
     }
 
     /// Mapps an entire PCI-BAR after performing a region-request on it.
-    pub fn iomap_region(&self, bar: u32, name: &CStr) -> Result<Devres<Bar>> {
+    pub fn iomap_region<'a>(
+        &'a self,
+        bar: u32,
+        name: &'a CStr,
+    ) -> impl PinInit<Devres<Bar>, Error> + 'a {
         self.iomap_region_sized::<0>(bar, name)
     }
 }
diff --git a/samples/rust/rust_driver_pci.rs b/samples/rust/rust_driver_pci.rs
index 15147e4401b2..5c35f1414172 100644
--- a/samples/rust/rust_driver_pci.rs
+++ b/samples/rust/rust_driver_pci.rs
@@ -25,8 +25,10 @@ impl TestIndex {
     const NO_EVENTFD: Self = Self(0);
 }
 
+#[pin_data(PinnedDrop)]
 struct SampleDriver {
     pdev: ARef<pci::Device>,
+    #[pin]
     bar: Devres<Bar0>,
 }
 
@@ -73,13 +75,11 @@ fn probe(pdev: &pci::Device<Core>, info: &Self::IdInfo) -> Result<Pin<KBox<Self>
         pdev.enable_device_mem()?;
         pdev.set_master();
 
-        let bar = pdev.iomap_region_sized::<{ Regs::END }>(0, c_str!("rust_driver_pci"))?;
-
-        let drvdata = KBox::new(
-            Self {
+        let drvdata = KBox::pin_init(
+            try_pin_init!(Self {
                 pdev: pdev.into(),
-                bar,
-            },
+                bar <- pdev.iomap_region_sized::<{ Regs::END }>(0, c_str!("rust_driver_pci")),
+            }),
             GFP_KERNEL,
         )?;
 
@@ -90,12 +90,13 @@ fn probe(pdev: &pci::Device<Core>, info: &Self::IdInfo) -> Result<Pin<KBox<Self>
             Self::testdev(info, bar)?
         );
 
-        Ok(drvdata.into())
+        Ok(drvdata)
     }
 }
 
-impl Drop for SampleDriver {
-    fn drop(&mut self) {
+#[pinned_drop]
+impl PinnedDrop for SampleDriver {
+    fn drop(self: Pin<&mut Self>) {
         dev_dbg!(self.pdev.as_ref(), "Remove Rust PCI driver sample.\n");
     }
 }
-- 
2.49.0

[PATCH v2 4/4] rust: devres: implement register_release()

[Danilo Krummrich]

register_release() is useful when a device resource has associated data,
but does not require the capability of accessing it or manually releasing
it.

If we would want to be able to access the device resource and release the
device resource manually before the device is unbound, but still keep
access to the associated data, we could implement it as follows.

	struct Registration<T> {
	   inner: Devres<RegistrationInner>,
	   data: T,
	}

However, if we never need to access the resource or release it manually,
register_release() is great optimization for the above, since it does not
require the synchronization of the Devres type.

Suggested-by: Alice Ryhl <aliceryhl@google.com>
Signed-off-by: Danilo Krummrich <dakr@kernel.org>
---
 rust/kernel/devres.rs | 84 +++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 84 insertions(+)

diff --git a/rust/kernel/devres.rs b/rust/kernel/devres.rs
index 15a0a94e992b..4b61e94d34a0 100644
--- a/rust/kernel/devres.rs
+++ b/rust/kernel/devres.rs
@@ -16,6 +16,7 @@
     sync::{rcu, Completion},
     types::{ARef, ForeignOwnable, Opaque},
 };
+use core::ops::Deref;
 
 use pin_init::Wrapper;
 
@@ -345,3 +346,86 @@ pub fn register<T, E>(dev: &Device<Bound>, data: impl PinInit<T, E>, flags: Flag
 
     register_foreign(dev, data)
 }
+
+/// [`Devres`]-releaseable resource.
+///
+/// Register an object implementing this trait with [`register_release`]. Its `release`
+/// function will be called once the device is being unbound.
+pub trait Release {
+    /// Called once the [`Device`] given to [`register_release`] is unbound.
+    fn release(&self);
+}
+
+impl<T: Release> Release for crate::sync::ArcBorrow<'_, T> {
+    fn release(&self) {
+        self.deref().release();
+    }
+}
+
+impl<T: Release> Release for Pin<&'_ T> {
+    fn release(&self) {
+        self.deref().release();
+    }
+}
+
+/// Consume the `data`, [`Release::release`] and [`Drop::drop`] `data` once `dev` is unbound.
+///
+/// # Examples
+///
+/// ```no_run
+/// use kernel::{device::{Bound, Device}, devres, devres::Release, sync::Arc};
+///
+/// /// Registration of e.g. a class device, IRQ, etc.
+/// struct Registration<T> {
+///     data: T,
+/// }
+///
+/// impl<T> Registration<T> {
+///     fn new(data: T) -> Result<Arc<Self>> {
+///         // register
+///
+///         Ok(Arc::new(Self { data }, GFP_KERNEL)?)
+///     }
+/// }
+///
+/// impl<T> Release for Registration<T> {
+///     fn release(&self) {
+///        // unregister
+///     }
+/// }
+///
+/// fn from_bound_context(dev: &Device<Bound>) -> Result {
+///     let reg = Registration::new(0x42)?;
+///
+///     devres::register_release(dev, reg.clone())
+/// }
+/// ```
+pub fn register_release<P>(dev: &Device<Bound>, data: P) -> Result
+where
+    P: ForeignOwnable,
+    for<'a> P::Borrowed<'a>: Release,
+{
+    let ptr = data.into_foreign();
+
+    #[allow(clippy::missing_safety_doc)]
+    unsafe extern "C" fn callback<P>(ptr: *mut kernel::ffi::c_void)
+    where
+        P: ForeignOwnable,
+        for<'a> P::Borrowed<'a>: Release,
+    {
+        // SAFETY: `ptr` is the pointer to the `ForeignOwnable` leaked above and hence valid.
+        unsafe { P::borrow(ptr.cast()) }.release();
+
+        // SAFETY: `ptr` is the pointer to the `ForeignOwnable` leaked above and hence valid.
+        let _ = 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::<P>), ptr.cast())
+    })
+}
-- 
2.49.0

Re: [PATCH v2 1/4] rust: revocable: support fallible PinInit types

[Benno Lossin]

On Sun Jun 22, 2025 at 6:40 PM CEST, Danilo Krummrich wrote:
> Currently, Revocable::new() only supports infallible PinInit
> implementations, i.e. impl PinInit<T, Infallible>.
>
> 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.
>
> Signed-off-by: Danilo Krummrich <dakr@kernel.org>

Reviewed-by: Benno Lossin <lossin@kernel.org>

---
Cheers,
Benno

> ---
>  rust/kernel/devres.rs    | 2 +-
>  rust/kernel/revocable.rs | 6 +++---
>  2 files changed, 4 insertions(+), 4 deletions(-)

Re: [PATCH v2 2/4] rust: devres: replace Devres::new_foreign_owned()

[Benno Lossin]

On Sun Jun 22, 2025 at 6:40 PM CEST, Danilo Krummrich wrote:
> 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 <airlied@redhat.com>
> Cc: Simona Vetter <simona.vetter@ffwll.ch>
> Cc: Viresh Kumar <viresh.kumar@linaro.org>
> Acked-by: Viresh Kumar <viresh.kumar@linaro.org>
> Signed-off-by: Danilo Krummrich <dakr@kernel.org>
> ---

Reviewed-by: Benno Lossin <lossin@kernel.org>

---
Cheers,
Benno

>  rust/helpers/device.c     |  7 ++++
>  rust/kernel/cpufreq.rs    | 11 +++---
>  rust/kernel/devres.rs     | 70 +++++++++++++++++++++++++++++++++------
>  rust/kernel/drm/driver.rs | 14 ++++----
>  4 files changed, 82 insertions(+), 20 deletions(-)

Re: [PATCH v2 1/4] rust: revocable: support fallible PinInit types

[Miguel Ojeda]

On Sun, Jun 22, 2025 at 6:41 PM Danilo Krummrich <dakr@kernel.org> wrote:
>
> Currently, Revocable::new() only supports infallible PinInit
> implementations, i.e. impl PinInit<T, Infallible>.
>
> 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.
>
> Signed-off-by: Danilo Krummrich <dakr@kernel.org>

If the series goes through driver-core, please feel free to take:

Acked-by: Miguel Ojeda <ojeda@kernel.org>

Cheers,
Miguel

Re: [PATCH v2 3/4] rust: devres: get rid of Devres' inner Arc

[Benno Lossin]

On Sun Jun 22, 2025 at 6:40 PM CEST, Danilo Krummrich wrote:
> diff --git a/rust/kernel/devres.rs b/rust/kernel/devres.rs
> index 250073749279..15a0a94e992b 100644
> --- a/rust/kernel/devres.rs
> +++ b/rust/kernel/devres.rs
> @@ -13,20 +13,31 @@
>      ffi::c_void,
>      prelude::*,
>      revocable::{Revocable, RevocableGuard},
> -    sync::{rcu, Arc, Completion},
> -    types::{ARef, ForeignOwnable},
> +    sync::{rcu, Completion},
> +    types::{ARef, ForeignOwnable, Opaque},
>  };
>  
> +use pin_init::Wrapper;
> +
> +/// [`Devres`] inner data accessed from [`Devres::callback`].
>  #[pin_data]
> -struct DevresInner<T> {
> -    dev: ARef<Device>,
> -    callback: unsafe extern "C" fn(*mut c_void),
> +struct Inner<T> {
>      #[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,
>  }
>  
> +impl<T> Inner<T> {
> +    fn as_ptr(&self) -> *const Self {
> +        self
> +    }
> +}

Instead of creating this function, you can use `ptr::from_ref`.

> +
>  /// This abstraction is meant to be used by subsystems to containerize [`Device`] bound resources to
>  /// manage their lifetime.
>  ///
> @@ -88,100 +99,106 @@ struct DevresInner<T> {
>  /// # 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 = 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<T>(Arc<DevresInner<T>>);
> +#[pin_data(PinnedDrop)]
> +pub struct Devres<T> {
> +    dev: ARef<Device>,
> +    /// 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.
> +    #[pin]
> +    inner: Opaque<Inner<T>>,
> +}
>  
> -impl<T> DevresInner<T> {
> -    fn new(dev: &Device<Bound>, data: T, flags: Flags) -> Result<Arc<DevresInner<T>>> {
> -        let inner = Arc::pin_init(
> -            try_pin_init!( DevresInner {
> -                dev: dev.into(),
> -                callback: Self::devres_callback,
> +impl<T> Devres<T> {
> +    /// 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<Bound>,
> +        data: impl PinInit<T, E> + 'a,
> +    ) -> impl PinInit<Self, Error> + 'a
> +    where
> +        T: 'a,
> +        Error: From<E>,
> +    {
> +        let callback = Self::devres_callback;
> +
> +        try_pin_init!(&this in Self {
> +            inner <- Opaque::pin_init(try_pin_init!(Inner {
>                  data <- Revocable::new(data),
> +                devm <- Completion::new(),
>                  revoke <- Completion::new(),
> -            }),
> -            flags,
> -        )?;
> -
> -        // Convert `Arc<DevresInner>` 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: It is valid to dereference `this` to find the address of `inner`.

    // SAFETY: `this` is a valid pointer to uninitialized memory.

> +                let inner = unsafe { core::ptr::addr_of_mut!((*this.as_ptr()).inner) };

We can use `raw` instead of `addr_of_mut!`:

    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<Self, Error>`.

Just wanted to highlight that this is a very cool application of the
borrow checker :)

> +                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<T> {
> +        // SAFETY: `inner` is valid and properly initialized.

We should have an invariant here that `inner` is always accessed
read-only and that it is initialized.

---
Cheers,
Benno

> +        unsafe { &*self.inner.get() }
>      }
>  
> -    fn remove_action(this: &Arc<Self>) -> 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<T> {
> +        &self.inner().data
>      }

Re: [PATCH v2 4/4] rust: devres: implement register_release()

[Benno Lossin]

On Sun Jun 22, 2025 at 6:40 PM CEST, Danilo Krummrich wrote:
> +impl<T: Release> Release for crate::sync::ArcBorrow<'_, T> {
> +    fn release(&self) {
> +        self.deref().release();
> +    }
> +}
> +
> +impl<T: Release> Release for Pin<&'_ T> {

You don't need the `'_` here.

> +    fn release(&self) {
> +        self.deref().release();
> +    }
> +}

I still think we're missing a `impl<T: Release> Release for &T`.

And maybe a closure design is better, depending on how much code is
usually run in `release`, if it's a lot, then we should use the trait
design. If it's only 1-5 lines, then a closure would also be fine. I
don't have a strong preference, but if it's mostly one liners, then
closures would be better.

If you keep the trait design & we resolve the `&T: Release` question:

Reviewed-by: Benno Lossin <lossin@kernel.org>

---
Cheers,
Benno

Re: [PATCH v2 3/4] rust: devres: get rid of Devres' inner Arc

[Danilo Krummrich]

On Sun, Jun 22, 2025 at 10:45:02PM +0200, Benno Lossin wrote:
> On Sun Jun 22, 2025 at 6:40 PM CEST, Danilo Krummrich wrote:
> > +impl<T> Inner<T> {
> > +    fn as_ptr(&self) -> *const Self {
> > +        self
> > +    }
> > +}
> 
> Instead of creating this function, you can use `ptr::from_ref`.

Yeah, I like this much better.

> > +impl<T> Devres<T> {
> > +    /// 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<Bound>,
> > +        data: impl PinInit<T, E> + 'a,
> > +    ) -> impl PinInit<Self, Error> + 'a
> > +    where
> > +        T: 'a,
> > +        Error: From<E>,
> > +    {
> > +        let callback = Self::devres_callback;
> > +
> > +        try_pin_init!(&this in Self {
> > +            inner <- Opaque::pin_init(try_pin_init!(Inner {
> >                  data <- Revocable::new(data),
> > +                devm <- Completion::new(),
> >                  revoke <- Completion::new(),
> > -            }),
> > -            flags,
> > -        )?;
> > -
> > -        // Convert `Arc<DevresInner>` 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: It is valid to dereference `this` to find the address of `inner`.
> 
>     // SAFETY: `this` is a valid pointer to uninitialized memory.
> 
> > +                let inner = unsafe { core::ptr::addr_of_mut!((*this.as_ptr()).inner) };
> 
> We can use `raw` instead of `addr_of_mut!`:
> 
>     let inner = unsafe { &raw mut (*this.as_ptr()).inner };

Indeed, forgot we finally have raw_ref_op since v6.15. :)

> >  
> > -        // 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<Self, Error>`.
> 
> Just wanted to highlight that this is a very cool application of the
> borrow checker :)

It is indeed -- I really like this one!

> > +                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<T> {
> > +        // SAFETY: `inner` is valid and properly initialized.
> 
> We should have an invariant here that `inner` is always accessed
> read-only and that it is initialized.

Makes sense, gonna add it.

Re: [PATCH v2 4/4] rust: devres: implement register_release()

[Danilo Krummrich]

On Sun, Jun 22, 2025 at 10:47:55PM +0200, Benno Lossin wrote:
> On Sun Jun 22, 2025 at 6:40 PM CEST, Danilo Krummrich wrote:
> > +impl<T: Release> Release for crate::sync::ArcBorrow<'_, T> {
> > +    fn release(&self) {
> > +        self.deref().release();
> > +    }
> > +}
> > +
> > +impl<T: Release> Release for Pin<&'_ T> {
> 
> You don't need the `'_` here.
> 
> > +    fn release(&self) {
> > +        self.deref().release();
> > +    }
> > +}
> 
> I still think we're missing a `impl<T: Release> Release for &T`.

Yeah, I really thought the compile can figure this one out.

> And maybe a closure design is better, depending on how much code is
> usually run in `release`, if it's a lot, then we should use the trait
> design. If it's only 1-5 lines, then a closure would also be fine. I
> don't have a strong preference, but if it's mostly one liners, then
> closures would be better.

It should usually be rather short, so probably makes sense.

> If you keep the trait design & we resolve the `&T: Release` question:
> 
> Reviewed-by: Benno Lossin <lossin@kernel.org>
> 
> ---
> Cheers,
> Benno

Re: [PATCH v2 4/4] rust: devres: implement register_release()

[Benno Lossin]

On Sun Jun 22, 2025 at 11:12 PM CEST, Danilo Krummrich wrote:
> On Sun, Jun 22, 2025 at 10:47:55PM +0200, Benno Lossin wrote:
>> On Sun Jun 22, 2025 at 6:40 PM CEST, Danilo Krummrich wrote:
>> > +impl<T: Release> Release for crate::sync::ArcBorrow<'_, T> {
>> > +    fn release(&self) {
>> > +        self.deref().release();
>> > +    }
>> > +}
>> > +
>> > +impl<T: Release> Release for Pin<&'_ T> {
>> 
>> You don't need the `'_` here.
>> 
>> > +    fn release(&self) {
>> > +        self.deref().release();
>> > +    }
>> > +}
>> 
>> I still think we're missing a `impl<T: Release> Release for &T`.
>
> Yeah, I really thought the compile can figure this one out.

To me it makes perfect sense that it doesn't work, since `T: Release`,
but `&T: Release` is not satisfied.

Also a funny note: if you add the impl any number of `&` before the `T`
will implement `Release` :)

---
Cheers,
Benno

Re: [PATCH v2 4/4] rust: devres: implement register_release()

[Danilo Krummrich]

On Sun, Jun 22, 2025 at 11:12:28PM +0200, Danilo Krummrich wrote:
> On Sun, Jun 22, 2025 at 10:47:55PM +0200, Benno Lossin wrote:
> > And maybe a closure design is better, depending on how much code is
> > usually run in `release`, if it's a lot, then we should use the trait
> > design. If it's only 1-5 lines, then a closure would also be fine. I
> > don't have a strong preference, but if it's mostly one liners, then
> > closures would be better.
> 
> It should usually be rather short, so probably makes sense.

Quickly tried how it turns out with a closure: The only way I know to capture
the closure within the

	unsafe extern "C" fn callback<P>(ptr: *mut kernel::ffi::c_void)

is with another dynamic allocation, which isn't worth it.

Unless there's another way I'm not aware of, I'd keep the Release trait.

Re: [PATCH v2 4/4] rust: devres: implement register_release()

[Benno Lossin]

On Sun Jun 22, 2025 at 11:24 PM CEST, Danilo Krummrich wrote:
> On Sun, Jun 22, 2025 at 11:12:28PM +0200, Danilo Krummrich wrote:
>> On Sun, Jun 22, 2025 at 10:47:55PM +0200, Benno Lossin wrote:
>> > And maybe a closure design is better, depending on how much code is
>> > usually run in `release`, if it's a lot, then we should use the trait
>> > design. If it's only 1-5 lines, then a closure would also be fine. I
>> > don't have a strong preference, but if it's mostly one liners, then
>> > closures would be better.
>> 
>> It should usually be rather short, so probably makes sense.
>
> Quickly tried how it turns out with a closure: The only way I know to capture
> the closure within the
>
> 	unsafe extern "C" fn callback<P>(ptr: *mut kernel::ffi::c_void)
>
> is with another dynamic allocation, which isn't worth it.
>
> Unless there's another way I'm not aware of, I'd keep the Release trait.

Ah right that makes sens.

---
Cheers,
Benno

Re: [PATCH v2 3/4] rust: devres: get rid of Devres' inner Arc

[Boqun Feng]

On Sun, Jun 22, 2025 at 06:40:40PM +0200, Danilo Krummrich wrote:
> 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

I like the idea ;-)

> Devres consume an `impl PinInit<T, E>` in order to return an
> `impl PinInit<Devres<T>, 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.
> 
> Signed-off-by: Danilo Krummrich <dakr@kernel.org>
> ---
[...]
> +impl<T> Devres<T> {
[...]
>  
>      #[allow(clippy::missing_safety_doc)]
>      unsafe extern "C" fn devres_callback(ptr: *mut kernel::ffi::c_void) {
> -        let ptr = ptr as *mut DevresInner<T>;
> -        // 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`.

I think you also need to mention that `inner` only remains valid until
`inner.devm.complete_all()` unblocks `Devres::drop()`, because after
`Devres::drop()`'s `devm.wait_for_completion()` returns, `inner` may be
dropped or freed.

Regards,
Boqun

> +        let inner = unsafe { &*ptr.cast::<Inner<T>>() };
>  
>          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();
>          }
[...]
> +        // Signal that we're done using `inner`.
> +        inner.devm.complete_all();
> +    }
[...]

Re: [PATCH v2 1/4] rust: revocable: support fallible PinInit types

[Alice Ryhl]

On Sun, Jun 22, 2025 at 06:40:38PM +0200, Danilo Krummrich wrote:
> Currently, Revocable::new() only supports infallible PinInit
> implementations, i.e. impl PinInit<T, Infallible>.
> 
> 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.
> 
> Signed-off-by: Danilo Krummrich <dakr@kernel.org>

Reviewed-by: Alice Ryhl <aliceryhl@google.com>

Re: [PATCH v2 2/4] rust: devres: replace Devres::new_foreign_owned()

[Alice Ryhl]

On Sun, Jun 22, 2025 at 06:40:39PM +0200, Danilo Krummrich wrote:
> 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 <airlied@redhat.com>
> Cc: Simona Vetter <simona.vetter@ffwll.ch>
> Cc: Viresh Kumar <viresh.kumar@linaro.org>
> Acked-by: Viresh Kumar <viresh.kumar@linaro.org>
> Signed-off-by: Danilo Krummrich <dakr@kernel.org>

overall looks good
Reviewed-by: Alice Ryhl <aliceryhl@google.com>

>  rust/helpers/device.c     |  7 ++++
>  rust/kernel/cpufreq.rs    | 11 +++---
>  rust/kernel/devres.rs     | 70 +++++++++++++++++++++++++++++++++------
>  rust/kernel/drm/driver.rs | 14 ++++----
>  4 files changed, 82 insertions(+), 20 deletions(-)
> 
> diff --git a/rust/helpers/device.c b/rust/helpers/device.c
> index b2135c6686b0..502fef7e9ae8 100644
> --- a/rust/helpers/device.c
> +++ b/rust/helpers/device.c
> @@ -8,3 +8,10 @@ int rust_helper_devm_add_action(struct device *dev,
>  {
>  	return devm_add_action(dev, action, data);
>  }
> +
> +int rust_helper_devm_add_action_or_reset(struct device *dev,
> +					 void (*action)(void *),
> +					 void *data)
> +{
> +	return devm_add_action_or_reset(dev, action, data);
> +}
> diff --git a/rust/kernel/cpufreq.rs b/rust/kernel/cpufreq.rs
> index 11b03e9d7e89..dd84e2b4d7ae 100644
> --- a/rust/kernel/cpufreq.rs
> +++ b/rust/kernel/cpufreq.rs
> @@ -13,7 +13,7 @@
>      cpu::CpuId,
>      cpumask,
>      device::{Bound, Device},
> -    devres::Devres,
> +    devres,
>      error::{code::*, from_err_ptr, from_result, to_result, Result, VTABLE_DEFAULT_ERROR},
>      ffi::{c_char, c_ulong},
>      prelude::*,
> @@ -1046,10 +1046,13 @@ pub fn new() -> Result<Self> {
>  
>      /// Same as [`Registration::new`], but does not return a [`Registration`] instance.
>      ///
> -    /// Instead the [`Registration`] is owned by [`Devres`] and will be revoked / dropped, once the
> +    /// Instead the [`Registration`] is owned by [`devres::register`] and will be dropped, once the
>      /// device is detached.
> -    pub fn new_foreign_owned(dev: &Device<Bound>) -> Result {
> -        Devres::new_foreign_owned(dev, Self::new()?, GFP_KERNEL)
> +    pub fn new_foreign_owned(dev: &Device<Bound>) -> Result
> +    where
> +        T: 'static,
> +    {
> +        devres::register(dev, Self::new()?, GFP_KERNEL)
>      }
>  }
>  
> diff --git a/rust/kernel/devres.rs b/rust/kernel/devres.rs
> index 544e50efab43..250073749279 100644
> --- a/rust/kernel/devres.rs
> +++ b/rust/kernel/devres.rs
> @@ -9,12 +9,12 @@
>      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 @@ pub fn new(dev: &Device<Bound>, data: T, flags: Flags) -> Result<Self> {
>          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<Bound>, 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,61 @@ fn drop(&mut self) {
>          }
>      }
>  }
> +
> +/// Consume `data` and [`Drop::drop`] `data` once `dev` is unbound.
> +fn register_foreign<P: ForeignOwnable>(dev: &Device<Bound>, data: P) -> Result {
> +    let ptr = data.into_foreign();
> +
> +    #[allow(clippy::missing_safety_doc)]
> +    unsafe extern "C" fn callback<P: ForeignOwnable>(ptr: *mut kernel::ffi::c_void) {
> +        // SAFETY: `ptr` is the pointer to the `ForeignOwnable` leaked above and hence valid.
> +        let _ = unsafe { P::from_foreign(ptr.cast()) };

Nit: I usually write this
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::<P>), 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<Bound>) -> Result {
> +///     devres::register(dev, Registration::new(), GFP_KERNEL)
> +/// }
> +/// ```
> +pub fn register<T, E>(dev: &Device<Bound>, data: impl PinInit<T, E>, flags: Flags) -> Result
> +where
> +    T: 'static,
> +    Error: From<E>,
> +{
> +    let data = KBox::pin_init(data, flags)?;

Wouldn't we also want to expose the ForeignOwnable version? It seems
likely someone would want to avoid the allocation.

> +    register_foreign(dev, data)
> +}
> diff --git a/rust/kernel/drm/driver.rs b/rust/kernel/drm/driver.rs
> index acb638086131..f63addaf7235 100644
> --- a/rust/kernel/drm/driver.rs
> +++ b/rust/kernel/drm/driver.rs
> @@ -5,9 +5,7 @@
>  //! C header: [`include/linux/drm/drm_drv.h`](srctree/include/linux/drm/drm_drv.h)
>  
>  use crate::{
> -    bindings, device,
> -    devres::Devres,
> -    drm,
> +    bindings, device, devres, drm,
>      error::{to_result, Result},
>      prelude::*,
>      str::CStr,
> @@ -130,18 +128,22 @@ fn new(drm: &drm::Device<T>, flags: usize) -> Result<Self> {
>      }
>  
>      /// Same as [`Registration::new`}, but transfers ownership of the [`Registration`] to
> -    /// [`Devres`].
> +    /// [`devres::register`].
>      pub fn new_foreign_owned(
>          drm: &drm::Device<T>,
>          dev: &device::Device<device::Bound>,
>          flags: usize,
> -    ) -> Result {
> +    ) -> Result
> +    where
> +        T: 'static,
> +    {
>          if drm.as_ref().as_raw() != dev.as_raw() {
>              return Err(EINVAL);
>          }
>  
>          let reg = Registration::<T>::new(drm, flags)?;
> -        Devres::new_foreign_owned(dev, reg, GFP_KERNEL)
> +
> +        devres::register(dev, reg, GFP_KERNEL)
>      }
>  
>      /// Returns a reference to the `Device` instance for this registration.
> -- 
> 2.49.0
> 

Re: [PATCH v2 4/4] rust: devres: implement register_release()

[Alice Ryhl]

On Sun, Jun 22, 2025 at 06:40:41PM +0200, Danilo Krummrich wrote:
> +pub fn register_release<P>(dev: &Device<Bound>, data: P) -> Result
> +where
> +    P: ForeignOwnable,
> +    for<'a> P::Borrowed<'a>: Release,

I think we need where P: ForeignOwnable + 'static too.

otherwise I can pass something with a reference that expires before the
device is unbound and access it in the devm callback as a UAF.

Re: [PATCH v2 2/4] rust: devres: replace Devres::new_foreign_owned()

[Danilo Krummrich]

On Mon, Jun 23, 2025 at 11:56:39AM +0000, Alice Ryhl wrote:
> On Sun, Jun 22, 2025 at 06:40:39PM +0200, Danilo Krummrich wrote:
> > +pub fn register<T, E>(dev: &Device<Bound>, data: impl PinInit<T, E>, flags: Flags) -> Result
> > +where
> > +    T: 'static,
> > +    Error: From<E>,
> > +{
> > +    let data = KBox::pin_init(data, flags)?;
> 
> Wouldn't we also want to expose the ForeignOwnable version? It seems
> likely someone would want to avoid the allocation.

Allowing other things than Box changes the semantics of the API. For instance,
if it'd be a reference counted thing (e.g. Arc), it would basically mean "please
make this object live *at least* as long as the device is bound".

Before considering to allow such things, I really want a potential user to
present a valid use-case for that.

Re: [PATCH v2 4/4] rust: devres: implement register_release()

[Danilo Krummrich]

On Mon, Jun 23, 2025 at 12:01:14PM +0000, Alice Ryhl wrote:
> On Sun, Jun 22, 2025 at 06:40:41PM +0200, Danilo Krummrich wrote:
> > +pub fn register_release<P>(dev: &Device<Bound>, data: P) -> Result
> > +where
> > +    P: ForeignOwnable,
> > +    for<'a> P::Borrowed<'a>: Release,
> 
> I think we need where P: ForeignOwnable + 'static too.
> 
> otherwise I can pass something with a reference that expires before the
> device is unbound and access it in the devm callback as a UAF.

I can't really come up with an example for such a case, mind providing one? :)

Re: [PATCH v2 4/4] rust: devres: implement register_release()

[Benno Lossin]

On Mon Jun 23, 2025 at 2:51 PM CEST, Danilo Krummrich wrote:
> On Mon, Jun 23, 2025 at 12:01:14PM +0000, Alice Ryhl wrote:
>> On Sun, Jun 22, 2025 at 06:40:41PM +0200, Danilo Krummrich wrote:
>> > +pub fn register_release<P>(dev: &Device<Bound>, data: P) -> Result
>> > +where
>> > +    P: ForeignOwnable,
>> > +    for<'a> P::Borrowed<'a>: Release,
>> 
>> I think we need where P: ForeignOwnable + 'static too.
>> 
>> otherwise I can pass something with a reference that expires before the
>> device is unbound and access it in the devm callback as a UAF.
>
> I can't really come up with an example for such a case, mind providing one? :)

    {
        let local = MyLocalData { /* ... */ };
        let data = Arc::new(Data { r: &local });
        devres::register_release(dev, data)?;
    }
    // devres still holds onto `data`, but that points at `MyLocalData`
    // which is on the stack and freed here...

---
Cheers,
Benno

Re: [PATCH v2 4/4] rust: devres: implement register_release()

[Danilo Krummrich]

On Mon, Jun 23, 2025 at 03:40:05PM +0200, Benno Lossin wrote:
> On Mon Jun 23, 2025 at 2:51 PM CEST, Danilo Krummrich wrote:
> > On Mon, Jun 23, 2025 at 12:01:14PM +0000, Alice Ryhl wrote:
> >> On Sun, Jun 22, 2025 at 06:40:41PM +0200, Danilo Krummrich wrote:
> >> > +pub fn register_release<P>(dev: &Device<Bound>, data: P) -> Result
> >> > +where
> >> > +    P: ForeignOwnable,
> >> > +    for<'a> P::Borrowed<'a>: Release,
> >> 
> >> I think we need where P: ForeignOwnable + 'static too.
> >> 
> >> otherwise I can pass something with a reference that expires before the
> >> device is unbound and access it in the devm callback as a UAF.
> >
> > I can't really come up with an example for such a case, mind providing one? :)
> 
>     {
>         let local = MyLocalData { /* ... */ };
>         let data = Arc::new(Data { r: &local });
>         devres::register_release(dev, data)?;
>     }
>     // devres still holds onto `data`, but that points at `MyLocalData`
>     // which is on the stack and freed here...

Thanks for providing the example, I think I slightly misunderstood. Gonna add
the corresponding lifetime bound.

Re: [PATCH v2 3/4] rust: devres: get rid of Devres' inner Arc

[Danilo Krummrich]

On Sun, Jun 22, 2025 at 06:54:07PM -0700, Boqun Feng wrote:
> I think you also need to mention that `inner` only remains valid until
> `inner.devm.complete_all()` unblocks `Devres::drop()`, because after
> `Devres::drop()`'s `devm.wait_for_completion()` returns, `inner` may be
> dropped or freed.

I think of it the other way around: The invariant guarantees that `inner` is
*always* valid.

The the `drop_in_place(inner)` call has to justify that it upholds this
invariant, by ensuring that at the time it is called no other code that accesses
`inner` can ever run.

Defining it the other way around would make the `inner()` accessor unsafe.

Re: [PATCH v2 3/4] rust: devres: get rid of Devres' inner Arc

[Boqun Feng]

On Tue, Jun 24, 2025 at 05:18:23PM +0200, Danilo Krummrich wrote:
> On Sun, Jun 22, 2025 at 06:54:07PM -0700, Boqun Feng wrote:
> > I think you also need to mention that `inner` only remains valid until
> > `inner.devm.complete_all()` unblocks `Devres::drop()`, because after
> > `Devres::drop()`'s `devm.wait_for_completion()` returns, `inner` may be
> > dropped or freed.
> 
> I think of it the other way around: The invariant guarantees that `inner` is
> *always* valid.
> 
> The the `drop_in_place(inner)` call has to justify that it upholds this
> invariant, by ensuring that at the time it is called no other code that accesses
> `inner` can ever run.
> 
> Defining it the other way around would make the `inner()` accessor unsafe.

Maybe I wasn't clear enough, I meant in the following function:

    unsafe extern "C" fn devres_callback(ptr: *mut kernel::ffi::c_void) {
-        let ptr = ptr as *mut DevresInner<T>;
-        // 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::<Inner<T>>() };

^ this `inner` was constructed by reborrowing from `ptr`, but it should
only be used before the following `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();
[...]
+        // Signal that we're done using `inner`.
+        inner.devm.complete_all();

... because the `DevresInner` might be freed after we signal the
`Devres::drop()`. And for example, doing a:

    inner.data.try_access();

after the above line would be unsound.

+    }

And I would prefer we document this or use `ScopeGuard`, does it make
sense?

Regards,
Boqun

Re: [PATCH v2 3/4] rust: devres: get rid of Devres' inner Arc

[Danilo Krummrich]

On Tue, Jun 24, 2025 at 08:46:53AM -0700, Boqun Feng wrote:
> On Tue, Jun 24, 2025 at 05:18:23PM +0200, Danilo Krummrich wrote:
> > On Sun, Jun 22, 2025 at 06:54:07PM -0700, Boqun Feng wrote:
> > > I think you also need to mention that `inner` only remains valid until
> > > `inner.devm.complete_all()` unblocks `Devres::drop()`, because after
> > > `Devres::drop()`'s `devm.wait_for_completion()` returns, `inner` may be
> > > dropped or freed.
> > 
> > I think of it the other way around: The invariant guarantees that `inner` is
> > *always* valid.
> > 
> > The the `drop_in_place(inner)` call has to justify that it upholds this
> > invariant, by ensuring that at the time it is called no other code that accesses
> > `inner` can ever run.
> > 
> > Defining it the other way around would make the `inner()` accessor unsafe.
> 
> Maybe I wasn't clear enough, I meant in the following function:
> 
>     unsafe extern "C" fn devres_callback(ptr: *mut kernel::ffi::c_void) {
> -        let ptr = ptr as *mut DevresInner<T>;
> -        // 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::<Inner<T>>() };
> 
> ^ this `inner` was constructed by reborrowing from `ptr`, but it should
> only be used before the following `inner.devm.complete_all()`...

Oh, so you meant adding this to the safety comment. Yes, that makes sense. Maybe
ScopeGuard works too, as you say.

Re: [PATCH v2 3/4] rust: devres: get rid of Devres' inner Arc

[Danilo Krummrich]

On Tue, Jun 24, 2025 at 06:15:43PM +0200, Danilo Krummrich wrote:
> Oh, so you meant adding this to the safety comment. Yes, that makes sense. Maybe
> ScopeGuard works too, as you say.

diff --git a/rust/kernel/devres.rs b/rust/kernel/devres.rs
index 2591cacecb7b..afd73a8c6012 100644
--- a/rust/kernel/devres.rs
+++ b/rust/kernel/devres.rs
@@ -14,7 +14,7 @@
     prelude::*,
     revocable::{Revocable, RevocableGuard},
     sync::{rcu, Completion},
-    types::{ARef, ForeignOwnable, Opaque},
+    types::{ARef, ForeignOwnable, ScopeGuard, Opaque},
 };
 use core::ops::Deref;

@@ -177,15 +177,15 @@ fn data(&self) -> &Revocable<T> {
         // hence `ptr` must be a valid pointer to `Inner`.
         let inner = unsafe { &*ptr.cast::<Inner<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();
         }
-
-        // Signal that we're done using `inner`.
-        inner.devm.complete_all();
     }

     fn remove_action(&self) -> bool {

Is this what you thought of?

Re: [PATCH v2 3/4] rust: devres: get rid of Devres' inner Arc

[Boqun Feng]

On Tue, Jun 24, 2025 at 06:20:41PM +0200, Danilo Krummrich wrote:
> On Tue, Jun 24, 2025 at 06:15:43PM +0200, Danilo Krummrich wrote:
> > Oh, so you meant adding this to the safety comment. Yes, that makes sense. Maybe
> > ScopeGuard works too, as you say.
> 
> diff --git a/rust/kernel/devres.rs b/rust/kernel/devres.rs
> index 2591cacecb7b..afd73a8c6012 100644
> --- a/rust/kernel/devres.rs
> +++ b/rust/kernel/devres.rs
> @@ -14,7 +14,7 @@
>      prelude::*,
>      revocable::{Revocable, RevocableGuard},
>      sync::{rcu, Completion},
> -    types::{ARef, ForeignOwnable, Opaque},
> +    types::{ARef, ForeignOwnable, ScopeGuard, Opaque},
>  };
>  use core::ops::Deref;
> 
> @@ -177,15 +177,15 @@ fn data(&self) -> &Revocable<T> {
>          // hence `ptr` must be a valid pointer to `Inner`.
>          let inner = unsafe { &*ptr.cast::<Inner<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();
>          }
> -
> -        // Signal that we're done using `inner`.
> -        inner.devm.complete_all();
>      }
> 
>      fn remove_action(&self) -> bool {
> 
> Is this what you thought of?

Yep, looks good to me. Thanks!

Regards,
Boqun