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] [day] [month] [year] [list]

Message-ID: <abk5cp3hi3o6tyfnfz3lq5t6l4vgb5txnmyle4lvf56kj7zhx5@i4rddy5xdm6f>
Date: Mon, 28 Jul 2025 10:09:02 +1000
From: Alistair Popple <apopple@...dia.com>
To: Danilo Krummrich <dakr@...nel.org>
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 11:51:48AM +0200, Danilo Krummrich wrote:
> 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.

Thanks, although I don't have any burning need to get them in for this cycle.
Next cycle would be fine too, I just wanted to make progress getting them off my
TODO list and it wasn't clear if more changes were needed.

On that front is seems like the discussion has settled on maybe we need to do
something in future, but not for these patches? So aside from the minor nit
below (which I will fix) I don't know of anything else that needs changing.

> 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.