linux-kernel - Re: [PATCH 7/8] rust: pci: add physfn(), to return PF device for VF device

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: <aTH4HifpRnqWA6X8@earendel>
Date: Thu, 4 Dec 2025 16:07:42 -0500
From: Peter Colberg <pcolberg@...hat.com>
To: Jason Gunthorpe <jgg@...pe.ca>
Cc: Danilo Krummrich <dakr@...nel.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>,
	Benno Lossin <lossin@...nel.org>,
	Andreas Hindborg <a.hindborg@...nel.org>,
	Alice Ryhl <aliceryhl@...gle.com>, Trevor Gross <tmgross@...ch.edu>,
	Abdiel Janulgue <abdiel.janulgue@...il.com>,
	Daniel Almeida <daniel.almeida@...labora.com>,
	Robin Murphy <robin.murphy@....com>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	Dave Ertman <david.m.ertman@...el.com>,
	Ira Weiny <ira.weiny@...el.com>, Leon Romanovsky <leon@...nel.org>,
	linux-pci@...r.kernel.org, rust-for-linux@...r.kernel.org,
	linux-kernel@...r.kernel.org,
	Alexandre Courbot <acourbot@...dia.com>,
	Alistair Popple <apopple@...dia.com>,
	Joel Fernandes <joelagnelf@...dia.com>,
	John Hubbard <jhubbard@...dia.com>, Zhi Wang <zhiw@...dia.com>
Subject: Re: [PATCH 7/8] rust: pci: add physfn(), to return PF device for VF
 device

On Fri, Nov 21, 2025 at 07:26:42PM -0400, Jason Gunthorpe wrote:
> On Wed, Nov 19, 2025 at 05:19:11PM -0500, Peter Colberg wrote:
> > Add a method to return the Physical Function (PF) device for a Virtual
> > Function (VF) device in the bound device context.
> > 
> > Unlike for a PCI driver written in C, guarantee that when a VF device is
> > bound to a driver, the underlying PF device is bound to a driver, too.
> 
> You can't do this as an absolutely statement from rust code alone,
> this statement is confused.
> 
> > When a device with enabled VFs is unbound from a driver, invoke the
> > sriov_configure() callback to disable SR-IOV before the unbind()
> > callback. To ensure the guarantee is upheld, call disable_sriov()
> > to remove all VF devices if the driver has not done so already.
> 
> This doesn't seem like it should be in this patch.
> 
> Good drivers using the PCI APIs should be calling pci_disable_sriov()
> during their unbind. 
> 
> The prior patch #3 should not have added "safe" bindings for
> enable_sriov that allow this lifetime rule to be violated.
> 
> > +    #[cfg(CONFIG_PCI_IOV)]
> > +    pub fn physfn(&self) -> Result<&Device<device::Bound>> {
> > +        if !self.is_virtfn() {
> > +            return Err(EINVAL);
> > +        }
> > +        // SAFETY:
> > +        // `self.as_raw` returns a valid pointer to a `struct pci_dev`.
> > +        //
> > +        // `physfn` is a valid pointer to a `struct pci_dev` since self.is_virtfn() is `true`.
> > +        //
> > +        // `physfn` may be cast to a `Device<device::Bound>` since `pci::Driver::remove()` calls
> > +        // `disable_sriov()` to remove all VF devices, which guarantees that the underlying
> > +        // PF device is always bound to a driver when the VF device is bound to a driver.
> 
> Wrong safety statement. There are drivers that don't call
> disable_sriov(). You need to also check that the driver attached to
> the PF is actually working properly.

Thank you for the review. I missed the obvious case where the
PF driver is a C driver that does not disable SR-IOV on unbind
and the VF driver is a Rust driver that invokes physfn().

> I do not like to see this attempt to open code the tricky login of
> pci_iov_get_pf_drvdata() in rust without understanding the issues :(

I will work on a proper solution based on Danilo's proposal [1].

[1] https://lore.kernel.org/all/DEFL4TG0WX1C.2GLH4417EPU3V@kernel.org/

Thanks,
Peter