Re: [PATCH 1/2] vfio: platform: Fix reset module leak in error path

2018-02-26 Thread Auger Eric
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 
Reviewed-by: Eric Auger 

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, _platform_ops, vdev);
> - if (ret) {
> - vfio_iommu_group_put(group, dev);
> - return ret;
> - }
> + if (ret)
> + goto put_iommu;
>  
>   mutex_init(>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);
>  
> 


Re: [PATCH 1/2] vfio: platform: Fix reset module leak in error path

2018-02-26 Thread Auger Eric
Hi Geert,

On 21/02/18 17:07, Geert Uytterhoeven wrote:
> Hi Eric,
> 
> On Wed, Feb 14, 2018 at 10:32 AM, Geert Uytterhoeven
>  wrote:
>> On Wed, Feb 14, 2018 at 9:36 AM, Auger Eric  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 -- 
> ge...@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
> 


Re: [PATCH 1/2] vfio: platform: Fix reset module leak in error path

2018-02-21 Thread Geert Uytterhoeven
Hi Eric,

On Wed, Feb 14, 2018 at 10:32 AM, Geert Uytterhoeven
 wrote:
> On Wed, Feb 14, 2018 at 9:36 AM, Auger Eric  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 -- ge...@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


Re: [PATCH 1/2] vfio: platform: Fix reset module leak in error path

2018-02-14 Thread Geert Uytterhoeven
Hi Eric,

On Wed, Feb 14, 2018 at 9:36 AM, Auger Eric  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 -- ge...@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


Re: [PATCH 1/2] vfio: platform: Fix reset module leak in error path

2018-02-14 Thread Auger Eric
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 
> ---
>  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, _platform_ops, vdev);
> - if (ret) {
> - vfio_iommu_group_put(group, dev);
> - return ret;
> - }
> + if (ret)
> + goto put_iommu;
>  
>   mutex_init(>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);
>  
>