On Tue, Mar 16, 2021 at 03:02:40PM +0200, Max Gurtovoy wrote:
>
> 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
> > +++ 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.
The value in doing mutex_destroy is that it triggers a useful
debugging check that the mutex is not locked while being destructed.
In this case it is impossible for the mutex to be locked because the
pointer hasn't left the local stack
Thanks,
Jason
