Re: [PATCH v6 2/5] PCI: Add locking of upstream bridge for pci_reset_function()

[Alex Williamson <alex.williamson@redhat.com>]

On Thu, 23 May 2024 14:17:58 -0700
Dan Williams <dan.j.williams@intel.com> wrote:

> Alex Williamson wrote:
> > On Thu,  2 May 2024 09:57:31 -0700
> > Dave Jiang <dave.jiang@intel.com> 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@intel.com>
> > > Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> > > Signed-off-by: Dave Jiang <dave.jiang@intel.com>
> > > ---  
> [..]
> > > 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.

Thanks, Dan!  Yes, this fixes it.  Please feel free to add
Reported/Tested-by tags when you post it.  Thanks!

Alex

> -- >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);
>  
>