Hi,
On 3/13/21 1:55 AM, Jason Gunthorpe wrote:
> vfio_pci_probe() is quite complicated, with optional VF and VGA sub
> components. Move these into clear init/uninit functions and have a linear
> flow in probe/remove.
>
> This fixes a few little buglets:
> - vfio_pci_remove() is in the wrong order, vga_client_register() removes
> a notifier and is after kfree(vdev), but the notifier refers to vdev,
> so it can use after free in a race.
> - vga_client_register() can fail but was ignored
>
> Organize things so destruction order is the reverse of creation order.
>
> Fixes: ecaa1f6a0154 ("vfio-pci: Add VGA arbiter client")
> Signed-off-by: Jason Gunthorpe <jgg@xxxxxxxxxx>
Reviewed-by: Eric Auger <eric.auger@xxxxxxxxxx>
Eric
> ---
> drivers/vfio/pci/vfio_pci.c | 116 +++++++++++++++++++++++-------------
> 1 file changed, 74 insertions(+), 42 deletions(-)
>
> diff --git a/drivers/vfio/pci/vfio_pci.c b/drivers/vfio/pci/vfio_pci.c
> index 65e7e6b44578c2..f95b58376156a0 100644
> --- a/drivers/vfio/pci/vfio_pci.c
> +++ b/drivers/vfio/pci/vfio_pci.c
> @@ -1922,6 +1922,68 @@ static int vfio_pci_bus_notifier(struct notifier_block *nb,
> return 0;
> }
>
> +static int vfio_pci_vf_init(struct vfio_pci_device *vdev)
> +{
> + struct pci_dev *pdev = vdev->pdev;
> + int ret;
> +
> + if (!pdev->is_physfn)
> + return 0;
> +
> + vdev->vf_token = kzalloc(sizeof(*vdev->vf_token), GFP_KERNEL);
> + if (!vdev->vf_token)
> + return -ENOMEM;
> +
> + mutex_init(&vdev->vf_token->lock);
> + uuid_gen(&vdev->vf_token->uuid);
> +
> + vdev->nb.notifier_call = vfio_pci_bus_notifier;
> + ret = bus_register_notifier(&pci_bus_type, &vdev->nb);
> + if (ret) {
> + kfree(vdev->vf_token);> + return ret;
> + }
> + return 0;
> +}
> +
> +static void vfio_pci_vf_uninit(struct vfio_pci_device *vdev)
> +{
> + if (!vdev->vf_token)
> + return;
> +
> + bus_unregister_notifier(&pci_bus_type, &vdev->nb);
> + WARN_ON(vdev->vf_token->users);
> + mutex_destroy(&vdev->vf_token->lock);
> + kfree(vdev->vf_token);
> +}
> +
> +static int vfio_pci_vga_init(struct vfio_pci_device *vdev)
> +{
> + struct pci_dev *pdev = vdev->pdev;
> + int ret;
> +
> + if (!vfio_pci_is_vga(pdev))
> + return 0;
> +
> + ret = vga_client_register(pdev, vdev, NULL, vfio_pci_set_vga_decode);
> + if (ret)
> + return ret;
> + vga_set_legacy_decoding(pdev, vfio_pci_set_vga_decode(vdev, false));
> + return 0;
> +}
> +
> +static void vfio_pci_vga_uninit(struct vfio_pci_device *vdev)
> +{
> + struct pci_dev *pdev = vdev->pdev;
> +
> + if (!vfio_pci_is_vga(pdev))
> + return;
> + vga_client_register(pdev, NULL, NULL, NULL);
> + vga_set_legacy_decoding(pdev, VGA_RSRC_NORMAL_IO | VGA_RSRC_NORMAL_MEM |
> + VGA_RSRC_LEGACY_IO |
> + VGA_RSRC_LEGACY_MEM);
> +}
> +
> static int vfio_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
> {
> struct vfio_pci_device *vdev;
> @@ -1975,28 +2037,12 @@ static int vfio_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
> ret = vfio_pci_reflck_attach(vdev);
> if (ret)
> goto out_del_group_dev;
> -
> - if (pdev->is_physfn) {
> - vdev->vf_token = kzalloc(sizeof(*vdev->vf_token), GFP_KERNEL);
> - if (!vdev->vf_token) {
> - ret = -ENOMEM;
> - goto out_reflck;
> - }
> -
> - mutex_init(&vdev->vf_token->lock);
> - uuid_gen(&vdev->vf_token->uuid);
> -
> - vdev->nb.notifier_call = vfio_pci_bus_notifier;
> - ret = bus_register_notifier(&pci_bus_type, &vdev->nb);
> - if (ret)
> - goto out_vf_token;
> - }
> -
> - if (vfio_pci_is_vga(pdev)) {
> - vga_client_register(pdev, vdev, NULL, vfio_pci_set_vga_decode);
> - vga_set_legacy_decoding(pdev,
> - vfio_pci_set_vga_decode(vdev, false));
> - }
> + ret = vfio_pci_vf_init(vdev);
> + if (ret)
> + goto out_reflck;
> + ret = vfio_pci_vga_init(vdev);
> + if (ret)
> + goto out_vf;
>
> vfio_pci_probe_power_state(vdev);
>
> @@ -2016,8 +2062,8 @@ static int vfio_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
>
> return ret;
>
> -out_vf_token:
> - kfree(vdev->vf_token);
> +out_vf:
> + vfio_pci_vf_uninit(vdev);
> out_reflck:
> vfio_pci_reflck_put(vdev->reflck);
> out_del_group_dev:
> @@ -2039,33 +2085,19 @@ static void vfio_pci_remove(struct pci_dev *pdev)
> if (!vdev)
> return;
>
> - if (vdev->vf_token) {
> - WARN_ON(vdev->vf_token->users);
> - mutex_destroy(&vdev->vf_token->lock);
> - kfree(vdev->vf_token);
> - }
> -
> - if (vdev->nb.notifier_call)
> - bus_unregister_notifier(&pci_bus_type, &vdev->nb);
> -
> + vfio_pci_vf_uninit(vdev);
> vfio_pci_reflck_put(vdev->reflck);
> + vfio_pci_vga_uninit(vdev);
>
> vfio_iommu_group_put(pdev->dev.iommu_group, &pdev->dev);
> - kfree(vdev->region);
> - mutex_destroy(&vdev->ioeventfds_lock);
>
> if (!disable_idle_d3)
> vfio_pci_set_power_state(vdev, PCI_D0);
>
> + mutex_destroy(&vdev->ioeventfds_lock);
> + kfree(vdev->region);
> kfree(vdev->pm_save);
> kfree(vdev);
> -
> - if (vfio_pci_is_vga(pdev)) {
> - vga_client_register(pdev, NULL, NULL, NULL);
> - vga_set_legacy_decoding(pdev,
> - VGA_RSRC_NORMAL_IO | VGA_RSRC_NORMAL_MEM |
> - VGA_RSRC_LEGACY_IO | VGA_RSRC_LEGACY_MEM);
> - }
> }
>
> static pci_ers_result_t vfio_pci_aer_err_detected(struct pci_dev *pdev,
>
