rust/kernel/devres.rs | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-)
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.1On 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
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) };
})?;
},
})
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
© 2016 - 2025 Red Hat, Inc.