[PATCH 1/2] devres: add devm_remove_action_nowarn()

[PATCH 1/2] devres: add devm_remove_action_nowarn()

[Danilo Krummrich]

devm_remove_action() warns if the action to remove does not exist
(anymore).

The Rust devres abstraction, however, has a use-case to call
devm_remove_action() at a point where it can't be guaranteed that the
corresponding action hasn't been released yet.

In particular, an instance of `Devres<T>` may be dropped after the
action has been released. So far, `Devres<T>` worked around this by
keeping the inner type alive.

Hence, add devm_remove_action_nowarn(), which returns an error code if
the action has been removed already.

A subsequent patch uses devm_remove_action_nowarn() to remove the action
when `Devres<T>` is dropped.

Signed-off-by: Danilo Krummrich <dakr@kernel.org>
---
 drivers/base/devres.c  | 17 ++++++++++++-----
 include/linux/device.h | 18 +++++++++++++++++-
 2 files changed, 29 insertions(+), 6 deletions(-)

diff --git a/drivers/base/devres.c b/drivers/base/devres.c
index 2152eec0c135..d59b8078fc33 100644
--- a/drivers/base/devres.c
+++ b/drivers/base/devres.c
@@ -750,25 +750,32 @@ int __devm_add_action(struct device *dev, void (*action)(void *), void *data, co
 EXPORT_SYMBOL_GPL(__devm_add_action);
 
 /**
- * devm_remove_action() - removes previously added custom action
+ * devm_remove_action_nowarn() - removes previously added custom action
  * @dev: Device that owns the action
  * @action: Function implementing the action
  * @data: Pointer to data passed to @action implementation
  *
  * Removes instance of @action previously added by devm_add_action().
  * Both action and data should match one of the existing entries.
+ *
+ * In contrast to devm_remove_action(), this function does not WARN() if no
+ * entry could have been found.
+ *
+ * Returns: 0 on success, -ENOENT if no entry could have been found.
  */
-void devm_remove_action(struct device *dev, void (*action)(void *), void *data)
+int devm_remove_action_nowarn(struct device *dev,
+			      void (*action)(void *),
+			      void *data)
 {
 	struct action_devres devres = {
 		.data = data,
 		.action = action,
 	};
 
-	WARN_ON(devres_destroy(dev, devm_action_release, devm_action_match,
-			       &devres));
+	return devres_destroy(dev, devm_action_release, devm_action_match,
+			      &devres);
 }
-EXPORT_SYMBOL_GPL(devm_remove_action);
+EXPORT_SYMBOL_GPL(devm_remove_action_nowarn);
 
 /**
  * devm_release_action() - release previously added custom action
diff --git a/include/linux/device.h b/include/linux/device.h
index 667cb6db9019..6879d5e8ac3d 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -399,7 +399,23 @@ void __iomem *devm_of_iomap(struct device *dev,
 #endif
 
 /* allows to add/remove a custom action to devres stack */
-void devm_remove_action(struct device *dev, void (*action)(void *), void *data);
+int devm_remove_action_nowarn(struct device *dev, void (*action)(void *), void *data);
+
+/**
+ * devm_remove_action() - removes previously added custom action
+ * @dev: Device that owns the action
+ * @action: Function implementing the action
+ * @data: Pointer to data passed to @action implementation
+ *
+ * Removes instance of @action previously added by devm_add_action().
+ * Both action and data should match one of the existing entries.
+ */
+static inline
+void devm_remove_action(struct device *dev, void (*action)(void *), void *data)
+{
+	WARN_ON(devm_remove_action_nowarn(dev, action, data));
+}
+
 void devm_release_action(struct device *dev, void (*action)(void *), void *data);
 
 int __devm_add_action(struct device *dev, void (*action)(void *), void *data, const char *name);

base-commit: 06e843bbbf2107463249ea6f6b1a736f5647e24a
-- 
2.47.1

[PATCH 2/2] rust: devres: remove action in `Devres::drop`

[Danilo Krummrich]

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@kernel.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() };
+    }
+
     #[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

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

[Danilo Krummrich]

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@kernel.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.

> +    }
> +
>      #[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
> 

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

[Greg KH]

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@kernel.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

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

[Gary Guo]

