Re: [PATCH v2 1/2] PCI: Export pci_parent_bus_reset() for drivers to use

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Unprotected list traversal and data race in `pci_parent_bus_reset()` exposed to external drivers.
- [High] Exporting `pci_parent_bus_reset()` creates an unsafe API that bypasses mandatory PCI core state management and synchronization.
--

commit 73a6ae560868e4a44decb13db21d60108f4c09f1
Author: Maciej W. Rozycki <macro@orcam.me.uk>

PCI: Export pci_parent_bus_reset() for drivers to use

This commit removes the static modifier from pci_parent_bus_reset(),
adds kernel doc comments, and exports it using EXPORT_SYMBOL_GPL()
so that external drivers can trigger a secondary bus reset via the
device's immediate upstream bridge.

> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -4820,7 +4820,21 @@ int pci_bridge_secondary_bus_reset(struct pci_dev *dev)
[ ... ]
> +int pci_parent_bus_reset(struct pci_dev *dev, bool probe)
>  {
>  	struct pci_dev *pdev;
>  

Internally, pci_parent_bus_reset() traverses dev->bus->devices using
list_for_each_entry() locklessly.

Reading this list typically requires holding pci_bus_sem to protect against
concurrent modifications from hotplug events.

Since pci_bus_sem is an internal lock and not exported to modules, drivers
utilizing this newly exported API will not be able to acquire the required
locks prior to calling it.

Does exporting this function introduce a regression regarding list
corruption or use-after-free?

> @@ -4837,6 +4851,7 @@ static int pci_parent_bus_reset(struct pci_dev *dev, bool probe)
>  
>  	return pci_bridge_secondary_bus_reset(dev->bus->self);
>  }
> +EXPORT_SYMBOL_GPL(pci_parent_bus_reset);

Unlike higher-level APIs like pci_reset_function(), this newly exported
function does not save and restore the PCI configuration space or prepare
the IOMMU for DMA reset.

Additionally, pci_bridge_secondary_bus_reset() expects the bridge
configuration access to be locked, and emits a warning if it is not:

pci_bridge_secondary_bus_reset() {
    if (!dev->block_cfg_access)
        pci_warn_once(dev, "unlocked secondary bus reset via...");
}

Could drivers utilizing this export trigger system instability or
uncorrectable errors while DMA is active, since it bypasses state
management and locking steps like acquiring pci_dev_lock() on the upstream
bridge?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/alpine.DEB.2.21.2605050042390.46195@angie.orcam.me.uk?part=1