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 | 210 +++++++++++++++++++-------------
rust/kernel/pci.rs | 20 +--
samples/rust/rust_driver_pci.rs | 19 +--
5 files changed, 151 insertions(+), 111 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 47ead37faf4c..eeffdc8115aa 100644
--- a/rust/kernel/devres.rs
+++ b/rust/kernel/devres.rs
@@ -13,16 +13,21 @@
ffi::c_void,
prelude::*,
revocable::{Revocable, RevocableGuard},
- sync::{rcu, Arc, Completion},
- types::{ARef, ForeignOwnable},
+ sync::{rcu, Completion},
+ types::{ARef, ForeignOwnable, ScopeGuard, Opaque},
};
+use pin_init::Wrapper;
+
+/// [`Devres`] inner data accessed from [`Devres::callback`].
#[pin_data]
-struct DevresInner<T> {
- dev: ARef<Device>,
- callback: unsafe extern "C" fn(*mut c_void),
+struct Inner<T> {
#[pin]
data: Revocable<T>,
+ /// Tracks whether [`Devres::callback`] has been completed.
+ #[pin]
+ devm: Completion,
+ /// Tracks whether revoking [`Self::data`] has been completed.
#[pin]
revoke: Completion,
}
@@ -44,6 +49,10 @@ struct DevresInner<T> {
/// [`Devres`] users should make sure to simply free the corresponding backing resource in `T`'s
/// [`Drop`] implementation.
///
+/// # Invariants
+///
+/// [`Self::inner`] is guaranteed to be initialized and is always accessed read-only.
+///
/// # Example
///
/// ```no_run
@@ -88,100 +97,108 @@ 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>>);
+#[pin_data(PinnedDrop)]
+pub struct Devres<T> {
+ dev: ARef<Device>,
+ /// Pointer to [`Self::devres_callback`].
+ ///
+ /// Has to be stored, since Rust does not guarantee to always return the same address for a
+ /// function. However, the C API uses the address as a key.
+ callback: unsafe extern "C" fn(*mut c_void),
+ /// Contains all the fields shared with [`Self::callback`].
+ // TODO: Replace with `UnsafePinned`, once available.
+ #[pin]
+ inner: Opaque<Inner<T>>,
+}
+
+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)[`Revocable`] 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;
-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,
+ try_pin_init!(&this in Self {
+ // INVARIANT: `inner` is properly initialized.
+ inner <- Opaque::pin_init(try_pin_init!(Inner {
data <- Revocable::new(data),
+ devm <- Completion::new(),
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();
+ })),
+ callback,
+ dev: {
+ // SAFETY: `this` is a valid pointer to uninitialized memory.
+ let inner = unsafe { &raw mut (*this.as_ptr()).inner };
- // 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 _) };
-
- 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));
- }
+ // SAFETY:
+ // - `dev.as_raw()` is a pointer to a valid bound device.
+ // - `inner` 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), inner.cast())
+ })?;
- Ok(inner)
+ dev.into()
+ },
+ })
}
- fn as_ptr(&self) -> *const Self {
- self as _
+ fn inner(&self) -> &Inner<T> {
+ // SAFETY: By the type invairants of `Self`, `inner` is properly initialized and always
+ // accessed read-only.
+ unsafe { &*self.inner.get() }
}
- 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
+ fn data(&self) -> &Revocable<T> {
+ &self.inner().data
}
#[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 `Inner` to `devm_add_action()`,
+ // hence `ptr` must be a valid pointer to `Inner`.
+ let inner = unsafe { &*ptr.cast::<Inner<T>>() };
+
+ // Ensure that `inner` can't be used anymore after we signal completion of this callback.
+ let inner = ScopeGuard::new_with_data(inner, |inner| inner.devm.complete_all());
if !inner.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`.
+ // `data` for us. Hence we have to wait until `Devres::drop` signals that it
+ // completed revoking `data`.
inner.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)?;
-
- 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,
+ (unsafe {
+ bindings::devm_remove_action_nowarn(
+ self.dev.as_raw(),
+ Some(self.callback),
+ core::ptr::from_ref(self.inner()).cast_mut().cast(),
+ )
+ } == 0)
}
/// Obtain `&'a T`, bypassing the [`Revocable`].
@@ -213,44 +230,63 @@ 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) {
+// SAFETY: `Devres` can be send to any task, if `T: Send`.
+unsafe impl<T: Send> Send for Devres<T> {}
+
+// SAFETY: `Devres` can be shared with any task, if `T: Sync`.
+unsafe impl<T: Sync> Sync for Devres<T> {}
+
+#[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.inner().revoke.complete_all();
+
+ // Wait for `Self::devres_callback` to be done using this object.
+ self.inner().devm.wait_for_completion();
}
+ } else {
+ // `Self::devres_callback` revokes `self.data` for us, hence wait for it to be done
+ // using this object.
+ self.inner().devm.wait_for_completion();
}
+
+ // INVARIANT: At this point it is guaranteed that `inner` can't be accessed any more.
+ //
+ // SAFETY: `inner` is valid for dropping.
+ unsafe { core::ptr::drop_in_place(self.inner.get()) };
}
}
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 Tue, Jun 24, 2025 at 11:54:01PM +0200, Danilo Krummrich wrote:
[...]
> +#[pin_data(PinnedDrop)]
> +pub struct Devres<T> {
It makes me realize: I think we need to make `T` being `Send`? Because
the devm callback can happen on a different thread other than
`Devres::new()` and the callback may drop `T` because of revoke(), so we
are essientially sending `T`. Alternatively we can make `Devres::new()`
and its friend require `T` being `Send`.
If it's true, we need a separate patch that "Fixes" this.
(Imagine a Devres<MutexGuard>)
> + dev: ARef<Device>,
> + /// Pointer to [`Self::devres_callback`].
> + ///
> + /// Has to be stored, since Rust does not guarantee to always return the same address for a
> + /// function. However, the C API uses the address as a key.
> + callback: unsafe extern "C" fn(*mut c_void),
> + /// Contains all the fields shared with [`Self::callback`].
> + // TODO: Replace with `UnsafePinned`, once available.
nit: Maybe also reference the `drop_in_place()` in Devres::drop() as
well, because once we use `UnsafePinned`, we don't need that
`drop_in_place()`. But not a big deal, just trying to help the people
who would handle that "TODO" ;-)
> + #[pin]
> + inner: Opaque<Inner<T>>,
> +}
> +
[...]
> +// SAFETY: `Devres` can be send to any task, if `T: Send`.
> +unsafe impl<T: Send> Send for Devres<T> {}
> +
> +// SAFETY: `Devres` can be shared with any task, if `T: Sync`.
> +unsafe impl<T: Sync> Sync for Devres<T> {}
`T` also need to be `Send` for `Devres<T>` to be `Sync` because that's
what `Revocable<T>` requires. (Unless we want `T` always being `Send`
because of the issue I mentioned above)
The rest looks good to me.
Regards,
Boqun
> +
> +#[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.inner().revoke.complete_all();
> +
> + // Wait for `Self::devres_callback` to be done using this object.
> + self.inner().devm.wait_for_completion();
> }
> + } else {
> + // `Self::devres_callback` revokes `self.data` for us, hence wait for it to be done
> + // using this object.
> + self.inner().devm.wait_for_completion();
> }
> +
> + // INVARIANT: At this point it is guaranteed that `inner` can't be accessed any more.
> + //
> + // SAFETY: `inner` is valid for dropping.
> + unsafe { core::ptr::drop_in_place(self.inner.get()) };
> }
> }
>
[...]
On Wed, Jun 25, 2025 at 09:13:24PM -0700, Boqun Feng wrote: > On Tue, Jun 24, 2025 at 11:54:01PM +0200, Danilo Krummrich wrote: > `T` also need to be `Send` for `Devres<T>` to be `Sync` because that's > what `Revocable<T>` requires. I think that's because of Revocable::revoke(), however you can't call Devres::revoke(). > (Unless we want `T` always being `Send` > because of the issue I mentioned above) Yes, we do. > The rest looks good to me. Great! :)
On Thu, Jun 26, 2025 at 09:47:18PM +0200, Danilo Krummrich wrote: > On Wed, Jun 25, 2025 at 09:13:24PM -0700, Boqun Feng wrote: > > On Tue, Jun 24, 2025 at 11:54:01PM +0200, Danilo Krummrich wrote: > > `T` also need to be `Send` for `Devres<T>` to be `Sync` because that's > > what `Revocable<T>` requires. > > I think that's because of Revocable::revoke(), however you can't call > Devres::revoke(). Well, what could happen is we create a `Arc<Devres<T>>` on one thread, and because it's `Sync`, we can have a clone of it in another thread, and if that's the last one call `Arc::drop()`, the `Revocable` will be `revoke()`d in a different thread than it's created. But with `T` always being `Send`, this is not a problem now. Regards, Boqun > > > (Unless we want `T` always being `Send` > > because of the issue I mentioned above) > > Yes, we do. > > > The rest looks good to me. > > Great! :)
On Wed, Jun 25, 2025 at 09:13:24PM -0700, Boqun Feng wrote:
> On Tue, Jun 24, 2025 at 11:54:01PM +0200, Danilo Krummrich wrote:
> [...]
> > +#[pin_data(PinnedDrop)]
> > +pub struct Devres<T> {
>
> It makes me realize: I think we need to make `T` being `Send`? Because
> the devm callback can happen on a different thread other than
> `Devres::new()` and the callback may drop `T` because of revoke(), so we
> are essientially sending `T`. Alternatively we can make `Devres::new()`
> and its friend require `T` being `Send`.
>
> If it's true, we need a separate patch that "Fixes" this.
Indeed, that needs a fix.
> (Imagine a Devres<MutexGuard>)
>
> > + dev: ARef<Device>,
> > + /// Pointer to [`Self::devres_callback`].
> > + ///
> > + /// Has to be stored, since Rust does not guarantee to always return the same address for a
> > + /// function. However, the C API uses the address as a key.
> > + callback: unsafe extern "C" fn(*mut c_void),
> > + /// Contains all the fields shared with [`Self::callback`].
> > + // TODO: Replace with `UnsafePinned`, once available.
>
> nit: Maybe also reference the `drop_in_place()` in Devres::drop() as
> well, because once we use `UnsafePinned`, we don't need that
> `drop_in_place()`. But not a big deal, just trying to help the people
> who would handle that "TODO" ;-)
Makes sense, the same is true for the Send and Sync impls, I think. AFAIK,
UnsafePinned should cover them automatically.
On Thu Jun 26, 2025 at 12:01 PM CEST, Danilo Krummrich wrote:
> On Wed, Jun 25, 2025 at 09:13:24PM -0700, Boqun Feng wrote:
>> On Tue, Jun 24, 2025 at 11:54:01PM +0200, Danilo Krummrich wrote:
>> [...]
>> > +#[pin_data(PinnedDrop)]
>> > +pub struct Devres<T> {
>>
>> It makes me realize: I think we need to make `T` being `Send`? Because
>> the devm callback can happen on a different thread other than
>> `Devres::new()` and the callback may drop `T` because of revoke(), so we
>> are essientially sending `T`. Alternatively we can make `Devres::new()`
>> and its friend require `T` being `Send`.
>>
>> If it's true, we need a separate patch that "Fixes" this.
>
> Indeed, that needs a fix.
Oh and we have no `'static` bound on `T` either... We should require
that as well.
---
Cheers,
Benno
On Thu, Jun 26, 2025 at 12:27:18PM +0200, Benno Lossin wrote:
> On Thu Jun 26, 2025 at 12:01 PM CEST, Danilo Krummrich wrote:
> > On Wed, Jun 25, 2025 at 09:13:24PM -0700, Boqun Feng wrote:
> >> On Tue, Jun 24, 2025 at 11:54:01PM +0200, Danilo Krummrich wrote:
> >> [...]
> >> > +#[pin_data(PinnedDrop)]
> >> > +pub struct Devres<T> {
> >>
> >> It makes me realize: I think we need to make `T` being `Send`? Because
> >> the devm callback can happen on a different thread other than
> >> `Devres::new()` and the callback may drop `T` because of revoke(), so we
> >> are essientially sending `T`. Alternatively we can make `Devres::new()`
> >> and its friend require `T` being `Send`.
> >>
> >> If it's true, we need a separate patch that "Fixes" this.
> >
> > Indeed, that needs a fix.
>
> Oh and we have no `'static` bound on `T` either... We should require
> that as well.
I don't think we actually need that, The Devres instance can't out-live a &T
passed into it. And the &T can't out-live the &Device<Bound>, hence we're
guaranteed that devres_callback() is never called because Devres::drop() will be
able successfully unregister the callback given that we're still in the
&Device<Bound> scope.
The only thing that could technically out-live the &Device<Bound> would be
&'static T, but that would obviously be fine.
Do I miss anything?
On Thu Jun 26, 2025 at 1:07 PM CEST, Danilo Krummrich wrote:
> On Thu, Jun 26, 2025 at 12:27:18PM +0200, Benno Lossin wrote:
>> On Thu Jun 26, 2025 at 12:01 PM CEST, Danilo Krummrich wrote:
>> > On Wed, Jun 25, 2025 at 09:13:24PM -0700, Boqun Feng wrote:
>> >> On Tue, Jun 24, 2025 at 11:54:01PM +0200, Danilo Krummrich wrote:
>> >> [...]
>> >> > +#[pin_data(PinnedDrop)]
>> >> > +pub struct Devres<T> {
>> >>
>> >> It makes me realize: I think we need to make `T` being `Send`? Because
>> >> the devm callback can happen on a different thread other than
>> >> `Devres::new()` and the callback may drop `T` because of revoke(), so we
>> >> are essientially sending `T`. Alternatively we can make `Devres::new()`
>> >> and its friend require `T` being `Send`.
>> >>
>> >> If it's true, we need a separate patch that "Fixes" this.
>> >
>> > Indeed, that needs a fix.
>>
>> Oh and we have no `'static` bound on `T` either... We should require
>> that as well.
>
> I don't think we actually need that, The Devres instance can't out-live a &T
> passed into it. And the &T can't out-live the &Device<Bound>, hence we're
> guaranteed that devres_callback() is never called because Devres::drop() will be
> able successfully unregister the callback given that we're still in the
> &Device<Bound> scope.
Yeah that's correct, I got confused.
> The only thing that could technically out-live the &Device<Bound> would be
> &'static T, but that would obviously be fine.
>
> Do I miss anything?
Nope :)
---
Cheers,
Benno
On Thu, Jun 26, 2025 at 01:07:25PM +0200, Danilo Krummrich wrote:
> On Thu, Jun 26, 2025 at 12:27:18PM +0200, Benno Lossin wrote:
> > On Thu Jun 26, 2025 at 12:01 PM CEST, Danilo Krummrich wrote:
> > > On Wed, Jun 25, 2025 at 09:13:24PM -0700, Boqun Feng wrote:
> > >> On Tue, Jun 24, 2025 at 11:54:01PM +0200, Danilo Krummrich wrote:
> > >> [...]
> > >> > +#[pin_data(PinnedDrop)]
> > >> > +pub struct Devres<T> {
> > >>
> > >> It makes me realize: I think we need to make `T` being `Send`? Because
> > >> the devm callback can happen on a different thread other than
> > >> `Devres::new()` and the callback may drop `T` because of revoke(), so we
> > >> are essientially sending `T`. Alternatively we can make `Devres::new()`
> > >> and its friend require `T` being `Send`.
> > >>
> > >> If it's true, we need a separate patch that "Fixes" this.
> > >
> > > Indeed, that needs a fix.
> >
> > Oh and we have no `'static` bound on `T` either... We should require
> > that as well.
>
> I don't think we actually need that, The Devres instance can't out-live a &T
> passed into it. And the &T can't out-live the &Device<Bound>, hence we're
> guaranteed that devres_callback() is never called because Devres::drop() will be
> able successfully unregister the callback given that we're still in the
> &Device<Bound> scope.
>
> The only thing that could technically out-live the &Device<Bound> would be
> &'static T, but that would obviously be fine.
>
> Do I miss anything?
Thinking a bit more about it, a similar argumentation is true for not needing
T: Send. The only way to leave the &Device<Bound> scope and hence the thread
would be to stuff the Devres into a ForeignOwnable container, no?
Analogous to Benno asking for ForeignOwnable: 'static, should we also require
ForeignOwnable: Send + Sync?
Alternatively, the safety requirements of ForeignOwnable:::from_foreign() and
ForeignOwnable::borrow() would need to cover this, which they currently they
are not.
On Thu Jun 26, 2025 at 1:40 PM CEST, Danilo Krummrich wrote:
> On Thu, Jun 26, 2025 at 01:07:25PM +0200, Danilo Krummrich wrote:
>> On Thu, Jun 26, 2025 at 12:27:18PM +0200, Benno Lossin wrote:
>> > On Thu Jun 26, 2025 at 12:01 PM CEST, Danilo Krummrich wrote:
>> > > On Wed, Jun 25, 2025 at 09:13:24PM -0700, Boqun Feng wrote:
>> > >> On Tue, Jun 24, 2025 at 11:54:01PM +0200, Danilo Krummrich wrote:
>> > >> [...]
>> > >> > +#[pin_data(PinnedDrop)]
>> > >> > +pub struct Devres<T> {
>> > >>
>> > >> It makes me realize: I think we need to make `T` being `Send`? Because
>> > >> the devm callback can happen on a different thread other than
>> > >> `Devres::new()` and the callback may drop `T` because of revoke(), so we
>> > >> are essientially sending `T`. Alternatively we can make `Devres::new()`
>> > >> and its friend require `T` being `Send`.
>> > >>
>> > >> If it's true, we need a separate patch that "Fixes" this.
>> > >
>> > > Indeed, that needs a fix.
>> >
>> > Oh and we have no `'static` bound on `T` either... We should require
>> > that as well.
>>
>> I don't think we actually need that, The Devres instance can't out-live a &T
>> passed into it. And the &T can't out-live the &Device<Bound>, hence we're
>> guaranteed that devres_callback() is never called because Devres::drop() will be
>> able successfully unregister the callback given that we're still in the
>> &Device<Bound> scope.
>>
>> The only thing that could technically out-live the &Device<Bound> would be
>> &'static T, but that would obviously be fine.
>>
>> Do I miss anything?
>
> Thinking a bit more about it, a similar argumentation is true for not needing
> T: Send. The only way to leave the &Device<Bound> scope and hence the thread
> would be to stuff the Devres into a ForeignOwnable container, no?
I think `T: Send` is required, since we drop the `T` in the other thread
when `devres_callback` is called from the device unbinding.
> Analogous to Benno asking for ForeignOwnable: 'static, should we also require
> ForeignOwnable: Send + Sync?
I don't think so, you could have a type that stores the pointer in C,
but only ever allows access from the same thread.
> Alternatively, the safety requirements of ForeignOwnable:::from_foreign() and
> ForeignOwnable::borrow() would need to cover this, which they currently they
> are not.
Oh right, yeah they should cover that.
---
Cheers,
Benno
© 2016 - 2025 Red Hat, Inc.