Re: [PATCH v2 07/14] vfio/pci: Move VGA and VF initialization to functions

[Auger Eric <eric.auger@redhat.com>]

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@nvidia.com>
Reviewed-by: Eric Auger <eric.auger@redhat.com>

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,
>