Re: [PATCH] pci: iov: use device lock to protect IOV sysfs accesses

[Bjorn Helgaas]

Hi Jakub,

On Mon, May 22, 2017 at 03:50:23PM -0700, Jakub Kicinski wrote:
> PCI core sets the driver pointer before calling ->probe() and only
> clears it after ->remove().  This means driver's ->sriov_configure()
> callback will happily race with probe() and remove(), most likely
> leading to BUGs, since drivers don't expect this.

I guess you're referring to the pci_dev->driver pointer set by
local_pci_probe(), and this is important because sriov_numvfs_store()
checks that pointer, right?

I was trying to make sure there weren't other similar problems elsewhere.
But I don't see any other sysfs functions that use pci_dev->driver in that
way, so I think this is the only one.

I think this looks good.

    pci_bus_add_devices
      pci_bus_add_device
        pci_create_sysfs_dev_files
        device_attach
          __device_attach
            device_lock(dev)
            __device_attach_driver
              ...
                local_pci_probe
                  pci_dev->driver = pci_drv       <--- set
                  pci_drv->probe()
            device_unlock(dev)

    sriov_numvfs_store
  -   mutex_lock(&iov->dev->sriov->lock)
  +   device_lock(&pdev->dev)
      if (pdev->driver && pdev->driver->sriov_configure)  <--- test
        pdev->driver->sriov_configure
  -   mutex_unlock(&iov->dev->sriov->lock)
  +   device_unlock(&pdev->dev)

> We could reorder pointer assignments, or try detecting races in all
> drivers, but it seems simpler and cleaner to just hold the device lock
> instead of special SR-IOV lock, since that lock is already supposed
> to synchronize the driver callbacks.
> 
> Remove the iov lock completely, since we remove the last user.
> 
> Signed-off-by: Jakub Kicinski <jakub.kicinski@xxxxxxxxxxxxx>
> ---
>  drivers/pci/iov.c       | 4 ----
>  drivers/pci/pci-sysfs.c | 5 ++---
>  drivers/pci/pci.h       | 1 -
>  3 files changed, 2 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/pci/iov.c b/drivers/pci/iov.c
> index d9dc7363ac77..120485d6f352 100644
> --- a/drivers/pci/iov.c
> +++ b/drivers/pci/iov.c
> @@ -461,8 +461,6 @@ static int sriov_init(struct pci_dev *dev, int pos)
>  	else
>  		iov->dev = dev;
>  
> -	mutex_init(&iov->lock);
> -
>  	dev->sriov = iov;
>  	dev->is_physfn = 1;
>  	rc = compute_max_vf_buses(dev);
> @@ -491,8 +489,6 @@ static void sriov_release(struct pci_dev *dev)
>  	if (dev != dev->sriov->dev)
>  		pci_dev_put(dev->sriov->dev);
>  
> -	mutex_destroy(&dev->sriov->lock);
> -
>  	kfree(dev->sriov);
>  	dev->sriov = NULL;
>  }
> diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
> index 31e99613a12e..7755559558df 100644
> --- a/drivers/pci/pci-sysfs.c
> +++ b/drivers/pci/pci-sysfs.c
> @@ -472,7 +472,6 @@ static ssize_t sriov_numvfs_store(struct device *dev,
>  				  const char *buf, size_t count)
>  {
>  	struct pci_dev *pdev = to_pci_dev(dev);
> -	struct pci_sriov *iov = pdev->sriov;
>  	int ret;
>  	u16 num_vfs;
>  
> @@ -483,7 +482,7 @@ static ssize_t sriov_numvfs_store(struct device *dev,
>  	if (num_vfs > pci_sriov_get_totalvfs(pdev))
>  		return -ERANGE;
>  
> -	mutex_lock(&iov->dev->sriov->lock);
> +	device_lock(&pdev->dev);
>  
>  	if (num_vfs == pdev->sriov->num_VFs)
>  		goto exit;
> @@ -518,7 +517,7 @@ static ssize_t sriov_numvfs_store(struct device *dev,
>  			 num_vfs, ret);
>  
>  exit:
> -	mutex_unlock(&iov->dev->sriov->lock);
> +	device_unlock(&pdev->dev);
>  
>  	if (ret < 0)
>  		return ret;
> diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
> index f8113e5b9812..93f4044b8f4b 100644
> --- a/drivers/pci/pci.h
> +++ b/drivers/pci/pci.h
> @@ -272,7 +272,6 @@ struct pci_sriov {
>  	u16 driver_max_VFs;	/* max num VFs driver supports */
>  	struct pci_dev *dev;	/* lowest numbered PF */
>  	struct pci_dev *self;	/* this PF */
> -	struct mutex lock;	/* lock for setting sriov_numvfs in sysfs */
>  	resource_size_t barsz[PCI_SRIOV_NUM_BARS];	/* VF BAR size */
>  	bool drivers_autoprobe;	/* auto probing of VFs by driver */
>  };
> -- 
> 2.11.0
>