pci_bridge_d3_update() does an unlocked read-modify-write of
bridge->bridge_d3, and its callers are not mutually serialized: the
d3cold_allowed sysfs write and the driver-context D3cold helpers hold
neither pci_rescan_remove_lock nor device_lock. A concurrent write can
lose an update and leave bridge_d3 stale, costing a wrong D3cold decision
rather than memory safety. An upcoming change runs pci_bus_add_device()
for sibling VFs concurrently, making sibling additions concurrent callers
too, so this must land first.
Add a mutex around the whole update, taken once for the propagation
loop. A device with no D3cold-capable port above it returns before the
mutex, so the common add is not funneled through a global lock, and the
loop re-evaluates both conditions under it. The mutex serializes the
updaters against each other only; the d3cold_allowed store itself still
writes an adjacent bit of the same word unlocked, a pre-existing
exposure this change neither widens nor closes. The resulting order is
pci_rescan_remove_lock, device_lock(any) -> pci_bridge_d3_lock ->
pci_bus_sem (read), so pci_bridge_d3_lock must never be acquired while
holding pci_bus_sem and no pci_walk_bus() callback may call into this
path.
The race dates back to commit 9d26d3a8f1b0 ("PCI: Put PCIe ports into
D3 during suspend"), is theoretical with no known report, and so
carries no Fixes: tag and no stable designation; it claims no measured
performance contribution.
Assisted-by: LLM
Signed-off-by: Pavol Sakac <[email protected]>
---
drivers/pci/pci.c | 26 ++++++++++++++++++++++++++
1 file changed, 26 insertions(+)
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index c62a315c0b4c..b2a159ef125b 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -3095,17 +3095,36 @@ static int pci_dev_check_d3cold(struct pci_dev *dev,
void *data)
}
/*
+ * Serializes pci_bridge_d3_update()'s bridge_d3 read-modify-writes and
+ * their upstream propagation. Ordering: pci_rescan_remove_lock,
+ * device_lock(any) -> pci_bridge_d3_lock -> pci_bus_sem (read); no
+ * pci_walk_bus() callback may call into this path.
+ */
+static DEFINE_MUTEX(pci_bridge_d3_lock);
+
+/**
* pci_bridge_d3_update - Update bridge D3 capabilities
* @dev: PCI device which is changed
*
* Update upstream bridge PM capabilities accordingly depending on if the
* device PM configuration was changed or the device is being removed. The
* change is also propagated upstream.
+ *
+ * Context: Process context. Takes and releases pci_bridge_d3_lock.
*/
void pci_bridge_d3_update(struct pci_dev *dev)
{
struct pci_dev *bridge;
+ /*
+ * Unlocked fast path; the loop condition re-evaluates both checks
+ * under the lock.
+ */
+ bridge = pci_upstream_bridge(dev);
+ if (!bridge || !pci_bridge_d3_possible(bridge))
+ return;
+
+ mutex_lock(&pci_bridge_d3_lock);
while ((bridge = pci_upstream_bridge(dev)) &&
pci_bridge_d3_possible(bridge)) {
bool remove = !device_is_registered(&dev->dev);
@@ -3148,6 +3167,7 @@ void pci_bridge_d3_update(struct pci_dev *dev)
/* Propagate change to upstream bridges */
dev = bridge;
}
+ mutex_unlock(&pci_bridge_d3_lock);
}
/**
@@ -3157,6 +3177,9 @@ void pci_bridge_d3_update(struct pci_dev *dev)
* This function can be used in drivers to enable D3cold from the device
* they handle. It also updates upstream PCI bridge PM capabilities
* accordingly.
+ *
+ * Context: Process context. Takes and releases pci_bridge_d3_lock;
+ * must not be called from a pci_walk_bus() callback.
*/
void pci_d3cold_enable(struct pci_dev *dev)
{
@@ -3174,6 +3197,9 @@ EXPORT_SYMBOL_GPL(pci_d3cold_enable);
* This function can be used in drivers to disable D3cold from the device
* they handle. It also updates upstream PCI bridge PM capabilities
* accordingly.
+ *
+ * Context: Process context. Takes and releases pci_bridge_d3_lock;
+ * must not be called from a pci_walk_bus() callback.
*/
void pci_d3cold_disable(struct pci_dev *dev)
{
--
2.47.3
