[PATCH v2 resend 0/2] gpu: host1x: possible double free and memory leak

[PATCH v2 resend 0/2] gpu: host1x: possible double free and memory leak

[Yang Yingliang]

This patchset fix two possible problems in the error path in
host1x_memory_context_list_init().

v1 -> v2:
  patch #1 don't return error code, if IOMMU is disabled.
  patch #2 make error handling code more clean.

  Put the two patches in one patchset.
  http://patchwork.ozlabs.org/project/linux-tegra/patch/20221028130300.1133520-1-yangyingliang@huawei.com/
  http://patchwork.ozlabs.org/project/linux-tegra/patch/20221124080559.3592650-1-yangyingliang@huawei.com/

Yang Yingliang (2):
  gpu: host1x: fix potential double free if IOMMU is disabled
  gpu: host1x: fix memory leak of device names

 drivers/gpu/host1x/context.c | 24 ++++++++++++++++--------
 1 file changed, 16 insertions(+), 8 deletions(-)

-- 
2.25.1

[PATCH v2 resend 1/2] gpu: host1x: fix potential double free if IOMMU is disabled

[Yang Yingliang]

If context device has no IOMMU, the 'cdl->devs' is freed in
error path, but host1x_memory_context_list_init() doesn't
return an error code, so the module can be loaded successfully,
when it's unloading, the host1x_memory_context_list_free() is
called in host1x_remove(), it will cause double free. Set the
'cdl->devs' to NULL after freeing it to avoid double free.

Fixes: 8aa5bcb61612 ("gpu: host1x: Add context device management code")
Signed-off-by: Yang Yingliang <yangyingliang@huawei.com>
---
 drivers/gpu/host1x/context.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/gpu/host1x/context.c b/drivers/gpu/host1x/context.c
index b08cf11f9a66..291f34562e2e 100644
--- a/drivers/gpu/host1x/context.c
+++ b/drivers/gpu/host1x/context.c
@@ -87,6 +87,7 @@ int host1x_memory_context_list_init(struct host1x *host1x)
 		device_del(&cdl->devs[i].dev);
 
 	kfree(cdl->devs);
+	cdl->devs = NULL;
 	cdl->len = 0;
 
 	return err;
-- 
2.25.1

Re: [PATCH v2 resend 1/2] gpu: host1x: fix potential double free if IOMMU is disabled

[Mikko Perttunen]

On 11/26/22 09:33, Yang Yingliang wrote:
> If context device has no IOMMU, the 'cdl->devs' is freed in
> error path, but host1x_memory_context_list_init() doesn't
> return an error code, so the module can be loaded successfully,
> when it's unloading, the host1x_memory_context_list_free() is
> called in host1x_remove(), it will cause double free. Set the
> 'cdl->devs' to NULL after freeing it to avoid double free.
> 
> Fixes: 8aa5bcb61612 ("gpu: host1x: Add context device management code")
> Signed-off-by: Yang Yingliang <yangyingliang@huawei.com>
> ---
>   drivers/gpu/host1x/context.c | 1 +
>   1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/gpu/host1x/context.c b/drivers/gpu/host1x/context.c
> index b08cf11f9a66..291f34562e2e 100644
> --- a/drivers/gpu/host1x/context.c
> +++ b/drivers/gpu/host1x/context.c
> @@ -87,6 +87,7 @@ int host1x_memory_context_list_init(struct host1x *host1x)
>   		device_del(&cdl->devs[i].dev);
>   
>   	kfree(cdl->devs);
> +	cdl->devs = NULL;
>   	cdl->len = 0;
>   
>   	return err;

Thanks!

Reviewed-by: Mikko Perttunen <mperttunen@nvidia.com>

Re: (subset) [PATCH v2 resend 1/2] gpu: host1x: fix potential double free if IOMMU is disabled

[Thierry Reding]

From: Thierry Reding <treding@nvidia.com>

On Sat, 26 Nov 2022 15:33:14 +0800, Yang Yingliang wrote:
> If context device has no IOMMU, the 'cdl->devs' is freed in
> error path, but host1x_memory_context_list_init() doesn't
> return an error code, so the module can be loaded successfully,
> when it's unloading, the host1x_memory_context_list_free() is
> called in host1x_remove(), it will cause double free. Set the
> 'cdl->devs' to NULL after freeing it to avoid double free.
> 
> [...]

