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

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.

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? :)

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

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.

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

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

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.

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

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