| Message ID | 7-v2-20d933792272+4ff-vfio1_jgg@nvidia.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Embed struct vfio_device in all sub-structures | expand |
> +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; > +static void vfio_pci_vf_uninit(struct vfio_pci_device *vdev) > +{ > + if (!vdev->vf_token) > + return; I'd really prefer to keep these checks in the callers, as it makes the intent of the code much more clear. Same for the VGA side. But in general I like these helpers.
On Mon, Mar 15, 2021 at 09:45:34AM +0100, Christoph Hellwig wrote: > > +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; > > > +static void vfio_pci_vf_uninit(struct vfio_pci_device *vdev) > > +{ > > + if (!vdev->vf_token) > > + return; > > I'd really prefer to keep these checks in the callers, as it makes the > intent of the code much more clear. Same for the VGA side. > > But in general I like these helpers. I'm here because I needed to make the error unwind tidy before I could re-order everything in the next patch, as re-ordering with the existing unwind quickly became a mess. It ends up like this: out_power: if (!disable_idle_d3) vfio_pci_set_power_state(vdev, PCI_D0); out_vf: vfio_pci_vf_uninit(vdev); out_reflck: vfio_pci_reflck_put(vdev->reflck); out_free: kfree(vdev->pm_save); kfree(vdev); I'm always leery about adding conditionals to these unwinds, it is easy to make a mistake. Particularly in this case the init/uninit checks are not symmetric: + if (!pdev->is_physfn) + return 0; vs + if (!vdev->vf_token) + return; So the goto unwind looks quite odd when this is open coded. At least with the helpers you can read the init then uninit and go 'yah, OK, this makes sense' Thanks, Jason
On Mon, Mar 15, 2021 at 08:07:46PM -0300, Jason Gunthorpe wrote: > So the goto unwind looks quite odd when this is open coded. At least > with the helpers you can read the init then uninit and go 'yah, OK, > this makes sense' Still looks odd to me. But this is your series and overall a major improvements, so: Reviewed-by: Christoph Hellwig <hch@lst.de>
> From: Jason Gunthorpe <jgg@nvidia.com> > Sent: Saturday, March 13, 2021 8:56 AM > > 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: Kevin Tian <kevin.tian@intel.com> > --- > 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, > -- > 2.30.2
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@nvidia.com> > --- > 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@nvidia.com> > + 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); > +}
On Fri, 12 Mar 2021 20:55:59 -0400 Jason Gunthorpe <jgg@nvidia.com> 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> > --- > drivers/vfio/pci/vfio_pci.c | 116 +++++++++++++++++++++++------------- > 1 file changed, 74 insertions(+), 42 deletions(-) Reviewed-by: Cornelia Huck <cohuck@redhat.com>
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@nvidia.com> > > 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
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, >
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,
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> --- drivers/vfio/pci/vfio_pci.c | 116 +++++++++++++++++++++++------------- 1 file changed, 74 insertions(+), 42 deletions(-)