Applied, thanks!

[1/2] gpu: host1x: fix potential double free if IOMMU is disabled
      (no commit info)

Best regards,
-- 
Thierry Reding <treding@nvidia.com>

[PATCH v2 resend 2/2] gpu: host1x: fix memory leak of device names

[Yang Yingliang]

The device names allocated by dev_set_name() need be freed
before module unloading, but they can not be freed because
the kobject's refcount which was set in device_initialize()
has not be decreased to 0.

As comment of device_add() says, if it fails, use only
put_device() drop the refcount, then the name will be
freed in kobejct_cleanup().

device_del() and put_device() can be replaced with
device_unregister(), so call it to unregister the added
successfully devices, and just call put_device() to the
not added device.

Add a release() function to device to avoid null release()
function WARNING in device_release(), it's empty, because
the context devices are freed together in
host1x_memory_context_list_free().

Fixes: 8aa5bcb61612 ("gpu: host1x: Add context device management code")
Signed-off-by: Yang Yingliang <yangyingliang@huawei.com>
---
 drivers/gpu/host1x/context.c | 23 +++++++++++++++--------
 1 file changed, 15 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/host1x/context.c b/drivers/gpu/host1x/context.c
index 291f34562e2e..047696432eb2 100644
--- a/drivers/gpu/host1x/context.c
+++ b/drivers/gpu/host1x/context.c
@@ -13,6 +13,11 @@
 #include "context.h"
 #include "dev.h"
 
