> From: Jason Gunthorpe <jgg@xxxxxxxxxx>
> Sent: Saturday, March 13, 2021 8:56 AM
>
> vfio_add_group_dev() must be called only after all of the private data in
> vdev is fully setup and ready, otherwise there could be races with user
> space instantiating a device file descriptor and starting to call ops.
>
> For instance vfio_pci_reflck_attach() sets vdev->reflck and
> vfio_pci_open(), called by fops open, unconditionally derefs it, which
> will crash if things get out of order.
>
> Fixes: cc20d7999000 ("vfio/pci: Introduce VF token")
> Fixes: e309df5b0c9e ("vfio/pci: Parallelize device open and release")
> Fixes: 6eb7018705de ("vfio-pci: Move idle devices to D3hot power state")
> Fixes: ecaa1f6a0154 ("vfio-pci: Add VGA arbiter client")
> Signed-off-by: Jason Gunthorpe <jgg@xxxxxxxxxx>
> ---
> drivers/vfio/pci/vfio_pci.c | 17 +++++++++--------
> 1 file changed, 9 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/vfio/pci/vfio_pci.c b/drivers/vfio/pci/vfio_pci.c
> index f95b58376156a0..0e7682e7a0b478 100644
> --- a/drivers/vfio/pci/vfio_pci.c
> +++ b/drivers/vfio/pci/vfio_pci.c
> @@ -2030,13 +2030,9 @@ static int vfio_pci_probe(struct pci_dev *pdev,
> const struct pci_device_id *id)
> INIT_LIST_HEAD(&vdev->vma_list);
> init_rwsem(&vdev->memory_lock);
>
> - ret = vfio_add_group_dev(&pdev->dev, &vfio_pci_ops, vdev);
> - if (ret)
> - goto out_free;
> -
> ret = vfio_pci_reflck_attach(vdev);
> if (ret)
> - goto out_del_group_dev;
> + goto out_free;
> ret = vfio_pci_vf_init(vdev);
> if (ret)
> goto out_reflck;
> @@ -2060,15 +2056,20 @@ static int vfio_pci_probe(struct pci_dev *pdev,
> const struct pci_device_id *id)
> vfio_pci_set_power_state(vdev, PCI_D3hot);
> }
>
> - return ret;
> + ret = vfio_add_group_dev(&pdev->dev, &vfio_pci_ops, vdev);
> + if (ret)
> + goto out_power;
> + return 0;
>
> +out_power:
> + if (!disable_idle_d3)
> + vfio_pci_set_power_state(vdev, PCI_D0);
Just curious whether the power state must be recovered upon failure here.
>From the comment several lines above, the power state is set to an unknown
state before doing D3 transaction. From this point it looks fine if leaving the
device in D3 since there is no expected state to be recovered?
> out_vf:
> vfio_pci_vf_uninit(vdev);
> out_reflck:
> vfio_pci_reflck_put(vdev->reflck);
> -out_del_group_dev:
> - vfio_del_group_dev(&pdev->dev);
> out_free:
> + kfree(vdev->pm_save);
> kfree(vdev);
> out_group_put:
> vfio_iommu_group_put(group, &pdev->dev);
> --
> 2.30.2
