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@xxxxxxxxxx> 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@xxxxxxxxxx>
> > 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
