Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [High] The new `enable_device_managed` API uses `pcim_enable_device()`, which activates legacy automatic IRQ management that conflicts with Rust's `alloc_irq_vectors()` and violates a documented kernel API contract, leading to double-free issues. - [High] `disable_device` and `enable_device` are exposed as safe functions, allowing safe Rust code to unbalance the PCI device's `enable_cnt` reference count, causing soundness holes that lead to DMA-after-free. --
commit 47e71534c620f329a7271d6914a49b70de710719 Author: Maurice Hieronymus <[email protected]> Subject: rust: pci: rework device enabling API This commit replaces enable_device_mem() with enable_device_managed() to provide automatic cleanup on unbind via pcim_enable_device(). It also introduces an unmanaged enable_device()/disable_device() pair for paths like runtime PM, enabling both I/O and memory resources. Link: https://lore.kernel.org/r/[email protected] [1] Link: https://lore.kernel.org/rust-for-linux/[email protected]/ [2] [ ... ] > diff --git a/rust/kernel/pci.rs b/rust/kernel/pci.rs > index 5071cae6543fd..a408b34120777 100644 > --- a/rust/kernel/pci.rs > +++ b/rust/kernel/pci.rs > @@ -453,10 +453,30 @@ pub fn pci_class(&self) -> Class { > } > > 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. > + /// > + /// This function is unmanaged and does not perform any cleanup when the > device is unbound. > + /// For a managed function take a look at > [`Device::enable_device_managed`]. > + #[inline] > + pub fn enable_device(&self) -> Result { > + // SAFETY: `self.as_raw` is guaranteed to be a pointer to a valid > `struct pci_dev`. > + to_result(unsafe { bindings::pci_enable_device(self.as_raw()) }) > + } > + > + /// Disable I/O and memory resources for this device. > + /// > + /// This function is the counterpart to [`Device::enable_device`]. > + #[inline] > + pub fn disable_device(&self) { [Severity: High] Is it safe to expose these as safe Rust functions? Since they directly modify the underlying PCI device's enable_cnt reference count, safe Rust code could call them in an unbalanced way. For instance, calling enable_device() multiple times would inflate the count. During driver unload, the cleanup path would only decrement it once, leaving bus-mastering enabled and potentially leading to DMA-after-free when memory buffers are freed. Should these be marked unsafe, or require an RAII guard to track the enable count? > + // SAFETY: `self.as_raw` is guaranteed to be a pointer to a valid > `struct pci_dev`. > + unsafe { bindings::pci_disable_device(self.as_raw()) } > + } > + > + /// Enable I/O and memory resources for this device, with automatic > cleanup. > + #[inline] > + pub fn enable_device_managed(&self) -> Result { > // 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::pcim_enable_device(self.as_raw()) }) > } [Severity: High] Will wrapping pcim_enable_device() cause double-free issues with IRQ vectors? This function activates legacy C devres management for the device by setting pdev->is_managed = true. If a driver later calls alloc_irq_vectors(), devres will automatically register pcim_msi_release() as a cleanup action. However, Rust's IrqVectorRegistration object also explicitly calls pci_free_irq_vectors() on Drop. When the driver is unbound, Rust will drop the registration (freeing the vectors), and then the devres subsystem will execute pcim_msi_release(), attempting to free them again. -- Sashiko AI review ยท https://sashiko.dev/#/patchset/20260702-rust-pci-enable-device-managed-v1-1-75bc4ff29...@mailbox.org?part=1
