From a674fefd17324fc467f043568e738b80ca22f2b4 Mon Sep 17 00:00:00 2001 From: Danilo Krummrich Date: Tue, 18 Jun 2024 17:48:34 +0200 Subject: rust: add abstraction for struct device Add an (always) reference-counted abstraction for a generic C `struct device`. This abstraction encapsulates existing `struct device` instances and manages its reference count. Subsystems may use this abstraction as a base to abstract subsystem specific device instances based on a generic `struct device`, such as `struct pci_dev`. Co-developed-by: Wedson Almeida Filho Signed-off-by: Wedson Almeida Filho Signed-off-by: Danilo Krummrich Link: https://lore.kernel.org/r/20240618154841.6716-2-dakr@redhat.com Signed-off-by: Greg Kroah-Hartman --- rust/kernel/device.rs | 102 ++++++++++++++++++++++++++++++++++++++++++++++++++ rust/kernel/lib.rs | 1 + 2 files changed, 103 insertions(+) create mode 100644 rust/kernel/device.rs (limited to 'rust/kernel') diff --git a/rust/kernel/device.rs b/rust/kernel/device.rs new file mode 100644 index 000000000000..e445e87fb7d7 --- /dev/null +++ b/rust/kernel/device.rs @@ -0,0 +1,102 @@ +// SPDX-License-Identifier: GPL-2.0 + +//! Generic devices that are part of the kernel's driver model. +//! +//! C header: [`include/linux/device.h`](srctree/include/linux/device.h) + +use crate::{ + bindings, + types::{ARef, Opaque}, +}; +use core::ptr; + +/// A reference-counted device. +/// +/// This structure represents the Rust abstraction for a C `struct device`. This implementation +/// abstracts the usage of an already existing C `struct device` within Rust code that we get +/// passed from the C side. +/// +/// An instance of this abstraction can be obtained temporarily or permanent. +/// +/// A temporary one is bound to the lifetime of the C `struct device` pointer used for creation. +/// A permanent instance is always reference-counted and hence not restricted by any lifetime +/// boundaries. +/// +/// For subsystems it is recommended to create a permanent instance to wrap into a subsystem +/// specific device structure (e.g. `pci::Device`). This is useful for passing it to drivers in +/// `T::probe()`, such that a driver can store the `ARef` (equivalent to storing a +/// `struct device` pointer in a C driver) for arbitrary purposes, e.g. allocating DMA coherent +/// memory. +/// +/// # Invariants +/// +/// The pointer stored in `Self` is non-null and valid for the lifetime of the `ARef` instance. In +/// particular, the `ARef` instance owns an increment on the underlying object’s reference count. +/// +/// `bindings::device::release` is valid to be called from any thread, hence `ARef` can be +/// dropped from any thread. +#[repr(transparent)] +pub struct Device(Opaque); + +impl Device { + /// Creates a new reference-counted abstraction instance of an existing `struct device` pointer. + /// + /// # Safety + /// + /// Callers must ensure that `ptr` is valid, non-null, and has a non-zero reference count, + /// i.e. it must be ensured that the reference count of the C `struct device` `ptr` points to + /// can't drop to zero, for the duration of this function call. + /// + /// It must also be ensured that `bindings::device::release` can be called from any thread. + /// While not officially documented, this should be the case for any `struct device`. + pub unsafe fn from_raw(ptr: *mut bindings::device) -> ARef { + // SAFETY: By the safety requirements, ptr is valid. + // Initially increase the reference count by one to compensate for the final decrement once + // this newly created `ARef` instance is dropped. + unsafe { bindings::get_device(ptr) }; + + // CAST: `Self` is a `repr(transparent)` wrapper around `bindings::device`. + let ptr = ptr.cast::(); + + // SAFETY: By the safety requirements, ptr is valid. + unsafe { ARef::from_raw(ptr::NonNull::new_unchecked(ptr)) } + } + + /// Obtain the raw `struct device *`. + pub(crate) fn as_raw(&self) -> *mut bindings::device { + self.0.get() + } + + /// Convert a raw C `struct device` pointer to a `&'a Device`. + /// + /// # Safety + /// + /// Callers must ensure that `ptr` is valid, non-null, and has a non-zero reference count, + /// i.e. it must be ensured that the reference count of the C `struct device` `ptr` points to + /// can't drop to zero, for the duration of this function call and the entire duration when the + /// returned reference exists. + pub unsafe fn as_ref<'a>(ptr: *mut bindings::device) -> &'a Self { + // SAFETY: Guaranteed by the safety requirements of the function. + unsafe { &*ptr.cast() } + } +} + +// SAFETY: Instances of `Device` are always reference-counted. +unsafe impl crate::types::AlwaysRefCounted for Device { + fn inc_ref(&self) { + // SAFETY: The existence of a shared reference guarantees that the refcount is non-zero. + unsafe { bindings::get_device(self.as_raw()) }; + } + + unsafe fn dec_ref(obj: ptr::NonNull) { + // SAFETY: The safety requirements guarantee that the refcount is non-zero. + unsafe { bindings::put_device(obj.cast().as_ptr()) } + } +} + +// SAFETY: As by the type invariant `Device` can be sent to any thread. +unsafe impl Send for Device {} + +// SAFETY: `Device` can be shared among threads because all immutable methods are protected by the +// synchronization in `struct device`. +unsafe impl Sync for Device {} diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs index fbd91a48ff8b..dd1207f1a873 100644 --- a/rust/kernel/lib.rs +++ b/rust/kernel/lib.rs @@ -28,6 +28,7 @@ extern crate self as kernel; pub mod alloc; mod build_assert; +pub mod device; pub mod error; pub mod init; pub mod ioctl; -- cgit v1.2.3 From de6582833db0e695ba0c548e3cc2ad7dbb6aa260 Mon Sep 17 00:00:00 2001 From: Danilo Krummrich Date: Tue, 18 Jun 2024 17:48:35 +0200 Subject: rust: add firmware abstractions Add an abstraction around the kernels firmware API to request firmware images. The abstraction provides functions to access the firmware's size and backing buffer. The firmware is released once the abstraction instance is dropped. Signed-off-by: Danilo Krummrich Acked-by: Boqun Feng Link: https://lore.kernel.org/r/20240618154841.6716-3-dakr@redhat.com Signed-off-by: Greg Kroah-Hartman --- drivers/base/firmware_loader/Kconfig | 7 +++ rust/bindings/bindings_helper.h | 1 + rust/kernel/firmware.rs | 101 +++++++++++++++++++++++++++++++++++ rust/kernel/lib.rs | 2 + 4 files changed, 111 insertions(+) create mode 100644 rust/kernel/firmware.rs (limited to 'rust/kernel') diff --git a/drivers/base/firmware_loader/Kconfig b/drivers/base/firmware_loader/Kconfig index 5ca00e02fe82..a03701674265 100644 --- a/drivers/base/firmware_loader/Kconfig +++ b/drivers/base/firmware_loader/Kconfig @@ -37,6 +37,13 @@ config FW_LOADER_DEBUG SHA256 checksums to the kernel log for each firmware file that is loaded. +config RUST_FW_LOADER_ABSTRACTIONS + bool "Rust Firmware Loader abstractions" + depends on RUST + depends on FW_LOADER=y + help + This enables the Rust abstractions for the firmware loader API. + if FW_LOADER config FW_LOADER_PAGED_BUF diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h index ddb5644d4fd9..18a3f05115cb 100644 --- a/rust/bindings/bindings_helper.h +++ b/rust/bindings/bindings_helper.h @@ -9,6 +9,7 @@ #include #include #include +#include #include #include #include diff --git a/rust/kernel/firmware.rs b/rust/kernel/firmware.rs new file mode 100644 index 000000000000..b55ea1b45368 --- /dev/null +++ b/rust/kernel/firmware.rs @@ -0,0 +1,101 @@ +// SPDX-License-Identifier: GPL-2.0 + +//! Firmware abstraction +//! +//! C header: [`include/linux/firmware.h`](srctree/include/linux/firmware.h") + +use crate::{bindings, device::Device, error::Error, error::Result, str::CStr}; +use core::ptr::NonNull; + +// One of the following: `bindings::request_firmware`, `bindings::firmware_request_nowarn`, +// `firmware_request_platform`, `bindings::request_firmware_direct` +type FwFunc = + unsafe extern "C" fn(*mut *const bindings::firmware, *const i8, *mut bindings::device) -> i32; + +/// Abstraction around a C `struct firmware`. +/// +/// This is a simple abstraction around the C firmware API. Just like with the C API, firmware can +/// be requested. Once requested the abstraction provides direct access to the firmware buffer as +/// `&[u8]`. The firmware is released once [`Firmware`] is dropped. +/// +/// # Invariants +/// +/// The pointer is valid, and has ownership over the instance of `struct firmware`. +/// +/// Once requested, the `Firmware` backing buffer is not modified until it is freed when `Firmware` +/// is dropped. +/// +/// # Examples +/// +/// ``` +/// # use kernel::{c_str, device::Device, firmware::Firmware}; +/// +/// # // SAFETY: *NOT* safe, just for the example to get an `ARef` instance +/// # let dev = unsafe { Device::from_raw(core::ptr::null_mut()) }; +/// +/// let fw = Firmware::request(c_str!("path/to/firmware.bin"), &dev).unwrap(); +/// let blob = fw.data(); +/// ``` +pub struct Firmware(NonNull); + +impl Firmware { + fn request_internal(name: &CStr, dev: &Device, func: FwFunc) -> Result { + let mut fw: *mut bindings::firmware = core::ptr::null_mut(); + let pfw: *mut *mut bindings::firmware = &mut fw; + + // SAFETY: `pfw` is a valid pointer to a NULL initialized `bindings::firmware` pointer. + // `name` and `dev` are valid as by their type invariants. + let ret = unsafe { func(pfw as _, name.as_char_ptr(), dev.as_raw()) }; + if ret != 0 { + return Err(Error::from_errno(ret)); + } + + // SAFETY: `func` not bailing out with a non-zero error code, guarantees that `fw` is a + // valid pointer to `bindings::firmware`. + Ok(Firmware(unsafe { NonNull::new_unchecked(fw) })) + } + + /// Send a firmware request and wait for it. See also `bindings::request_firmware`. + pub fn request(name: &CStr, dev: &Device) -> Result { + Self::request_internal(name, dev, bindings::request_firmware) + } + + /// Send a request for an optional firmware module. See also + /// `bindings::firmware_request_nowarn`. + pub fn request_nowarn(name: &CStr, dev: &Device) -> Result { + Self::request_internal(name, dev, bindings::firmware_request_nowarn) + } + + fn as_raw(&self) -> *mut bindings::firmware { + self.0.as_ptr() + } + + /// Returns the size of the requested firmware in bytes. + pub fn size(&self) -> usize { + // SAFETY: Safe by the type invariant. + unsafe { (*self.as_raw()).size } + } + + /// Returns the requested firmware as `&[u8]`. + pub fn data(&self) -> &[u8] { + // SAFETY: Safe by the type invariant. Additionally, `bindings::firmware` guarantees, if + // successfully requested, that `bindings::firmware::data` has a size of + // `bindings::firmware::size` bytes. + unsafe { core::slice::from_raw_parts((*self.as_raw()).data, self.size()) } + } +} + +impl Drop for Firmware { + fn drop(&mut self) { + // SAFETY: Safe by the type invariant. + unsafe { bindings::release_firmware(self.as_raw()) }; + } +} + +// SAFETY: `Firmware` only holds a pointer to a C `struct firmware`, which is safe to be used from +// any thread. +unsafe impl Send for Firmware {} + +// SAFETY: `Firmware` only holds a pointer to a C `struct firmware`, references to which are safe to +// be used from any thread. +unsafe impl Sync for Firmware {} diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs index dd1207f1a873..7707cb013ce9 100644 --- a/rust/kernel/lib.rs +++ b/rust/kernel/lib.rs @@ -30,6 +30,8 @@ pub mod alloc; mod build_assert; pub mod device; pub mod error; +#[cfg(CONFIG_RUST_FW_LOADER_ABSTRACTIONS)] +pub mod firmware; pub mod init; pub mod ioctl; #[cfg(CONFIG_KUNIT)] -- cgit v1.2.3 From bbe98f4fde5a52aa01a1e1d754e1398228815fb0 Mon Sep 17 00:00:00 2001 From: Danilo Krummrich Date: Wed, 19 Jun 2024 15:20:12 +0200 Subject: firmware: rust: improve safety comments Improve the wording of safety comments to be more explicit about what exactly is guaranteed to be valid. Suggested-by: Benno Lossin Signed-off-by: Danilo Krummrich Link: https://lore.kernel.org/r/20240619132029.59296-1-dakr@redhat.com Signed-off-by: Greg Kroah-Hartman --- rust/kernel/firmware.rs | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) (limited to 'rust/kernel') diff --git a/rust/kernel/firmware.rs b/rust/kernel/firmware.rs index b55ea1b45368..386c8fb44785 100644 --- a/rust/kernel/firmware.rs +++ b/rust/kernel/firmware.rs @@ -22,8 +22,7 @@ type FwFunc = /// /// The pointer is valid, and has ownership over the instance of `struct firmware`. /// -/// Once requested, the `Firmware` backing buffer is not modified until it is freed when `Firmware` -/// is dropped. +/// The `Firmware`'s backing buffer is not modified. /// /// # Examples /// @@ -72,22 +71,22 @@ impl Firmware { /// Returns the size of the requested firmware in bytes. pub fn size(&self) -> usize { - // SAFETY: Safe by the type invariant. + // SAFETY: `self.as_raw()` is valid by the type invariant. unsafe { (*self.as_raw()).size } } /// Returns the requested firmware as `&[u8]`. pub fn data(&self) -> &[u8] { - // SAFETY: Safe by the type invariant. Additionally, `bindings::firmware` guarantees, if - // successfully requested, that `bindings::firmware::data` has a size of - // `bindings::firmware::size` bytes. + // SAFETY: `self.as_raw()` is valid by the type invariant. Additionally, + // `bindings::firmware` guarantees, if successfully requested, that + // `bindings::firmware::data` has a size of `bindings::firmware::size` bytes. unsafe { core::slice::from_raw_parts((*self.as_raw()).data, self.size()) } } } impl Drop for Firmware { fn drop(&mut self) { - // SAFETY: Safe by the type invariant. + // SAFETY: `self.as_raw()` is valid by the type invariant. unsafe { bindings::release_firmware(self.as_raw()) }; } } -- cgit v1.2.3 From 4ead6c37b04aa35943ea270f09db18ebb38e63ff Mon Sep 17 00:00:00 2001 From: Danilo Krummrich Date: Wed, 19 Jun 2024 15:39:17 +0200 Subject: device: rust: improve safety comments Improve the wording of safety comments to be more explicit about what exactly is guaranteed to be valid. Suggested-by: Benno Lossin Signed-off-by: Danilo Krummrich Link: https://lore.kernel.org/r/20240619133949.64638-1-dakr@redhat.com Signed-off-by: Greg Kroah-Hartman --- rust/kernel/device.rs | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) (limited to 'rust/kernel') diff --git a/rust/kernel/device.rs b/rust/kernel/device.rs index e445e87fb7d7..851018eef885 100644 --- a/rust/kernel/device.rs +++ b/rust/kernel/device.rs @@ -30,8 +30,10 @@ use core::ptr; /// /// # Invariants /// -/// The pointer stored in `Self` is non-null and valid for the lifetime of the `ARef` instance. In -/// particular, the `ARef` instance owns an increment on the underlying object’s reference count. +/// A `Device` instance represents a valid `struct device` created by the C portion of the kernel. +/// +/// Instances of this type are always reference-counted, that is, a call to `get_device` ensures +/// that the allocation remains valid at least until the matching call to `put_device`. /// /// `bindings::device::release` is valid to be called from any thread, hence `ARef` can be /// dropped from any thread. @@ -58,7 +60,8 @@ impl Device { // CAST: `Self` is a `repr(transparent)` wrapper around `bindings::device`. let ptr = ptr.cast::(); - // SAFETY: By the safety requirements, ptr is valid. + // SAFETY: `ptr` is valid by the safety requirements of this function. By the above call to + // `bindings::get_device` we also own a reference to the underlying `struct device`. unsafe { ARef::from_raw(ptr::NonNull::new_unchecked(ptr)) } } -- cgit v1.2.3 From 2c61b8c51d21d1b10c2881aa9c9918ff49f6fb7d Mon Sep 17 00:00:00 2001 From: Danilo Krummrich Date: Mon, 8 Jul 2024 22:07:20 +0200 Subject: firmware_loader: annotate doctests as `no_run` The doctests of `Firmware` are compile-time only tests, since they require a proper `Device` and a valid path to a (firmware) blob in order to do something sane on runtime - we can't satisfy both of those requirements. Hence, configure the example as `no_run`. Unfortunately, the kernel's Rust build system can't consider the `no_run` attribute yet. Hence, for the meantime, wrap the example code into a new function and never actually call it. Fixes: de6582833db0 ("rust: add firmware abstractions") Signed-off-by: Danilo Krummrich Link: https://lore.kernel.org/r/20240708200724.3203-1-dakr@redhat.com Signed-off-by: Greg Kroah-Hartman --- rust/kernel/firmware.rs | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) (limited to 'rust/kernel') diff --git a/rust/kernel/firmware.rs b/rust/kernel/firmware.rs index 386c8fb44785..106a928a535e 100644 --- a/rust/kernel/firmware.rs +++ b/rust/kernel/firmware.rs @@ -26,14 +26,18 @@ type FwFunc = /// /// # Examples /// -/// ``` +/// ```no_run /// # use kernel::{c_str, device::Device, firmware::Firmware}; /// +/// # fn no_run() -> Result<(), Error> { /// # // SAFETY: *NOT* safe, just for the example to get an `ARef` instance /// # let dev = unsafe { Device::from_raw(core::ptr::null_mut()) }; /// -/// let fw = Firmware::request(c_str!("path/to/firmware.bin"), &dev).unwrap(); +/// let fw = Firmware::request(c_str!("path/to/firmware.bin"), &dev)?; /// let blob = fw.data(); +/// +/// # Ok(()) +/// # } /// ``` pub struct Firmware(NonNull); -- cgit v1.2.3 From a23b018c3bf646274f02edd46bf448c20c826d94 Mon Sep 17 00:00:00 2001 From: Danilo Krummrich Date: Mon, 8 Jul 2024 22:07:21 +0200 Subject: firmware_loader: fix soundness issue in `request_internal` `request_internal` must be called with one of the following function pointers: request_firmware(), firmware_request_nowarn(), firmware_request_platform() or request_firmware_direct(). The previous `FwFunc` alias did not guarantee this, which is unsound. In order to fix this up, implement `FwFunc` as new type with a corresponding type invariant. Reported-by: Gary Guo Closes: https://lore.kernel.org/lkml/20240620143611.7995e0bb@eugeo/ Signed-off-by: Danilo Krummrich Reviewed-by: Christian Schrefl Link: https://lore.kernel.org/r/20240708200724.3203-2-dakr@redhat.com Signed-off-by: Greg Kroah-Hartman --- rust/kernel/firmware.rs | 27 ++++++++++++++++++++------- 1 file changed, 20 insertions(+), 7 deletions(-) (limited to 'rust/kernel') diff --git a/rust/kernel/firmware.rs b/rust/kernel/firmware.rs index 106a928a535e..2ba03af9f036 100644 --- a/rust/kernel/firmware.rs +++ b/rust/kernel/firmware.rs @@ -7,10 +7,23 @@ use crate::{bindings, device::Device, error::Error, error::Result, str::CStr}; use core::ptr::NonNull; -// One of the following: `bindings::request_firmware`, `bindings::firmware_request_nowarn`, -// `firmware_request_platform`, `bindings::request_firmware_direct` -type FwFunc = - unsafe extern "C" fn(*mut *const bindings::firmware, *const i8, *mut bindings::device) -> i32; +/// # Invariants +/// +/// One of the following: `bindings::request_firmware`, `bindings::firmware_request_nowarn`, +/// `bindings::firmware_request_platform`, `bindings::request_firmware_direct`. +struct FwFunc( + unsafe extern "C" fn(*mut *const bindings::firmware, *const i8, *mut bindings::device) -> i32, +); + +impl FwFunc { + fn request() -> Self { + Self(bindings::request_firmware) + } + + fn request_nowarn() -> Self { + Self(bindings::firmware_request_nowarn) + } +} /// Abstraction around a C `struct firmware`. /// @@ -48,7 +61,7 @@ impl Firmware { // SAFETY: `pfw` is a valid pointer to a NULL initialized `bindings::firmware` pointer. // `name` and `dev` are valid as by their type invariants. - let ret = unsafe { func(pfw as _, name.as_char_ptr(), dev.as_raw()) }; + let ret = unsafe { func.0(pfw as _, name.as_char_ptr(), dev.as_raw()) }; if ret != 0 { return Err(Error::from_errno(ret)); } @@ -60,13 +73,13 @@ impl Firmware { /// Send a firmware request and wait for it. See also `bindings::request_firmware`. pub fn request(name: &CStr, dev: &Device) -> Result { - Self::request_internal(name, dev, bindings::request_firmware) + Self::request_internal(name, dev, FwFunc::request()) } /// Send a request for an optional firmware module. See also /// `bindings::firmware_request_nowarn`. pub fn request_nowarn(name: &CStr, dev: &Device) -> Result { - Self::request_internal(name, dev, bindings::firmware_request_nowarn) + Self::request_internal(name, dev, FwFunc::request_nowarn()) } fn as_raw(&self) -> *mut bindings::firmware { -- cgit v1.2.3