+static void host1x_memory_context_release(struct device *dev)
+{
+	/* context device is freed in host1x_memory_context_list_free() */
+}
+
 int host1x_memory_context_list_init(struct host1x *host1x)
 {
 	struct host1x_memory_context_list *cdl = &host1x->context_list;
@@ -53,28 +58,30 @@ int host1x_memory_context_list_init(struct host1x *host1x)
 		dev_set_name(&ctx->dev, "host1x-ctx.%d", i);
 		ctx->dev.bus = &host1x_context_device_bus_type;
 		ctx->dev.parent = host1x->dev;
+		ctx->dev.release = host1x_memory_context_release;
 
 		dma_set_max_seg_size(&ctx->dev, UINT_MAX);
 
 		err = device_add(&ctx->dev);
 		if (err) {
 			dev_err(host1x->dev, "could not add context device %d: %d\n", i, err);
-			goto del_devices;
+			put_device(&ctx->dev);
+			goto unreg_devices;
 		}
 
 		err = of_dma_configure_id(&ctx->dev, node, true, &i);
 		if (err) {
 			dev_err(host1x->dev, "IOMMU configuration failed for context device %d: %d\n",
 				i, err);
-			device_del(&ctx->dev);
-			goto del_devices;
+			device_unregister(&ctx->dev);
+			goto unreg_devices;
 		}
 
 		fwspec = dev_iommu_fwspec_get(&ctx->dev);
 		if (!fwspec || !device_iommu_mapped(&ctx->dev)) {
 			dev_err(host1x->dev, "Context device %d has no IOMMU!\n", i);
-			device_del(&ctx->dev);
-			goto del_devices;
+			device_unregister(&ctx->dev);
+			goto unreg_devices;
 		}
 
 		ctx->stream_id = fwspec->ids[0] & 0xffff;
@@ -82,9 +89,9 @@ int host1x_memory_context_list_init(struct host1x *host1x)
 
 	return 0;
 
-del_devices:
+unreg_devices:
 	while (i--)
-		device_del(&cdl->devs[i].dev);
+		device_unregister(&cdl->devs[i].dev);
 
 	kfree(cdl->devs);
 	cdl->devs = NULL;
@@ -98,7 +105,7 @@ void host1x_memory_context_list_free(struct host1x_memory_context_list *cdl)
 	unsigned int i;
 
 	for (i = 0; i < cdl->len; i++)
-		device_del(&cdl->devs[i].dev);
+		device_unregister(&cdl->devs[i].dev);
 
 	kfree(cdl->devs);
 	cdl->len = 0;
-- 
2.25.1

Re: [PATCH v2 resend 2/2] gpu: host1x: fix memory leak of device names

[Mikko Perttunen]

On 11/26/22 09:33, Yang Yingliang wrote:
> The device names allocated by dev_set_name() need be freed
> before module unloading, but they can not be freed because
> the kobject's refcount which was set in device_initialize()
> has not be decreased to 0.
> 
> As comment of device_add() says, if it fails, use only
> put_device() drop the refcount, then the name will be
> freed in kobejct_cleanup().
> 
> device_del() and put_device() can be replaced with
> device_unregister(), so call it to unregister the added
> successfully devices, and just call put_device() to the
> not added device.
> 
> Add a release() function to device to avoid null release()
> function WARNING in device_release(), it's empty, because
> the context devices are freed together in
> host1x_memory_context_list_free().
> 
> Fixes: 8aa5bcb61612 ("gpu: host1x: Add context device management code")
> Signed-off-by: Yang Yingliang <yangyingliang@huawei.com>
> ---
>   drivers/gpu/host1x/context.c | 23 +++++++++++++++--------
>   1 file changed, 15 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/gpu/host1x/context.c b/drivers/gpu/host1x/context.c
> index 291f34562e2e..047696432eb2 100644
> --- a/drivers/gpu/host1x/context.c
> +++ b/drivers/gpu/host1x/context.c
> @@ -13,6 +13,11 @@
>   #include "context.h"
>   #include "dev.h"
>   
> +static void host1x_memory_context_release(struct device *dev)
> +{
> +	/* context device is freed in host1x_memory_context_list_free() */
> +}
> +
>   int host1x_memory_context_list_init(struct host1x *host1x)
>   {
>   	struct host1x_memory_context_list *cdl = &host1x->context_list;
> @@ -53,28 +58,30 @@ int host1x_memory_context_list_init(struct host1x *host1x)
>   		dev_set_name(&ctx->dev, "host1x-ctx.%d", i);
>   		ctx->dev.bus = &host1x_context_device_bus_type;
>   		ctx->dev.parent = host1x->dev;
> +		ctx->dev.release = host1x_memory_context_release;
>   
>   		dma_set_max_seg_size(&ctx->dev, UINT_MAX);
>   
>   		err = device_add(&ctx->dev);
>   		if (err) {
>   			dev_err(host1x->dev, "could not add context device %d: %d\n", i, err);
> -			goto del_devices;
> +			put_device(&ctx->dev);
> +			goto unreg_devices;
>   		}
>   
>   		err = of_dma_configure_id(&ctx->dev, node, true, &i);
>   		if (err) {
>   			dev_err(host1x->dev, "IOMMU configuration failed for context device %d: %d\n",
>   				i, err);
> -			device_del(&ctx->dev);
> -			goto del_devices;
> +			device_unregister(&ctx->dev);
> +			goto unreg_devices;
>   		}
>   
>   		fwspec = dev_iommu_fwspec_get(&ctx->dev);
>   		if (!fwspec || !device_iommu_mapped(&ctx->dev)) {
>   			dev_err(host1x->dev, "Context device %d has no IOMMU!\n", i);
> -			device_del(&ctx->dev);
> -			goto del_devices;
> +			device_unregister(&ctx->dev);
> +			goto unreg_devices;
>   		}
>   
>   		ctx->stream_id = fwspec->ids[0] & 0xffff;
> @@ -82,9 +89,9 @@ int host1x_memory_context_list_init(struct host1x *host1x)
>   
>   	return 0;
>   
> -del_devices:
> +unreg_devices:
>   	while (i--)
> -		device_del(&cdl->devs[i].dev);
> +		device_unregister(&cdl->devs[i].dev);
>   
>   	kfree(cdl->devs);
>   	cdl->devs = NULL;
> @@ -98,7 +105,7 @@ void host1x_memory_context_list_free(struct host1x_memory_context_list *cdl)
>   	unsigned int i;
>   
>   	for (i = 0; i < cdl->len; i++)
> -		device_del(&cdl->devs[i].dev);
> +		device_unregister(&cdl->devs[i].dev);
>   
>   	kfree(cdl->devs);
>   	cdl->len = 0;

Reviewed-by: Mikko Perttunen <mperttunen@nvidia.com>