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
