[PATCH v2] rust: devres: fix leaking call to devm_add_action()

[PATCH v2] rust: devres: fix leaking call to devm_add_action()

[Danilo Krummrich]

When the data argument of Devres::new() is Err(), we leak the preceding
call to devm_add_action().

In order to fix this, call devm_add_action() in a unit type initializer in
try_pin_init!() after the initializers of all other fields.

Fixes: f5d3ef25d238 ("rust: devres: get rid of Devres' inner Arc")
Signed-off-by: Danilo Krummrich <dakr@kernel.org>
---
Changes in v2:
  - Drop inner in-place when devm_add_action() fails.
  - Document to remove drop_in_place() once we can switch to UnsafePinned.
---
 rust/kernel/devres.rs | 27 ++++++++++++++++++---------
 1 file changed, 18 insertions(+), 9 deletions(-)

diff --git a/rust/kernel/devres.rs b/rust/kernel/devres.rs
index da18091143a6..d04e3fcebafb 100644
--- a/rust/kernel/devres.rs
+++ b/rust/kernel/devres.rs
@@ -115,10 +115,11 @@ pub struct Devres<T: Send> {
     /// Contains all the fields shared with [`Self::callback`].
     // TODO: Replace with `UnsafePinned`, once available.
     //
-    // Subsequently, the `drop_in_place()` in `Devres::drop` and the explicit `Send` and `Sync'
-    // impls can be removed.
+    // Subsequently, the `drop_in_place()` in `Devres::drop` and `Devres::new` as well as the
+    // explicit `Send` and `Sync' impls can be removed.
     #[pin]
     inner: Opaque<Inner<T>>,
+    _add_action: (),
 }
 
 impl<T: Send> Devres<T> {
@@ -140,7 +141,15 @@ pub fn new<'a, E>(
             dev: dev.into(),
             callback,
             // INVARIANT: `inner` is properly initialized.
-            inner <- {
+            inner <- Opaque::pin_init(try_pin_init!(Inner {
+                    devm <- Completion::new(),
+                    revoke <- Completion::new(),
+                    data <- Revocable::new(data),
+            })),
+            // TODO: Replace with "initializer code blocks" [1] once available.
+            //
+            // [1] https://github.com/Rust-for-Linux/pin-init/pull/69
+            _add_action: {
                 // SAFETY: `this` is a valid pointer to uninitialized memory.
                 let inner = unsafe { &raw mut (*this.as_ptr()).inner };
 
@@ -152,13 +161,13 @@ pub fn new<'a, E>(
                 //    live at least as long as the returned `impl PinInit<Self, Error>`.
                 to_result(unsafe {
                     bindings::devm_add_action(dev.as_raw(), Some(callback), inner.cast())
-                })?;
+                }).inspect_err(|_| {
+                    let inner = Opaque::cast_into(inner);
 
-                Opaque::pin_init(try_pin_init!(Inner {
-                    devm <- Completion::new(),
-                    revoke <- Completion::new(),
-                    data <- Revocable::new(data),
-                }))
+                    // SAFETY: `inner` is a valid pointer to an `Inner<T>` and valid for both reads
+                    // and writes.
+                    unsafe { core::ptr::drop_in_place(inner) };
+                })?;
             },
         })
     }

base-commit: 8f5ae30d69d7543eee0d70083daf4de8fe15d585
-- 
2.50.1

Re: [PATCH v2] rust: devres: fix leaking call to devm_add_action()

[Benno Lossin]

On Tue Aug 12, 2025 at 3:09 PM CEST, Danilo Krummrich wrote:
> When the data argument of Devres::new() is Err(), we leak the preceding
> call to devm_add_action().
>
> In order to fix this, call devm_add_action() in a unit type initializer in
> try_pin_init!() after the initializers of all other fields.
>
> Fixes: f5d3ef25d238 ("rust: devres: get rid of Devres' inner Arc")
> Signed-off-by: Danilo Krummrich <dakr@kernel.org>

Reviewed-by: Benno Lossin <lossin@kernel.org>

---
Cheers,
Benno

> ---
> Changes in v2:
>   - Drop inner in-place when devm_add_action() fails.
>   - Document to remove drop_in_place() once we can switch to UnsafePinned.
> ---
>  rust/kernel/devres.rs | 27 ++++++++++++++++++---------
>  1 file changed, 18 insertions(+), 9 deletions(-)

Re: [PATCH v2] rust: devres: fix leaking call to devm_add_action()

[Alice Ryhl]

On Tue, Aug 12, 2025 at 03:09:06PM +0200, Danilo Krummrich wrote:
> When the data argument of Devres::new() is Err(), we leak the preceding
> call to devm_add_action().
> 
> In order to fix this, call devm_add_action() in a unit type initializer in
> try_pin_init!() after the initializers of all other fields.
> 
> Fixes: f5d3ef25d238 ("rust: devres: get rid of Devres' inner Arc")
> Signed-off-by: Danilo Krummrich <dakr@kernel.org>

