Re: [PATCH v4 8/9] rust: pci: add SR-IOV enable and disable callbacks

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] rust: pci: unsound lifetime unification in sriov callbacks allows token escaping
- [Low] rust: pci: missing #[inline] on small abstraction functions
--

commit 953cc4a950356a2c3c6e81d99e604484bd175fb4
Author: Peter Colberg <peter@colberg.org>

rust: pci: add SR-IOV enable and disable callbacks

Allow Rust PCI drivers to configure SR-IOV through sriov_numvfs.
Split the C configure callback into sriov_enable() and sriov_disable(),
passing pinned driver data and a token for the requested operation.
Require drivers to implement both callbacks together.

> diff --git a/rust/kernel/pci.rs b/rust/kernel/pci.rs
> index 7486c51799659..f4f92d79bac4b 100644
> --- a/rust/kernel/pci.rs
> +++ b/rust/kernel/pci.rs
[ ... ]
> @@ -341,6 +351,87 @@ fn probe<'bound>(
>      fn unbind<'bound>(dev: &'bound Device<device::Core<'_>>, this: Pin<&Self::Data<'bound>>) {
>          let _ = (dev, this);
>      }
> +
> +    /// Enables Single Root I/O Virtualization (SR-IOV) for a PF.
[ ... ]
> +    #[cfg(CONFIG_PCI_IOV)]
> +    fn sriov_enable<'a>(
> +        dev: &'a Device<device::Core<'_>>,
> +        data: Pin<&Self::Data<'a>>,
> +        token: SriovEnable<'a>,
> +    ) -> Result<SriovEnabled<'a>> {

[Severity: Low]
Should this default trait method be annotated with #[inline]? According to the
Rust subsystem guidelines, small functions or functions forwarding to C
bindings in the kernel abstraction crate should be inlined.

[Severity: High]
Does this signature allow a driver to escape the SriovEnabled token?

By unifying the lifetime 'a across the data parameter and the token parameter,
it appears a driver could safely store the short-lived token inside its
long-lived Data structure.

If this happens, could the driver dereference a dangling Device reference
when the data is dropped during device unbind, leading to a use-after-free?

Would separating the lifetimes fix this, as shown below?

    fn sriov_enable<'a, 'b>(
        dev: &'a Device<device::Core<'_>>,
        data: Pin<&Self::Data<'b>>,
        token: SriovEnable<'a>,
    ) -> Result<SriovEnabled<'a>> {

