linux-kernel - Re: [PATCH v4 5/5] 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: <aF24vbtHgP-kYuBg@tardis.local>
Date: Thu, 26 Jun 2025 14:16:45 -0700
From: Boqun Feng <boqun.feng@...il.com>
To: Danilo Krummrich <dakr@...nel.org>
Cc: gregkh@...uxfoundation.org, rafael@...nel.org, ojeda@...nel.org,
	alex.gaynor@...il.com, gary@...yguo.net, bjorn3_gh@...tonmail.com,
	lossin@...nel.org, 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,
	rust-for-linux@...r.kernel.org, linux-kernel@...r.kernel.org,
	linux-pci@...r.kernel.org
Subject: Re: [PATCH v4 5/5] rust: devres: implement register_release()

On Thu, Jun 26, 2025 at 10:48:03PM +0200, Danilo Krummrich wrote:
> On Thu, Jun 26, 2025 at 01:37:22PM -0700, Boqun Feng wrote:
> > On Thu, Jun 26, 2025 at 10:00:43PM +0200, Danilo Krummrich wrote:
> > > 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@...gle.com>
> > > Signed-off-by: Danilo Krummrich <dakr@...nel.org>
> > > ---
> > >  rust/kernel/devres.rs | 73 +++++++++++++++++++++++++++++++++++++++++++
> > >  1 file changed, 73 insertions(+)
> > > 
> > > diff --git a/rust/kernel/devres.rs b/rust/kernel/devres.rs
> > > index 3ce8d6161778..92aca78874ff 100644
> > > --- a/rust/kernel/devres.rs
> > > +++ b/rust/kernel/devres.rs
> > > @@ -353,3 +353,76 @@ 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 {
> > > +    /// The [`ForeignOwnable`] pointer type consumed by [`register_release`].
> > > +    type Ptr: ForeignOwnable;
> > > +
> > > +    /// Called once the [`Device`] given to [`register_release`] is unbound.
> > > +    fn release(this: Self::Ptr);
> > > +}
> > > +
> > 
> > I would like to point out the limitation of this design, say you have a
> > `Foo` that can ipml `Release`, with this, I think you could only support
> > either `Arc<Foo>` or `KBox<Foo>`. You cannot support both as the input
> > for `register_release()`. Maybe we want:
> > 
> >     pub trait Release<Ptr: ForeignOwnable> {
> >         fn release(this: Ptr);
> >     }
> 
> Good catch! I think this wasn't possible without ForeignOwnable::Target.
> 
> Here's the diff for the change:
> 

Looks good to me.

> diff --git a/rust/kernel/devres.rs b/rust/kernel/devres.rs
> index 92aca78874ff..42a9cd2812d8 100644
> --- a/rust/kernel/devres.rs
> +++ b/rust/kernel/devres.rs
> @@ -358,12 +358,9 @@ pub fn register<T, E>(dev: &Device<Bound>, data: impl PinInit<T, E>, flags: Flag
>  ///
>  /// Register an object implementing this trait with [`register_release`]. Its `release`
>  /// function will be called once the device is being unbound.
> -pub trait Release {
> -    /// The [`ForeignOwnable`] pointer type consumed by [`register_release`].
> -    type Ptr: ForeignOwnable;
> -
> +pub trait Release<Ptr: ForeignOwnable> {

My bad, is it possible to do

	pub trait Release<Ptr: ForeignOwnable<Target=Self>> {

? that's better IMO.

Regards,
Boqun

>      /// Called once the [`Device`] given to [`register_release`] is unbound.
> -    fn release(this: Self::Ptr);
> +    fn release(this: Ptr);
>  }
> 
>  /// Consume the `data`, [`Release::release`] and [`Drop::drop`] `data` once `dev` is unbound.
> @@ -384,9 +381,7 @@ pub trait Release {
>  ///     }
>  /// }
>  ///
> -/// impl Release for Registration {
> -///     type Ptr = Arc<Self>;
> -///
> +/// impl Release<Arc<Self>> for Registration {
>  ///     fn release(this: Arc<Self>) {
>  ///        // unregister
>  ///     }
> @@ -401,7 +396,7 @@ pub trait Release {
>  pub fn register_release<P>(dev: &Device<Bound>, data: P) -> Result
>  where
>      P: ForeignOwnable,
> -    P::Target: Release<Ptr = P> + Send,
> +    P::Target: Release<P> + Send,
>  {
>      let ptr = data.into_foreign();
> 
> @@ -409,7 +404,7 @@ pub fn register_release<P>(dev: &Device<Bound>, data: P) -> Result
>      unsafe extern "C" fn callback<P>(ptr: *mut kernel::ffi::c_void)
>      where
>          P: ForeignOwnable,
> -        P::Target: Release<Ptr = P>,
> +        P::Target: Release<P>,
>      {
>          // SAFETY: `ptr` is the pointer to the `ForeignOwnable` leaked above and hence valid.
>          let data = unsafe { P::from_foreign(ptr.cast()) };