linux-kernel - Re: [PATCH v4 6/9] rust: device: Add bindings for reading device properties

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <DA0TDQ2DPTRX.1T9O4NZEME2JX@kernel.org>
Date: Tue, 20 May 2025 09:37:02 +0200
From: "Benno Lossin" <lossin@...nel.org>
To: "Remo Senekowitsch" <remo@...nzli.dev>, "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>,
 "Danilo Krummrich" <dakr@...nel.org>, "Greg Kroah-Hartman"
 <gregkh@...uxfoundation.org>, "Rafael J. Wysocki" <rafael@...nel.org>,
 "Dirk Behme" <dirk.behme@...bosch.com>
Cc: <linux-kernel@...r.kernel.org>, <devicetree@...r.kernel.org>,
 <rust-for-linux@...r.kernel.org>
Subject: Re: [PATCH v4 6/9] rust: device: Add bindings for reading device
 properties

On Sun May 4, 2025 at 7:31 PM CEST, Remo Senekowitsch wrote:
> +/// 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_from_fwnode_property(
> +        fwnode: *const bindings::fwnode_handle,
> +        propname: *const ffi::c_char,
> +        val: *mut Self,
> +        nval: usize,
> +    ) -> ffi::c_int;

I really, really dislike that this trait has to have an unsafe function
with all those raw pointer inputs.

> +}
> +// 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_from_fwnode_property(
> +                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_from_fwnode_property(fwnode.as_raw(), name.as_char_ptr(), out_param, len)
> +    }
> +}

Why does this function exist? It doesn't do anything and just delegates
the call to `T::read_array_from_fwnode_property`.

This feels like you're dragging the C interface through the lower layers
of your Rust abstractions, which I wouldn't do. I also looked a bit at
the C code and saw this comment in `driver/base/property.c:324`:

     * It's recommended to call fwnode_property_count_u8() instead of calling
     * this function with @val equals %NULL and @nval equals 0.

That probably holds also for the other functions, so maybe we should do
that instead? (although `fwnode_property_count_u8` just delegates and
calls with `fwnode_property_read_u8_array`...)

How about we do it like this:

    pub trait PropertyInt: Copy + Sized + private::Sealed {
        /// ...
        ///
        /// Returns a reference to `out` containing all written elements.
        fn read<'a>(
            fwnode: &FwNode,
            name: &CStr,
            out: &'a mut [MaybeUninit<Self>],
        ) -> Result<&'a mut [Self]>;

        fn length(fwnode: &FwNode, name: &CStr) -> Result<usize>;
    }

And then have a macro to implement it on all the integers.

Hope this helps!

---
Cheers,
Benno