Alex Williamson wrote:
> On Thu, 2 May 2024 09:57:31 -0700
> Dave Jiang <dave.jiang@xxxxxxxxx> wrote:
>
> > Fix a long standing locking gap for missing pci_cfg_access_lock() while
> > manipulating bridge reset registers and configuration during
> > pci_reset_bus_function(). Add calling of pci_dev_lock() against the
> > bridge device before locking the device. The locking is conditional
> > depending on whether the trigger device has an upstream bridge. If
> > the device is a root port then there would be no upstream bridge and
> > thus the locking of the bridge is unnecessary. As part of calling
> > pci_dev_lock(), pci_cfg_access_lock() happens and blocks the writing
> > of PCI config space by user space.
> >
> > Add lockdep assertion via pci_dev->cfg_access_lock in order to verify
> > pci_dev->block_cfg_access is set.
> >
> > Co-developed-by: Dan Williams <dan.j.williams@xxxxxxxxx>
> > Signed-off-by: Dan Williams <dan.j.williams@xxxxxxxxx>
> > Signed-off-by: Dave Jiang <dave.jiang@xxxxxxxxx>
> > ---
[..]
> > diff --git a/drivers/pci/access.c b/drivers/pci/access.c
> > index 6449056b57dd..36f10c7f9ef5 100644
> > --- a/drivers/pci/access.c
> > +++ b/drivers/pci/access.c
> > @@ -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);
>
>
> This doesn't account for config access locks acquired via
> pci_cfg_access_trylock(), such as the pci_dev_trylock() through
> pci_try_reset_function() resulting in a new lockdep warning for
> vfio-pci when we try to release a lock that was never acquired.
> Thanks,
Hey Alex, sorry about that, does this fix it up for you? Note I move the
lock_map_acquire() relative to the global pci_lock just for symmetry
purposes.
-- >8 --
diff --git a/drivers/pci/access.c b/drivers/pci/access.c
index 30f031de9cfe..3595130ff719 100644
--- a/drivers/pci/access.c
+++ b/drivers/pci/access.c
@@ -289,11 +289,10 @@ 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);
+ lock_map_acquire(&dev->cfg_access_lock);
dev->block_cfg_access = 1;
raw_spin_unlock_irq(&pci_lock);
}
@@ -315,8 +314,10 @@ bool pci_cfg_access_trylock(struct pci_dev *dev)
raw_spin_lock_irqsave(&pci_lock, flags);
if (dev->block_cfg_access)
locked = false;
- else
+ else {
+ lock_map_acquire(&dev->cfg_access_lock);
dev->block_cfg_access = 1;
+ }
raw_spin_unlock_irqrestore(&pci_lock, flags);
return locked;
@@ -342,11 +343,10 @@ void pci_cfg_access_unlock(struct pci_dev *dev)
WARN_ON(!dev->block_cfg_access);
dev->block_cfg_access = 0;
+ lock_map_release(&dev->cfg_access_lock);
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);
