Commit 7e89efc6 authored by Dave Jiang's avatar Dave Jiang Committed by Bjorn Helgaas
Browse files

PCI: Lock upstream bridge for pci_reset_function() 



Fix a long-standing locking gap for missing pci_cfg_access_lock() while
manipulating bridge reset registers and configuration during
pci_reset_bus_function().

If there is an upstream bridge, lock it before locking the device itself.
pci_dev_lock() calls pci_cfg_access_lock(), which blocks the writing of PCI
config space by user space.

Add lockdep assertion via pci_dev->cfg_access_lock to verify
pci_dev->block_cfg_access is set.

Co-developed-by: default avatarDan Williams <dan.j.williams@intel.com>
Link: https://lore.kernel.org/r/20240502165851.1948523-3-dave.jiang@intel.com


Signed-off-by: default avatarDan Williams <dan.j.williams@intel.com>
Signed-off-by: default avatarDave Jiang <dave.jiang@intel.com>
[bhelgaas: commit log]
Signed-off-by: default avatarBjorn Helgaas <bhelgaas@google.com>
parent 962f1e79

drivers/pci/access.c

+4 −0
Original line number Diff line number Diff line
@@ -275,6 +275,8 @@ void pci_cfg_access_lock(struct pci_dev *dev) 
{

	might_sleep();



	lock_map_acquire(&dev->cfg_access_lock);



	raw_spin_lock_irq(&pci_lock);

	if (dev->block_cfg_access)

		pci_wait_cfg(dev);
@@ -329,6 +331,8 @@ void pci_cfg_access_unlock(struct pci_dev *dev) 
	raw_spin_unlock_irqrestore(&pci_lock, flags);



	wake_up_all(&pci_cfg_wait);



	lock_map_release(&dev->cfg_access_lock);

}

EXPORT_SYMBOL_GPL(pci_cfg_access_unlock);

drivers/pci/pci.c

+13 −0
Original line number Diff line number Diff line
@@ -4879,6 +4879,7 @@ void __weak pcibios_reset_secondary_bus(struct pci_dev *dev) 
 */

int pci_bridge_secondary_bus_reset(struct pci_dev *dev)

{

	lock_map_assert_held(&dev->cfg_access_lock);

	pcibios_reset_secondary_bus(dev);



	return pci_bridge_wait_for_secondary_bus(dev, "bus reset");
@@ -5245,11 +5246,20 @@ void pci_init_reset_methods(struct pci_dev *dev) 
 */

int pci_reset_function(struct pci_dev *dev)

{

	struct pci_dev *bridge;

	int rc;



	if (!pci_reset_supported(dev))

		return -ENOTTY;



	/*

	 * If there's no upstream bridge, no locking is needed since there is

	 * no upstream bridge configuration to hold consistent.

	 */

	bridge = pci_upstream_bridge(dev);

	if (bridge)

		pci_dev_lock(bridge);



	pci_dev_lock(dev);

	pci_dev_save_and_disable(dev);
@@ -5258,6 +5268,9 @@ int pci_reset_function(struct pci_dev *dev) 
	pci_dev_restore(dev);

	pci_dev_unlock(dev);



	if (bridge)

		pci_dev_unlock(bridge);



	return rc;

}

EXPORT_SYMBOL_GPL(pci_reset_function);

drivers/pci/probe.c

+3 −0
Original line number Diff line number Diff line
@@ -2543,6 +2543,9 @@ void pci_device_add(struct pci_dev *dev, struct pci_bus *bus) 
	dev->dev.dma_mask = &dev->dma_mask;

	dev->dev.dma_parms = &dev->dma_parms;

	dev->dev.coherent_dma_mask = 0xffffffffull;

	lockdep_register_key(&dev->cfg_access_key);

	lockdep_init_map(&dev->cfg_access_lock, dev_name(&dev->dev),

			 &dev->cfg_access_key, 0);



	dma_set_max_seg_size(&dev->dev, 65536);

	dma_set_seg_boundary(&dev->dev, 0xffffffff);

include/linux/lockdep.h

+5 −0
Original line number Diff line number Diff line
@@ -297,6 +297,9 @@ extern void lock_unpin_lock(struct lockdep_map *lock, struct pin_cookie); 
		.wait_type_inner = _wait_type,		\

		.lock_type = LD_LOCK_WAIT_OVERRIDE, }



#define lock_map_assert_held(l)		\

	lockdep_assert(lock_is_held(l) != LOCK_STATE_NOT_HELD)



#else /* !CONFIG_LOCKDEP */



static inline void lockdep_init_task(struct task_struct *task)
@@ -388,6 +391,8 @@ extern int lockdep_is_held(const void *); 
#define DEFINE_WAIT_OVERRIDE_MAP(_name, _wait_type)	\

	struct lockdep_map __maybe_unused _name = {}



#define lock_map_assert_held(l)			do { (void)(l); } while (0)



#endif /* !LOCKDEP */



#ifdef CONFIG_PROVE_LOCKING

include/linux/pci.h

+2 −0
Original line number Diff line number Diff line
@@ -413,6 +413,8 @@ struct pci_dev { 
	struct resource driver_exclusive_resource;	 /* driver exclusive resource ranges */



	bool		match_driver;		/* Skip attaching driver */

	struct lock_class_key cfg_access_key;

	struct lockdep_map cfg_access_lock;



	unsigned int	transparent:1;		/* Subtractive decode bridge */

	unsigned int	io_window:1;		/* Bridge has I/O window */