So far Devres uses an inner memory allocation and reference count, i.e.
an inner Arc, in order to ensure that the devres callback can't run into
a use-after-free in case where the Devres object is dropped while the
devres callback runs concurrently.
Instead, use a completion in order to avoid a potential UAF: In
Devres::drop(), if we detect that we can't remove the devres action
anymore, we wait for the completion that is completed from the devres
callback. If, in turn, we were able to successfully remove the devres
action, we can just go ahead.
This, again, allows us to get rid of the internal Arc, and instead let
Devres consume an `impl PinInit<T, E>` in order to return an
`impl PinInit<Devres<T>, E>`, which enables us to get away with less
memory allocations.
Additionally, having the resulting explicit synchronization in
Devres::drop() prevents potential subtle undesired side effects of the
devres callback dropping the final Arc reference asynchronously within
the devres callback.
Signed-off-by: Danilo Krummrich <dakr@kernel.org>
---
drivers/gpu/nova-core/driver.rs | 7 +-
drivers/gpu/nova-core/gpu.rs | 6 +-
rust/kernel/devres.rs | 187 +++++++++++++++-----------------
rust/kernel/pci.rs | 20 ++--
samples/rust/rust_driver_pci.rs | 19 ++--
5 files changed, 117 insertions(+), 122 deletions(-)
diff --git a/drivers/gpu/nova-core/driver.rs b/drivers/gpu/nova-core/driver.rs
index 8c86101c26cb..110f2b355db4 100644
--- a/drivers/gpu/nova-core/driver.rs
+++ b/drivers/gpu/nova-core/driver.rs
@@ -1,6 +1,6 @@
// SPDX-License-Identifier: GPL-2.0
-use kernel::{auxiliary, bindings, c_str, device::Core, pci, prelude::*};
+use kernel::{auxiliary, bindings, c_str, device::Core, pci, prelude::*, sync::Arc};
use crate::gpu::Gpu;
@@ -34,7 +34,10 @@ fn probe(pdev: &pci::Device<Core>, _info: &Self::IdInfo) -> Result<Pin<KBox<Self
pdev.enable_device_mem()?;
pdev.set_master();
- let bar = pdev.iomap_region_sized::<BAR0_SIZE>(0, c_str!("nova-core/bar0"))?;
+ let bar = Arc::pin_init(
+ pdev.iomap_region_sized::<BAR0_SIZE>(0, c_str!("nova-core/bar0")),
+ GFP_KERNEL,
+ )?;
let this = KBox::pin_init(
try_pin_init!(Self {
diff --git a/drivers/gpu/nova-core/gpu.rs b/drivers/gpu/nova-core/gpu.rs
index 60b86f370284..47653c14838b 100644
--- a/drivers/gpu/nova-core/gpu.rs
+++ b/drivers/gpu/nova-core/gpu.rs
@@ -1,6 +1,6 @@
// SPDX-License-Identifier: GPL-2.0
-use kernel::{device, devres::Devres, error::code::*, pci, prelude::*};
+use kernel::{device, devres::Devres, error::code::*, pci, prelude::*, sync::Arc};
use crate::driver::Bar0;
use crate::firmware::{Firmware, FIRMWARE_VERSION};
@@ -161,14 +161,14 @@ fn new(bar: &Bar0) -> Result<Spec> {
pub(crate) struct Gpu {
spec: Spec,
/// MMIO mapping of PCI BAR 0
- bar: Devres<Bar0>,
+ bar: Arc<Devres<Bar0>>,
fw: Firmware,
}
impl Gpu {
pub(crate) fn new(
pdev: &pci::Device<device::Bound>,
- devres_bar: Devres<Bar0>,
+ devres_bar: Arc<Devres<Bar0>>,
) -> Result<impl PinInit<Self>> {
let bar = devres_bar.access(pdev.as_ref())?;
let spec = Spec::new(bar)?;
diff --git a/rust/kernel/devres.rs b/rust/kernel/devres.rs
index 04435e810249..4ee9037f4ad4 100644
--- a/rust/kernel/devres.rs
+++ b/rust/kernel/devres.rs
@@ -13,20 +13,10 @@
ffi::c_void,
prelude::*,
revocable::{Revocable, RevocableGuard},
- sync::{rcu, Arc, Completion},
+ sync::{rcu, Completion},
types::{ARef, ForeignOwnable},
};
-#[pin_data]
-struct DevresInner<T> {
- dev: ARef<Device>,
- callback: unsafe extern "C" fn(*mut c_void),
- #[pin]
- data: Revocable<T>,
- #[pin]
- revoke: Completion,
-}
-
/// This abstraction is meant to be used by subsystems to containerize [`Device`] bound resources to
/// manage their lifetime.
///
@@ -86,100 +76,93 @@ struct DevresInner<T> {
/// # fn no_run(dev: &Device<Bound>) -> Result<(), Error> {
/// // SAFETY: Invalid usage for example purposes.
/// let iomem = unsafe { IoMem::<{ core::mem::size_of::<u32>() }>::new(0xBAAAAAAD)? };
-/// let devres = Devres::new(dev, iomem, GFP_KERNEL)?;
+/// let devres = KBox::pin_init(Devres::new(dev, iomem), GFP_KERNEL)?;
///
/// let res = devres.try_access().ok_or(ENXIO)?;
/// res.write8(0x42, 0x0);
/// # Ok(())
/// # }
/// ```
-pub struct Devres<T>(Arc<DevresInner<T>>);
-
-impl<T> DevresInner<T> {
- fn new(dev: &Device<Bound>, data: T, flags: Flags) -> Result<Arc<DevresInner<T>>> {
- let inner = Arc::pin_init(
- try_pin_init!( DevresInner {
- dev: dev.into(),
- callback: Self::devres_callback,
- data <- Revocable::new(data),
- revoke <- Completion::new(),
- }),
- flags,
- )?;
-
- // Convert `Arc<DevresInner>` into a raw pointer and make devres own this reference until
- // `Self::devres_callback` is called.
- let data = inner.clone().into_raw();
+#[pin_data(PinnedDrop)]
+pub struct Devres<T> {
+ dev: ARef<Device>,
+ callback: unsafe extern "C" fn(*mut c_void),
+ #[pin]
+ data: Revocable<T>,
+ #[pin]
+ devm: Completion,
+ #[pin]
+ revoke: Completion,
+}
- // 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(inner.callback), data as _) };
+impl<T> Devres<T> {
+ /// Creates a new [`Devres`] instance of the given `data`. The `data` encapsulated within the
+ /// returned `Devres` instance' `data` will be revoked once the device is detached.
+ pub fn new<'a, E>(
+ dev: &'a Device<Bound>,
+ data: impl PinInit<T, E> + 'a,
+ ) -> impl PinInit<Self, Error> + 'a
+ where
+ T: 'a,
+ Error: From<E>,
+ {
+ let callback = Self::devres_callback;
- if ret != 0 {
- // SAFETY: We just created another reference to `inner` in order to pass it to
- // `bindings::devm_add_action`. If `bindings::devm_add_action` fails, we have to drop
- // this reference accordingly.
- let _ = unsafe { Arc::from_raw(data) };
- return Err(Error::from_errno(ret));
- }
+ try_pin_init!(&this in Self {
+ data <- Revocable::new(data),
+ devm <- Completion::new(),
+ revoke <- Completion::new(),
+ callback,
+ dev: {
+ // SAFETY:
+ // - `dev.as_raw()` is a pointer to a valid bound device.
+ // - `this` is guaranteed to be a valid for the duration of the lifetime of `Self`.
+ // - `devm_add_action()` is guaranteed not to call `callback` until `this` has been
+ // properly initialized, because we require `dev` (i.e. the *bound* device) to
+ // live at least as long as the returned `impl PinInit<Self, Error>`.
+ to_result(unsafe {
+ bindings::devm_add_action(dev.as_raw(), Some(callback), this.as_ptr().cast())
+ })?;
- Ok(inner)
+ dev.into()
+ },
+ })
}
fn as_ptr(&self) -> *const Self {
- self as _
- }
-
- fn remove_action(this: &Arc<Self>) -> bool {
- // 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 success = unsafe {
- bindings::devm_remove_action_nowarn(
- this.dev.as_raw(),
- Some(this.callback),
- this.as_ptr() as _,
- )
- } == 0;
-
- if success {
- // 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(this.as_ptr()) };
- }
-
- success
+ self
}
#[allow(clippy::missing_safety_doc)]
unsafe extern "C" fn devres_callback(ptr: *mut kernel::ffi::c_void) {
- let ptr = ptr as *mut DevresInner<T>;
- // Devres owned this memory; now that we received the callback, drop the `Arc` and hence the
- // reference.
- // SAFETY: Safe, since we leaked an `Arc` reference to devm_add_action() in
- // `DevresInner::new`.
- let inner = unsafe { Arc::from_raw(ptr) };
+ // SAFETY: In `Self::new` we've passed a valid pointer to `Self` to `devm_add_action()`,
+ // hence `ptr` must be a valid pointer to `Self`.
+ let this = unsafe { &*ptr.cast::<Self>() };
- if !inner.data.revoke() {
+ if !this.data.revoke() {
// If `revoke()` returns false, it means that `Devres::drop` already started revoking
- // `inner.data` for us. Hence we have to wait until `Devres::drop()` signals that it
- // completed revoking `inner.data`.
- inner.revoke.wait_for_completion();
+ // `data` for us. Hence we have to wait until `Devres::drop` signals that it
+ // completed revoking `data`.
+ this.revoke.wait_for_completion();
}
- }
-}
-impl<T> Devres<T> {
- /// Creates a new [`Devres`] instance of the given `data`. The `data` encapsulated within the
- /// returned `Devres` instance' `data` will be revoked once the device is detached.
- pub fn new(dev: &Device<Bound>, data: T, flags: Flags) -> Result<Self> {
- let inner = DevresInner::new(dev, data, flags)?;
+ // Signal that we're done using `this`.
+ this.devm.complete_all();
+ }
- Ok(Devres(inner))
+ fn remove_action(&self) -> bool {
+ // SAFETY:
+ // - `self.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.
+ (unsafe {
+ bindings::devm_remove_action_nowarn(
+ self.dev.as_raw(),
+ Some(self.callback),
+ self.as_ptr().cast_mut().cast(),
+ )
+ } == 0)
}
/// Obtain `&'a T`, bypassing the [`Revocable`].
@@ -211,43 +194,51 @@ pub fn new(dev: &Device<Bound>, data: T, flags: Flags) -> Result<Self> {
/// }
/// ```
pub fn access<'a>(&'a self, dev: &'a Device<Bound>) -> Result<&'a T> {
- if self.0.dev.as_raw() != dev.as_raw() {
+ if self.dev.as_raw() != dev.as_raw() {
return Err(EINVAL);
}
// SAFETY: `dev` being the same device as the device this `Devres` has been created for
- // proves that `self.0.data` hasn't been revoked and is guaranteed to not be revoked as
- // long as `dev` lives; `dev` lives at least as long as `self`.
- Ok(unsafe { self.0.data.access() })
+ // proves that `self.data` hasn't been revoked and is guaranteed to not be revoked as long
+ // as `dev` lives; `dev` lives at least as long as `self`.
+ Ok(unsafe { self.data.access() })
}
/// [`Devres`] accessor for [`Revocable::try_access`].
pub fn try_access(&self) -> Option<RevocableGuard<'_, T>> {
- self.0.data.try_access()
+ self.data.try_access()
}
/// [`Devres`] accessor for [`Revocable::try_access_with`].
pub fn try_access_with<R, F: FnOnce(&T) -> R>(&self, f: F) -> Option<R> {
- self.0.data.try_access_with(f)
+ self.data.try_access_with(f)
}
/// [`Devres`] accessor for [`Revocable::try_access_with_guard`].
pub fn try_access_with_guard<'a>(&'a self, guard: &'a rcu::Guard) -> Option<&'a T> {
- self.0.data.try_access_with_guard(guard)
+ self.data.try_access_with_guard(guard)
}
}
-impl<T> Drop for Devres<T> {
- fn drop(&mut self) {
+#[pinned_drop]
+impl<T> PinnedDrop for Devres<T> {
+ fn drop(self: Pin<&mut Self>) {
// SAFETY: When `drop` runs, it is guaranteed that nobody is accessing the revocable data
// anymore, hence it is safe not to wait for the grace period to finish.
- if unsafe { self.0.data.revoke_nosync() } {
- // We revoked `self.0.data` before the devres action did, hence try to remove it.
- if !DevresInner::remove_action(&self.0) {
+ if unsafe { self.data.revoke_nosync() } {
+ // We revoked `self.data` before the devres action did, hence try to remove it.
+ if !self.remove_action() {
// We could not remove the devres action, which means that it now runs concurrently,
- // hence signal that `self.0.data` has been revoked successfully.
- self.0.revoke.complete_all();
+ // hence signal that `self.data` has been revoked by us successfully.
+ self.revoke.complete_all();
+
+ // Wait for `Self::devres_callback` to be done using this object.
+ self.devm.wait_for_completion();
}
+ } else {
+ // `Self::devres_callback` revokes `self.data` for us, hence wait for it to be done
+ // using this object.
+ self.devm.wait_for_completion();
}
}
}
diff --git a/rust/kernel/pci.rs b/rust/kernel/pci.rs
index 8435f8132e38..db0eb7eaf9b1 100644
--- a/rust/kernel/pci.rs
+++ b/rust/kernel/pci.rs
@@ -5,7 +5,6 @@
//! C header: [`include/linux/pci.h`](srctree/include/linux/pci.h)
use crate::{
- alloc::flags::*,
bindings, container_of, device,
device_id::RawDeviceId,
devres::Devres,
@@ -398,19 +397,20 @@ pub fn resource_len(&self, bar: u32) -> Result<bindings::resource_size_t> {
impl Device<device::Bound> {
/// Mapps an entire PCI-BAR after performing a region-request on it. I/O operation bound checks
/// can be performed on compile time for offsets (plus the requested type size) < SIZE.
- pub fn iomap_region_sized<const SIZE: usize>(
- &self,
+ pub fn iomap_region_sized<'a, const SIZE: usize>(
+ &'a self,
bar: u32,
- name: &CStr,
- ) -> Result<Devres<Bar<SIZE>>> {
- let bar = Bar::<SIZE>::new(self, bar, name)?;
- let devres = Devres::new(self.as_ref(), bar, GFP_KERNEL)?;
-
- Ok(devres)
+ name: &'a CStr,
+ ) -> impl PinInit<Devres<Bar<SIZE>>, Error> + 'a {
+ Devres::new(self.as_ref(), Bar::<SIZE>::new(self, bar, name))
}
/// Mapps an entire PCI-BAR after performing a region-request on it.
- pub fn iomap_region(&self, bar: u32, name: &CStr) -> Result<Devres<Bar>> {
+ pub fn iomap_region<'a>(
+ &'a self,
+ bar: u32,
+ name: &'a CStr,
+ ) -> impl PinInit<Devres<Bar>, Error> + 'a {
self.iomap_region_sized::<0>(bar, name)
}
}
diff --git a/samples/rust/rust_driver_pci.rs b/samples/rust/rust_driver_pci.rs
index 15147e4401b2..5c35f1414172 100644
--- a/samples/rust/rust_driver_pci.rs
+++ b/samples/rust/rust_driver_pci.rs
@@ -25,8 +25,10 @@ impl TestIndex {
const NO_EVENTFD: Self = Self(0);
}
+#[pin_data(PinnedDrop)]
struct SampleDriver {
pdev: ARef<pci::Device>,
+ #[pin]
bar: Devres<Bar0>,
}
@@ -73,13 +75,11 @@ fn probe(pdev: &pci::Device<Core>, info: &Self::IdInfo) -> Result<Pin<KBox<Self>
pdev.enable_device_mem()?;
pdev.set_master();
- let bar = pdev.iomap_region_sized::<{ Regs::END }>(0, c_str!("rust_driver_pci"))?;
-
- let drvdata = KBox::new(
- Self {
+ let drvdata = KBox::pin_init(
+ try_pin_init!(Self {
pdev: pdev.into(),
- bar,
- },
+ bar <- pdev.iomap_region_sized::<{ Regs::END }>(0, c_str!("rust_driver_pci")),
+ }),
GFP_KERNEL,
)?;
@@ -90,12 +90,13 @@ fn probe(pdev: &pci::Device<Core>, info: &Self::IdInfo) -> Result<Pin<KBox<Self>
Self::testdev(info, bar)?
);
- Ok(drvdata.into())
+ Ok(drvdata)
}
}
-impl Drop for SampleDriver {
- fn drop(&mut self) {
+#[pinned_drop]
+impl PinnedDrop for SampleDriver {
+ fn drop(self: Pin<&mut Self>) {
dev_dbg!(self.pdev.as_ref(), "Remove Rust PCI driver sample.\n");
}
}
--
2.49.0On Thu Jun 12, 2025 at 4:51 PM CEST, Danilo Krummrich wrote:
> So far Devres uses an inner memory allocation and reference count, i.e.
> an inner Arc, in order to ensure that the devres callback can't run into
> a use-after-free in case where the Devres object is dropped while the
> devres callback runs concurrently.
>
> Instead, use a completion in order to avoid a potential UAF: In
> Devres::drop(), if we detect that we can't remove the devres action
> anymore, we wait for the completion that is completed from the devres
> callback. If, in turn, we were able to successfully remove the devres
> action, we can just go ahead.
>
> This, again, allows us to get rid of the internal Arc, and instead let
> Devres consume an `impl PinInit<T, E>` in order to return an
> `impl PinInit<Devres<T>, E>`, which enables us to get away with less
> memory allocations.
>
> Additionally, having the resulting explicit synchronization in
> Devres::drop() prevents potential subtle undesired side effects of the
> devres callback dropping the final Arc reference asynchronously within
> the devres callback.
>
> Signed-off-by: Danilo Krummrich <dakr@kernel.org>
This is really nice, good to see the extra allocations gone :)
> ---
> drivers/gpu/nova-core/driver.rs | 7 +-
> drivers/gpu/nova-core/gpu.rs | 6 +-
> rust/kernel/devres.rs | 187 +++++++++++++++-----------------
> rust/kernel/pci.rs | 20 ++--
> samples/rust/rust_driver_pci.rs | 19 ++--
> 5 files changed, 117 insertions(+), 122 deletions(-)
> @@ -86,100 +76,93 @@ struct DevresInner<T> {
> /// # fn no_run(dev: &Device<Bound>) -> Result<(), Error> {
> /// // SAFETY: Invalid usage for example purposes.
> /// let iomem = unsafe { IoMem::<{ core::mem::size_of::<u32>() }>::new(0xBAAAAAAD)? };
> -/// let devres = Devres::new(dev, iomem, GFP_KERNEL)?;
> +/// let devres = KBox::pin_init(Devres::new(dev, iomem), GFP_KERNEL)?;
> ///
> /// let res = devres.try_access().ok_or(ENXIO)?;
> /// res.write8(0x42, 0x0);
> /// # Ok(())
> /// # }
> /// ```
> -pub struct Devres<T>(Arc<DevresInner<T>>);
> -
> -impl<T> DevresInner<T> {
> - fn new(dev: &Device<Bound>, data: T, flags: Flags) -> Result<Arc<DevresInner<T>>> {
> - let inner = Arc::pin_init(
> - try_pin_init!( DevresInner {
> - dev: dev.into(),
> - callback: Self::devres_callback,
> - data <- Revocable::new(data),
> - revoke <- Completion::new(),
> - }),
> - flags,
> - )?;
> -
> - // Convert `Arc<DevresInner>` into a raw pointer and make devres own this reference until
> - // `Self::devres_callback` is called.
> - let data = inner.clone().into_raw();
> +#[pin_data(PinnedDrop)]
> +pub struct Devres<T> {
> + dev: ARef<Device>,
> + callback: unsafe extern "C" fn(*mut c_void),
Do I remember correctly that we at some point talked about adding a
comment here for why this is needed? (ie it's needed, because
`Self::callback` might return different addresses?)
> + #[pin]
> + data: Revocable<T>,
> + #[pin]
> + devm: Completion,
> + #[pin]
> + revoke: Completion,
Probably a good idea to add some doc comments explaining what these two
completions track.
(feel free to do these in another patch or in a follow-up)
> +}
>
> - // 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(inner.callback), data as _) };
> +impl<T> Devres<T> {
> + /// Creates a new [`Devres`] instance of the given `data`. The `data` encapsulated within the
Missing double newline after the first sentence.
> + /// returned `Devres` instance' `data` will be revoked once the device is detached.
Maybe we should link to `Revocable` on the word `revoked`?
> + pub fn new<'a, E>(
> + dev: &'a Device<Bound>,
> + data: impl PinInit<T, E> + 'a,
> + ) -> impl PinInit<Self, Error> + 'a
> + where
> + T: 'a,
> + Error: From<E>,
> + {
> + let callback = Self::devres_callback;
> - Ok(Devres(inner))
> + fn remove_action(&self) -> bool {
> + // SAFETY:
> + // - `self.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.
> + (unsafe {
> + bindings::devm_remove_action_nowarn(
> + self.dev.as_raw(),
> + Some(self.callback),
> + self.as_ptr().cast_mut().cast(),
> + )
> + } == 0)
I don't think the parenthesis are required?
> }
>
> /// Obtain `&'a T`, bypassing the [`Revocable`].
> -impl<T> Drop for Devres<T> {
> - fn drop(&mut self) {
> +#[pinned_drop]
> +impl<T> PinnedDrop for Devres<T> {
> + fn drop(self: Pin<&mut Self>) {
> // SAFETY: When `drop` runs, it is guaranteed that nobody is accessing the revocable data
> // anymore, hence it is safe not to wait for the grace period to finish.
> - if unsafe { self.0.data.revoke_nosync() } {
> - // We revoked `self.0.data` before the devres action did, hence try to remove it.
> - if !DevresInner::remove_action(&self.0) {
> + if unsafe { self.data.revoke_nosync() } {
> + // We revoked `self.data` before the devres action did, hence try to remove it.
> + if !self.remove_action() {
> // We could not remove the devres action, which means that it now runs concurrently,
> - // hence signal that `self.0.data` has been revoked successfully.
> - self.0.revoke.complete_all();
> + // hence signal that `self.data` has been revoked by us successfully.
> + self.revoke.complete_all();
> +
> + // Wait for `Self::devres_callback` to be done using this object.
> + self.devm.wait_for_completion();
> }
> + } else {
> + // `Self::devres_callback` revokes `self.data` for us, hence wait for it to be done
> + // using this object.
> + self.devm.wait_for_completion();
I don't understand this change, maybe it's best to move that into a
separate commit?
---
Cheers,
Benno
On Sun, Jun 22, 2025 at 09:05:51AM +0200, Benno Lossin wrote:
> On Thu Jun 12, 2025 at 4:51 PM CEST, Danilo Krummrich wrote:
> > + fn remove_action(&self) -> bool {
> > + // SAFETY:
> > + // - `self.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.
> > + (unsafe {
> > + bindings::devm_remove_action_nowarn(
> > + self.dev.as_raw(),
> > + Some(self.callback),
> > + self.as_ptr().cast_mut().cast(),
> > + )
> > + } == 0)
>
> I don't think the parenthesis are required?
At least the compiler doesn't seem to be happy about removing them:
error: expected expression, found `==`
--> rust/kernel/devres.rs:199:11
|
199 | } == 0
| ^^ expected expression
error[E0308]: mismatched types
--> rust/kernel/devres.rs:194:13
|
194 | / bindings::devm_remove_action_nowarn(
195 | | self.dev.as_raw(),
196 | | Some(self.callback),
197 | | self.inner().as_ptr().cast_mut().cast(),
198 | | )
| | ^- help: consider using a semicolon here: `;`
| |_____________|
| expected `()`, found `i32`
error: aborting due to 2 previous errors
On Sun Jun 22, 2025 at 5:45 PM CEST, Danilo Krummrich wrote:
> On Sun, Jun 22, 2025 at 09:05:51AM +0200, Benno Lossin wrote:
>> On Thu Jun 12, 2025 at 4:51 PM CEST, Danilo Krummrich wrote:
>> > + fn remove_action(&self) -> bool {
>> > + // SAFETY:
>> > + // - `self.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.
>> > + (unsafe {
>> > + bindings::devm_remove_action_nowarn(
>> > + self.dev.as_raw(),
>> > + Some(self.callback),
>> > + self.as_ptr().cast_mut().cast(),
>> > + )
>> > + } == 0)
>>
>> I don't think the parenthesis are required?
>
> At least the compiler doesn't seem to be happy about removing them:
>
> error: expected expression, found `==`
> --> rust/kernel/devres.rs:199:11
> |
> 199 | } == 0
> | ^^ expected expression
>
> error[E0308]: mismatched types
> --> rust/kernel/devres.rs:194:13
> |
> 194 | / bindings::devm_remove_action_nowarn(
> 195 | | self.dev.as_raw(),
> 196 | | Some(self.callback),
> 197 | | self.inner().as_ptr().cast_mut().cast(),
> 198 | | )
> | | ^- help: consider using a semicolon here: `;`
> | |_____________|
> | expected `()`, found `i32`
>
> error: aborting due to 2 previous errors
Huh... Do you like this better?:
let res = unsafe {
bindings::devm_remove_action_nowarn(
/*
* ...
*/
)
};
res == 0
Maybe it's more readable, but I'm not sure what is more idiomatic in
this case.
---
Cheers,
Benno
On Sun, Jun 22, 2025 at 09:05:51AM +0200, Benno Lossin wrote:
> On Thu Jun 12, 2025 at 4:51 PM CEST, Danilo Krummrich wrote:
> > +#[pin_data(PinnedDrop)]
> > +pub struct Devres<T> {
> > + dev: ARef<Device>,
> > + callback: unsafe extern "C" fn(*mut c_void),
>
> Do I remember correctly that we at some point talked about adding a
> comment here for why this is needed? (ie it's needed, because
> `Self::callback` might return different addresses?)
Correct -- thanks for reminding me of that. Will add the corresponding comment.
> > + #[pin]
> > + data: Revocable<T>,
> > + #[pin]
> > + devm: Completion,
> > + #[pin]
> > + revoke: Completion,
>
> Probably a good idea to add some doc comments explaining what these two
> completions track.
>
> (feel free to do these in another patch or in a follow-up)
No, I think it'd be good to do it right away -- will add them.
> > +#[pinned_drop]
> > +impl<T> PinnedDrop for Devres<T> {
> > + fn drop(self: Pin<&mut Self>) {
> > // SAFETY: When `drop` runs, it is guaranteed that nobody is accessing the revocable data
> > // anymore, hence it is safe not to wait for the grace period to finish.
> > - if unsafe { self.0.data.revoke_nosync() } {
> > - // We revoked `self.0.data` before the devres action did, hence try to remove it.
> > - if !DevresInner::remove_action(&self.0) {
> > + if unsafe { self.data.revoke_nosync() } {
> > + // We revoked `self.data` before the devres action did, hence try to remove it.
> > + if !self.remove_action() {
> > // We could not remove the devres action, which means that it now runs concurrently,
> > - // hence signal that `self.0.data` has been revoked successfully.
> > - self.0.revoke.complete_all();
> > + // hence signal that `self.data` has been revoked by us successfully.
> > + self.revoke.complete_all();
> > +
> > + // Wait for `Self::devres_callback` to be done using this object.
> > + self.devm.wait_for_completion();
> > }
> > + } else {
> > + // `Self::devres_callback` revokes `self.data` for us, hence wait for it to be done
> > + // using this object.
> > + self.devm.wait_for_completion();
>
> I don't understand this change, maybe it's best to move that into a
> separate commit?
We can't do that, without this change the code would be incorrect.
What happens here is that, if drop() races with devres_callback() we have to
make drop() wait until devres_callback() is completed, because otherwise
devres_callback() might experience a use-after-free.
Previoulsly this has been taken care of by Arc<DevresInner>, which C devres held
a reference of.
On Sun Jun 22, 2025 at 2:08 PM CEST, Danilo Krummrich wrote:
> On Sun, Jun 22, 2025 at 09:05:51AM +0200, Benno Lossin wrote:
>> On Thu Jun 12, 2025 at 4:51 PM CEST, Danilo Krummrich wrote:
>> > +#[pinned_drop]
>> > +impl<T> PinnedDrop for Devres<T> {
>> > + fn drop(self: Pin<&mut Self>) {
>> > // SAFETY: When `drop` runs, it is guaranteed that nobody is accessing the revocable data
>> > // anymore, hence it is safe not to wait for the grace period to finish.
>> > - if unsafe { self.0.data.revoke_nosync() } {
>> > - // We revoked `self.0.data` before the devres action did, hence try to remove it.
>> > - if !DevresInner::remove_action(&self.0) {
>> > + if unsafe { self.data.revoke_nosync() } {
>> > + // We revoked `self.data` before the devres action did, hence try to remove it.
>> > + if !self.remove_action() {
>> > // We could not remove the devres action, which means that it now runs concurrently,
>> > - // hence signal that `self.0.data` has been revoked successfully.
>> > - self.0.revoke.complete_all();
>> > + // hence signal that `self.data` has been revoked by us successfully.
>> > + self.revoke.complete_all();
>> > +
>> > + // Wait for `Self::devres_callback` to be done using this object.
>> > + self.devm.wait_for_completion();
>> > }
>> > + } else {
>> > + // `Self::devres_callback` revokes `self.data` for us, hence wait for it to be done
>> > + // using this object.
>> > + self.devm.wait_for_completion();
>>
>> I don't understand this change, maybe it's best to move that into a
>> separate commit?
>
> We can't do that, without this change the code would be incorrect.
>
> What happens here is that, if drop() races with devres_callback() we have to
> make drop() wait until devres_callback() is completed, because otherwise
> devres_callback() might experience a use-after-free.
>
> Previoulsly this has been taken care of by Arc<DevresInner>, which C devres held
> a reference of.
Yeah I understand it now, the diff was adding too much noise and looking
at it directly was helpful :)
Theoretically, you could add it in a commit before removing the Arc, but
probably not worth it.
---
Cheers,
Benno
© 2016 - 2025 Red Hat, Inc.