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

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: <DAWED7BIC32G.338MXRHK4NSJG@kernel.org>
Date: Thu, 26 Jun 2025 12:36:23 +0200
From: "Benno Lossin" <lossin@...nel.org>
To: "Danilo Krummrich" <dakr@...nel.org>, <gregkh@...uxfoundation.org>,
 <rafael@...nel.org>, <ojeda@...nel.org>, <alex.gaynor@...il.com>,
 <boqun.feng@...il.com>, <gary@...yguo.net>, <bjorn3_gh@...tonmail.com>,
 <a.hindborg@...nel.org>, <aliceryhl@...gle.com>, <tmgross@...ch.edu>,
 <david.m.ertman@...el.com>, <ira.weiny@...el.com>, <leon@...nel.org>,
 <kwilczynski@...nel.org>, <bhelgaas@...gle.com>
Cc: <rust-for-linux@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
 <linux-pci@...r.kernel.org>
Subject: Re: [PATCH v3 4/4] rust: devres: implement register_release()

On Tue Jun 24, 2025 at 11:54 PM CEST, Danilo Krummrich wrote:
> +pub fn register_release<P>(dev: &Device<Bound>, data: P) -> Result
> +where
> +    P: ForeignOwnable + 'static,
> +    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.
> +        drop(unsafe { P::from_foreign(ptr.cast()) });

Maybe this function should just be:

    let p = unsafe { P::from_foreign(ptr.cast()) };
    p.release();

And we require `P: ForeignOwnable + Release + 'static`?

We then need these impls instead:

    impl<T: Release, A: Allocator> Release for Pin<Box<T, A>>;
    impl<T: Release, A: Allocator> Release for Box<T, A>;
    impl<T: Release> Release for Arc<T>;

Or, we could change `Release` to be:

    pub trait Release {
        type Ptr: ForeignOwnable;

        fn release(this: Self::Ptr);
    }

and then `register_release` is:

    pub fn register_release<T: Release>(dev: &Device<Bound>, data: T::Ptr) -> Result

This way, one can store a `Box<T>` and get access to the `T` at the end.
Or if they store the value in an `Arc<T>`, they have the option to clone
it and give it to somewhere else.

Related questions:

* should we implement `ForeignOwnable` for `&'static T`?
* should we require `'static` in `ForeignOwnable`? At the moment we only
  have those kinds supported and it only makes sense, a foreign owned
  object can be owned for any amount of time (so it must stay valid
  indefinitely).

---
Cheers,
Benno

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