Message-ID: <Z3_pTK4_CsvpF8cX@boqun-archlinux>
Date: Thu, 9 Jan 2025 07:20:44 -0800
From: Boqun Feng <boqun.feng@...il.com>
To: Danilo Krummrich <dakr@...nel.org>, 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 Thu, Jan 09, 2025 at 10:50:38AM +0100, Simona Vetter wrote:
> On Wed, Jan 08, 2025 at 02:53:23PM +0100, Simona Vetter wrote:
> > On Tue, Jan 07, 2025 at 10:49:40AM +0100, Danilo Krummrich wrote:
> > > 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!
> > 
> > Just for my own learning I've tried to understand why there's an issue
> > here, but no in DevresInner.devres_callback. In both cases we take the

I also learned this in a hard way ;-) let me try to explain my
understanding.

First the difference between here and `DevresInner::devres_callback()`
is "how the pointer was derived" or "where did the pointer come from".

* In `DevresInner::devres_callback()` case, it uses a pointer value
  directly, and that value came from a previous `Arc::into_raw()` which
  uses the pointer directly with an offset (note that offsetting doesn't
  change the pointer provenance), so that pointer has the same
  provenance as an `Arc`.

* In here, what we did was getting a pointer to `DevresInner` from a
  `&DevresInner` and that pointer doesn't have the provenane to the
  whole `ArcInner<DervesInner>`, nor does it have the write permission
  because it comes from an immutable reference.

> > exact same bag of bits and convert it into an Arc, relying on the C side
> > guaranteeing to us that we exclusively own whatever object that bag of
> > bits points to when converted into a real reference.
> > 
> > I don't think we rely on the provance of self here at all, because we just
> > pass that bag of bits to devm_remove_action_nowarn as a magic lookup key,
> > and in the ret == 0 case the C side guarantee is that we own the resulting
> > object if we convert it into one using Arc::from_raw.
> > 

Other than `Arc::from_raw()` there is another side in this picture: the
one that provides the immutable reference, so if you do:

    let p: &Arc<T> = ...; // say you have an `Arc<T>` reference.
    let o = p.deref();    // you got a reference to the internal object.

    let ptr = o as *const T;
    let new_arc = Arc::from_raw(ptr); // use the pointer of `o` to
                                      // construct a new Arc. Even if
				      // there is a spare refcount some
				      // where due to a Arc::into_raw()
				      // previously.
    do_something(new_arc);

    use(p); // here the compiler is within its right to assume if there
            // is no other access the underlying `Arc<T>`, the refcount
	    // should be unchanged (more precisely, the whole `ArcInner`
	    // should be unchanged).

And by voliating pointer provenance, the assumption is broken and there
is no way to tell compiler about that (at least in the current
operational semantics of Rust IIUC). Intuitively, `o` only has the
permission to read the object, so we cannot use it for construct an
`Arc`.

This to me is how pointer provenance actually works: it's a model that
describes how compilers can make assumptions so that programmers and
compilers have the same picture about whether there could be an
optimization based on an assumption.

Does this make sense to you?

Regards,
Boqun

> > I think you could replace self.as_ptr in this function with a random bit
> > value, and aside from being functionally nonsense and resulting in a
> > randomized leak on the C side I dont think it would be unsafe/unsound.
> > 
> > What am I missing?
> 
> Trying to sharpen my argument a bit after chatting with Danilo on irc:
> 
> My take is that Arc::from_raw must discard provenance of the argument, and
> instead entirely relies on Arc::into_raw having been call beforehand with
> a return value that bitwise matches what we stuff into from_raw. So it
> should inherit the provenance of the reference we passed to Arc::into_raw.
> 
> If that's not the case then from_raw is busted, and it only happens to
> work for ffi callbacks because the compiler cannot see behind the ffi
> curtain.
> -Sima
> 
> > 
> > Cheers, Sima
> > 
> > > 
> > > > 
> > > > 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
> > > > > 
> > 
> > -- 
> > Simona Vetter
> > Software Engineer, Intel Corporation
> > http://blog.ffwll.ch
> 
> -- 
> Simona Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch

Powered by blists - more mailing lists