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: <DA0T1M8YEHZ9.1AW3IGD1IZX7Z@kernel.org>
Date: Tue, 20 May 2025 09:21:13 +0200
From: "Benno Lossin" <lossin@...nel.org>
To: "Remo Senekowitsch" <remo@...nzli.dev>, "Danilo Krummrich"
 <dakr@...nel.org>
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>, "Dirk Behme" <dirk.behme@...bosch.com>,
 <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 Mon May 19, 2025 at 9:51 PM CEST, Remo Senekowitsch wrote:
> On Mon May 19, 2025 at 6:55 PM CEST, Danilo Krummrich wrote:
>> On Mon, May 19, 2025 at 05:43:17PM +0200, Remo Senekowitsch wrote:
>>> On Mon May 12, 2025 at 3:36 PM CEST, Danilo Krummrich 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.
>>> >
>>> > I think you have additional requirements on the fwnode, propname and val
>>> > pointers as well as on nval, please document them as well.
>>> 
>>> What are the additional requirements? The implementation just calls the
>>> underlying `fwnode_property_read_*_array` with the exact same arguments,
>>> so I don't know what the additional requirements are.
>>
>> First of all, I don't think you can refer to the safety requirements of the
>> `fwnode_property_read_*_array` functions, since they don't have any documented
>> safety requirements.

Yes. We do sometimes link to other function for safety requirements if
they are very repetitive, but in that case we use a rustdoc link (so
[`my_other_function`]).

In this case, one doesn't even have a name to search for, only the
`fwnode_property_read_` prefix (and who is going to bother doing that?).
Additionally, the functions that you are referring to are from the
`bindings`! Those functions do not have safety documentation!

Another thing, functions do not have safety invariants, they only have
safety requirements and guarantees.

>> So, I think you have safety requirements regarding pointer validity of fwnode,
>> propname and val.
>>
>> Additionally, there's the requirement that val has to be an array of nval
>> length.

Yes these two are probably required, but to be sure one would have to
dig through the C code.

>> Also, the PropertyInt trait itself has to be unsafe, given that it contains
>> unsafe functions.
>
> I don't think a trait necessarily has to be marked unsafe just because
> it has unsafe methods. Marking a trait as unsafe means that implementors
> of the trait must uphold some invariants. This is not the case here
> IIUC. Here's a good explanation of my understanding: [1]

Yes this is correct, I don't think that the trait itself should be
unsafe.

> But I should anyway seal the two traits. They're not supposed to be
> implemented outside the kernel crate.

Yes that sounds like a good idea.

I'll send a separate email for some more comments on the design.

---
Cheers,
Benno

> [1] https://users.rust-lang.org/t/safe-trait-with-an-unsafe-method/67993/3
>
>> I also pinged Benno about it, he usually knows best how to cover such things
>> properly. :)
>>
>>> >> +    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;
>>> >> +}