On 3/13/2021 2: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>
---
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);
you can consider "mutex_destroy(&vdev->vf_token->lock);" like you use in
the uninit function.
I know it's not in the orig code and only for code symmetry.
otherwise looks good,
Reviewed-by: Max Gurtovoy <mgurtovoy@xxxxxxxxxx>
+ 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);
+}
