Re: [PATCH] vfio/pci: Fix vf_token mechanism when device-specific VF drivers are used

[Jason Gunthorpe <jgg@nvidia.com>]

On Fri, Apr 08, 2022 at 10:53:05AM -0600, Alex Williamson wrote:
> On Fri,  8 Apr 2022 12:10:15 -0300
> Jason Gunthorpe <jgg@nvidia.com> wrote:
> 
> > get_pf_vdev() tries to check if a PF is a VFIO PF by looking at the driver:
> > 
> >        if (pci_dev_driver(physfn) != pci_dev_driver(vdev->pdev)) {
> > 
> > However now that we have multiple VF and PF drivers this is no longer
> > reliable.
> > 
> > This means that security tests realted to vf_token can be skipped by
> > mixing and matching different VFIO PCI drivers.
> > 
> > Instead of trying to use the driver core to find the PF devices maintain a
> > linked list of all PF vfio_pci_core_device's that we have called
> > pci_enable_sriov() on.
> > 
> > When registering a VF just search the list to see if the PF is present and
> > record the match permanently in the struct. PCI core locking prevents a PF
> > from passing pci_disable_sriov() while VF drivers are attached so the VFIO
> > owned PF becomes a static property of the VF.
> > 
> > In common cases where vfio does not own the PF the global list remains
> > empty and the VF's pointer is statically NULL.
> > 
> > This also fixes a lockdep splat from recursive locking of the
> > vfio_group::device_lock between vfio_device_get_from_name() and
> > vfio_device_get_from_dev(). If the VF and PF share the same group this
> > would deadlock.
> > 
> > Fixes: ff53edf6d6ab ("vfio/pci: Split the pci_driver code out of vfio_pci_core.c")
> > Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
> >  drivers/vfio/pci/vfio_pci_core.c | 109 ++++++++++++++++---------------
> >  include/linux/vfio_pci_core.h    |   2 +
> >  2 files changed, 60 insertions(+), 51 deletions(-)
> > 
> > This is probably for the rc cycle since it only became a problem when the
> > migration drivers were merged.
> ...  
> > @@ -1942,14 +1935,28 @@ int vfio_pci_core_sriov_configure(struct pci_dev *pdev, int nr_virtfn)
> >  	if (!device)
> >  		return -ENODEV;
> >  
> > -	if (nr_virtfn == 0)
> > -		pci_disable_sriov(pdev);
> > -	else
> > +	vdev = container_of(device, struct vfio_pci_core_device, vdev);
> > +
> > +	if (nr_virtfn) {
> > +		mutex_lock(&vfio_pci_sriov_pfs_mutex);
> > +		list_add_tail(&vdev->sriov_pfs_item, &vfio_pci_sriov_pfs);
> > +		mutex_unlock(&vfio_pci_sriov_pfs_mutex);
> >  		ret = pci_enable_sriov(pdev, nr_virtfn);
> > +		if (ret)
> > +			goto out_del;
> > +		ret = nr_virtfn;
> > +		goto out_put;
> > +	}
> 
> If a user were to do:
> 
> 	# echo 1 > sriov_numvfs
> 	# echo 2 > sriov_numvfs
> 
> Don't we have a problem that we've botched the list and the PF still
> exists with 1 VF?  Thanks,

Yes, that is a mistake. We need to do the list_add before the
pci_enable_sriov because the probe() will inspect the
vfio_pci_sriov_pfs list.

But since pci_enable_sriov can only be called once we can just gaurd
directly against that.

I fixed it like this:

		mutex_lock(&vfio_pci_sriov_pfs_mutex);
		/*
		 * The thread that adds the vdev to the list is the only thread
		 * that gets to call pci_enable_sriov() and we will only allow
		 * it to be called once without going through
		 * pci_disable_sriov()
		 */
		if (!list_empty(&vdev->sriov_pfs_item)) {
			ret = -EINVAL;
			goto out_unlock;
		}
		list_add_tail(&vdev->sriov_pfs_item, &vfio_pci_sriov_pfs);
		mutex_unlock(&vfio_pci_sriov_pfs_mutex);
		ret = pci_enable_sriov(pdev, nr_virtfn);
		if (ret)
			goto out_del;

Let me know if you have any other notes and I will fix them before
resending

Thanks,
Jason