linux-kernel - Re: [PATCH v3 1/3] rust: device_id: split out index support into a separate trait

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: <20250711.121709.1360562848053380480.fujita.tomonori@gmail.com>
Date: Fri, 11 Jul 2025 12:17:09 +0900 (JST)
From: FUJITA Tomonori <fujita.tomonori@...il.com>
To: tmgross@...ch.edu
Cc: fujita.tomonori@...il.com, alex.gaynor@...il.com, dakr@...nel.org,
 gregkh@...uxfoundation.org, ojeda@...nel.org, rafael@...nel.org,
 robh@...nel.org, saravanak@...gle.com, a.hindborg@...nel.org,
 aliceryhl@...gle.com, bhelgaas@...gle.com, bjorn3_gh@...tonmail.com,
 boqun.feng@...il.com, david.m.ertman@...el.com,
 devicetree@...r.kernel.org, gary@...yguo.net, ira.weiny@...el.com,
 kwilczynski@...nel.org, leon@...nel.org, linux-kernel@...r.kernel.org,
 linux-pci@...r.kernel.org, lossin@...nel.org, netdev@...r.kernel.org,
 rust-for-linux@...r.kernel.org
Subject: Re: [PATCH v3 1/3] rust: device_id: split out index support into a
 separate trait

On Tue, 08 Jul 2025 23:10:48 -0400
"Trevor Gross" <tmgross@...ch.edu> wrote:

> On Fri Jul 4, 2025 at 12:10 AM EDT, FUJITA Tomonori wrote:
>> Introduce a new trait `RawDeviceIdIndex`, which extends `RawDeviceId`
>> to provide support for device ID types that include an index or
>> context field (e.g., `driver_data`). This separates the concerns of
>> layout compatibility and index-based data embedding, and allows
>> `RawDeviceId` to be implemented for types that do not contain a
>> `driver_data` field. Several such structures are defined in
>> include/linux/mod_devicetable.h.
>>
>> Refactor `IdArray::new()` into a generic `build()` function, which
>> takes an optional offset. Based on the presence of `RawDeviceIdIndex`,
>> index writing is conditionally enabled. A new `new_without_index()`
>> constructor is also provided for use cases where no index should be
>> written.
>>
>> This refactoring is a preparation for enabling the PHY abstractions to
>> use device_id trait.
>>
>> Acked-by: Danilo Krummrich <dakr@...nel.org>
>> Signed-off-by: FUJITA Tomonori <fujita.tomonori@...il.com>
>> ---
>>  rust/kernel/auxiliary.rs | 11 ++---
>>  rust/kernel/device_id.rs | 91 ++++++++++++++++++++++++++++------------
>>  rust/kernel/of.rs        | 15 ++++---
>>  rust/kernel/pci.rs       | 11 ++---
>>  4 files changed, 87 insertions(+), 41 deletions(-)
> 
> Few small suggestions if you wind up spinning this again:
> 
>> diff --git a/rust/kernel/device_id.rs b/rust/kernel/device_id.rs
>> [...]
>> @@ -68,7 +77,14 @@ impl<T: RawDeviceId, U, const N: usize> IdArray<T, U, N> {
>>      /// Creates a new instance of the array.
>>      ///
>>      /// The contents are derived from the given identifiers and context information.
>> -    pub const fn new(ids: [(T, U); N]) -> Self {
>> +    ///
>> +    /// # Safety
>> +    ///
>> +    /// If `offset` is `Some(offset)`, then:
>> +    /// - `offset` must be the correct offset (in bytes) to the context/data field
>> +    ///   (e.g., the `driver_data` field) within the raw device ID structure.
>> +    /// - The field at `offset` must be correctly sized to hold a `usize`.
>> +    const unsafe fn build(ids: [(T, U); N], offset: Option<usize>) -> Self {
> 
> Could you mention that calling with `offset` as `None` is always safe?

Indeed, added.

> Also calling the arg `data_offset` might be more clear.

Yeah, changed.

>> @@ -92,7 +111,6 @@ impl<T: RawDeviceId, U, const N: usize> IdArray<T, U, N> {
>>              infos[i] = MaybeUninit::new(unsafe { core::ptr::read(&ids[i].1) });
>>              i += 1;
>>          }
>> -
>>          core::mem::forget(ids);
> 
> This removes the space between a block and an expression, possibly
> unintentional? :)

Oops, unintentional. Dropped the change.

>> @@ -109,12 +127,33 @@ impl<T: RawDeviceId, U, const N: usize> IdArray<T, U, N> {
>>          }
>>      }
>>  
>> +    /// Creates a new instance of the array without writing index values.
>> +    ///
>> +    /// The contents are derived from the given identifiers and context information.
> 
> Maybe the docs here should crosslink:
> 
>     If the device implements [`RawDeviceIdIndex`], consider using
>     [`new`] instead.

Looks nice, added. [`new`] doesn't work so I use [`IdArray::new`].

>> +    pub const fn new_without_index(ids: [(T, U); N]) -> Self {
>> +        // SAFETY: Calling `Self::build` with `offset = None` is always safe,
>> +        // because no raw memory writes are performed in this case.
>> +        unsafe { Self::build(ids, None) }
>> +    }
>> +
> 
> With those changes, or as-is if there winds up not being another
> version:
> 
> Reviewed-by: Trevor Gross <tmgross@...ch.edu>

Thanks!