Message-ID: <Z3wKFTkmjdaGHKIn@tardis.local>
Date: Mon, 6 Jan 2025 08:51:33 -0800
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,
	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: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?

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