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

[PATCH] 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.

Cc: stable@vger.kernel.org
Fixes: f5d3ef25d238 ("rust: devres: get rid of Devres' inner Arc")
Signed-off-by: Danilo Krummrich <dakr@kernel.org>
---
 rust/kernel/devres.rs | 17 ++++++++++-------
 1 file changed, 10 insertions(+), 7 deletions(-)

diff --git a/rust/kernel/devres.rs b/rust/kernel/devres.rs
index da18091143a6..bfccf4177644 100644
--- a/rust/kernel/devres.rs
+++ b/rust/kernel/devres.rs
@@ -119,6 +119,7 @@ pub struct Devres<T: Send> {
     // 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 };
 
@@ -153,12 +162,6 @@ pub fn new<'a, E>(
                 to_result(unsafe {
                     bindings::devm_add_action(dev.as_raw(), Some(callback), inner.cast())
                 })?;
-
-                Opaque::pin_init(try_pin_init!(Inner {
-                    devm <- Completion::new(),
-                    revoke <- Completion::new(),
-                    data <- Revocable::new(data),
-                }))
             },
         })
     }

base-commit: 8f5ae30d69d7543eee0d70083daf4de8fe15d585
-- 
2.50.1

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

[Benno Lossin]

On Mon Aug 11, 2025 at 11:44 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.
>
> Cc: stable@vger.kernel.org
> Fixes: f5d3ef25d238 ("rust: devres: get rid of Devres' inner Arc")
> Signed-off-by: Danilo Krummrich <dakr@kernel.org>
> ---
>  rust/kernel/devres.rs | 17 ++++++++++-------
>  1 file changed, 10 insertions(+), 7 deletions(-)
>
> diff --git a/rust/kernel/devres.rs b/rust/kernel/devres.rs
> index da18091143a6..bfccf4177644 100644
> --- a/rust/kernel/devres.rs
> +++ b/rust/kernel/devres.rs
> @@ -119,6 +119,7 @@ pub struct Devres<T: Send> {
>      // 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 };
>  
> @@ -153,12 +162,6 @@ pub fn new<'a, E>(
>                  to_result(unsafe {
>                      bindings::devm_add_action(dev.as_raw(), Some(callback), inner.cast())
>                  })?;

I have some bad news, I think this is also wrong: if the
`devm_add_action` fails, we never drop the contents of `inner`, since
the destructor of `Opaque` does nothing and we never finished
construction of `Devres`, so its `Drop` will never be called.

One solution would be to use `pin_chain` on the initializer for `Inner`
(not opaque). Another one would be to not use opaque, `UnsafePinned`
actually looks like the better fit for this use-case.

This also made me re-think `Opaque::pin_init`. It seems wrong and
probably shouldn't exist, as `Opaque` violates the drop guarantee
required by pinned data. So it cannot structurally pin the data inside.

---
Cheers,
Benno

> -
> -                Opaque::pin_init(try_pin_init!(Inner {
> -                    devm <- Completion::new(),
> -                    revoke <- Completion::new(),
> -                    data <- Revocable::new(data),
> -                }))
>              },
>          })
>      }
>
> base-commit: 8f5ae30d69d7543eee0d70083daf4de8fe15d585

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

[Danilo Krummrich]

On Tue Aug 12, 2025 at 12:45 AM CEST, Benno Lossin wrote:
> On Mon Aug 11, 2025 at 11:44 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.
>>
>> Cc: stable@vger.kernel.org
>> Fixes: f5d3ef25d238 ("rust: devres: get rid of Devres' inner Arc")
>> Signed-off-by: Danilo Krummrich <dakr@kernel.org>
>> ---
>>  rust/kernel/devres.rs | 17 ++++++++++-------
>>  1 file changed, 10 insertions(+), 7 deletions(-)
>>
>> diff --git a/rust/kernel/devres.rs b/rust/kernel/devres.rs
>> index da18091143a6..bfccf4177644 100644
>> --- a/rust/kernel/devres.rs
>> +++ b/rust/kernel/devres.rs
>> @@ -119,6 +119,7 @@ pub struct Devres<T: Send> {
>>      // 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 };
>>  
>> @@ -153,12 +162,6 @@ pub fn new<'a, E>(
>>                  to_result(unsafe {
>>                      bindings::devm_add_action(dev.as_raw(), Some(callback), inner.cast())
>>                  })?;
>
> I have some bad news, I think this is also wrong: if the
> `devm_add_action` fails, we never drop the contents of `inner`, since
> the destructor of `Opaque` does nothing and we never finished
> construction of `Devres`, so its `Drop` will never be called.

Good catch! For some reason I already had UnsafePinned in mind, but it's still a
TODO to replace the Opaque with UnsafePinned.

> One solution would be to use `pin_chain` on the initializer for `Inner`
> (not opaque). Another one would be to not use opaque, `UnsafePinned`
> actually looks like the better fit for this use-case.

Yeah, the problem should go away with UnsafePinned. Maybe, until we have it, we
can just do the following:

diff --git a/rust/kernel/devres.rs b/rust/kernel/devres.rs
index bfccf4177644..1981201fa7f9 100644
--- a/rust/kernel/devres.rs
+++ b/rust/kernel/devres.rs
@@ -161,6 +161,9 @@ 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(|_| {
+                    // SAFETY: `inner` is valid for dropping.
+                    unsafe { core::ptr::drop_in_place(inner) };
                 })?;
             },
         })

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

[Benno Lossin]

On Tue Aug 12, 2025 at 1:15 AM CEST, Danilo Krummrich wrote:
> On Tue Aug 12, 2025 at 12:45 AM CEST, Benno Lossin wrote:
>> On Mon Aug 11, 2025 at 11:44 PM CEST, Danilo Krummrich wrote:
>> One solution would be to use `pin_chain` on the initializer for `Inner`
>> (not opaque). Another one would be to not use opaque, `UnsafePinned`
>> actually looks like the better fit for this use-case.
>
> Yeah, the problem should go away with UnsafePinned. Maybe, until we have it, we
> can just do the following:
>
> diff --git a/rust/kernel/devres.rs b/rust/kernel/devres.rs
> index bfccf4177644..1981201fa7f9 100644
> --- a/rust/kernel/devres.rs
> +++ b/rust/kernel/devres.rs
> @@ -161,6 +161,9 @@ 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(|_| {
> +                    // SAFETY: `inner` is valid for dropping.
> +                    unsafe { core::ptr::drop_in_place(inner) };

Yeah that works too. Though I'd add a comment & improve the safety
comment.

---
Cheers,
Benno