On Fri, 8 Apr 2022 14:08:39 -0300
Jason Gunthorpe <jgg@xxxxxxxxxx> wrote:
> 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
Nope, that's all I spotted. Looks like a reasonable fix. Thanks,
Alex