On Fri, 3 Jan 2025 17:58:37 +0100
Danilo Krummrich <dakr@kernel.org> 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@kernel.org>

Hi Danilo,

Thanks for sending this.

Feel free to add

	Reviewed-by: Gary Guo <gary@garyguo.net>

for both patches with the `revoke_nosync` removed as you mentioned.

Best,
Gary

> > ---
> >  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.
> 
> > +    }
> > +
> >      #[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
> >   

Re: [PATCH 1/2] devres: add devm_remove_action_nowarn()

[Simona Vetter]

On Fri, Jan 03, 2025 at 05:44:30PM +0100, Danilo Krummrich wrote:
> devm_remove_action() warns if the action to remove does not exist
> (anymore).
> 
> The Rust devres abstraction, however, has a use-case to call
> devm_remove_action() at a point where it can't be guaranteed that the
> corresponding action hasn't been released yet.
> 
> In particular, an instance of `Devres<T>` may be dropped after the
> action has been released. So far, `Devres<T>` worked around this by
> keeping the inner type alive.
> 
> Hence, add devm_remove_action_nowarn(), which returns an error code if
> the action has been removed already.
> 
> A subsequent patch uses devm_remove_action_nowarn() to remove the action
> when `Devres<T>` is dropped.
> 
> Signed-off-by: Danilo Krummrich <dakr@kernel.org>
> ---
>  drivers/base/devres.c  | 17 ++++++++++++-----
>  include/linux/device.h | 18 +++++++++++++++++-
>  2 files changed, 29 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/base/devres.c b/drivers/base/devres.c
> index 2152eec0c135..d59b8078fc33 100644
> --- a/drivers/base/devres.c
> +++ b/drivers/base/devres.c
> @@ -750,25 +750,32 @@ int __devm_add_action(struct device *dev, void (*action)(void *), void *data, co
>  EXPORT_SYMBOL_GPL(__devm_add_action);
>  
>  /**
> - * devm_remove_action() - removes previously added custom action
> + * devm_remove_action_nowarn() - removes previously added custom action
>   * @dev: Device that owns the action
>   * @action: Function implementing the action
>   * @data: Pointer to data passed to @action implementation
>   *
>   * Removes instance of @action previously added by devm_add_action().
>   * Both action and data should match one of the existing entries.
> + *
> + * In contrast to devm_remove_action(), this function does not WARN() if no
> + * entry could have been found.

I'd put a caution here that most likely, using this is a bad idea. Maybe
something like:

"This should only be used if the action is contained in an object with
independent lifetime management, like the Devres rust abstraction.
Anywhere is the warning most likely indicates a driver bug."

At least I really can't come up with a reasonable design in a C driver
that would ever need this.

Cheers, Sima

> + *
> + * Returns: 0 on success, -ENOENT if no entry could have been found.
>   */
> -void devm_remove_action(struct device *dev, void (*action)(void *), void *data)
> +int devm_remove_action_nowarn(struct device *dev,
> +			      void (*action)(void *),
> +			      void *data)
>  {
>  	struct action_devres devres = {
>  		.data = data,
>  		.action = action,
>  	};
>  
> -	WARN_ON(devres_destroy(dev, devm_action_release, devm_action_match,
> -			       &devres));
> +	return devres_destroy(dev, devm_action_release, devm_action_match,
> +			      &devres);
>  }
> -EXPORT_SYMBOL_GPL(devm_remove_action);
> +EXPORT_SYMBOL_GPL(devm_remove_action_nowarn);
>  
>  /**
>   * devm_release_action() - release previously added custom action
> diff --git a/include/linux/device.h b/include/linux/device.h
> index 667cb6db9019..6879d5e8ac3d 100644
> --- a/include/linux/device.h
> +++ b/include/linux/device.h
> @@ -399,7 +399,23 @@ void __iomem *devm_of_iomap(struct device *dev,
>  #endif
>  
>  /* allows to add/remove a custom action to devres stack */
> -void devm_remove_action(struct device *dev, void (*action)(void *), void *data);
> +int devm_remove_action_nowarn(struct device *dev, void (*action)(void *), void *data);
> +
> +/**
> + * devm_remove_action() - removes previously added custom action
> + * @dev: Device that owns the action
> + * @action: Function implementing the action
> + * @data: Pointer to data passed to @action implementation
> + *
> + * Removes instance of @action previously added by devm_add_action().
> + * Both action and data should match one of the existing entries.
> + */
> +static inline
> +void devm_remove_action(struct device *dev, void (*action)(void *), void *data)
> +{
> +	WARN_ON(devm_remove_action_nowarn(dev, action, data));
> +}
> +
>  void devm_release_action(struct device *dev, void (*action)(void *), void *data);
>  
>  int __devm_add_action(struct device *dev, void (*action)(void *), void *data, const char *name);
> 
> base-commit: 06e843bbbf2107463249ea6f6b1a736f5647e24a
> -- 
> 2.47.1
> 

-- 
Simona Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

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

[Boqun Feng]

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@kernel.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
> 

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

[Danilo Krummrich]

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@kernel.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
> > 

Re: [PATCH 1/2] devres: add devm_remove_action_nowarn()

[Danilo Krummrich]

On Mon, Jan 06, 2025 at 12:47:52PM +0100, Simona Vetter wrote:
> On Fri, Jan 03, 2025 at 05:44:30PM +0100, Danilo Krummrich wrote:
> > devm_remove_action() warns if the action to remove does not exist
> > (anymore).
> > 
> > The Rust devres abstraction, however, has a use-case to call
> > devm_remove_action() at a point where it can't be guaranteed that the
> > corresponding action hasn't been released yet.
> > 
> > In particular, an instance of `Devres<T>` may be dropped after the
> > action has been released. So far, `Devres<T>` worked around this by
> > keeping the inner type alive.
> > 
> > Hence, add devm_remove_action_nowarn(), which returns an error code if
> > the action has been removed already.
> > 
> > A subsequent patch uses devm_remove_action_nowarn() to remove the action
> > when `Devres<T>` is dropped.
> > 
> > Signed-off-by: Danilo Krummrich <dakr@kernel.org>
> > ---
> >  drivers/base/devres.c  | 17 ++++++++++++-----
> >  include/linux/device.h | 18 +++++++++++++++++-
> >  2 files changed, 29 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/base/devres.c b/drivers/base/devres.c
> > index 2152eec0c135..d59b8078fc33 100644
> > --- a/drivers/base/devres.c
> > +++ b/drivers/base/devres.c
> > @@ -750,25 +750,32 @@ int __devm_add_action(struct device *dev, void (*action)(void *), void *data, co
> >  EXPORT_SYMBOL_GPL(__devm_add_action);
> >  
> >  /**
> > - * devm_remove_action() - removes previously added custom action
> > + * devm_remove_action_nowarn() - removes previously added custom action
> >   * @dev: Device that owns the action
> >   * @action: Function implementing the action
> >   * @data: Pointer to data passed to @action implementation
> >   *
> >   * Removes instance of @action previously added by devm_add_action().
> >   * Both action and data should match one of the existing entries.
> > + *
> > + * In contrast to devm_remove_action(), this function does not WARN() if no
> > + * entry could have been found.
> 
> I'd put a caution here that most likely, using this is a bad idea. Maybe
> something like:
> 
> "This should only be used if the action is contained in an object with
> independent lifetime management, like the Devres rust abstraction.
> Anywhere is the warning most likely indicates a driver bug."

Yes, I thought of something similar too, but wasn't quite sure if it's needed.
At least for me, if something has the postfix "nowarn", it already makes me
wonder if I should really use it.

I'll add a paragraph.

> 
> At least I really can't come up with a reasonable design in a C driver
> that would ever need this.

I tried, but couldn't either. The only thing I could think of was a revocable
thing in C.

> 
> Cheers, Sima
> 
> > + *
> > + * Returns: 0 on success, -ENOENT if no entry could have been found.
> >   */
> > -void devm_remove_action(struct device *dev, void (*action)(void *), void *data)
> > +int devm_remove_action_nowarn(struct device *dev,
> > +			      void (*action)(void *),
> > +			      void *data)
> >  {
> >  	struct action_devres devres = {
> >  		.data = data,
> >  		.action = action,
> >  	};
> >  
> > -	WARN_ON(devres_destroy(dev, devm_action_release, devm_action_match,
> > -			       &devres));
> > +	return devres_destroy(dev, devm_action_release, devm_action_match,
> > +			      &devres);
> >  }
> > -EXPORT_SYMBOL_GPL(devm_remove_action);
> > +EXPORT_SYMBOL_GPL(devm_remove_action_nowarn);
> >  
> >  /**
> >   * devm_release_action() - release previously added custom action
> > diff --git a/include/linux/device.h b/include/linux/device.h
> > index 667cb6db9019..6879d5e8ac3d 100644
> > --- a/include/linux/device.h
> > +++ b/include/linux/device.h
> > @@ -399,7 +399,23 @@ void __iomem *devm_of_iomap(struct device *dev,
> >  #endif
> >  
> >  /* allows to add/remove a custom action to devres stack */
> > -void devm_remove_action(struct device *dev, void (*action)(void *), void *data);
> > +int devm_remove_action_nowarn(struct device *dev, void (*action)(void *), void *data);
> > +
> > +/**
> > + * devm_remove_action() - removes previously added custom action
> > + * @dev: Device that owns the action
> > + * @action: Function implementing the action
> > + * @data: Pointer to data passed to @action implementation
> > + *
> > + * Removes instance of @action previously added by devm_add_action().
> > + * Both action and data should match one of the existing entries.
> > + */
> > +static inline
> > +void devm_remove_action(struct device *dev, void (*action)(void *), void *data)
> > +{
> > +	WARN_ON(devm_remove_action_nowarn(dev, action, data));
> > +}
> > +
> >  void devm_release_action(struct device *dev, void (*action)(void *), void *data);
> >  
> >  int __devm_add_action(struct device *dev, void (*action)(void *), void *data, const char *name);
> > 
> > base-commit: 06e843bbbf2107463249ea6f6b1a736f5647e24a
> > -- 
> > 2.47.1
> > 
> 
> -- 
> Simona Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch

Re: [PATCH 1/2] devres: add devm_remove_action_nowarn()

[Alice Ryhl]

On Tue, Jan 7, 2025 at 11:05 AM Danilo Krummrich <dakr@kernel.org> wrote:
>
> On Mon, Jan 06, 2025 at 12:47:52PM +0100, Simona Vetter wrote:
> > On Fri, Jan 03, 2025 at 05:44:30PM +0100, Danilo Krummrich wrote:
> > > devm_remove_action() warns if the action to remove does not exist
> > > (anymore).
> > >
> > > The Rust devres abstraction, however, has a use-case to call
> > > devm_remove_action() at a point where it can't be guaranteed that the
> > > corresponding action hasn't been released yet.
> > >
> > > In particular, an instance of `Devres<T>` may be dropped after the
> > > action has been released. So far, `Devres<T>` worked around this by
> > > keeping the inner type alive.
> > >
> > > Hence, add devm_remove_action_nowarn(), which returns an error code if
> > > the action has been removed already.
> > >
> > > A subsequent patch uses devm_remove_action_nowarn() to remove the action
> > > when `Devres<T>` is dropped.
> > >
> > > Signed-off-by: Danilo Krummrich <dakr@kernel.org>
> > > ---
> > >  drivers/base/devres.c  | 17 ++++++++++++-----
> > >  include/linux/device.h | 18 +++++++++++++++++-
> > >  2 files changed, 29 insertions(+), 6 deletions(-)
> > >
> > > diff --git a/drivers/base/devres.c b/drivers/base/devres.c
> > > index 2152eec0c135..d59b8078fc33 100644
> > > --- a/drivers/base/devres.c
> > > +++ b/drivers/base/devres.c
> > > @@ -750,25 +750,32 @@ int __devm_add_action(struct device *dev, void (*action)(void *), void *data, co
> > >  EXPORT_SYMBOL_GPL(__devm_add_action);
> > >
> > >  /**
> > > - * devm_remove_action() - removes previously added custom action
> > > + * devm_remove_action_nowarn() - removes previously added custom action
> > >   * @dev: Device that owns the action
> > >   * @action: Function implementing the action
> > >   * @data: Pointer to data passed to @action implementation
> > >   *
> > >   * Removes instance of @action previously added by devm_add_action().
> > >   * Both action and data should match one of the existing entries.
> > > + *
> > > + * In contrast to devm_remove_action(), this function does not WARN() if no
> > > + * entry could have been found.
> >
> > I'd put a caution here that most likely, using this is a bad idea. Maybe
> > something like:
> >
> > "This should only be used if the action is contained in an object with
> > independent lifetime management, like the Devres rust abstraction.
> > Anywhere is the warning most likely indicates a driver bug."
>
> Yes, I thought of something similar too, but wasn't quite sure if it's needed.
> At least for me, if something has the postfix "nowarn", it already makes me
> wonder if I should really use it.
>
> I'll add a paragraph.
>
> >
> > At least I really can't come up with a reasonable design in a C driver
> > that would ever need this.
>
> I tried, but couldn't either. The only thing I could think of was a revocable
> thing in C.

Potentially if there are two cleanup paths that could run in parallel,
they could use this to avoid needing to synchronize which one removes
it?

Alice

Re: [PATCH 1/2] devres: add devm_remove_action_nowarn()

[Danilo Krummrich]

On Tue, Jan 07, 2025 at 11:11:20AM +0100, Alice Ryhl wrote:
> On Tue, Jan 7, 2025 at 11:05 AM Danilo Krummrich <dakr@kernel.org> wrote:
> >
> > On Mon, Jan 06, 2025 at 12:47:52PM +0100, Simona Vetter wrote:
> > > On Fri, Jan 03, 2025 at 05:44:30PM +0100, Danilo Krummrich wrote:
> > > > devm_remove_action() warns if the action to remove does not exist
> > > > (anymore).
> > > >
> > > > The Rust devres abstraction, however, has a use-case to call
> > > > devm_remove_action() at a point where it can't be guaranteed that the
> > > > corresponding action hasn't been released yet.
> > > >
> > > > In particular, an instance of `Devres<T>` may be dropped after the
> > > > action has been released. So far, `Devres<T>` worked around this by
> > > > keeping the inner type alive.
> > > >
> > > > Hence, add devm_remove_action_nowarn(), which returns an error code if
> > > > the action has been removed already.
> > > >
> > > > A subsequent patch uses devm_remove_action_nowarn() to remove the action
> > > > when `Devres<T>` is dropped.
> > > >
> > > > Signed-off-by: Danilo Krummrich <dakr@kernel.org>
> > > > ---
> > > >  drivers/base/devres.c  | 17 ++++++++++++-----
> > > >  include/linux/device.h | 18 +++++++++++++++++-
> > > >  2 files changed, 29 insertions(+), 6 deletions(-)
> > > >
> > > > diff --git a/drivers/base/devres.c b/drivers/base/devres.c
> > > > index 2152eec0c135..d59b8078fc33 100644
> > > > --- a/drivers/base/devres.c
> > > > +++ b/drivers/base/devres.c
> > > > @@ -750,25 +750,32 @@ int __devm_add_action(struct device *dev, void (*action)(void *), void *data, co
> > > >  EXPORT_SYMBOL_GPL(__devm_add_action);
> > > >
> > > >  /**
> > > > - * devm_remove_action() - removes previously added custom action
> > > > + * devm_remove_action_nowarn() - removes previously added custom action
> > > >   * @dev: Device that owns the action
> > > >   * @action: Function implementing the action
> > > >   * @data: Pointer to data passed to @action implementation
> > > >   *
> > > >   * Removes instance of @action previously added by devm_add_action().
> > > >   * Both action and data should match one of the existing entries.
> > > > + *
> > > > + * In contrast to devm_remove_action(), this function does not WARN() if no
> > > > + * entry could have been found.
> > >
> > > I'd put a caution here that most likely, using this is a bad idea. Maybe
> > > something like:
> > >
> > > "This should only be used if the action is contained in an object with
> > > independent lifetime management, like the Devres rust abstraction.
> > > Anywhere is the warning most likely indicates a driver bug."
> >
> > Yes, I thought of something similar too, but wasn't quite sure if it's needed.
> > At least for me, if something has the postfix "nowarn", it already makes me
> > wonder if I should really use it.
> >
> > I'll add a paragraph.
> >
> > >
> > > At least I really can't come up with a reasonable design in a C driver
> > > that would ever need this.
> >
> > I tried, but couldn't either. The only thing I could think of was a revocable
> > thing in C.
> 
> Potentially if there are two cleanup paths that could run in parallel,
> they could use this to avoid needing to synchronize which one removes
> it?

Yeah, I also though if I can make up such a case. But I think the real issue is
that even if we can find one, it's probably an abuse of devres.

Devres is there to indicate that the driver was unbound from the device, which
causes remove() for the driver to cleanup. So, rather than removing the action
from some async path, we can just wait for remove() to clean up. The only
exception can hence be probe().

> 
> Alice

Re: [PATCH 1/2] devres: add devm_remove_action_nowarn()

[Simona Vetter]

On Tue, Jan 07, 2025 at 11:23:23AM +0100, Danilo Krummrich wrote:
> On Tue, Jan 07, 2025 at 11:11:20AM +0100, Alice Ryhl wrote:
> > On Tue, Jan 7, 2025 at 11:05 AM Danilo Krummrich <dakr@kernel.org> wrote:
> > >
> > > On Mon, Jan 06, 2025 at 12:47:52PM +0100, Simona Vetter wrote:
> > > > On Fri, Jan 03, 2025 at 05:44:30PM +0100, Danilo Krummrich wrote:
> > > > > devm_remove_action() warns if the action to remove does not exist
> > > > > (anymore).
> > > > >
> > > > > The Rust devres abstraction, however, has a use-case to call
> > > > > devm_remove_action() at a point where it can't be guaranteed that the
> > > > > corresponding action hasn't been released yet.
> > > > >
> > > > > In particular, an instance of `Devres<T>` may be dropped after the
> > > > > action has been released. So far, `Devres<T>` worked around this by
> > > > > keeping the inner type alive.
> > > > >
> > > > > Hence, add devm_remove_action_nowarn(), which returns an error code if
> > > > > the action has been removed already.
> > > > >
> > > > > A subsequent patch uses devm_remove_action_nowarn() to remove the action
> > > > > when `Devres<T>` is dropped.
> > > > >
> > > > > Signed-off-by: Danilo Krummrich <dakr@kernel.org>
> > > > > ---
> > > > >  drivers/base/devres.c  | 17 ++++++++++++-----
> > > > >  include/linux/device.h | 18 +++++++++++++++++-
> > > > >  2 files changed, 29 insertions(+), 6 deletions(-)
> > > > >
> > > > > diff --git a/drivers/base/devres.c b/drivers/base/devres.c
> > > > > index 2152eec0c135..d59b8078fc33 100644
> > > > > --- a/drivers/base/devres.c
> > > > > +++ b/drivers/base/devres.c
> > > > > @@ -750,25 +750,32 @@ int __devm_add_action(struct device *dev, void (*action)(void *), void *data, co
> > > > >  EXPORT_SYMBOL_GPL(__devm_add_action);
> > > > >
> > > > >  /**
> > > > > - * devm_remove_action() - removes previously added custom action
> > > > > + * devm_remove_action_nowarn() - removes previously added custom action
> > > > >   * @dev: Device that owns the action
> > > > >   * @action: Function implementing the action
> > > > >   * @data: Pointer to data passed to @action implementation
> > > > >   *
> > > > >   * Removes instance of @action previously added by devm_add_action().
> > > > >   * Both action and data should match one of the existing entries.
> > > > > + *
> > > > > + * In contrast to devm_remove_action(), this function does not WARN() if no
> > > > > + * entry could have been found.
> > > >
> > > > I'd put a caution here that most likely, using this is a bad idea. Maybe
> > > > something like:
> > > >
> > > > "This should only be used if the action is contained in an object with
> > > > independent lifetime management, like the Devres rust abstraction.
> > > > Anywhere is the warning most likely indicates a driver bug."
> > >
> > > Yes, I thought of something similar too, but wasn't quite sure if it's needed.
> > > At least for me, if something has the postfix "nowarn", it already makes me
> > > wonder if I should really use it.
> > >
> > > I'll add a paragraph.
> > >
> > > >
> > > > At least I really can't come up with a reasonable design in a C driver
> > > > that would ever need this.
> > >
> > > I tried, but couldn't either. The only thing I could think of was a revocable
> > > thing in C.
> > 
> > Potentially if there are two cleanup paths that could run in parallel,
> > they could use this to avoid needing to synchronize which one removes
> > it?
> 
> Yeah, I also though if I can make up such a case. But I think the real issue is
> that even if we can find one, it's probably an abuse of devres.
> 
> Devres is there to indicate that the driver was unbound from the device, which
> causes remove() for the driver to cleanup. So, rather than removing the action
> from some async path, we can just wait for remove() to clean up. The only
> exception can hence be probe().

Yeah writing a correct ->remove implementation in C is already really
hard, even when you're using devres. If you think you can write one that
runs concurrently with other stuff in a way you need this _nowarn variant,
you're just too dangerous to write driver code in C imo.
-Sima
-- 
Simona Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

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

[Simona Vetter]

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@kernel.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
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.

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?

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

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

[Simona Vetter]

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@kernel.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
> 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.
> 
> 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

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

[Boqun Feng]

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@kernel.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

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

[Simona Vetter]

On Thu, Jan 09, 2025 at 07:20:44AM -0800, Boqun Feng wrote:
> 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@kernel.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.

Yeah I think I got that part of understanding provenance, it's essentially
all the things the compiler keeps track of for optimization passes.

> > > 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?

Yes I think so, but your example above has the crucial issue that we've
picked the wrong raw pointer to pass to from_raw, because it's one that we
didn't get back from into_raw. So with my understanding of this all from
that pov, the from_raw in v1 of this patch is broken.

The issue is that the fix still doesn't get things right, because while we
supply a pointer with enough provenance to hopefully prevent the compiler
from doing bad things, it's still not the pointer we've received from
into_raw.  They happen to bit bit identical ofc, but the entire provenance
thing is about all the stuff besides the raw pointer value.

There's two cases for how I think the provenance is passed on:

Arc::into_raw -> devm_add_action -> devm_callback -> Arc::from_raw

This one is correct, because we pass the right raw pointer into
Arc::from_raw.

Arc::into_raw -> devm_add_action -> (in remove_actio)
devm_remove_action_nowarn -> Arc::from_raw

Now the thing to note is that devm_remove_action_nowarn does not return a
void * with the data we need to clean up, because the void * data happens
to bit-vise identical to the lookup key we pass in. But what it does
return is the provenance of the bitwise identical pointer that was passed
into devm_add_action, and that provenance is the right one because it's
the pointer we got from Arc::into_raw. On the other hand the lookup key
does not have the right provenance, because we did not get that one from
into_raw.

I think there's a few options here:

- we hope the compiler isn't smart enough

- we pass some provenance that should allow us to do all the right access
  as a stand-in, and hope the compiler isn't smart enough. This only works
  if we have a suitable reference we can pass down the call chain to where
  we need it (like is done in v2), and I don't think this is possible in
  all cases.

  I can think of two examples where this might happen:
  - We only get an immutable reference, and the C ffi call is what gives
    us the provenance/proof for write access.
  - We only have a pointer to an inner type (probably gotten from C ffi),
    and the C ffi call does a runtime upcast and so supplies the proof
    that our reference is part of something much bigger.
  Neither of these make sense with pure rust code, but I do think it's
  stuff that can pop up when interfacing kernel C functions.

- we add a fake void * out parameter to the ffi wrapper so that we can
  pretend to get the provenance back from the C side (it's just a copy of
  the lookup key we passed in when ignoring the non-value parts of a
  pointer). Maybe could even do that properly with the new strict
  provenance extensions that rust has, so that it's a true phantom
  value.

- we have a much more magic replacement for into_raw/from_raw which
  magically transfers the provenance behind the scenes. So kinda like
  free() consumes provenance and malloc() creates new provenance (at least
  for mutable references, for immutable ones it's a bit different), except
  the pointer value stays the same and the memory contents of the object
  also stays the same. So something like Arc::into_raw_c_void and
  Arc::from_raw_c_void, since this magic provenance transfer would only
  really make sense for ffi with C code where the void * is just an opaque
  reference that's passed around, and the C code never looks at what's
  behind it. Well except for runtime type casting, where the type is
  probably a bit more specific than void *.

I don't think we can ignore this issue, because using void * as the lookup
key is fairly common in the kernel, and these lookup functions don't
bother with returning that pointer again. But semantically they do return
the provenance for the full object that you pass in and cast to void *,
it's just that C totally sucks at type safety.

Or maybe I'm just really confused about all this still ...

Cheers, Sima


> 
> 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

-- 
Simona Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch