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

["Benno Lossin" <lossin@kernel.org>]

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