Re: [PATCH v4 6/9] rust: pci: add typed SR-IOV PF registration data

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] rust: pci: UB in VfRegistration::drop due to mutable aliasing
- [Low] rust: pci: missing #[inline] on small abstraction functions
--

commit b691159fa250909baaaa09e4f8331c57968a80d1
Author: Zhi Wang <zhiw@nvidia.com>

rust: pci: add typed SR-IOV PF registration data

Rust PF and VF drivers bind to separate PCI devices and may reside
in different modules. A VF driver that calls PF operations needs
typed access to the data exposed by the PF driver. This data must
remain valid until VF driver removal completes.

Co-developed-by: Danilo Krummrich <dakr@kernel.org>
Signed-off-by: Danilo Krummrich <dakr@kernel.org>
Signed-off-by: Zhi Wang <zhiw@nvidia.com>

> diff --git a/rust/kernel/pci/iov.rs b/rust/kernel/pci/iov.rs
> index 4ed6ba658e818..c61462595141f 100644
> --- a/rust/kernel/pci/iov.rs
> +++ b/rust/kernel/pci/iov.rs

[ ... ]

> +impl<'a, F: ForLt + 'static> VfRegistrationData<'a, F> {
> +    /// Pin-initializer for the registration data.
> +    fn new<E>(data: impl PinInit<F::Of<'a>, E>) -> impl PinInit<Self, E> {

[Severity: Low]
Does this small helper method need the #[inline] attribute?

The Rust subsystem guidelines explicitly state that small abstraction
functions forwarding to C bindings or performing minimal work should be
annotated with #[inline]. Missing this attribute can prevent the compiler
from properly inlining cross-crate calls, leading to a minor performance
penalty.

> +        try_pin_init!(Self {
> +            type_id: TypeId::of::<F>(),
> +            data <- data,
> +        }? E)
> +    }
> +}

[ ... ]

> +impl<'a, F: ForLt> VfRegistration<'a, F> {
> +    /// Borrows the registered data for use by the PF driver.
> +    ///
> +    /// The returned reference is tied to this registration, rather than the PF device: the
> +    /// registration may be destroyed before the rest of the PF's private data is dropped.
> +    pub fn data(self: Pin<&Self>) -> Pin<&F::Of<'a>> {

[Severity: Low]
Should this small forwarding abstraction function also be marked #[inline]?

> +        // SAFETY: Both `inner` and its `data` field are structurally pinned.
> +        unsafe { self.map_unchecked(|registration| &registration.inner.data) }
> +    }
> +}
> +
> +#[pinned_drop]
> +impl<F: ForLt + 'static> PinnedDrop for VfRegistration<'_, F> {
> +    fn drop(self: Pin<&mut Self>) {
> +        // SAFETY: `pci_disable_sriov()` is safe to call on any `pci_dev`; it
> +        // is a no-op if the device has no VFs enabled. When VFs are enabled,
> +        // this blocks until all VF `remove()` callbacks complete.
> +        unsafe { bindings::pci_disable_sriov(self.pdev.as_raw()) };

[Severity: High]
Can creating Pin<&mut Self> for this drop method cause undefined behavior
due to mutable aliasing?

When a PF driver is unbound, its VfRegistration is dropped, invoking this
drop method. This creates a mutable reference, asserting exclusive access to
the memory.

However, the code subsequently calls bindings::pci_disable_sriov(), which
blocks until all active VF drivers' remove callbacks finish. During this
blocked time, VFs are still executing and holding shared references
(Pin<&F::Of<'a>>) to the inner registration data.

Creating a unique mutable reference while shared references still exist
invalidates the shared references and violates Rust's aliasing rules.

> +
> +        // SAFETY: The device is valid and all VF remove callbacks have completed, so no VF
> +        // can access the registration pointer. The data remains alive until this returns.
> +        unsafe { (*self.pdev.as_raw()).vf_registration_data_rust = core::ptr::null_mut() };
> +    }
> +}

[ ... ]

> +    /// Access the VF registration data through a closure with an HRTB lifetime.
> +    ///
> +    /// `F` is the [`ForLt`](trait@ForLt) encoding of the data type. Returns
> +    /// [`ENODEV`] if this is not a VF or no data was registered,
> +    /// or [`EINVAL`] if `F` does not match the type registered by the PF.
> +    ///
> +    /// The reference is borrowed from this VF, while the registration data's lifetime remains
> +    /// abstract so the closure cannot store shorter-lived references in invariant data.
> +    pub fn vf_registration_data_with<'this, F: ForLt + 'static, R>(
> +        &'this self,
> +        f: impl for<'a> FnOnce(Pin<&'this F::Of<'a>>) -> R,
> +    ) -> Result<R> {

[Severity: Low]
Does this small abstraction function need the #[inline] attribute?

> +        // SAFETY: The closure is higher-ranked over the data lifetime, independently of `'this`.
> +        // It cannot insert shorter-lived references, and the outer borrow cannot outlive this VF.
> +        let pinned = unsafe { self.vf_registration_data_pinned::<F>()? };
> +        Ok(f(pinned))
> +    }
> +
> +    /// Returns a pinned reference to the VF registration data.
> +    ///
> +    /// Available only when `F` implements [`CovariantForLt`](trait@crate::types::CovariantForLt),
> +    /// guaranteeing that shortening the PF data lifetime is sound.
> +    ///
> +    /// For non-covariant types, use [`Self::vf_registration_data_with()`].
> +    ///
> +    /// PF drivers access their own data through [`VfRegistration::data()`].
> +    ///
> +    /// It returns the same errors as [`Self::vf_registration_data_with()`].
> +    pub fn vf_registration_data<F: CovariantForLt + 'static>(&self) -> Result<Pin<&F::Of<'_>>> {

[Severity: Low]
Should this small forwarding abstraction function also be marked #[inline]?

> +        // SAFETY: `CovariantForLt` permits shortening the encoded lifetime to this borrow.
> +        unsafe { self.vf_registration_data_pinned::<F>() }
> +    }
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261004120732.1045629-1-zhiw@nvidia.com?part=6