linux-kernel - Re: [PATCH 4/4] rust: devres: implement register_foreign

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <aFf7QVhqE6dhp0m2@pollux>
Date: Sun, 22 Jun 2025 14:46:57 +0200
From: Danilo Krummrich <dakr@...nel.org>
To: Benno Lossin <lossin@...nel.org>
Cc: gregkh@...uxfoundation.org, rafael@...nel.org, ojeda@...nel.org,
	alex.gaynor@...il.com, boqun.feng@...il.com, gary@...yguo.net,
	bjorn3_gh@...tonmail.com, benno.lossin@...ton.me,
	a.hindborg@...nel.org, aliceryhl@...gle.com, tmgross@...ch.edu,
	rust-for-linux@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 4/4] rust: devres: implement register_foreign_release()

On Sun, Jun 22, 2025 at 09:26:33AM +0200, Benno Lossin wrote:
> On Thu Jun 12, 2025 at 4:51 PM CEST, Danilo Krummrich wrote:
> > +/// To be implemented by an object passed to [`register_foreign_release`].
> 
>     /// [`Devres`]-releaseable resource.
>     ///
>     /// Register an object implementing this trait with [`register_foreign_release`]. It's `release`
>     /// function will be called once the device unbinds.

Sounds good, thanks!

> > +pub trait Release {
> > +    /// Called once the [`Device`] given to [`register_foreign_release`] is unbound.
> > +    fn release(&self);
> 
> Would it make sense to also supply the `Device` that this is attached
> to? In case you have one object in multiple `register_foreign_release`
> calls with different devices, or is that something that doesn't happen?

No, doing that wouldn't make any sense. A resource should only be bound to the
lifetime of a single device.

> > +}
> > +
> > +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();
> > +    }
> > +}
> 
> We should also implement it for `&T`, since that is `Box`'s `Borrowed`.

That should implicitly be the case when T: Release, where T is P<T>.

> > +
> > +/// 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};
> > +///
> > +/// struct Registration<T> {
> 
> Maybe add some explanation above/below this example. It looks like a new
> bus registration?

*class device registration, see Registration::new() below. But I can also add a
brief comment to the struct. It's indeed a bit subtly this way. :)

> > +///     data: T,
> > +/// }
> > +///
> > +/// impl<T> Registration<T> {
> > +///     fn new(data: T) -> Result<Arc<Self>> {
> > +///         // register (e.g. class device, IRQ, etc.)
> > +///
> > +///         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_foreign_release(dev, reg.clone())
> > +/// }
> > +/// ```
> > +pub fn register_foreign_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())
> > +    })
> > +}
>