Re: [PATCH v3 2/5] rust: pci: rework device enabling API

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:

New issues:
- [High] Replacing `pci_enable_device_mem()` with `pci_enable_device()` causes probe failure on platforms with unassigned legacy I/O BARs.
- [Low] Missing `#[must_use]` attribute on `DeviceEnableGuard` allows silent, immediate device disablement.
- [Low] Missing `#[inline]` attribute on the `enable_device()` abstraction method.

Pre-existing issues:
- [High] Incorrect struct field drop order causes MMIO unmap before child device unregistration (Use-After-Free).
--

--- Patch [2]: [PATCH v3 2/5] rust: pci: rework device enabling API ---
commit d9e225d5ab32388fa1a595e83eeef86ed5e5b2b9
Author: Maurice Hieronymus <[email protected]>

rust: pci: rework device enabling API

The commit replaces the unmanaged enable_device_mem() with enable_device(),
returning a resource guard DeviceEnableGuard that automatically runs
pci_disable_device() on drop. It also converts the only user, nova-core, to
use the new API.

Link: https://lore.kernel.org/rust-for-linux/[email protected]

> diff --git a/drivers/gpu/nova-core/driver.rs b/drivers/gpu/nova-core/driver.rs
> index 5738d4ac521b7..99b15da59e819 100644
> --- a/drivers/gpu/nova-core/driver.rs
> +++ b/drivers/gpu/nova-core/driver.rs
> @@ -30,6 +30,8 @@ pub(crate) struct NovaCore<'bound> {
>      bar: pci::Bar<'bound, BAR0_SIZE>,
>      #[allow(clippy::type_complexity)]
>      _reg: auxiliary::Registration<'bound, ForLt!(())>,

[Severity: High]
This is a pre-existing issue, but does the struct field drop order in NovaCore
cause memory mapped I/O to be unmapped before child devices are unregistered?

Rust drops struct fields in their exact declaration order. Here, bar is
declared before _reg. When the driver is unbound, bar is dropped (unmapping
memory mapped I/O) before _reg is dropped (unregistering auxiliary devices).
If those child devices attempt to access hardware to quiesce it during their
remove callbacks, could this result in a use-after-free or page fault by
accessing unmapped memory?

> +    // Declared last so the device stays enabled until everything above is torn down.
> +    _enable: pci::DeviceEnableGuard<'bound>,
>  }

[ ... ]

> @@ -75,7 +77,7 @@ fn probe<'bound>(
>          pin_init::pin_init_scope(move || {
>              dev_dbg!(pdev, "Probe Nova Core GPU driver.\n");
>  
> -            pdev.enable_device_mem()?;
> +            let enable = pdev.enable_device()?;

[Severity: High]
Does replacing pci_enable_device_mem() with pci_enable_device() cause probe
failures on platforms with unassigned legacy I/O base address registers?

The previous method, pci_enable_device_mem(), safely bypassed this by only
requesting memory resources, which is typically all modern devices need. Since
pci_enable_device() internally requests both memory and I/O resources, could
this cause probe failures on platforms (such as many arm64 systems or modern
x86 without legacy vga I/O) where a device has an I/O register but the
firmware or pci core did not assign resources for it?

>              pdev.set_master();

[ ... ]

> diff --git a/rust/kernel/pci.rs b/rust/kernel/pci.rs
> index 4def9ca1824ce..bd9a8113af358 100644
> --- a/rust/kernel/pci.rs
> +++ b/rust/kernel/pci.rs
> @@ -452,11 +452,36 @@ pub fn pci_class(&self) -> Class {
>      }
>  }
>  
> +/// A guard that keeps the device's I/O and memory resources enabled.
> +///
> +/// # Invariants
> +///
> +/// The device's enable count was incremented once for this guard; dropping the guard decrements
> +/// it again.
> +pub struct DeviceEnableGuard<'a> {

[Severity: Low]
Could the lack of a #[must_use] attribute on DeviceEnableGuard lead to silent
logic errors?

If a caller uses the API as pdev.enable_device()?; without binding the
returned guard to a variable, the guard is immediately dropped, running
pci_disable_device() right after enablement. While nova-core uses it
correctly, omitting #[must_use] on guards might allow logic errors to
compile without warnings.

> +    dev: &'a Device<device::Bound>,
> +}

[ ... ]

>  impl<'a> Device<device::Core<'a>> {
> -    /// Enable memory resources for this device.
> -    pub fn enable_device_mem(&self) -> Result {
> +    /// Enable I/O and memory resources for this device.
> +    ///
> +    /// The device stays enabled for the lifetime of the returned guard; dropping the guard
> +    /// disables the device again. The guard borrows the device's bound scope, so it cannot
> +    /// outlive the driver binding.
> +    pub fn enable_device(&self) -> Result<DeviceEnableGuard<'_>> {

[Severity: Low]
This isn't a bug, but should the enable_device() forwarding abstraction
function be annotated with #[inline]?

The rust guidelines state: "For abstractions ONLY: Functions that are
small or forwarding to a binding call should be annotated with #[inline]."

>          // SAFETY: `self.as_raw` is guaranteed to be a pointer to a valid `struct pci_dev`.
> -        to_result(unsafe { bindings::pci_enable_device_mem(self.as_raw()) })
> +        to_result(unsafe { bindings::pci_enable_device(self.as_raw()) })?;
> +
> +        // INVARIANT: `pci_enable_device()` succeeded, so the enable count was incremented once.
> +        Ok(DeviceEnableGuard { dev: self })
>      }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2