Nouveau - Jan 2016 - [PATCH] device: call nvkm_device_fini if nvkm_device_init fails

nvkm_device_fini is never called if a failure occurs in
nvkm_device_init, even when unloading the module. This can lead to a
resources leak (one example is the Tegra interrupt which would never be
freed in that case). Fix this by calling nvkm_device_fini in
nvkm_device_init's failure path.

Signed-off-by: Alexandre Courbot <acourbot at nvidia.com>
---
 drm/nouveau/nvkm/engine/device/base.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drm/nouveau/nvkm/engine/device/base.c
b/drm/nouveau/nvkm/engine/device/base.c
index b1ba1c782a2b..8ef0ae854038 100644
--- a/drm/nouveau/nvkm/engine/device/base.c
+++ b/drm/nouveau/nvkm/engine/device/base.c
@@ -2261,6 +2261,8 @@ fail_subdev:
 	} while (--i >= 0);
 
 fail:
+	nvkm_device_fini(device, false);
+
 	nvdev_error(device, "init failed with %d\n", ret);
 	return ret;
 }
-- 
2.7.0

nvkm_device_tegra_new initializes the irq member of the Tegra device
to -1 in order to signal that it is uninitialized. However,
nvkm_device_tegra_fini tests it against 0 to check whether an IRQ has
been allocated or not. This leads to free_irq being called on -1 during
device initialization.

Signed-off-by: Alexandre Courbot <acourbot at nvidia.com>
---
 drm/nouveau/nvkm/engine/device/tegra.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drm/nouveau/nvkm/engine/device/tegra.c
b/drm/nouveau/nvkm/engine/device/tegra.c
index 7f8a42721eb2..0a22439fd9ae 100644
--- a/drm/nouveau/nvkm/engine/device/tegra.c
+++ b/drm/nouveau/nvkm/engine/device/tegra.c
@@ -195,9 +195,9 @@ static void
 nvkm_device_tegra_fini(struct nvkm_device *device, bool suspend)
 {
 	struct nvkm_device_tegra *tdev = nvkm_device_tegra(device);
-	if (tdev->irq) {
+	if (tdev->irq != -1) {
 		free_irq(tdev->irq, tdev);
-		tdev->irq = 0;
+		tdev->irq = -1;
 	};
 }
 
-- 
2.7.0

Should these get back-ported to any released kernels?

On Mon, Jan 25, 2016 at 4:44 AM, Alexandre Courbot <acourbot at
nvidia.com> wrote:
> nvkm_device_fini is never called if a failure occurs in
> nvkm_device_init, even when unloading the module. This can lead to a
> resources leak (one example is the Tegra interrupt which would never be
> freed in that case). Fix this by calling nvkm_device_fini in
> nvkm_device_init's failure path.
>
> Signed-off-by: Alexandre Courbot <acourbot at nvidia.com>
> ---
>  drm/nouveau/nvkm/engine/device/base.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/drm/nouveau/nvkm/engine/device/base.c
b/drm/nouveau/nvkm/engine/device/base.c
> index b1ba1c782a2b..8ef0ae854038 100644
> --- a/drm/nouveau/nvkm/engine/device/base.c
> +++ b/drm/nouveau/nvkm/engine/device/base.c
> @@ -2261,6 +2261,8 @@ fail_subdev:
>         } while (--i >= 0);
>
>  fail:
> +       nvkm_device_fini(device, false);
> +
>         nvdev_error(device, "init failed with %d\n", ret);
>         return ret;
>  }
> --
> 2.7.0
>
> _______________________________________________
> Nouveau mailing list
> Nouveau at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/nouveau

On Tue, Jan 26, 2016 at 12:17 AM, Ilia Mirkin <imirkin at alum.mit.edu>
wrote:
> Should these get back-ported to any released kernels?
Probably ok not to, I found these while testing the error paths for secure boot.
>
> On Mon, Jan 25, 2016 at 4:44 AM, Alexandre Courbot <acourbot at
nvidia.com> wrote:
>> nvkm_device_fini is never called if a failure occurs in
>> nvkm_device_init, even when unloading the module. This can lead to a
>> resources leak (one example is the Tegra interrupt which would never be
>> freed in that case). Fix this by calling nvkm_device_fini in
>> nvkm_device_init's failure path.
>>
>> Signed-off-by: Alexandre Courbot <acourbot at nvidia.com>
>> ---
>>  drm/nouveau/nvkm/engine/device/base.c | 2 ++
>>  1 file changed, 2 insertions(+)
>>
>> diff --git a/drm/nouveau/nvkm/engine/device/base.c
b/drm/nouveau/nvkm/engine/device/base.c
>> index b1ba1c782a2b..8ef0ae854038 100644
>> --- a/drm/nouveau/nvkm/engine/device/base.c
>> +++ b/drm/nouveau/nvkm/engine/device/base.c
>> @@ -2261,6 +2261,8 @@ fail_subdev:
>>         } while (--i >= 0);
>>
>>  fail:
>> +       nvkm_device_fini(device, false);
>> +
>>         nvdev_error(device, "init failed with %d\n", ret);
>>         return ret;
>>  }
>> --
>> 2.7.0
>>
>> _______________________________________________
>> Nouveau mailing list
>> Nouveau at lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/nouveau
> _______________________________________________
> Nouveau mailing list
> Nouveau at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/nouveau

Am Montag, den 25.01.2016, 18:44 +0900 schrieb Alexandre
Courbot:
> nvkm_device_tegra_new initializes the irq member of the Tegra device
> to -1 in order to signal that it is uninitialized. However,
> nvkm_device_tegra_fini tests it against 0 to check whether an IRQ has
> been allocated or not. This leads to free_irq being called on -1
> during
> device initialization.
> 
The convention in other parts of the Linux kernel is that IRQ number 0
means unallocated/invalid IRQ. So I think it is the initialization to
-1 that should be fixed instead.
> Signed-off-by: Alexandre Courbot <acourbot at nvidia.com>
> ---
>  drm/nouveau/nvkm/engine/device/tegra.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drm/nouveau/nvkm/engine/device/tegra.c
> b/drm/nouveau/nvkm/engine/device/tegra.c
> index 7f8a42721eb2..0a22439fd9ae 100644
> --- a/drm/nouveau/nvkm/engine/device/tegra.c
> +++ b/drm/nouveau/nvkm/engine/device/tegra.c
> @@ -195,9 +195,9 @@ static void
>  nvkm_device_tegra_fini(struct nvkm_device *device, bool suspend)
>  {
>  	struct nvkm_device_tegra *tdev = nvkm_device_tegra(device);
> -	if (tdev->irq) {
> +	if (tdev->irq != -1) {
>  		free_irq(tdev->irq, tdev);
> -		tdev->irq = 0;
> +		tdev->irq = -1;
>  	};
>  }
>