| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <aFlA92ooeQsSThLh@google.com>
Date: Mon, 23 Jun 2025 11:56:39 +0000
From: Alice Ryhl <aliceryhl@...gle.com>
To: Danilo Krummrich <dakr@...nel.org>
Cc: gregkh@...uxfoundation.org, rafael@...nel.org, ojeda@...nel.org,
alex.gaynor@...il.com, boqun.feng@...il.com, gary@...yguo.net,
bjorn3_gh@...tonmail.com, lossin@...nel.org, a.hindborg@...nel.org,
tmgross@...ch.edu, david.m.ertman@...el.com, ira.weiny@...el.com,
leon@...nel.org, kwilczynski@...nel.org, bhelgaas@...gle.com,
rust-for-linux@...r.kernel.org, linux-kernel@...r.kernel.org,
linux-pci@...r.kernel.org, Dave Airlie <airlied@...hat.com>,
Simona Vetter <simona.vetter@...ll.ch>, Viresh Kumar <viresh.kumar@...aro.org>
Subject: Re: [PATCH v2 2/4] rust: devres: replace Devres::new_foreign_owned()
On Sun, Jun 22, 2025 at 06:40:39PM +0200, Danilo Krummrich wrote:
> Replace Devres::new_foreign_owned() with devres::register().
>
> The current implementation of Devres::new_foreign_owned() creates a full
> Devres container instance, including the internal Revocable and
> completion.
>
> However, none of that is necessary for the intended use of giving full
> ownership of an object to devres and getting it dropped once the given
> device is unbound.
>
> Hence, implement devres::register(), which is limited to consume the
> given data, wrap it in a KBox and drop the KBox once the given device is
> unbound, without any other synchronization.
>
> Cc: Dave Airlie <airlied@...hat.com>
> Cc: Simona Vetter <simona.vetter@...ll.ch>
> Cc: Viresh Kumar <viresh.kumar@...aro.org>
> Acked-by: Viresh Kumar <viresh.kumar@...aro.org>
> Signed-off-by: Danilo Krummrich <dakr@...nel.org>
overall looks good
Reviewed-by: Alice Ryhl <aliceryhl@...gle.com>
> rust/helpers/device.c | 7 ++++
> rust/kernel/cpufreq.rs | 11 +++---
> rust/kernel/devres.rs | 70 +++++++++++++++++++++++++++++++++------
> rust/kernel/drm/driver.rs | 14 ++++----
> 4 files changed, 82 insertions(+), 20 deletions(-)
>
> diff --git a/rust/helpers/device.c b/rust/helpers/device.c
> index b2135c6686b0..502fef7e9ae8 100644
> --- a/rust/helpers/device.c
> +++ b/rust/helpers/device.c
> @@ -8,3 +8,10 @@ int rust_helper_devm_add_action(struct device *dev,
> {
> return devm_add_action(dev, action, data);
> }
> +
> +int rust_helper_devm_add_action_or_reset(struct device *dev,
> + void (*action)(void *),
> + void *data)
> +{
> + return devm_add_action_or_reset(dev, action, data);
> +}
> diff --git a/rust/kernel/cpufreq.rs b/rust/kernel/cpufreq.rs
> index 11b03e9d7e89..dd84e2b4d7ae 100644
> --- a/rust/kernel/cpufreq.rs
> +++ b/rust/kernel/cpufreq.rs
> @@ -13,7 +13,7 @@
> cpu::CpuId,
> cpumask,
> device::{Bound, Device},
> - devres::Devres,
> + devres,
> error::{code::*, from_err_ptr, from_result, to_result, Result, VTABLE_DEFAULT_ERROR},
> ffi::{c_char, c_ulong},
> prelude::*,
> @@ -1046,10 +1046,13 @@ pub fn new() -> Result<Self> {
>
> /// Same as [`Registration::new`], but does not return a [`Registration`] instance.
> ///
> - /// Instead the [`Registration`] is owned by [`Devres`] and will be revoked / dropped, once the
> + /// Instead the [`Registration`] is owned by [`devres::register`] and will be dropped, once the
> /// device is detached.
> - pub fn new_foreign_owned(dev: &Device<Bound>) -> Result {
> - Devres::new_foreign_owned(dev, Self::new()?, GFP_KERNEL)
> + pub fn new_foreign_owned(dev: &Device<Bound>) -> Result
> + where
> + T: 'static,
> + {
> + devres::register(dev, Self::new()?, GFP_KERNEL)
> }
> }
>
> diff --git a/rust/kernel/devres.rs b/rust/kernel/devres.rs
> index 544e50efab43..250073749279 100644
> --- a/rust/kernel/devres.rs
> +++ b/rust/kernel/devres.rs
> @@ -9,12 +9,12 @@
> alloc::Flags,
> bindings,
> device::{Bound, Device},
> - error::{Error, Result},
> + error::{to_result, Error, Result},
> ffi::c_void,
> prelude::*,
> revocable::{Revocable, RevocableGuard},
> sync::{rcu, Arc, Completion},
> - types::ARef,
> + types::{ARef, ForeignOwnable},
> };
>
> #[pin_data]
> @@ -184,14 +184,6 @@ pub fn new(dev: &Device<Bound>, data: T, flags: Flags) -> Result<Self> {
> Ok(Devres(inner))
> }
>
> - /// Same as [`Devres::new`], but does not return a `Devres` instance. Instead the given `data`
> - /// is owned by devres and will be revoked / dropped, once the device is detached.
> - pub fn new_foreign_owned(dev: &Device<Bound>, data: T, flags: Flags) -> Result {
> - let _ = DevresInner::new(dev, data, flags)?;
> -
> - Ok(())
> - }
> -
> /// Obtain `&'a T`, bypassing the [`Revocable`].
> ///
> /// This method allows to directly obtain a `&'a T`, bypassing the [`Revocable`], by presenting
> @@ -261,3 +253,61 @@ fn drop(&mut self) {
> }
> }
> }
> +
> +/// Consume `data` and [`Drop::drop`] `data` once `dev` is unbound.
> +fn register_foreign<P: ForeignOwnable>(dev: &Device<Bound>, data: P) -> Result {
> + let ptr = data.into_foreign();
> +
> + #[allow(clippy::missing_safety_doc)]
> + unsafe extern "C" fn callback<P: ForeignOwnable>(ptr: *mut kernel::ffi::c_void) {
> + // SAFETY: `ptr` is the pointer to the `ForeignOwnable` leaked above and hence valid.
> + let _ = unsafe { P::from_foreign(ptr.cast()) };
Nit: I usually write this
drop(unsafe { P::from_foreign(ptr.cast()) });
> + }
> +
> + // SAFETY:
> + // - `dev.as_raw()` is a pointer to a valid and bound device.
> + // - `ptr` is a valid pointer the `ForeignOwnable` devres takes ownership of.
> + to_result(unsafe {
> + // `devm_add_action_or_reset()` also calls `callback` on failure, such that the
> + // `ForeignOwnable` is released eventually.
> + bindings::devm_add_action_or_reset(dev.as_raw(), Some(callback::<P>), ptr.cast())
> + })
> +}
> +
> +/// Encapsulate `data` in a [`KBox`] and [`Drop::drop`] `data` once `dev` is unbound.
> +///
> +/// # Examples
> +///
> +/// ```no_run
> +/// use kernel::{device::{Bound, Device}, devres};
> +///
> +/// /// Registration of e.g. a class device, IRQ, etc.
> +/// struct Registration;
> +///
> +/// impl Registration {
> +/// fn new() -> Self {
> +/// // register
> +///
> +/// Self
> +/// }
> +/// }
> +///
> +/// impl Drop for Registration {
> +/// fn drop(&mut self) {
> +/// // unregister
> +/// }
> +/// }
> +///
> +/// fn from_bound_context(dev: &Device<Bound>) -> Result {
> +/// devres::register(dev, Registration::new(), GFP_KERNEL)
> +/// }
> +/// ```
> +pub fn register<T, E>(dev: &Device<Bound>, data: impl PinInit<T, E>, flags: Flags) -> Result
> +where
> + T: 'static,
> + Error: From<E>,
> +{
> + let data = KBox::pin_init(data, flags)?;
Wouldn't we also want to expose the ForeignOwnable version? It seems
likely someone would want to avoid the allocation.
> + register_foreign(dev, data)
> +}
> diff --git a/rust/kernel/drm/driver.rs b/rust/kernel/drm/driver.rs
> index acb638086131..f63addaf7235 100644
> --- a/rust/kernel/drm/driver.rs
> +++ b/rust/kernel/drm/driver.rs
> @@ -5,9 +5,7 @@
> //! C header: [`include/linux/drm/drm_drv.h`](srctree/include/linux/drm/drm_drv.h)
>
> use crate::{
> - bindings, device,
> - devres::Devres,
> - drm,
> + bindings, device, devres, drm,
> error::{to_result, Result},
> prelude::*,
> str::CStr,
> @@ -130,18 +128,22 @@ fn new(drm: &drm::Device<T>, flags: usize) -> Result<Self> {
> }
>
> /// Same as [`Registration::new`}, but transfers ownership of the [`Registration`] to
> - /// [`Devres`].
> + /// [`devres::register`].
> pub fn new_foreign_owned(
> drm: &drm::Device<T>,
> dev: &device::Device<device::Bound>,
> flags: usize,
> - ) -> Result {
> + ) -> Result
> + where
> + T: 'static,
> + {
> if drm.as_ref().as_raw() != dev.as_raw() {
> return Err(EINVAL);
> }
>
> let reg = Registration::<T>::new(drm, flags)?;
> - Devres::new_foreign_owned(dev, reg, GFP_KERNEL)
> +
> + devres::register(dev, reg, GFP_KERNEL)
> }
>
> /// Returns a reference to the `Device` instance for this registration.
> --
> 2.49.0
>
Powered by blists - more mailing lists