linux-kernel - Re: [PATCH v2 1/2] rust: Update PCI binding safety comments and add inline compiler hint

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: <DBIHP8IP3OHA.8Y1S9ZV1Y1SZ@kernel.org>
Date: Tue, 22 Jul 2025 11:51:48 +0200
From: "Danilo Krummrich" <dakr@...nel.org>
To: "Alistair Popple" <apopple@...dia.com>
Cc: "Benno Lossin" <lossin@...nel.org>, <rust-for-linux@...r.kernel.org>,
 "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>, "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>,
 "John Hubbard" <jhubbard@...dia.com>, "Alexandre Courbot"
 <acourbot@...dia.com>, <linux-pci@...r.kernel.org>,
 <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2 1/2] rust: Update PCI binding safety comments and add
 inline compiler hint

On Tue Jul 22, 2025 at 7:17 AM CEST, Alistair Popple wrote:
> On Fri, Jul 11, 2025 at 10:46:13PM +0200, Benno Lossin wrote:
>> On Fri Jul 11, 2025 at 9:33 PM CEST, Danilo Krummrich wrote:
>> > On Fri Jul 11, 2025 at 8:30 PM CEST, Benno Lossin wrote:
>> >> On Fri Jul 11, 2025 at 5:02 PM CEST, Danilo Krummrich wrote:
>> >>> On Thu Jul 10, 2025 at 10:01 AM CEST, Benno Lossin wrote:
>> >>>> On Thu Jul 10, 2025 at 4:24 AM CEST, Alistair Popple wrote:
>> >>>>> diff --git a/rust/kernel/pci.rs b/rust/kernel/pci.rs
>> >>>>> index 8435f8132e38..5c35a66a5251 100644
>> >>>>> --- a/rust/kernel/pci.rs
>> >>>>> +++ b/rust/kernel/pci.rs
>> >>>>> @@ -371,14 +371,18 @@ fn as_raw(&self) -> *mut bindings::pci_dev {
>> >>>>>  
>> >>>>>  impl Device {
>> >>>>>      /// Returns the PCI vendor ID.
>> >>>>> +    #[inline]
>> >>>>>      pub fn vendor_id(&self) -> u16 {
>> >>>>> -        // SAFETY: `self.as_raw` is a valid pointer to a `struct pci_dev`.
>> >>>>> +        // SAFETY: by its type invariant `self.as_raw` is always a valid pointer to a
>> >>>>
>> >>>> s/by its type invariant/by the type invariants of `Self`,/
>> >>>> s/always//
>> >>>>
>> >>>> Also, which invariant does this refer to? The only one that I can see
>> >>>> is:
>> >>>>
>> >>>>     /// A [`Device`] instance represents a valid `struct device` created by the C portion of the kernel.
>> >>>>
>> >>>> And this doesn't say anything about the validity of `self.as_raw()`...
>> >>>
>> >>> Hm...why not? If an instance of Self always represents a valid struct pci_dev,
>> >>> then consequently self.as_raw() can only be a valid pointer to a struct pci_dev,
>> >>> no?
>> >>
>> >> While it's true, you need to look into the implementation of `as_raw`.
>> >> It could very well return a null pointer...
>> >>
>> >> This is where we can use a `Guarantee` on that function. But since it's
>> >> not shorter than `.0.get()`, I would just remove it.
>> >
>> > We have 15 to 20 as_raw() methods of this kind in the tree. If this really needs
>> > a `Guarantee` to be clean, we should probably fix it up in a treewide change.
>> >
>> > as_raw() is a common pattern and everyone knows what it does, `.0.get()` seems
>> > much less obvious.
>
> Coming from a C kernel programming background I agree `.as_raw()` is more
> obvious than `.0.get()`. However now I'm confused ... what if anything needs
> changing to get these two small patches merged?

I think they're good, but we're pretty late in the cycle now. That should be
fine though, we can probably take them through the nova tree, or in the worst
case share a tag, if needed.

Given that, it would probably be good to add the Guarantee section on as_raw(),
as proposed by Benno, right away.

@Benno: Any proposal on what this section should say?

One minor nit would be to start the safety comments with a capital letter
instead.