Improvements for Devres

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 <[email protected]>

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

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 <[email protected]>

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 <[email protected]>

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 <[email protected]>

Cc Simona Vetter <[email protected]>

Cc Viresh Kumar <[email protected]>

Acked-by Viresh Kumar <[email protected]>

Signed-off-by Danilo Krummrich <[email protected]>

---

 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(-)

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 <[email protected]>

Cc Simona Vetter <[email protected]>

Cc Viresh Kumar <[email protected]>

Acked-by Viresh Kumar <[email protected]>

Signed-off-by Danilo Krummrich <[email protected]>

 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(-)

rust/helpers/device.c +7 -0

index b2135c6686b0..502fef7e9ae8 100644
@@ -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);
+}

rust/kernel/cpufreq.rs +7 -4

index 11b03e9d7e89..dd84e2b4d7ae 100644
@@ -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)
     }
 }
 

rust/kernel/devres.rs +2 -10

index 544e50efab43..250073749279 100644
@@ -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) {
         }
     }

rust/kernel/devres.rs +36 -0

+    }
+
+    // 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
+///     }
+/// }
+///

rust/kernel/devres.rs +2 -0

+    register_foreign(dev, data)
+}

rust/kernel/drm/driver.rs +8 -6

index acb638086131..f63addaf7235 100644
@@ -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.

Signature · 1 line

2.49.0

rust/kernel/devres.rs fn drop(&mut self) { · line 310

Quoted 6 lines · Danilo Krummrich

rust/kernel/devres.rs +6 -0

+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.

rust/kernel/devres.rs +9 -5

index 250073749279..15a0a94e992b 100644
@@ -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,

rust/kernel/devres.rs +33 -9

+
 /// 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(),
-            }),

rust/kernel/devres.rs +0 -8

-        // 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

rust/kernel/devres.rs +3 -0

+                to_result(unsafe {
+                    bindings::devm_add_action(dev.as_raw(), Some(callback), inner.cast())
+                })?;
 

rust/kernel/devres.rs +3 -22

+        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
     }

rust/kernel/devres.rs line 39

Quoted 5 lines · Danilo Krummrich

rust/kernel/devres.rs +5 -0

+impl<T> Inner<T> {
+    fn as_ptr(&self) -> *const Self {
+        self
+    }
+}

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

Quoted 21 lines

rust/kernel/devres.rs +18 -1

+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(),
-            }),

rust/kernel/devres.rs struct DevresInner<T> { · line 146

Quoted 10 lines

rust/kernel/devres.rs +4 -6

-            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.

Quoted 1 line

+ 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 };

Quoted 9 lines

rust/kernel/devres.rs +0 -8

-        // 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

rust/kernel/devres.rs struct DevresInner<T> { · line 154

Quoted 10 lines

rust/kernel/devres.rs +6 -4

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

Quoted 10 lines

rust/kernel/devres.rs +6 -1

+                to_result(unsafe {
+                    bindings::devm_add_action(dev.as_raw(), Some(callback), inner.cast())
+                })?;
 
-        Ok(inner)
+                dev.into()
+            },
+        })
     }

rust/kernel/devres.rs struct DevresInner<T> { · line 165

Quoted 4 lines

rust/kernel/devres.rs +2 -2

-    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.

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 <[email protected]>

---

+impl<T> Devres<T> {

rust/kernel/devres.rs +3 -2

+        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();
         }

rust/kernel/devres.rs +3 -0

+        // Signal that we're done using `inner`.
+        inner.devm.complete_all();
+    }

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.

Quoted 4 lines · Boqun Feng

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.

Quoted 6 lines · Danilo Krummrich

Quoted 4 lines · Boqun Feng

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.

Quoted 8 lines

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) {

rust/kernel/devres.rs +3 -6

-        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.

Quoted 2 lines · Danilo Krummrich

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

rust/kernel/devres.rs +4 -4

index 2591cacecb7b..afd73a8c6012 100644
@@ -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?

rust/kernel/devres.rs pub fn register<T, E>(dev: &Device<Bound>, data: impl PinInit<T, E>, flags: Flag · line 365

Quoted 7 lines · Danilo Krummrich

rust/kernel/devres.rs +7 -0

+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.

rust/kernel/devres.rs pub fn register<T, E>(dev: &Device<Bound>, data: impl PinInit<T, E>, flags: Flag · line 363

Quoted 4 lines

rust/kernel/devres.rs +4 -0

+    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 <[email protected]>

---
Cheers,
Benno

Quoted 11 lines · Benno Lossin

rust/kernel/devres.rs pub fn register<T, E>(dev: &Device<Bound>, data: impl PinInit<T, E>, flags: Flag · line 365

Quoted 7 lines · Danilo Krummrich

rust/kernel/devres.rs +7 -0

+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.

Quoted 6 lines

rust/kernel/devres.rs pub fn register<T, E>(dev: &Device<Bound>, data: impl PinInit<T, E>, flags: Flag · line 363

Quoted 4 lines

rust/kernel/devres.rs +4 -0

+    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.

Quoted 5 lines · Benno Lossin

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.

Quoted 8 lines · Danilo Krummrich

Quoted 5 lines · Benno Lossin

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.

rust/kernel/devres.rs pub fn register<T, E>(dev: &Device<Bound>, data: impl PinInit<T, E>, flags: Flag · line 406

Quoted 4 lines · Danilo Krummrich

rust/kernel/devres.rs +4 -0

+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.

Quoted 8 lines · Alice Ryhl

rust/kernel/devres.rs pub fn register<T, E>(dev: &Device<Bound>, data: impl PinInit<T, E>, flags: Flag · line 406

Quoted 4 lines · Danilo Krummrich

rust/kernel/devres.rs +4 -0

+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.

Quoted 2 lines

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

Quoted 9 lines · Danilo Krummrich

Quoted 8 lines · Alice Ryhl

rust/kernel/devres.rs pub fn register<T, E>(dev: &Device<Bound>, data: impl PinInit<T, E>, flags: Flag · line 406

Quoted 4 lines · Danilo Krummrich

rust/kernel/devres.rs +4 -0

+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.

Quoted 4 lines

Quoted 2 lines

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...