linux-kernel - Re: [PATCH v6 2/5] rust: pci: provide access to PCI Vendor values

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: <b1cbdc99-317e-454c-bf03-d6793be5b13c@nvidia.com>
Date: Tue, 26 Aug 2025 13:38:18 -0700
From: John Hubbard <jhubbard@...dia.com>
To: Danilo Krummrich <dakr@...nel.org>,
 Alexandre Courbot <acourbot@...dia.com>
Cc: Joel Fernandes <joelagnelf@...dia.com>, Timur Tabi <ttabi@...dia.com>,
 Alistair Popple <apopple@...dia.com>, David Airlie <airlied@...il.com>,
 Simona Vetter <simona@...ll.ch>, Bjorn Helgaas <bhelgaas@...gle.com>,
 Krzysztof Wilczyński <kwilczynski@...nel.org>,
 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 <lossin@...nel.org>, Andreas Hindborg <a.hindborg@...nel.org>,
 Alice Ryhl <aliceryhl@...gle.com>, Trevor Gross <tmgross@...ch.edu>,
 nouveau@...ts.freedesktop.org, linux-pci@...r.kernel.org,
 rust-for-linux@...r.kernel.org, LKML <linux-kernel@...r.kernel.org>,
 Elle Rhumsaa <elle@...thered-steel.dev>
Subject: Re: [PATCH v6 2/5] rust: pci: provide access to PCI Vendor values

On 8/25/25 5:47 AM, Danilo Krummrich wrote:
> On Mon Aug 25, 2025 at 2:33 PM CEST, Alexandre Courbot wrote:
...
>> Naive question from someone with a device tree background and almost no
>> PCI experience: one consequence of using `From` here is that if I create
>> an non-registered Vendor value (e.g. `let vendor =
>> Vendor::from(0xf0f0)`), then do `vendor.as_raw()`, I won't get the value
>> passed initially but the one for `UNKNOWN`, e.g. `0xffff`. Are we ok
>> with this?
> 
> I think that's fine, since we shouldn't actually hit this. Drivers should only
> ever use the pre-defined constants of Vendor; consequently the
> Device::vendor_id() can't return UNKNOWN either.
> 
> So, I think the From impl is not ideal, since we can't limit its visibility. In
> order to improve this, I suggest to use Vendor::new() directly in the macro, and
> make Vendor::new() private. The same goes for Class, I guess.

Correction: when I went to implement this, I discovered that there is a better
way, which addresses both Alex's and your concerns. 

The incremental diff below shows how. It provides:

a) .from_raw(), which in this case matches conventions slightly better
   than new(). (I'm still learning that the Rust way is a bit different
   that the C++ way! haha).

b) Only the parent module (in this case, that's pci:: ) can call
   Class::from_raw(). This is exactly what we need. Fully private methods
   wouldn't work, but leaving it open for any caller to construct a
   Class item is also a problem.

c) Restored infallible operations, and with it, Alex's request for a
   reasonable behavior here now works once again:

       from_raw(0x10de).as_raw() == 0x10de
       

diff --git a/rust/kernel/pci.rs b/rust/kernel/pci.rs
index 0faec49bf8a2..40047a7433b1 100644
--- a/rust/kernel/pci.rs
+++ b/rust/kernel/pci.rs
@@ -418,7 +418,7 @@ pub fn resource_len(&self, bar: u32) -> Result<bindings::resource_size_t> {
     /// Returns the PCI class as a `Class` struct.
     pub fn pci_class(&self) -> Class {
         // SAFETY: `self.as_raw` is a valid pointer to a `struct pci_dev`.
-        Class::new(unsafe { (*self.as_raw()).class })
+        Class::from_raw(unsafe { (*self.as_raw()).class })
     }
 }
 
diff --git a/rust/kernel/pci/id.rs b/rust/kernel/pci/id.rs
index 1291553b4e15..399436ffaab9 100644
--- a/rust/kernel/pci/id.rs
+++ b/rust/kernel/pci/id.rs
@@ -51,24 +51,15 @@ impl Class {
                 pub const $variant: Self = Self(Self::to_24bit_class($binding));
             )+
         }
-
-        /// Convert a raw 24-bit class code value to a `Class`.
-        impl From<u32> for Class {
-            fn from(value: u32) -> Self {
-                match value {
-                    $(x if x == Self::$variant.0 => Self::$variant,)+
-                    _ => Self::UNKNOWN,
-                }
-            }
-        }
     };
 }
 
 /// Once constructed, a `Class` contains a valid PCI Class code.
 impl Class {
-    /// Create a new Class from a raw 24-bit class code.
-    pub fn new(class_code: u32) -> Self {
-        Self::from(class_code)
+    /// Create a Class from a raw 24-bit class code.
+    /// Only accessible from the parent pci module.
+    pub(super) fn from_raw(class_code: u32) -> Self {
+        Self(class_code)
     }
 
     /// Get the raw 24-bit class code value.
@@ -235,5 +226,4 @@ fn try_from(value: u32) -> Result<Self, Self::Error> {
     ACCELERATOR_PROCESSING     = bindings::PCI_CLASS_ACCELERATOR_PROCESSING,     // 0x120000
 
     OTHERS                     = bindings::PCI_CLASS_OTHERS,                     // 0xff0000
-    UNKNOWN                    = 0xffffff,
 }


thanks,
-- 
John Hubbard