linux-kernel - Re: [PATCH 2/2] rust: devres: remove action in `Devres::drop`

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: <2025010413-fountain-reflux-f6b0@gregkh>
Date: Sat, 4 Jan 2025 09:57:25 +0100
From: Greg KH <gregkh@...uxfoundation.org>
To: Danilo Krummrich <dakr@...nel.org>
Cc: 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, linux-kernel@...r.kernel.org,
	rust-for-linux@...r.kernel.org
Subject: Re: [PATCH 2/2] rust: devres: remove action in `Devres::drop`

On Fri, Jan 03, 2025 at 05:58:37PM +0100, Danilo Krummrich wrote:
> On Fri, Jan 03, 2025 at 05:44:31PM +0100, Danilo Krummrich wrote:
> > So far `DevresInner` is kept alive, even if `Devres` is dropped until
> > the devres callback is executed to avoid a WARN() when the action has
> > been released already.
> > 
> > With the introduction of devm_remove_action_nowarn() we can remove the
> > action in `Devres::drop`, handle the case where the action has been
> > released already and hence also free `DevresInner`.
> > 
> > Signed-off-by: Danilo Krummrich <dakr@...nel.org>
> > ---
> >  rust/kernel/devres.rs | 56 +++++++++++++++++++++++++++++++++----------
> >  1 file changed, 44 insertions(+), 12 deletions(-)
> > 
> > diff --git a/rust/kernel/devres.rs b/rust/kernel/devres.rs
> > index 9c9dd39584eb..7d3daac92109 100644
> > --- a/rust/kernel/devres.rs
> > +++ b/rust/kernel/devres.rs
> > @@ -10,15 +10,19 @@
> >      bindings,
> >      device::Device,
> >      error::{Error, Result},
> > +    ffi::c_void,
> >      prelude::*,
> >      revocable::Revocable,
> >      sync::Arc,
> > +    types::ARef,
> >  };
> >  
> >  use core::ops::Deref;
> >  
> >  #[pin_data]
> >  struct DevresInner<T> {
> > +    dev: ARef<Device>,
> > +    callback: unsafe extern "C" fn(*mut c_void),
> >      #[pin]
> >      data: Revocable<T>,
> >  }
> > @@ -98,6 +102,8 @@ impl<T> DevresInner<T> {
> >      fn new(dev: &Device, data: T, flags: Flags) -> Result<Arc<DevresInner<T>>> {
> >          let inner = Arc::pin_init(
> >              pin_init!( DevresInner {
> > +                dev: dev.into(),
> > +                callback: Self::devres_callback,
> >                  data <- Revocable::new(data),
> >              }),
> >              flags,
> > @@ -109,9 +115,8 @@ fn new(dev: &Device, data: T, flags: Flags) -> Result<Arc<DevresInner<T>>> {
> >  
> >          // 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(Self::devres_callback), data as _)
> > -        };
> > +        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
> > @@ -124,6 +129,41 @@ fn new(dev: &Device, data: T, flags: Flags) -> Result<Arc<DevresInner<T>>> {
> >          Ok(inner)
> >      }
> >  
> > +    fn as_ptr(&self) -> *const Self {
> > +        self as _
> > +    }
> > +
> > +    fn remove_action(&self) {
> > +        // 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 ret = unsafe {
> > +            bindings::devm_remove_action_nowarn(
> > +                self.dev.as_raw(),
> > +                Some(self.callback),
> > +                self.as_ptr() as _,
> > +            )
> > +        };
> > +
> > +        if ret != 0 {
> > +            // The devres action has been released already - nothing to do.
> > +            return;
> > +        }
> > +
> > +        // 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(self.as_ptr()) };
> > +
> > +        // Revoke the data, such that it gets dropped and the actual resource is freed.
> > +        //
> > +        // SAFETY: When `drop` runs, it's guaranteed that nobody is accessing the revocable data
> > +        // anymore, hence it is safe not to wait for the grace period to finish.
> > +        unsafe { self.data.revoke_nosync() };
> 
> With this patch, this shouldn't be needed any longer -- forgot to remove it.

Ok, I'll drop this series and wait for v2.

thanks,

greg k-h