| Message ID | 5-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 |
Looks good,
Reviewed-by: Christoph Hellwig <hch@lst.de>
Hi, I will test the fsl-mc related changes in the next couple of days. Thanks, Diana On 3/13/2021 2:55 AM, Jason Gunthorpe wrote: > vfio_add_group_dev() must be called only after all of the private data in > vdev is fully setup and ready, otherwise there could be races with user > space instantiating a device file descriptor and starting to call ops. > > For instance vfio_fsl_mc_reflck_attach() sets vdev->reflck and > vfio_fsl_mc_open(), called by fops open, unconditionally derefs it, which > will crash if things get out of order. > > This driver started life with the right sequence, but three commits added > stuff after vfio_add_group_dev(). > > Fixes: 2e0d29561f59 ("vfio/fsl-mc: Add irq infrastructure for fsl-mc devices") > Fixes: f2ba7e8c947b ("vfio/fsl-mc: Added lock support in preparation for interrupt handling") > Fixes: 704f5082d845 ("vfio/fsl-mc: Scan DPRC objects on vfio-fsl-mc driver bind") > Signed-off-by: Jason Gunthorpe <jgg@nvidia.com> > --- > drivers/vfio/fsl-mc/vfio_fsl_mc.c | 43 ++++++++++++++++--------------- > 1 file changed, 22 insertions(+), 21 deletions(-) > > diff --git a/drivers/vfio/fsl-mc/vfio_fsl_mc.c b/drivers/vfio/fsl-mc/vfio_fsl_mc.c > index f27e25112c4037..881849723b4dfb 100644 > --- a/drivers/vfio/fsl-mc/vfio_fsl_mc.c > +++ b/drivers/vfio/fsl-mc/vfio_fsl_mc.c > @@ -582,11 +582,21 @@ static int vfio_fsl_mc_init_device(struct vfio_fsl_mc_device *vdev) > dprc_cleanup(mc_dev); > out_nc_unreg: > bus_unregister_notifier(&fsl_mc_bus_type, &vdev->nb); > - vdev->nb.notifier_call = NULL; > - > return ret; > } > > +static void vfio_fsl_uninit_device(struct vfio_fsl_mc_device *vdev) > +{ > + struct fsl_mc_device *mc_dev = vdev->mc_dev; > + > + if (!is_fsl_mc_bus_dprc(mc_dev)) > + return; > + > + dprc_remove_devices(mc_dev, NULL, 0); > + dprc_cleanup(mc_dev); > + bus_unregister_notifier(&fsl_mc_bus_type, &vdev->nb); > +} > + > static int vfio_fsl_mc_probe(struct fsl_mc_device *mc_dev) > { > struct iommu_group *group; > @@ -607,29 +617,27 @@ static int vfio_fsl_mc_probe(struct fsl_mc_device *mc_dev) > } > > vdev->mc_dev = mc_dev; > - > - ret = vfio_add_group_dev(dev, &vfio_fsl_mc_ops, vdev); > - if (ret) { > - dev_err(dev, "VFIO_FSL_MC: Failed to add to vfio group\n"); > - goto out_group_put; > - } > + mutex_init(&vdev->igate); > > ret = vfio_fsl_mc_reflck_attach(vdev); > if (ret) > - goto out_group_dev; > + goto out_group_put; > > ret = vfio_fsl_mc_init_device(vdev); > if (ret) > goto out_reflck; > > - mutex_init(&vdev->igate); > - > + ret = vfio_add_group_dev(dev, &vfio_fsl_mc_ops, vdev); > + if (ret) { > + dev_err(dev, "VFIO_FSL_MC: Failed to add to vfio group\n"); > + goto out_device; > + } > return 0; > > +out_device: > + vfio_fsl_uninit_device(vdev); > out_reflck: > vfio_fsl_mc_reflck_put(vdev->reflck); > -out_group_dev: > - vfio_del_group_dev(dev); > out_group_put: > vfio_iommu_group_put(group, dev); > return ret; > @@ -646,16 +654,9 @@ static int vfio_fsl_mc_remove(struct fsl_mc_device *mc_dev) > > mutex_destroy(&vdev->igate); > > + vfio_fsl_uninit_device(vdev); > vfio_fsl_mc_reflck_put(vdev->reflck); > > - if (is_fsl_mc_bus_dprc(mc_dev)) { > - dprc_remove_devices(mc_dev, NULL, 0); > - dprc_cleanup(mc_dev); > - } > - > - if (vdev->nb.notifier_call) > - bus_unregister_notifier(&fsl_mc_bus_type, &vdev->nb); > - > vfio_iommu_group_put(mc_dev->dev.iommu_group, dev); > > return 0; >
On Fri, 12 Mar 2021 20:55:57 -0400 Jason Gunthorpe <jgg@nvidia.com> wrote: > vfio_add_group_dev() must be called only after all of the private data in > vdev is fully setup and ready, otherwise there could be races with user > space instantiating a device file descriptor and starting to call ops. > > For instance vfio_fsl_mc_reflck_attach() sets vdev->reflck and > vfio_fsl_mc_open(), called by fops open, unconditionally derefs it, which > will crash if things get out of order. > > This driver started life with the right sequence, but three commits added > stuff after vfio_add_group_dev(). > > Fixes: 2e0d29561f59 ("vfio/fsl-mc: Add irq infrastructure for fsl-mc devices") > Fixes: f2ba7e8c947b ("vfio/fsl-mc: Added lock support in preparation for interrupt handling") > Fixes: 704f5082d845 ("vfio/fsl-mc: Scan DPRC objects on vfio-fsl-mc driver bind") > Signed-off-by: Jason Gunthorpe <jgg@nvidia.com> > --- > drivers/vfio/fsl-mc/vfio_fsl_mc.c | 43 ++++++++++++++++--------------- > 1 file changed, 22 insertions(+), 21 deletions(-) Reviewed-by: Cornelia Huck <cohuck@redhat.com>
Hi,
Thanks for finding this!
I tested the series and currently the binding to vfio fails. The reason
is that it is assumed that the objects scan is done after
vfio_add_group_dev. But at this point the vdev structure is completly
initialized.
I'll add some more context.
There are two types of FSL MC devices:
- a DPRC device
- regular devices
A DPRC is some kind of container of the other devices. The DPRC VFIO
device is scanning for all the existing devices in the container and
triggers the probe function for those devices. However, there are some
pieces of code that needs to be protected by a lock, lock that is
created by vfio_fsl_mc_reflck_attach() function. This function is
searching for the DPRC vdev (having the physical device) in the vfio
group, so the "parent" device should have been added in the group before
the child devices are probed.
I did some changes on top of these series and this is how they look
like. I hope that I do not do something that violates the way the VFIO
is designed.
diff --git a/drivers/vfio/fsl-mc/vfio_fsl_mc.c
b/drivers/vfio/fsl-mc/vfio_fsl_mc.c
index 3af3ca59478f..9b4c9356515a 100644
--- a/drivers/vfio/fsl-mc/vfio_fsl_mc.c
+++ b/drivers/vfio/fsl-mc/vfio_fsl_mc.c
@@ -578,22 +578,32 @@ static int vfio_fsl_mc_init_device(struct
vfio_fsl_mc_device *vdev)
goto out_nc_unreg;
}
- ret = dprc_scan_container(mc_dev, false);
- if (ret) {
- dev_err(&mc_dev->dev, "VFIO_FSL_MC: Container scanning
failed (%d)\n", ret);
- goto out_dprc_cleanup;
- }
-
return 0;
-out_dprc_cleanup:
- dprc_remove_devices(mc_dev, NULL, 0);
- dprc_cleanup(mc_dev);
out_nc_unreg:
bus_unregister_notifier(&fsl_mc_bus_type, &vdev->nb);
return ret;
}
+static int vfio_fsl_mc_scan_container(struct vfio_fsl_mc_device *vdev)
+{
+ struct fsl_mc_device *mc_dev = vdev->mc_dev;
+ int ret;
+
+ /* non dprc devices do not scan for other devices */
+ if (is_fsl_mc_bus_dprc(mc_dev)) {
+ ret = dprc_scan_container(mc_dev, false);
+ if (ret) {
+ dev_err(&mc_dev->dev, "VFIO_FSL_MC: Container
scanning failed (%d)\n", ret);
+ dprc_remove_devices(mc_dev, NULL, 0);
+ return ret;
+ }
+ }
+
+ return 0;
+}
+
+
static void vfio_fsl_uninit_device(struct vfio_fsl_mc_device *vdev)
{
struct fsl_mc_device *mc_dev = vdev->mc_dev;
@@ -642,9 +652,16 @@ static int vfio_fsl_mc_probe(struct fsl_mc_device
*mc_dev)
dev_err(dev, "VFIO_FSL_MC: Failed to add to vfio group\n");
goto out_device;
}
+
+ ret = vfio_fsl_mc_scan_container(vdev);
+ if (ret)
+ goto out_group_dev;
+
dev_set_drvdata(dev, vdev);
return 0;
+out_group_dev:
+ vfio_unregister_group_dev(&vdev->vdev);
out_device:
vfio_fsl_uninit_device(vdev);
out_reflck:
Thanks,
Diana
On 3/13/2021 2:55 AM, Jason Gunthorpe wrote:
> vfio_add_group_dev() must be called only after all of the private data in
> vdev is fully setup and ready, otherwise there could be races with user
> space instantiating a device file descriptor and starting to call ops.
>
> For instance vfio_fsl_mc_reflck_attach() sets vdev->reflck and
> vfio_fsl_mc_open(), called by fops open, unconditionally derefs it, which
> will crash if things get out of order.
>
> This driver started life with the right sequence, but three commits added
> stuff after vfio_add_group_dev().
>
> Fixes: 2e0d29561f59 ("vfio/fsl-mc: Add irq infrastructure for fsl-mc devices")
> Fixes: f2ba7e8c947b ("vfio/fsl-mc: Added lock support in preparation for interrupt handling")
> Fixes: 704f5082d845 ("vfio/fsl-mc: Scan DPRC objects on vfio-fsl-mc driver bind")
> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
> ---
> drivers/vfio/fsl-mc/vfio_fsl_mc.c | 43 ++++++++++++++++---------------
> 1 file changed, 22 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/vfio/fsl-mc/vfio_fsl_mc.c b/drivers/vfio/fsl-mc/vfio_fsl_mc.c
> index f27e25112c4037..881849723b4dfb 100644
> --- a/drivers/vfio/fsl-mc/vfio_fsl_mc.c
> +++ b/drivers/vfio/fsl-mc/vfio_fsl_mc.c
> @@ -582,11 +582,21 @@ static int vfio_fsl_mc_init_device(struct vfio_fsl_mc_device *vdev)
> dprc_cleanup(mc_dev);
> out_nc_unreg:
> bus_unregister_notifier(&fsl_mc_bus_type, &vdev->nb);
> - vdev->nb.notifier_call = NULL;
> -
> return ret;
> }
>
> +static void vfio_fsl_uninit_device(struct vfio_fsl_mc_device *vdev)
> +{
> + struct fsl_mc_device *mc_dev = vdev->mc_dev;
> +
> + if (!is_fsl_mc_bus_dprc(mc_dev))
> + return;
> +
> + dprc_remove_devices(mc_dev, NULL, 0);
> + dprc_cleanup(mc_dev);
> + bus_unregister_notifier(&fsl_mc_bus_type, &vdev->nb);
> +}
> +
> static int vfio_fsl_mc_probe(struct fsl_mc_device *mc_dev)
> {
> struct iommu_group *group;
> @@ -607,29 +617,27 @@ static int vfio_fsl_mc_probe(struct fsl_mc_device *mc_dev)
> }
>
> vdev->mc_dev = mc_dev;
> -
> - ret = vfio_add_group_dev(dev, &vfio_fsl_mc_ops, vdev);
> - if (ret) {
> - dev_err(dev, "VFIO_FSL_MC: Failed to add to vfio group\n");
> - goto out_group_put;
> - }
> + mutex_init(&vdev->igate);
>
> ret = vfio_fsl_mc_reflck_attach(vdev);
> if (ret)
> - goto out_group_dev;
> + goto out_group_put;
>
> ret = vfio_fsl_mc_init_device(vdev);
> if (ret)
> goto out_reflck;
>
> - mutex_init(&vdev->igate);
> -
> + ret = vfio_add_group_dev(dev, &vfio_fsl_mc_ops, vdev);
> + if (ret) {
> + dev_err(dev, "VFIO_FSL_MC: Failed to add to vfio group\n");
> + goto out_device;
> + }
> return 0;
>
> +out_device:
> + vfio_fsl_uninit_device(vdev);
> out_reflck:
> vfio_fsl_mc_reflck_put(vdev->reflck);
> -out_group_dev:
> - vfio_del_group_dev(dev);
> out_group_put:
> vfio_iommu_group_put(group, dev);
> return ret;
> @@ -646,16 +654,9 @@ static int vfio_fsl_mc_remove(struct fsl_mc_device *mc_dev)
>
> mutex_destroy(&vdev->igate);
>
> + vfio_fsl_uninit_device(vdev);
> vfio_fsl_mc_reflck_put(vdev->reflck);
>
> - if (is_fsl_mc_bus_dprc(mc_dev)) {
> - dprc_remove_devices(mc_dev, NULL, 0);
> - dprc_cleanup(mc_dev);
> - }
> -
> - if (vdev->nb.notifier_call)
> - bus_unregister_notifier(&fsl_mc_bus_type, &vdev->nb);
> -
> vfio_iommu_group_put(mc_dev->dev.iommu_group, dev);
>
> return 0;
>
On Wed, Mar 17, 2021 at 06:36:09PM +0200, Diana Craciun OSS wrote: > Hi, > > Thanks for finding this! > > I tested the series and currently the binding to vfio fails. The reason is > that it is assumed that the objects scan is done after vfio_add_group_dev. > But at this point the vdev structure is completly initialized. > > I'll add some more context. > > There are two types of FSL MC devices: > - a DPRC device > - regular devices > > A DPRC is some kind of container of the other devices. The DPRC VFIO device > is scanning for all the existing devices in the container and triggers the > probe function for those devices. Oh. It ends up recursively calling probe() under the same stack frame? I don't feel good about that > However, there are some pieces of code > that needs to be protected by a lock, lock that is created by > vfio_fsl_mc_reflck_attach() function. This function is searching for the > DPRC vdev (having the physical device) in the vfio group, so the "parent" > device should have been added in the group before the child devices are > probed. Yes, I understood this part, but I didn't think it could be invoked recursively from vfio_fsl_mc_init_device() :( > I did some changes on top of these series and this is how they look like. I > hope that I do not do something that violates the way the VFIO is designed. Well, it is "ok" in that this is only about the reflck so it doesn't appear to break the core's assumptions, but I don't like it at all. I also have a later patch that revises the reflck search I now see I will have to throw out. I think it would be better to find the reflck entirely internally to the driver than involving both the vfio and driver core in the search. I will try to write that later For now, this solution seems OK, I will fold it in, thanks Jason
diff --git a/drivers/vfio/fsl-mc/vfio_fsl_mc.c b/drivers/vfio/fsl-mc/vfio_fsl_mc.c index f27e25112c4037..881849723b4dfb 100644 --- a/drivers/vfio/fsl-mc/vfio_fsl_mc.c +++ b/drivers/vfio/fsl-mc/vfio_fsl_mc.c @@ -582,11 +582,21 @@ static int vfio_fsl_mc_init_device(struct vfio_fsl_mc_device *vdev) dprc_cleanup(mc_dev); out_nc_unreg: bus_unregister_notifier(&fsl_mc_bus_type, &vdev->nb); - vdev->nb.notifier_call = NULL; - return ret; } +static void vfio_fsl_uninit_device(struct vfio_fsl_mc_device *vdev) +{ + struct fsl_mc_device *mc_dev = vdev->mc_dev; + + if (!is_fsl_mc_bus_dprc(mc_dev)) + return; + + dprc_remove_devices(mc_dev, NULL, 0); + dprc_cleanup(mc_dev); + bus_unregister_notifier(&fsl_mc_bus_type, &vdev->nb); +} + static int vfio_fsl_mc_probe(struct fsl_mc_device *mc_dev) { struct iommu_group *group; @@ -607,29 +617,27 @@ static int vfio_fsl_mc_probe(struct fsl_mc_device *mc_dev) } vdev->mc_dev = mc_dev; - - ret = vfio_add_group_dev(dev, &vfio_fsl_mc_ops, vdev); - if (ret) { - dev_err(dev, "VFIO_FSL_MC: Failed to add to vfio group\n"); - goto out_group_put; - } + mutex_init(&vdev->igate); ret = vfio_fsl_mc_reflck_attach(vdev); if (ret) - goto out_group_dev; + goto out_group_put; ret = vfio_fsl_mc_init_device(vdev); if (ret) goto out_reflck; - mutex_init(&vdev->igate); - + ret = vfio_add_group_dev(dev, &vfio_fsl_mc_ops, vdev); + if (ret) { + dev_err(dev, "VFIO_FSL_MC: Failed to add to vfio group\n"); + goto out_device; + } return 0; +out_device: + vfio_fsl_uninit_device(vdev); out_reflck: vfio_fsl_mc_reflck_put(vdev->reflck); -out_group_dev: - vfio_del_group_dev(dev); out_group_put: vfio_iommu_group_put(group, dev); return ret; @@ -646,16 +654,9 @@ static int vfio_fsl_mc_remove(struct fsl_mc_device *mc_dev) mutex_destroy(&vdev->igate); + vfio_fsl_uninit_device(vdev); vfio_fsl_mc_reflck_put(vdev->reflck); - if (is_fsl_mc_bus_dprc(mc_dev)) { - dprc_remove_devices(mc_dev, NULL, 0); - dprc_cleanup(mc_dev); - } - - if (vdev->nb.notifier_call) - bus_unregister_notifier(&fsl_mc_bus_type, &vdev->nb); - vfio_iommu_group_put(mc_dev->dev.iommu_group, dev); return 0;
vfio_add_group_dev() must be called only after all of the private data in vdev is fully setup and ready, otherwise there could be races with user space instantiating a device file descriptor and starting to call ops. For instance vfio_fsl_mc_reflck_attach() sets vdev->reflck and vfio_fsl_mc_open(), called by fops open, unconditionally derefs it, which will crash if things get out of order. This driver started life with the right sequence, but three commits added stuff after vfio_add_group_dev(). Fixes: 2e0d29561f59 ("vfio/fsl-mc: Add irq infrastructure for fsl-mc devices") Fixes: f2ba7e8c947b ("vfio/fsl-mc: Added lock support in preparation for interrupt handling") Fixes: 704f5082d845 ("vfio/fsl-mc: Scan DPRC objects on vfio-fsl-mc driver bind") Signed-off-by: Jason Gunthorpe <jgg@nvidia.com> --- drivers/vfio/fsl-mc/vfio_fsl_mc.c | 43 ++++++++++++++++--------------- 1 file changed, 22 insertions(+), 21 deletions(-)