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,
Alex
>
> + pci_disable_sriov(pdev);
> +
> +out_del:
> + mutex_lock(&vfio_pci_sriov_pfs_mutex);
> + list_del_init(&vdev->sriov_pfs_item);
> + mutex_unlock(&vfio_pci_sriov_pfs_mutex);
> +out_put:
> vfio_device_put(device);
> -
> - return ret < 0 ? ret : nr_virtfn;
> + return ret;
> }
> EXPORT_SYMBOL_GPL(vfio_pci_core_sriov_configure);
