Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
From:   Yicong Yang <yangyicong@hisilicon.com>
To:     <helgaas@kernel.org>, <linux-kernel@vger.kernel.org>
CC:     <linuxarm@huawei.com>, <yangyicong@hisilicon.com>
Subject: [PATCH] PCI: Fix race condition between block_cfg_access and msi_enabled/msix_enabled
Date:   Mon, 7 Sep 2020 19:06:05 +0800
Message-ID: <1599476765-47123-1-git-send-email-yangyicong@hisilicon.com>
MIME-Version: 1.0
Content-Type: text/plain
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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 <yangyicong@hisilicon.com>
---
 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