Previously we use bit field for block_cfg_access and
msi_enabled/msix_enabled, which is non-atomic and they may race
each other as they share the same memory region. A race condition
is met between driver bind vs FLR through sysfs:
for driver bind side in thread 1:
...
device_lock()
...
->probe()
pci_alloc_irq_vectors_affinity()
__pci_enable_msi_range()
msi_capability_init()
dev->msi_enabled=1 <---set here
request_irq(pci_irq_vector(),...)
when echo 1 > reset in thread 2:
pci_reset_function()
pci_dev_lock()
pci_cfg_access_lock()
dev->block_cfg_access=1 <---may overwrite msi_enabled bit
device_lock()
The msi_enabled bit may be overwritten to 0 and will trigger the WARN
assert in pci_irq_vector(). A similar issue has been addressed in
commit 44bda4b7d26e ("PCI: Fix is_added/is_busmaster race condition").
Move the block_cfg_access to the pci_dev->priv_flags and use atomic
bit operations to avoid the race condition.
Signed-off-by: Yicong Yang <[email protected]>
---
drivers/pci/access.c | 20 ++++++++++----------
drivers/pci/pci.h | 11 +++++++++++
include/linux/pci.h | 1 -
3 files changed, 21 insertions(+), 11 deletions(-)
diff --git a/drivers/pci/access.c b/drivers/pci/access.c
index 4693569..5826962 100644
--- a/drivers/pci/access.c
+++ b/drivers/pci/access.c
@@ -208,9 +208,9 @@ static noinline void pci_wait_cfg(struct pci_dev *dev)
{
do {
raw_spin_unlock_irq(&pci_lock);
- wait_event(pci_cfg_wait, !dev->block_cfg_access);
+ wait_event(pci_cfg_wait, !pci_dev_is_cfg_access_blocked(dev));
raw_spin_lock_irq(&pci_lock);
- } while (dev->block_cfg_access);
+ } while (pci_dev_is_cfg_access_blocked(dev));
}
/* Returns 0 on success, negative values indicate error. */
@@ -223,7 +223,7 @@ int pci_user_read_config_##size
\
if (PCI_##size##_BAD) \
return -EINVAL; \
raw_spin_lock_irq(&pci_lock); \
- if (unlikely(dev->block_cfg_access)) \
+ if (unlikely(pci_dev_is_cfg_access_blocked(dev)))
\
pci_wait_cfg(dev); \
ret = dev->bus->ops->read(dev->bus, dev->devfn, \
pos, sizeof(type), &data); \
@@ -242,7 +242,7 @@ int pci_user_write_config_##size
\
if (PCI_##size##_BAD) \
return -EINVAL; \
raw_spin_lock_irq(&pci_lock); \
- if (unlikely(dev->block_cfg_access)) \
+ if (unlikely(pci_dev_is_cfg_access_blocked(dev)))
\
pci_wait_cfg(dev); \
ret = dev->bus->ops->write(dev->bus, dev->devfn, \
pos, sizeof(type), val); \
@@ -271,9 +271,9 @@ void pci_cfg_access_lock(struct pci_dev *dev)
might_sleep();
raw_spin_lock_irq(&pci_lock);
- if (dev->block_cfg_access)
+ if (pci_dev_is_cfg_access_blocked(dev))
pci_wait_cfg(dev);
- dev->block_cfg_access = 1;
+ pci_dev_block_cfg_access(dev, true);
raw_spin_unlock_irq(&pci_lock);
}
EXPORT_SYMBOL_GPL(pci_cfg_access_lock);
@@ -292,10 +292,10 @@ bool pci_cfg_access_trylock(struct pci_dev *dev)
bool locked = true;
raw_spin_lock_irqsave(&pci_lock, flags);
- if (dev->block_cfg_access)
+ if (pci_dev_is_cfg_access_blocked(dev))
locked = false;
else
- dev->block_cfg_access = 1;
+ pci_dev_block_cfg_access(dev, true);
raw_spin_unlock_irqrestore(&pci_lock, flags);
return locked;
@@ -318,9 +318,9 @@ void pci_cfg_access_unlock(struct pci_dev *dev)
* This indicates a problem in the caller, but we don't need
* to kill them, unlike a double-block above.
*/
- WARN_ON(!dev->block_cfg_access);
+ WARN_ON(!pci_dev_is_cfg_access_blocked(dev));
- dev->block_cfg_access = 0;
+ pci_dev_block_cfg_access(dev, false);
raw_spin_unlock_irqrestore(&pci_lock, flags);
wake_up_all(&pci_cfg_wait);
diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index 73740dd..1cf3122 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -410,6 +410,7 @@ static inline bool pci_dev_is_disconnected(const struct
pci_dev *dev)
/* pci_dev priv_flags */
#define PCI_DEV_ADDED 0
+#define PCI_DEV_BLOCK_CFG_ACCESS 1 /* Config space access blocked */
static inline void pci_dev_assign_added(struct pci_dev *dev, bool added)
{
@@ -421,6 +422,16 @@ static inline bool pci_dev_is_added(const struct pci_dev
*dev)
return test_bit(PCI_DEV_ADDED, &dev->priv_flags);
}
+static inline void pci_dev_block_cfg_access(struct pci_dev *dev, bool block)
+{
+ assign_bit(PCI_DEV_BLOCK_CFG_ACCESS, &dev->priv_flags, block);
+}
+
+static inline bool pci_dev_is_cfg_access_blocked(struct pci_dev *dev)
+{
+ return test_bit(PCI_DEV_BLOCK_CFG_ACCESS, &dev->priv_flags);
+}
+
#ifdef CONFIG_PCIEAER
#include <linux/aer.h>
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 8355306..4ffb588 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -406,7 +406,6 @@ struct pci_dev {
unsigned int is_busmaster:1; /* Is busmaster */
unsigned int no_msi:1; /* May not use MSI */
unsigned int no_64bit_msi:1; /* May only use 32-bit MSIs */
- unsigned int block_cfg_access:1; /* Config space access blocked
*/
unsigned int broken_parity_status:1; /* Generates false positive
parity */
unsigned int irq_reroute_variant:2; /* Needs IRQ rerouting variant
*/
unsigned int msi_enabled:1;
--
2.8.1
