| 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: <Z_1Jfs5DXD2vuzLj@cassiopeiae>
Date: Mon, 14 Apr 2025 19:44:30 +0200
From: Danilo Krummrich <dakr@...nel.org>
To: Remo Senekowitsch <remo@...nzli.dev>
Cc: Rob Herring <robh@...nel.org>, Saravana Kannan <saravanak@...gle.com>,
Miguel Ojeda <ojeda@...nel.org>,
Alex Gaynor <alex.gaynor@...il.com>,
Boqun Feng <boqun.feng@...il.com>, Gary Guo <gary@...yguo.net>,
Björn Roy Baron <bjorn3_gh@...tonmail.com>,
Benno Lossin <benno.lossin@...ton.me>,
Andreas Hindborg <a.hindborg@...nel.org>,
Alice Ryhl <aliceryhl@...gle.com>, Trevor Gross <tmgross@...ch.edu>,
Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
"Rafael J. Wysocki" <rafael@...nel.org>,
linux-kernel@...r.kernel.org, devicetree@...r.kernel.org,
rust-for-linux@...r.kernel.org
Subject: Re: [PATCH v2 2/5] rust: Add bindings for reading device properties
On Mon, Apr 14, 2025 at 05:26:27PM +0200, Remo Senekowitsch wrote:
> The device property API is a firmware agnostic API for reading
> properties from firmware (DT/ACPI) devices nodes and swnodes.
>
> While the C API takes a pointer to a caller allocated variable/buffer,
> the rust API is designed to return a value and can be used in struct
> initialization. Rust generics are also utilized to support different
> types of properties where appropriate.
>
> The PropertyGuard is a way to force users to specify whether a property
> is supposed to be required or not. This allows us to move error
> logging of missing required properties into core, preventing a lot of
> boilerplate in drivers.
The patch adds a lot of thing, i.e.
* implement PropertyInt
* implement PropertyGuard
* extend FwNode by a lot of functions
* extend Device by some property functions
I see that from v1 a lot of things have been squashed, likely because there are
a few circular dependencies. Is there really no reasonable way to break this
down a bit?
> Co-developed-by: Rob Herring (Arm) <robh@...nel.org>
> Signed-off-by: Rob Herring (Arm) <robh@...nel.org>
> Signed-off-by: Remo Senekowitsch <remo@...nzli.dev>
> ---
> rust/kernel/property.rs | 385 +++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 383 insertions(+), 2 deletions(-)
>
> diff --git a/rust/kernel/property.rs b/rust/kernel/property.rs
> index f6e6c980d..0d4ea3168 100644
> --- a/rust/kernel/property.rs
> +++ b/rust/kernel/property.rs
> @@ -4,9 +4,17 @@
> //!
> //! C header: [`include/linux/property.h`](srctree/include/linux/property.h)
>
> -use core::ptr;
> +use core::{mem::MaybeUninit, ptr};
>
> -use crate::{bindings, device::Device, str::CStr, types::Opaque};
> +use crate::{
> + alloc::KVec,
> + bindings, c_str,
> + device::Device,
> + error::{to_result, Result},
> + prelude::*,
> + str::{BStr, CStr, CString},
> + types::Opaque,
> +};
>
> impl Device {
> /// Obtain the fwnode corresponding to the device.
> @@ -28,6 +36,38 @@ fn fwnode(&self) -> &FwNode {
> pub fn property_present(&self, name: &CStr) -> bool {
> self.fwnode().property_present(name)
> }
> +
> + /// Returns firmware property `name` boolean value
> + pub fn property_read_bool(&self, name: &CStr) -> bool {
> + self.fwnode().property_read_bool(name)
> + }
> +
> + /// Returns the index of matching string `match_str` for firmware string property `name`
> + pub fn property_match_string(&self, name: &CStr, match_str: &CStr) -> Result<usize> {
> + self.fwnode().property_match_string(name, match_str)
> + }
> +
> + /// Returns firmware property `name` integer array values in a KVec
> + pub fn property_read_array_vec<'fwnode, 'name, T: PropertyInt>(
> + &'fwnode self,
> + name: &'name CStr,
> + len: usize,
> + ) -> Result<PropertyGuard<'fwnode, 'name, KVec<T>>> {
> + self.fwnode().property_read_array_vec(name, len)
> + }
> +
> + /// Returns integer array length for firmware property `name`
> + pub fn property_count_elem<T: PropertyInt>(&self, name: &CStr) -> Result<usize> {
> + self.fwnode().property_count_elem::<T>(name)
> + }
> +
> + /// Returns firmware property `name` integer scalar value
> + pub fn property_read<'fwnode, 'name, T: Property>(
> + &'fwnode self,
> + name: &'name CStr,
> + ) -> PropertyGuard<'fwnode, 'name, T> {
> + self.fwnode().property_read(name)
> + }
> }
Okay, I start to see why you have all those Device functions in a separate file.
:)
I think we should move device.rs into its own module, i.e.
rust/kernel/device/mod.rs and then create rust/kernel/device/property.rs.
>
> /// A reference-counted fwnode_handle.
> @@ -59,6 +99,150 @@ pub fn property_present(&self, name: &CStr) -> bool {
> // SAFETY: By the invariant of `CStr`, `name` is null-terminated.
> unsafe { bindings::fwnode_property_present(self.as_raw().cast_const(), name.as_char_ptr()) }
> }
> +
> + /// Returns firmware property `name` boolean value
> + pub fn property_read_bool(&self, name: &CStr) -> bool {
> + // SAFETY: `name` is non-null and null-terminated. `self.as_raw()` is valid
> + // because `self` is valid.
> + unsafe { bindings::fwnode_property_read_bool(self.as_raw(), name.as_char_ptr()) }
> + }
> +
> + /// Returns the index of matching string `match_str` for firmware string property `name`
> + pub fn property_match_string(&self, name: &CStr, match_str: &CStr) -> Result<usize> {
> + // SAFETY: `name` and `match_str` are non-null and null-terminated. `self.as_raw` is
> + // valid because `self` is valid.
> + let ret = unsafe {
> + bindings::fwnode_property_match_string(
> + self.as_raw(),
> + name.as_char_ptr(),
> + match_str.as_char_ptr(),
> + )
> + };
> + to_result(ret)?;
> + Ok(ret as usize)
> + }
> +
> + /// Returns firmware property `name` integer array values in a KVec
> + pub fn property_read_array_vec<'fwnode, 'name, T: PropertyInt>(
> + &'fwnode self,
> + name: &'name CStr,
> + len: usize,
> + ) -> Result<PropertyGuard<'fwnode, 'name, KVec<T>>> {
> + let mut val: KVec<T> = KVec::with_capacity(len, GFP_KERNEL)?;
> +
> + // SAFETY: `val.as_mut_ptr()` is valid because `KVec::with_capacity`
> + // didn't return an error and it has at least space for `len` number
> + // of elements.
> + let err = unsafe { read_array_out_param::<T>(self, name, val.as_mut_ptr(), len) };
> + let res = if err < 0 {
> + Err(Error::from_errno(err))
> + } else {
> + // SAFETY: fwnode_property_read_int_array() writes exactly `len` entries on success
> + unsafe { val.set_len(len) }
> + Ok(val)
> + };
> + Ok(PropertyGuard {
> + inner: res,
> + fwnode: self,
> + name,
> + })
> + }
> +
> + /// Returns integer array length for firmware property `name`
> + pub fn property_count_elem<T: PropertyInt>(&self, name: &CStr) -> Result<usize> {
> + // SAFETY: `out_param` is allowed to be null because `len` is zero.
> + let ret = unsafe { read_array_out_param::<T>(self, name, ptr::null_mut(), 0) };
> + to_result(ret)?;
> + Ok(ret as usize)
> + }
> +
> + /// Returns the value of firmware property `name`.
> + ///
> + /// This method is generic over the type of value to read. Informally,
> + /// the types that can be read are booleans, strings, unsigned integers and
> + /// arrays of unsigned integers.
> + ///
> + /// Reading a `KVec` of integers is done with the separate
> + /// method [`Self::property_read_array_vec`], because it takes an
> + /// additional `len` argument.
> + ///
> + /// When reading a boolean, this method never fails. A missing property
> + /// is interpreted as `false`, whereas a present property is interpreted
> + /// as `true`.
> + ///
> + /// For more precise documentation about what types can be read, see
> + /// the [implementors of Property][Property#implementors] and [its
> + /// implementations on foreign types][Property#foreign-impls].
> + ///
> + /// # Examples
> + ///
> + /// ```
> + /// # use crate::{device::Device, types::CString};
> + /// fn examples(dev: &Device) -> Result {
> + /// let fwnode = dev.fwnode();
> + /// let b: bool = fwnode.property_read("some-bool").required()?;
> + /// if let Some(s) = fwnode.property_read::<CString>("some-str").optional() {
> + /// // ...
> + /// }
> + /// }
> + /// ```
> + pub fn property_read<'fwnode, 'name, T: Property>(
> + &'fwnode self,
> + name: &'name CStr,
> + ) -> PropertyGuard<'fwnode, 'name, T> {
> + PropertyGuard {
> + inner: T::read(self, name),
> + fwnode: self,
> + name,
> + }
> + }
> +
> + /// helper used to display name or path of a fwnode
> + ///
> + /// # Safety
> + ///
> + /// Callers must provide a valid format string for a fwnode.
> + unsafe fn fmt(&self, f: &mut core::fmt::Formatter<'_>, fmt_str: &CStr) -> core::fmt::Result {
> + let mut buf = [0; 256];
> + // SAFETY: `buf` is valid and `buf.len()` is its length. `self.as_raw()` is
> + // valid because `self` is valid.
> + let written = unsafe {
> + bindings::scnprintf(buf.as_mut_ptr(), buf.len(), fmt_str.as_ptr(), self.as_raw())
> + };
Why do we need this? Can't we use write! right away?
> + // SAFETY: `written` is smaller or equal to `buf.len()`.
> + let b: &[u8] = unsafe { core::slice::from_raw_parts(buf.as_ptr(), written as usize) };
> + write!(f, "{}", BStr::from_bytes(b))
> + }
> +
> + /// Returns an object that implements [`Display`](core::fmt::Display) for
> + /// printing the name of a node.
> + pub fn display_name(&self) -> impl core::fmt::Display + use<'_> {
> + struct FwNodeDisplayName<'a>(&'a FwNode);
> +
> + impl core::fmt::Display for FwNodeDisplayName<'_> {
> + fn fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result {
> + // SAFETY: "%pfwP" is a valid format string for fwnode
> + unsafe { self.0.fmt(f, c_str!("%pfwP")) }
> + }
> + }
> +
> + FwNodeDisplayName(self)
> + }
> +
> + /// Returns an object that implements [`Display`](core::fmt::Display) for
> + /// printing the full path of a node.
> + pub fn display_path(&self) -> impl core::fmt::Display + use<'_> {
> + struct FwNodeDisplayPath<'a>(&'a FwNode);
> +
> + impl core::fmt::Display for FwNodeDisplayPath<'_> {
> + fn fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result {
> + // SAFETY: "%pfwf" is a valid format string for fwnode
> + unsafe { self.0.fmt(f, c_str!("%pfwf")) }
> + }
> + }
> +
> + FwNodeDisplayPath(self)
> + }
> }
>
> // SAFETY: Instances of `FwNode` are always reference-counted.
> @@ -73,3 +257,200 @@ unsafe fn dec_ref(obj: ptr::NonNull<Self>) {
> unsafe { bindings::fwnode_handle_put(obj.cast().as_ptr()) }
> }
> }
> +
> +/// Implemented for several types that can be read as properties.
> +///
> +/// Informally, this is implemented for strings, integers and arrays of
> +/// integers. It's used to make [`FwNode::property_read`] generic over the
> +/// type of property being read. There are also two dedicated methods to read
> +/// other types, because they require more specialized function signatures:
> +/// - [`property_read_bool`](Device::property_read_bool)
> +/// - [`property_read_array_vec`](Device::property_read_array_vec)
> +pub trait Property: Sized {
> + /// Used to make [`FwNode::property_read`] generic.
> + fn read(fwnode: &FwNode, name: &CStr) -> Result<Self>;
> +}
> +
> +impl Property for CString {
> + fn read(fwnode: &FwNode, name: &CStr) -> Result<Self> {
> + let mut str: *mut u8 = ptr::null_mut();
> + let pstr: *mut _ = &mut str;
> +
> + // SAFETY: `name` is non-null and null-terminated. `fwnode.as_raw` is
> + // valid because `fwnode` is valid.
> + let ret = unsafe {
> + bindings::fwnode_property_read_string(fwnode.as_raw(), name.as_char_ptr(), pstr.cast())
> + };
> + to_result(ret)?;
> +
> + // SAFETY: `pstr` contains a non-null ptr on success
> + let str = unsafe { CStr::from_char_ptr(*pstr) };
> + Ok(str.try_into()?)
> + }
> +}
I think it would be pretty weird to have a function CString::read() that takes a
FwNode argument, no? Same for all the other types below.
I assume you do this for
pub fn property_read<'fwnode, 'name, T: Property>(
&'fwnode self,
name: &'name CStr,
)
but given that you have to do the separate impls anyways, is there so much value
having the generic variant? You could still generate all the
property_read_{int}() variants with a macro.
If you really want a generic property_read(), I think you should create new
types instead and implement the Property trait for them instead.
> +/// Implemented for all integers that can be read as properties.
> +///
> +/// This helper trait is needed on top of the existing [`Property`]
> +/// trait to associate the integer types of various sizes with their
> +/// corresponding `fwnode_property_read_*_array` functions.
> +pub trait PropertyInt: Copy {
> + /// # Safety
> + ///
> + /// Callers must uphold the same safety invariants as for the various
> + /// `fwnode_property_read_*_array` functions.
> + unsafe fn read_array(
> + fwnode: *const bindings::fwnode_handle,
> + propname: *const ffi::c_char,
> + val: *mut Self,
> + nval: usize,
> + ) -> ffi::c_int;
> +}
> +// This macro generates implementations of the traits `Property` and
> +// `PropertyInt` for integers of various sizes. Its input is a list
> +// of pairs separated by commas. The first element of the pair is the
> +// type of the integer, the second one is the name of its corresponding
> +// `fwnode_property_read_*_array` function.
> +macro_rules! impl_property_for_int {
> + ($($int:ty: $f:ident),* $(,)?) => { $(
> + impl PropertyInt for $int {
> + unsafe fn read_array(
> + fwnode: *const bindings::fwnode_handle,
> + propname: *const ffi::c_char,
> + val: *mut Self,
> + nval: usize,
> + ) -> ffi::c_int {
> + // SAFETY: The safety invariants on the trait require
> + // callers to uphold the invariants of the functions
> + // this macro is called with.
> + unsafe {
> + bindings::$f(fwnode, propname, val.cast(), nval)
> + }
> + }
> + }
> + )* };
> +}
> +impl_property_for_int! {
> + u8: fwnode_property_read_u8_array,
> + u16: fwnode_property_read_u16_array,
> + u32: fwnode_property_read_u32_array,
> + u64: fwnode_property_read_u64_array,
> + i8: fwnode_property_read_u8_array,
> + i16: fwnode_property_read_u16_array,
> + i32: fwnode_property_read_u32_array,
> + i64: fwnode_property_read_u64_array,
> +}
> +/// # Safety
> +///
> +/// Callers must ensure that if `len` is non-zero, `out_param` must be
> +/// valid and point to memory that has enough space to hold at least
> +/// `len` number of elements.
> +unsafe fn read_array_out_param<T: PropertyInt>(
> + fwnode: &FwNode,
> + name: &CStr,
> + out_param: *mut T,
> + len: usize,
> +) -> ffi::c_int {
> + // SAFETY: `name` is non-null and null-terminated.
> + // `fwnode.as_raw` is valid because `fwnode` is valid.
> + // `out_param` is valid and has enough space for at least
> + // `len` number of elements as per the safety requirement.
> + unsafe { T::read_array(fwnode.as_raw(), name.as_char_ptr(), out_param, len) }
> +}
> +impl<T: PropertyInt, const N: usize> Property for [T; N] {
> + fn read(fwnode: &FwNode, name: &CStr) -> Result<Self> {
> + let mut val: [MaybeUninit<T>; N] = [const { MaybeUninit::uninit() }; N];
> +
> + // SAFETY: `val.as_mut_ptr()` is valid and points to enough space for
> + // `N` elements. Casting from `*mut MaybeUninit<T>` to `*mut T` is safe
> + // because `MaybeUninit<T>` has the same memory layout as `T`.
> + let ret = unsafe { read_array_out_param::<T>(fwnode, name, val.as_mut_ptr().cast(), N) };
> + to_result(ret)?;
> +
> + // SAFETY: `val` is always initialized when
> + // fwnode_property_read_<T>_array is successful.
> + Ok(val.map(|v| unsafe { v.assume_init() }))
> + }
> +}
> +impl<T: PropertyInt> Property for T {
> + fn read(fwnode: &FwNode, name: &CStr) -> Result<Self> {
> + let val: [_; 1] = <[T; 1] as Property>::read(fwnode, name)?;
> + Ok(val[0])
> + }
> +}
> +
> +/// A helper for reading device properties.
> +///
> +/// Use [`Self::required`] if a missing property is considered a bug and
> +/// [`Self::optional`] otherwise.
> +///
> +/// For convenience, [`Self::or`] and [`Self::or_default`] are provided.
> +pub struct PropertyGuard<'fwnode, 'name, T> {
> + /// The result of reading the property.
> + inner: Result<T>,
> + /// The fwnode of the property, used for logging in the "required" case.
> + fwnode: &'fwnode FwNode,
> + /// The name of the property, used for logging in the "required" case.
> + name: &'name CStr,
> +}
> +
> +impl<T> PropertyGuard<'_, '_, T> {
> + /// Access the property, indicating it is required.
> + ///
> + /// If the property is not present, the error is automatically logged. If a
> + /// missing property is not an error, use [`Self::optional`] instead.
> + pub fn required(self) -> Result<T> {
> + if self.inner.is_err() {
> + // Get the device associated with the fwnode for device-associated
> + // logging.
> + // TODO: Are we allowed to do this? The field `fwnode_handle.dev`
> + // has a somewhat vague comment, which could mean we're not
> + // supposed to access it:
> + // https://elixir.bootlin.com/linux/v6.13.6/source/include/linux/fwnode.h#L51
> + // SAFETY: According to the invariant of FwNode, it is valid.
> + let dev = unsafe { (*self.fwnode.as_raw()).dev };
I don't think this is valid it do, AFAICS, a firmware node handle doesn't own a
reference to the device pointer.
> +
> + if dev.is_null() {
> + pr_err!(
> + "{}: property '{}' is missing\n",
> + self.fwnode.display_path(),
> + self.name
> + );
> + } else {
> + // SAFETY: If dev is not null, it points to a valid device.
> + let dev: &Device = unsafe { &*dev.cast() };
> + dev_err!(
> + dev,
> + "{}: property '{}' is missing\n",
> + self.fwnode.display_path(),
> + self.name
> + );
> + };
> + }
> + self.inner
> + }
> +
> + /// Access the property, indicating it is optional.
> + ///
> + /// In contrast to [`Self::required`], no error message is logged if
> + /// the property is not present.
> + pub fn optional(self) -> Option<T> {
> + self.inner.ok()
> + }
> +
> + /// Access the property or the specified default value.
> + ///
> + /// Do not pass a sentinel value as default to detect a missing property.
> + /// Use [`Self::required`] or [`Self::optional`] instead.
> + pub fn or(self, default: T) -> T {
> + self.inner.unwrap_or(default)
> + }
> +}
> +
> +impl<T: Default> PropertyGuard<'_, '_, T> {
> + /// Access the property or a default value.
> + ///
> + /// Use [`Self::or`] to specify a custom default value.
> + pub fn or_default(self) -> T {
> + self.inner.unwrap_or_default()
> + }
> +}
> --
> 2.49.0
>
Powered by blists - more mailing lists