> +        let _ = (dev, data, token);
> +        build_error!(crate::error::VTABLE_DEFAULT_ERROR)
> +    }
> +
> +    /// Disables all VFs of a PF in response to a userspace request.
[ ... ]
> +    #[cfg(CONFIG_PCI_IOV)]
> +    fn sriov_disable<'a>(
> +        dev: &'a Device<device::Core<'_>>,
> +        data: Pin<&Self::Data<'a>>,
> +        token: SriovDisable<'a>,
> +    ) -> Result {

[Severity: High]
As with sriov_enable(), does unifying the lifetime 'a here allow the
driver to improperly escape the short-lived token into its Data structure?

Should the data parameter use a separate lifetime 'b to prevent this
soundness issue?

> +        let _ = (dev, data, token);
> +        build_error!(crate::error::VTABLE_DEFAULT_ERROR)
> +    }
>  }
>  
>  /// The PCI device representation.
> diff --git a/rust/kernel/pci/iov.rs b/rust/kernel/pci/iov.rs
> index 449f1e9b12fda..24d1770226c32 100644
> --- a/rust/kernel/pci/iov.rs
> +++ b/rust/kernel/pci/iov.rs
[ ... ]
> @@ -100,7 +107,7 @@ pub fn num_vfs(&self) -> u16 {

[Severity: Low]
Should the SriovEnable::num_vfs getter be annotated with #[inline] to ensure
the optimizer can properly inline it?

>      ///
>      /// VF drivers can probe before this method returns, so the PF resources they access must
>      /// already be initialized. The returned guard disables the VFs if subsequent setup fails.
> -    /// Return it from `sriov_enable()` to leave the VFs enabled on callback success.
> +    /// Return it from [`Driver::sriov_enable()`] to leave the VFs enabled on callback success.
>      pub fn enable(self, num_vfs: u16) -> Result<SriovEnabled<'a>> {
>          if num_vfs == 0 || num_vfs > self.num_vfs {
>              return Err(EINVAL);
> @@ -120,14 +127,13 @@ pub fn enable(self, num_vfs: u16) -> Result<SriovEnabled<'a>> {
>      }
>  }
>  
> -/// Enabled VFs awaiting successful completion of `sriov_enable()`.
> +/// Enabled VFs awaiting successful completion of [`Driver::sriov_enable()`].
>  pub struct SriovEnabled<'a> {
>      pdev: &'a Device<device::CoreInternal<'a>>,
>      num_vfs: u16,
>  }
>  
>  impl SriovEnabled<'_> {
> -    #[expect(dead_code)]
>      fn disarm(self) -> u16 {

[Severity: Low]
Should this trivial abstraction function be annotated with #[inline]?

>          ManuallyDrop::new(self).num_vfs
>      }
[ ... ]
> @@ -139,7 +145,7 @@ fn drop(&mut self) {
>      }
>  }
>  
> -/// Permission to disable VFs during a driver's `sriov_disable()` callback.
> +/// Permission to disable VFs during a driver's [`Driver::sriov_disable()`] callback.
>  ///
>  /// The PCI adapter creates this token for the PF being configured. It cannot be cloned or sent
>  /// to another thread, and its lifetime is restricted to the callback. Dropping the token does
> @@ -157,6 +163,37 @@ pub fn disable(self) {

[Severity: Low]
Should this small wrapper forwarding to a C binding be annotated with
#[inline]?

>      }
>  }
>  
> +impl<T: Driver> Adapter<T> {
> +    pub(super) extern "C" fn sriov_configure_callback(
> +        pdev: *mut bindings::pci_dev,
> +        nr_virtfn: c_int,
> +    ) -> c_int {
> +        // SAFETY: The PCI core calls this through the PF-only `sriov_numvfs` sysfs attribute,
> +        // with a valid `struct pci_dev` and while holding the device lock.
> +        //
> +        // INVARIANT: `pdev` is valid for the duration of `sriov_configure_callback()`.
> +        let pdev = unsafe { &*pdev.cast::<Device<device::CoreInternal<'_>>>() };
> +
> +        // SAFETY: `sriov_configure` is called only after a successful probe and before unbind, so
> +        // the stored pointer has type `T::Data<'_>` and remains valid throughout this callback.
> +        let data = unsafe { pdev.as_ref().drvdata_borrow::<T::Data<'_>>() };

[Severity: High]
Does this unsafe cast combined with the unified lifetime in sriov_enable
and sriov_disable create a soundness hole?

Because drvdata_borrow performs an unchecked raw pointer cast, the compiler
infers the anonymous lifetime as the short callback lifetime 'a required by
the trait signatures. This circumvents variance rules and casts the originally
'bound data to T::Data<'a>.

If the lifetimes are separated in the trait definition, would that prevent the
compiler from allowing a short-lived token to be stored in the long-lived
driver data?

> +
> +        from_result(|| {
> +            if nr_virtfn == 0 {
> +                T::sriov_disable(pdev, data, SriovDisable { pdev })?;
> +                if pdev.num_vf() != 0 {
> +                    return Err(EBUSY);
> +                }
> +                Ok(0)
> +            } else {
> +                let num_vfs = u16::try_from(nr_virtfn).map_err(|_| EINVAL)?;
> +                let enabled = T::sriov_enable(pdev, data, SriovEnable { pdev, num_vfs })?;
> +                Ok(enabled.disarm().into())
> +            }
> +        })
> +    }
> +}

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