Message-ID: <Z3z4tHzwatjbvz1h@pollux>
Date: Tue, 7 Jan 2025 10:49:40 +0100
From: Danilo Krummrich <dakr@...nel.org>
To: Boqun Feng <boqun.feng@...il.com>
Cc: gregkh@...uxfoundation.org, rafael@...nel.org, ojeda@...nel.org,
	alex.gaynor@...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 Mon, Jan 06, 2025 at 08:51:33AM -0800, Boqun Feng 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()) };
> 
> There is a pointer provenance issue here I think. `self` is a immutable
> reference to `DevresInner<..>`, so the pointer derived from it doesn't
> have the provenance for writing nor does it have the provenance for the
> `refcount` field in `ArcInner`. Therefore it cannot be used to
> reconstruct an `Arc`.
> 
> We probably want to make `remove_action()` take an
> `&Arc<DevresInner<T>>`. Or am I missing something subtle?

Indeed, good catch!

> 
> Regards,
> Boqun
> 
> > +
> > +        // 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() };
> > +    }
> > +
> >      #[allow(clippy::missing_safety_doc)]
> >      unsafe extern "C" fn devres_callback(ptr: *mut kernel::ffi::c_void) {
> >          let ptr = ptr as *mut DevresInner<T>;
> > @@ -165,14 +205,6 @@ fn deref(&self) -> &Self::Target {
> >  
> >  impl<T> Drop for Devres<T> {
> >      fn drop(&mut self) {
> > -        // Revoke the data, such that it gets dropped already and the actual resource is freed.
> > -        //
> > -        // `DevresInner` has to stay alive until the devres callback has been called. This is
> > -        // necessary since we don't know when `Devres` is dropped and calling
> > -        // `devm_remove_action()` instead could race with `devres_release_all()`.
> > -        //
> > -        // 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.revoke_nosync() };
> > +        self.0.remove_action();
> >      }
> >  }
> > -- 
> > 2.47.1
> > 

Powered by blists - more mailing lists