This looks ok:

Reviewed-by: Alice Ryhl <aliceryhl@google.com>

But can't we do it like this instead?
1. Allocate devm job.
2. Initialize inner field.
3. Use allocation from (1.) to devm_add_action() infallibly.

This way, there's no risk that the inner value may get dropped, which
could be an expensive operation.

Alice

Re: [PATCH v2] rust: devres: fix leaking call to devm_add_action()

[Danilo Krummrich]

On Wed Aug 13, 2025 at 9:16 AM CEST, Alice Ryhl wrote:
> On Tue, Aug 12, 2025 at 03:09:06PM +0200, Danilo Krummrich wrote:
>> When the data argument of Devres::new() is Err(), we leak the preceding
>> call to devm_add_action().
>> 
>> In order to fix this, call devm_add_action() in a unit type initializer in
>> try_pin_init!() after the initializers of all other fields.
>> 
>> Fixes: f5d3ef25d238 ("rust: devres: get rid of Devres' inner Arc")
>> Signed-off-by: Danilo Krummrich <dakr@kernel.org>
>
> This looks ok:
>
> Reviewed-by: Alice Ryhl <aliceryhl@google.com>
>
> But can't we do it like this instead?
> 1. Allocate devm job.
> 2. Initialize inner field.
> 3. Use allocation from (1.) to devm_add_action() infallibly.

Theoretically, we could with a few additions to the C API. But I don't think
it's worth and I don't think we should do it in the context of this patch.

> This way, there's no risk that the inner value may get dropped, which
> could be an expensive operation.

If we actually fail to allocate a devres node on the C side, I'm not that
concerned about having to drop data.

However, there's also another reason why I think there's no need to consider it
now: I still have the rework on my list to get devres callbacks in place such
that we can first revoke the Revocable objects of all corresponding Devres
objects, call synchronize_rcu() once, and then drop the contained data in-place.

In this context I also plan to directly embedd a struct devres_node in the Rust
Devres type, such that the *only* allocation that remains is the final one when
the user of Devres allocates for the final impl PinInit, that directly or
indirectly contains the Devres.

Once we have that, adding the devres node will also always be infallible.

Re: [PATCH v2] rust: devres: fix leaking call to devm_add_action()

[Danilo Krummrich]

On Tue Aug 12, 2025 at 3:09 PM CEST, Danilo Krummrich wrote:
> When the data argument of Devres::new() is Err(), we leak the preceding
> call to devm_add_action().
>
> In order to fix this, call devm_add_action() in a unit type initializer in
> try_pin_init!() after the initializers of all other fields.
>
> Fixes: f5d3ef25d238 ("rust: devres: get rid of Devres' inner Arc")
> Signed-off-by: Danilo Krummrich <dakr@kernel.org>

Applied to driver-core-linus, thanks!

Re: [PATCH v2] rust: devres: fix leaking call to devm_add_action()

[Alice Ryhl]

On Wed, Aug 13, 2025 at 05:07:41PM +0200, Danilo Krummrich wrote:
> On Wed Aug 13, 2025 at 9:16 AM CEST, Alice Ryhl wrote:
> > On Tue, Aug 12, 2025 at 03:09:06PM +0200, Danilo Krummrich wrote:
> >> When the data argument of Devres::new() is Err(), we leak the preceding
> >> call to devm_add_action().
> >> 
> >> In order to fix this, call devm_add_action() in a unit type initializer in
> >> try_pin_init!() after the initializers of all other fields.
> >> 
> >> Fixes: f5d3ef25d238 ("rust: devres: get rid of Devres' inner Arc")
> >> Signed-off-by: Danilo Krummrich <dakr@kernel.org>
> >
> > This looks ok:
> >
> > Reviewed-by: Alice Ryhl <aliceryhl@google.com>
> >
> > But can't we do it like this instead?
> > 1. Allocate devm job.
> > 2. Initialize inner field.
> > 3. Use allocation from (1.) to devm_add_action() infallibly.
> 
> Theoretically, we could with a few additions to the C API. But I don't think
> it's worth and I don't think we should do it in the context of this patch.
> 
> > This way, there's no risk that the inner value may get dropped, which
> > could be an expensive operation.
> 
> If we actually fail to allocate a devres node on the C side, I'm not that
> concerned about having to drop data.
> 
> However, there's also another reason why I think there's no need to consider it
> now: I still have the rework on my list to get devres callbacks in place such
> that we can first revoke the Revocable objects of all corresponding Devres
> objects, call synchronize_rcu() once, and then drop the contained data in-place.
> 
> In this context I also plan to directly embedd a struct devres_node in the Rust
> Devres type, such that the *only* allocation that remains is the final one when
> the user of Devres allocates for the final impl PinInit, that directly or
> indirectly contains the Devres.
> 
> Once we have that, adding the devres node will also always be infallible.

Embedding devres_node in Devres sounds look a good plan.

Alice