| Message ID | 1518539815-13774-2-git-send-email-geert+renesas@glider.be (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Geert, On 13/02/18 17:36, Geert Uytterhoeven wrote: > If the IOMMU group setup fails, the reset module is not released. > > Fixes: b5add544d677d363 ("vfio, platform: make reset driver a requirement by default") > Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be> > --- > drivers/vfio/platform/vfio_platform_common.c | 15 ++++++++++----- > 1 file changed, 10 insertions(+), 5 deletions(-) > > diff --git a/drivers/vfio/platform/vfio_platform_common.c b/drivers/vfio/platform/vfio_platform_common.c > index 35469af87f88678e..b60bb5326668498c 100644 > --- a/drivers/vfio/platform/vfio_platform_common.c > +++ b/drivers/vfio/platform/vfio_platform_common.c > @@ -680,18 +680,23 @@ int vfio_platform_probe_common(struct vfio_platform_device *vdev, Thanks for fixing this. If I am not wrong we also leak the reset_module if vfio_platform_get_reset() fails to find the reset function (of_reset == NULL), in which case we should do the module_put() in vfio_platform_get_reset(). Thanks Eric > group = vfio_iommu_group_get(dev); > if (!group) { > pr_err("VFIO: No IOMMU group for device %s\n", vdev->name); > - return -EINVAL; > + ret = -EINVAL; > + goto put_reset; > } > > ret = vfio_add_group_dev(dev, &vfio_platform_ops, vdev); > - if (ret) { > - vfio_iommu_group_put(group, dev); > - return ret; > - } > + if (ret) > + goto put_iommu; > > mutex_init(&vdev->igate); > > return 0; > + > +put_iommu: > + vfio_iommu_group_put(group, dev); > +put_reset: > + vfio_platform_put_reset(vdev); > + return ret; > } > EXPORT_SYMBOL_GPL(vfio_platform_probe_common); > >
Hi Eric, On Wed, Feb 14, 2018 at 9:36 AM, Auger Eric <eric.auger@redhat.com> wrote: > If I am not wrong we also leak the reset_module if > vfio_platform_get_reset() fails to find the reset function (of_reset == > NULL), in which case we should do the module_put() in > vfio_platform_get_reset(). Correct. Will look into fixing it... Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
Hi Eric, On Wed, Feb 14, 2018 at 10:32 AM, Geert Uytterhoeven <geert@linux-m68k.org> wrote: > On Wed, Feb 14, 2018 at 9:36 AM, Auger Eric <eric.auger@redhat.com> wrote: >> If I am not wrong we also leak the reset_module if >> vfio_platform_get_reset() fails to find the reset function (of_reset == >> NULL), in which case we should do the module_put() in >> vfio_platform_get_reset(). > > Correct. Will look into fixing it... Upon second look, I don't think there's a leak in vfio_platform_get_reset(). If try_module_get() succeeded, there will always be a valid reset function (unless someone registered a vfio_reset_handler with a NULL reset function). Or do you mean the call to request_module()? That one doesn't do a module_get(), it merely tries to load a module. Hence there's no need to do a module_put() afterwards. Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
Hi Geert, On 21/02/18 17:07, Geert Uytterhoeven wrote: > Hi Eric, > > On Wed, Feb 14, 2018 at 10:32 AM, Geert Uytterhoeven > <geert@linux-m68k.org> wrote: >> On Wed, Feb 14, 2018 at 9:36 AM, Auger Eric <eric.auger@redhat.com> wrote: >>> If I am not wrong we also leak the reset_module if >>> vfio_platform_get_reset() fails to find the reset function (of_reset == >>> NULL), in which case we should do the module_put() in >>> vfio_platform_get_reset(). >> >> Correct. Will look into fixing it... > > Upon second look, I don't think there's a leak in vfio_platform_get_reset(). > > If try_module_get() succeeded, there will always be a valid reset function > (unless someone registered a vfio_reset_handler with a NULL reset function). Hum yes, you are right. So the code is fine as is. Sorry for the noise. Thanks Eric > > Or do you mean the call to request_module()? > That one doesn't do a module_get(), it merely tries to load a module. > Hence there's no need to do a module_put() afterwards. > > Gr{oetje,eeting}s, > > Geert > > -- > Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org > > In personal conversations with technical people, I call myself a hacker. But > when I'm talking to journalists I just say "programmer" or something like that. > -- Linus Torvalds >
Hi Geert, On 13/02/18 17:36, Geert Uytterhoeven wrote: > If the IOMMU group setup fails, the reset module is not released. > > Fixes: b5add544d677d363 ("vfio, platform: make reset driver a requirement by default") > Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be> Reviewed-by: Eric Auger <eric.auger@redhat.com> Thanks Eric > --- > drivers/vfio/platform/vfio_platform_common.c | 15 ++++++++++----- > 1 file changed, 10 insertions(+), 5 deletions(-) > > diff --git a/drivers/vfio/platform/vfio_platform_common.c b/drivers/vfio/platform/vfio_platform_common.c > index 35469af87f88678e..b60bb5326668498c 100644 > --- a/drivers/vfio/platform/vfio_platform_common.c > +++ b/drivers/vfio/platform/vfio_platform_common.c > @@ -680,18 +680,23 @@ int vfio_platform_probe_common(struct vfio_platform_device *vdev, > group = vfio_iommu_group_get(dev); > if (!group) { > pr_err("VFIO: No IOMMU group for device %s\n", vdev->name); > - return -EINVAL; > + ret = -EINVAL; > + goto put_reset; > } > > ret = vfio_add_group_dev(dev, &vfio_platform_ops, vdev); > - if (ret) { > - vfio_iommu_group_put(group, dev); > - return ret; > - } > + if (ret) > + goto put_iommu; > > mutex_init(&vdev->igate); > > return 0; > + > +put_iommu: > + vfio_iommu_group_put(group, dev); > +put_reset: > + vfio_platform_put_reset(vdev); > + return ret; > } > EXPORT_SYMBOL_GPL(vfio_platform_probe_common); > >
diff --git a/drivers/vfio/platform/vfio_platform_common.c b/drivers/vfio/platform/vfio_platform_common.c index 35469af87f88678e..b60bb5326668498c 100644 --- a/drivers/vfio/platform/vfio_platform_common.c +++ b/drivers/vfio/platform/vfio_platform_common.c @@ -680,18 +680,23 @@ int vfio_platform_probe_common(struct vfio_platform_device *vdev, group = vfio_iommu_group_get(dev); if (!group) { pr_err("VFIO: No IOMMU group for device %s\n", vdev->name); - return -EINVAL; + ret = -EINVAL; + goto put_reset; } ret = vfio_add_group_dev(dev, &vfio_platform_ops, vdev); - if (ret) { - vfio_iommu_group_put(group, dev); - return ret; - } + if (ret) + goto put_iommu; mutex_init(&vdev->igate); return 0; + +put_iommu: + vfio_iommu_group_put(group, dev); +put_reset: + vfio_platform_put_reset(vdev); + return ret; } EXPORT_SYMBOL_GPL(vfio_platform_probe_common);
If the IOMMU group setup fails, the reset module is not released. Fixes: b5add544d677d363 ("vfio, platform: make reset driver a requirement by default") Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be> --- drivers/vfio/platform/vfio_platform_common.c | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-)