[PATCH 0/2] Fix memory leak in coresight drivers

[PATCH 0/2] Fix memory leak in coresight drivers

[Junhao He]

When build kernel with CONFIG_KASAN=y
there are reports of memory leaks, like:
...
unreferenced object 0xffff2020510fe200 (size 64):
  comm "insmod", pid 4642, jiffies 4295983961 (age 46049.752s)
  hex dump (first 32 bytes):
    10 20 40 06 28 20 ff ff 10 40 7f 06 20 20 ff ff  . @.( ...@..  ..
    10 20 bb 8a 20 00 ff ff 10 e0 c7 8a 20 00 ff ff  . .. ....... ...
  backtrace:
    [<0000000034ec4724>] __kmem_cache_alloc_node+0x2f8/0x348
    [<0000000057fbc15d>] __kmalloc_node_track_caller+0x5c/0x110
    [<0000000055d5e34b>] krealloc+0x8c/0x178
    [<00000000a4635beb>] coresight_alloc_device_name+0x128/0x188 [coresight]
    [<000000000ce9d17b>] smb_probe+0x268/0x478 [ultrasoc_smb]
...
unreferenced object 0xffff00213c141000 (size 1024):
  comm "systemd-udevd", pid 2123, jiffies 4294909467 (age 6062.160s)
  hex dump (first 32 bytes):
    04 00 00 00 02 00 00 00 18 10 14 3c 21 00 ff ff  ...........<!...
    00 00 00 00 00 00 00 00 03 00 00 00 10 00 00 00  ................
  backtrace:
    [<000000004b7c9001>] __kmem_cache_alloc_node+0x2f8/0x348
    [<00000000b0fc7ceb>] __kmalloc+0x58/0x108
    [<0000000064ff4695>] acpi_os_allocate+0x2c/0x68
    [<000000007d57d116>] acpi_ut_initialize_buffer+0x54/0xe0
    [<0000000024583908>] acpi_evaluate_object+0x388/0x438
    [<0000000017b2e72b>] acpi_evaluate_object_typed+0xe8/0x240
    [<000000005df0eac2>] coresight_get_platform_data+0x1b4/0x988 [coresight]
...

The patchset based on "coresight: platform: acpi: Ignore the absence of graph"
https://git.kernel.org/pub/scm/linux/kernel/git/coresight/linux.git/commit/?h=next&id=3a2888aa1f962c55ca36119aebe67355c7bf54e4

Junhao He (2):
  coresight: Fix memory leak in acpi_buffer->pointer
  coresight: core: fix memory leak in dict->fwnode_list

 drivers/hwtracing/coresight/coresight-core.c  | 20 +++++++++-
 .../hwtracing/coresight/coresight-platform.c  | 40 ++++++++++++-------
 2 files changed, 45 insertions(+), 15 deletions(-)

-- 
2.33.0

[PATCH 1/2] coresight: Fix memory leak in acpi_buffer->pointer

[Junhao He]

There are memory leaks reported by kmemleak:
...
unreferenced object 0xffff00213c141000 (size 1024):
  comm "systemd-udevd", pid 2123, jiffies 4294909467 (age 6062.160s)
  hex dump (first 32 bytes):
    04 00 00 00 02 00 00 00 18 10 14 3c 21 00 ff ff  ...........<!...
    00 00 00 00 00 00 00 00 03 00 00 00 10 00 00 00  ................
  backtrace:
    [<000000004b7c9001>] __kmem_cache_alloc_node+0x2f8/0x348
    [<00000000b0fc7ceb>] __kmalloc+0x58/0x108
    [<0000000064ff4695>] acpi_os_allocate+0x2c/0x68
    [<000000007d57d116>] acpi_ut_initialize_buffer+0x54/0xe0
    [<0000000024583908>] acpi_evaluate_object+0x388/0x438
    [<0000000017b2e72b>] acpi_evaluate_object_typed+0xe8/0x240
    [<000000005df0eac2>] coresight_get_platform_data+0x1b4/0x988 [coresight]
...

The ACPI buffer memory (buf.pointer) should be freed. But the buffer
is also used after returning from acpi_get_dsd_graph().
Move the temporary variables buf to acpi_coresight_parse_graph(),
and free it before the function return to prevent memory leak.

Fixes: 76ffa5ab5b79 ("coresight: Support for ACPI bindings")
Signed-off-by: Junhao He <hejunhao3@huawei.com>
---
 .../hwtracing/coresight/coresight-platform.c  | 40 ++++++++++++-------
 1 file changed, 26 insertions(+), 14 deletions(-)

diff --git a/drivers/hwtracing/coresight/coresight-platform.c b/drivers/hwtracing/coresight/coresight-platform.c
index 7d7b641c0a71..9d550f5697fa 100644
--- a/drivers/hwtracing/coresight/coresight-platform.c
+++ b/drivers/hwtracing/coresight/coresight-platform.c
@@ -492,19 +492,18 @@ static inline bool acpi_validate_dsd_graph(const union acpi_object *graph)
 
 /* acpi_get_dsd_graph	- Find the _DSD Graph property for the given device. */
 static const union acpi_object *
-acpi_get_dsd_graph(struct acpi_device *adev)
+acpi_get_dsd_graph(struct acpi_device *adev, struct acpi_buffer *buf)
 {
 	int i;
-	struct acpi_buffer buf = { ACPI_ALLOCATE_BUFFER };
 	acpi_status status;
 	const union acpi_object *dsd;
 
 	status = acpi_evaluate_object_typed(adev->handle, "_DSD", NULL,
-					    &buf, ACPI_TYPE_PACKAGE);
+					    buf, ACPI_TYPE_PACKAGE);
 	if (ACPI_FAILURE(status))
 		return NULL;
 
-	dsd = buf.pointer;
+	dsd = buf->pointer;
 
 	/*
 	 * _DSD property consists tuples { Prop_UUID, Package() }
@@ -555,12 +554,12 @@ acpi_validate_coresight_graph(const union acpi_object *cs_graph)
  * returns NULL.
  */
 static const union acpi_object *
-acpi_get_coresight_graph(struct acpi_device *adev)
+acpi_get_coresight_graph(struct acpi_device *adev, struct acpi_buffer *buf)
 {
 	const union acpi_object *graph_list, *graph;
 	int i, nr_graphs;
 
-	graph_list = acpi_get_dsd_graph(adev);
+	graph_list = acpi_get_dsd_graph(adev, buf);
 	if (!graph_list)
 		return graph_list;
 
@@ -661,22 +660,24 @@ static int acpi_coresight_parse_graph(struct device *dev,
 				      struct acpi_device *adev,
 				      struct coresight_platform_data *pdata)
 {
+	int ret = 0;
 	int i, nlinks;
 	const union acpi_object *graph;
 	struct coresight_connection conn, zero_conn = {};
 	struct coresight_connection *new_conn;
+	struct acpi_buffer buf = { ACPI_ALLOCATE_BUFFER, NULL };
 
-	graph = acpi_get_coresight_graph(adev);
+	graph = acpi_get_coresight_graph(adev, &buf);
 	/*
 	 * There are no graph connections, which is fine for some components.
 	 * e.g., ETE
 	 */
 	if (!graph)
-		return 0;
+		goto free;
 
 	nlinks = graph->package.elements[2].integer.value;
 	if (!nlinks)
-		return 0;
+		goto free;
 
 	for (i = 0; i < nlinks; i++) {
 		const union acpi_object *link = &graph->package.elements[3 + i];
@@ -684,17 +685,28 @@ static int acpi_coresight_parse_graph(struct device *dev,
 
 		conn = zero_conn;
 		dir = acpi_coresight_parse_link(adev, link, &conn);
-		if (dir < 0)
-			return dir;
+		if (dir < 0) {
+			ret = dir;
+			goto free;
+		}
 
 		if (dir == ACPI_CORESIGHT_LINK_MASTER) {
 			new_conn = coresight_add_out_conn(dev, pdata, &conn);
-			if (IS_ERR(new_conn))
-				return PTR_ERR(new_conn);
+			if (IS_ERR(new_conn)) {
+				ret = PTR_ERR(new_conn);
+				goto free;
+			}
 		}
 	}
 
-	return 0;
+free:
+	/*
+	 * When ACPI fails to alloc a buffer, it will free the buffer
+	 * created via ACPI_ALLOCATE_BUFFER and set to NULL.
+	 * ACPI_FREE can handle NULL pointers, so free it directly.
+	 */
+	ACPI_FREE(buf.pointer);
+	return ret;
 }
 
 /*
-- 
2.33.0

Re: [PATCH 1/2] coresight: Fix memory leak in acpi_buffer->pointer

[James Clark]

On 17/08/2023 09:59, Junhao He wrote:
> There are memory leaks reported by kmemleak:
> ...
> unreferenced object 0xffff00213c141000 (size 1024):
>   comm "systemd-udevd", pid 2123, jiffies 4294909467 (age 6062.160s)
>   hex dump (first 32 bytes):
>     04 00 00 00 02 00 00 00 18 10 14 3c 21 00 ff ff  ...........<!...
>     00 00 00 00 00 00 00 00 03 00 00 00 10 00 00 00  ................
>   backtrace:
>     [<000000004b7c9001>] __kmem_cache_alloc_node+0x2f8/0x348
>     [<00000000b0fc7ceb>] __kmalloc+0x58/0x108
>     [<0000000064ff4695>] acpi_os_allocate+0x2c/0x68
>     [<000000007d57d116>] acpi_ut_initialize_buffer+0x54/0xe0
>     [<0000000024583908>] acpi_evaluate_object+0x388/0x438
>     [<0000000017b2e72b>] acpi_evaluate_object_typed+0xe8/0x240
>     [<000000005df0eac2>] coresight_get_platform_data+0x1b4/0x988 [coresight]
> ...
> 
> The ACPI buffer memory (buf.pointer) should be freed. But the buffer
> is also used after returning from acpi_get_dsd_graph().
> Move the temporary variables buf to acpi_coresight_parse_graph(),
> and free it before the function return to prevent memory leak.
> 
> Fixes: 76ffa5ab5b79 ("coresight: Support for ACPI bindings")
> Signed-off-by: Junhao He <hejunhao3@huawei.com>

I confirmed that the error gone. Thanks for the fix.

Reviewed-by: James Clark <james.clark@arm.com>

> ---
>  .../hwtracing/coresight/coresight-platform.c  | 40 ++++++++++++-------
>  1 file changed, 26 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/hwtracing/coresight/coresight-platform.c b/drivers/hwtracing/coresight/coresight-platform.c
> index 7d7b641c0a71..9d550f5697fa 100644
> --- a/drivers/hwtracing/coresight/coresight-platform.c
> +++ b/drivers/hwtracing/coresight/coresight-platform.c
> @@ -492,19 +492,18 @@ static inline bool acpi_validate_dsd_graph(const union acpi_object *graph)
>  
>  /* acpi_get_dsd_graph	- Find the _DSD Graph property for the given device. */
>  static const union acpi_object *
> -acpi_get_dsd_graph(struct acpi_device *adev)
> +acpi_get_dsd_graph(struct acpi_device *adev, struct acpi_buffer *buf)
>  {
>  	int i;
> -	struct acpi_buffer buf = { ACPI_ALLOCATE_BUFFER };
>  	acpi_status status;
>  	const union acpi_object *dsd;
>  
>  	status = acpi_evaluate_object_typed(adev->handle, "_DSD", NULL,
> -					    &buf, ACPI_TYPE_PACKAGE);
> +					    buf, ACPI_TYPE_PACKAGE);
>  	if (ACPI_FAILURE(status))
>  		return NULL;
>  
> -	dsd = buf.pointer;
> +	dsd = buf->pointer;
>  
>  	/*
>  	 * _DSD property consists tuples { Prop_UUID, Package() }
> @@ -555,12 +554,12 @@ acpi_validate_coresight_graph(const union acpi_object *cs_graph)
>   * returns NULL.
>   */
>  static const union acpi_object *
> -acpi_get_coresight_graph(struct acpi_device *adev)
> +acpi_get_coresight_graph(struct acpi_device *adev, struct acpi_buffer *buf)
>  {
>  	const union acpi_object *graph_list, *graph;
>  	int i, nr_graphs;
>  
> -	graph_list = acpi_get_dsd_graph(adev);
> +	graph_list = acpi_get_dsd_graph(adev, buf);
>  	if (!graph_list)
>  		return graph_list;
>  
> @@ -661,22 +660,24 @@ static int acpi_coresight_parse_graph(struct device *dev,
>  				      struct acpi_device *adev,
>  				      struct coresight_platform_data *pdata)
>  {
> +	int ret = 0;
>  	int i, nlinks;
>  	const union acpi_object *graph;
>  	struct coresight_connection conn, zero_conn = {};
>  	struct coresight_connection *new_conn;
> +	struct acpi_buffer buf = { ACPI_ALLOCATE_BUFFER, NULL };
>  
> -	graph = acpi_get_coresight_graph(adev);
> +	graph = acpi_get_coresight_graph(adev, &buf);
>  	/*
>  	 * There are no graph connections, which is fine for some components.
>  	 * e.g., ETE
>  	 */
>  	if (!graph)
> -		return 0;
> +		goto free;
>  
>  	nlinks = graph->package.elements[2].integer.value;
>  	if (!nlinks)
> -		return 0;
> +		goto free;
>  
>  	for (i = 0; i < nlinks; i++) {
>  		const union acpi_object *link = &graph->package.elements[3 + i];
> @@ -684,17 +685,28 @@ static int acpi_coresight_parse_graph(struct device *dev,
>  
>  		conn = zero_conn;
>  		dir = acpi_coresight_parse_link(adev, link, &conn);
> -		if (dir < 0)
> -			return dir;
> +		if (dir < 0) {
> +			ret = dir;
> +			goto free;
> +		}
>  
>  		if (dir == ACPI_CORESIGHT_LINK_MASTER) {
>  			new_conn = coresight_add_out_conn(dev, pdata, &conn);
> -			if (IS_ERR(new_conn))
> -				return PTR_ERR(new_conn);
> +			if (IS_ERR(new_conn)) {
> +				ret = PTR_ERR(new_conn);
> +				goto free;
> +			}
>  		}
>  	}
>  
> -	return 0;
> +free:
> +	/*
> +	 * When ACPI fails to alloc a buffer, it will free the buffer
> +	 * created via ACPI_ALLOCATE_BUFFER and set to NULL.
> +	 * ACPI_FREE can handle NULL pointers, so free it directly.
> +	 */
> +	ACPI_FREE(buf.pointer);
> +	return ret;
>  }
>  
>  /*

Re: [PATCH 1/2] coresight: Fix memory leak in acpi_buffer->pointer

[Suzuki K Poulose]

On 17/08/2023 15:03, James Clark wrote:
> 
> 
> On 17/08/2023 09:59, Junhao He wrote:
>> There are memory leaks reported by kmemleak:
>> ...
>> unreferenced object 0xffff00213c141000 (size 1024):
>>    comm "systemd-udevd", pid 2123, jiffies 4294909467 (age 6062.160s)
>>    hex dump (first 32 bytes):
>>      04 00 00 00 02 00 00 00 18 10 14 3c 21 00 ff ff  ...........<!...
>>      00 00 00 00 00 00 00 00 03 00 00 00 10 00 00 00  ................
>>    backtrace:
>>      [<000000004b7c9001>] __kmem_cache_alloc_node+0x2f8/0x348
>>      [<00000000b0fc7ceb>] __kmalloc+0x58/0x108
>>      [<0000000064ff4695>] acpi_os_allocate+0x2c/0x68
>>      [<000000007d57d116>] acpi_ut_initialize_buffer+0x54/0xe0
>>      [<0000000024583908>] acpi_evaluate_object+0x388/0x438
>>      [<0000000017b2e72b>] acpi_evaluate_object_typed+0xe8/0x240
>>      [<000000005df0eac2>] coresight_get_platform_data+0x1b4/0x988 [coresight]
>> ...
>>
>> The ACPI buffer memory (buf.pointer) should be freed. But the buffer
>> is also used after returning from acpi_get_dsd_graph().
>> Move the temporary variables buf to acpi_coresight_parse_graph(),
>> and free it before the function return to prevent memory leak.
>>
>> Fixes: 76ffa5ab5b79 ("coresight: Support for ACPI bindings")
>> Signed-off-by: Junhao He <hejunhao3@huawei.com>
> 
> I confirmed that the error gone. Thanks for the fix.
> 
> Reviewed-by: James Clark <james.clark@arm.com>

Queued:

https://git.kernel.org/coresight/c/1a9e02673e25

Thanks!

Suzuki

[PATCH 2/2] coresight: core: fix memory leak in dict->fwnode_list

[Junhao He]

There are memory leaks reported by kmemleak:
...
unreferenced object 0xffff2020103c3200 (size 256):
  comm "insmod", pid 4476, jiffies 4294978252 (age 50072.536s)
  hex dump (first 32 bytes):
    10 60 40 06 28 20 ff ff 10 c0 59 06 20 20 ff ff  .`@.( ....Y.  ..
    10 e0 47 06 28 20 ff ff 10 00 49 06 28 20 ff ff  ..G.( ....I.( ..
  backtrace:
    [<0000000034ec4724>] __kmem_cache_alloc_node+0x2f8/0x348
    [<0000000057fbc15d>] __kmalloc_node_track_caller+0x5c/0x110
    [<00000055d5e34b>] krealloc+0x8c/0x178
    [<00000000a4635beb>] coresight_alloc_device_name+0x128/0x188 [coresight]
    [<00000000a92ddfee>] funnel_cs_ops+0x10/0xfffffffffffedaa0 [coresight_funnel]
    [<00000000449e20f8>] dynamic_funnel_ids+0x80/0xfffffffffffed840 [coresight_funnel]
...

when remove driver, the golab variables defined by the macro
DEFINE_CORESIGHT_DEVLIST will be released, dict->nr_idx and
dict->fwnode_list are cleared to 0. The lifetime of the golab
variable has ended. So the buffer pointer is lost.

Use the callback of devm_add_action_or_reset() to free memory.

Fixes: 0f5f9b6ba9e1 ("coresight: Use platform agnostic names")
Signed-off-by: Junhao He <hejunhao3@huawei.com>
---
 drivers/hwtracing/coresight/coresight-core.c | 20 +++++++++++++++++++-
 1 file changed, 19 insertions(+), 1 deletion(-)

diff --git a/drivers/hwtracing/coresight/coresight-core.c b/drivers/hwtracing/coresight/coresight-core.c
index 9fabe00a40d6..6849faad697d 100644
--- a/drivers/hwtracing/coresight/coresight-core.c
+++ b/drivers/hwtracing/coresight/coresight-core.c
@@ -1756,6 +1756,20 @@ bool coresight_loses_context_with_cpu(struct device *dev)
 }
 EXPORT_SYMBOL_GPL(coresight_loses_context_with_cpu);
 
+void coresight_release_dev_list(void *data)
+{
+	struct coresight_dev_list *dict = data;
+
+	mutex_lock(&coresight_mutex);
+
+	if (dict->nr_idx) {
+		kfree(dict->fwnode_list);
+		dict->nr_idx = 0;
+	}
+
+	mutex_unlock(&coresight_mutex);
+}
+
 /*
  * coresight_alloc_device_name - Get an index for a given device in the
  * device index list specific to a driver. An index is allocated for a
@@ -1766,12 +1780,16 @@ EXPORT_SYMBOL_GPL(coresight_loses_context_with_cpu);
 char *coresight_alloc_device_name(struct coresight_dev_list *dict,
 				  struct device *dev)
 {
-	int idx;
+	int idx, ret;
 	char *name = NULL;
 	struct fwnode_handle **list;
 
 	mutex_lock(&coresight_mutex);
 
+	ret = devm_add_action_or_reset(dev, coresight_release_dev_list, dict);
+	if (ret)
+		goto done;
+
 	idx = coresight_search_device_idx(dict, dev_fwnode(dev));
 	if (idx < 0) {
 		/* Make space for the new entry */
-- 
2.33.0

Re: [PATCH 2/2] coresight: core: fix memory leak in dict->fwnode_list

[James Clark]

On 17/08/2023 09:59, Junhao He wrote:
> There are memory leaks reported by kmemleak:
> ...
> unreferenced object 0xffff2020103c3200 (size 256):
>   comm "insmod", pid 4476, jiffies 4294978252 (age 50072.536s)
>   hex dump (first 32 bytes):
>     10 60 40 06 28 20 ff ff 10 c0 59 06 20 20 ff ff  .`@.( ....Y.  ..
>     10 e0 47 06 28 20 ff ff 10 00 49 06 28 20 ff ff  ..G.( ....I.( ..
>   backtrace:
>     [<0000000034ec4724>] __kmem_cache_alloc_node+0x2f8/0x348
>     [<0000000057fbc15d>] __kmalloc_node_track_caller+0x5c/0x110
>     [<00000055d5e34b>] krealloc+0x8c/0x178
>     [<00000000a4635beb>] coresight_alloc_device_name+0x128/0x188 [coresight]
>     [<00000000a92ddfee>] funnel_cs_ops+0x10/0xfffffffffffedaa0 [coresight_funnel]
>     [<00000000449e20f8>] dynamic_funnel_ids+0x80/0xfffffffffffed840 [coresight_funnel]
> ...
> 
> when remove driver, the golab variables defined by the macro
> DEFINE_CORESIGHT_DEVLIST will be released, dict->nr_idx and
> dict->fwnode_list are cleared to 0. The lifetime of the golab
> variable has ended. So the buffer pointer is lost.
> 
> Use the callback of devm_add_action_or_reset() to free memory.
> 
> Fixes: 0f5f9b6ba9e1 ("coresight: Use platform agnostic names")
> Signed-off-by: Junhao He <hejunhao3@huawei.com>
> ---
>  drivers/hwtracing/coresight/coresight-core.c | 20 +++++++++++++++++++-
>  1 file changed, 19 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/hwtracing/coresight/coresight-core.c b/drivers/hwtracing/coresight/coresight-core.c
> index 9fabe00a40d6..6849faad697d 100644
> --- a/drivers/hwtracing/coresight/coresight-core.c
> +++ b/drivers/hwtracing/coresight/coresight-core.c
> @@ -1756,6 +1756,20 @@ bool coresight_loses_context_with_cpu(struct device *dev)
>  }
>  EXPORT_SYMBOL_GPL(coresight_loses_context_with_cpu);
>  
> +void coresight_release_dev_list(void *data)
> +{
> +	struct coresight_dev_list *dict = data;
> +
> +	mutex_lock(&coresight_mutex);
> +
> +	if (dict->nr_idx) {
> +		kfree(dict->fwnode_list);
> +		dict->nr_idx = 0;
> +	}
> +
> +	mutex_unlock(&coresight_mutex);
> +}
> +
>  /*
>   * coresight_alloc_device_name - Get an index for a given device in the
>   * device index list specific to a driver. An index is allocated for a
> @@ -1766,12 +1780,16 @@ EXPORT_SYMBOL_GPL(coresight_loses_context_with_cpu);
>  char *coresight_alloc_device_name(struct coresight_dev_list *dict,
>  				  struct device *dev)
>  {
> -	int idx;
> +	int idx, ret;
>  	char *name = NULL;
>  	struct fwnode_handle **list;
>  
>  	mutex_lock(&coresight_mutex);
>  
> +	ret = devm_add_action_or_reset(dev, coresight_release_dev_list, dict);
> +	if (ret)
> +		goto done;
> +

Hi Junhao,

Changing the list allocator to a devm one fixes the issue without having
to add the callback:

  -               list = krealloc_array(dict->fwnode_list,
  +               list = devm_krealloc_array(dev, dict->fwnode_list,

The callback stands out a bit and would make someone reading it wonder
why only that one is done that way but all other allocations in
Coresight avoid it.

The nr_idx variable doesn't need to be zeroed because its backed by a
static variable and is zeroed when the module is reloaded as far as I
can see.

Thanks
James

>  	idx = coresight_search_device_idx(dict, dev_fwnode(dev));
>  	if (idx < 0) {
>  		/* Make space for the new entry */

Re: [PATCH 2/2] coresight: core: fix memory leak in dict->fwnode_list

[Suzuki K Poulose]

On 17/08/2023 15:31, James Clark wrote:
> 
> 
> On 17/08/2023 09:59, Junhao He wrote:
>> There are memory leaks reported by kmemleak:
>> ...
>> unreferenced object 0xffff2020103c3200 (size 256):
>>    comm "insmod", pid 4476, jiffies 4294978252 (age 50072.536s)
>>    hex dump (first 32 bytes):
>>      10 60 40 06 28 20 ff ff 10 c0 59 06 20 20 ff ff  .`@.( ....Y.  ..
>>      10 e0 47 06 28 20 ff ff 10 00 49 06 28 20 ff ff  ..G.( ....I.( ..
>>    backtrace:
>>      [<0000000034ec4724>] __kmem_cache_alloc_node+0x2f8/0x348
>>      [<0000000057fbc15d>] __kmalloc_node_track_caller+0x5c/0x110
>>      [<00000055d5e34b>] krealloc+0x8c/0x178
>>      [<00000000a4635beb>] coresight_alloc_device_name+0x128/0x188 [coresight]
>>      [<00000000a92ddfee>] funnel_cs_ops+0x10/0xfffffffffffedaa0 [coresight_funnel]
>>      [<00000000449e20f8>] dynamic_funnel_ids+0x80/0xfffffffffffed840 [coresight_funnel]
>> ...
>>
>> when remove driver, the golab variables defined by the macro
>> DEFINE_CORESIGHT_DEVLIST will be released, dict->nr_idx and
>> dict->fwnode_list are cleared to 0. The lifetime of the golab
>> variable has ended. So the buffer pointer is lost.
>>
>> Use the callback of devm_add_action_or_reset() to free memory.
>>
>> Fixes: 0f5f9b6ba9e1 ("coresight: Use platform agnostic names")
>> Signed-off-by: Junhao He <hejunhao3@huawei.com>
>> ---
>>   drivers/hwtracing/coresight/coresight-core.c | 20 +++++++++++++++++++-
>>   1 file changed, 19 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/hwtracing/coresight/coresight-core.c b/drivers/hwtracing/coresight/coresight-core.c
>> index 9fabe00a40d6..6849faad697d 100644
>> --- a/drivers/hwtracing/coresight/coresight-core.c
>> +++ b/drivers/hwtracing/coresight/coresight-core.c
>> @@ -1756,6 +1756,20 @@ bool coresight_loses_context_with_cpu(struct device *dev)
>>   }
>>   EXPORT_SYMBOL_GPL(coresight_loses_context_with_cpu);
>>   
>> +void coresight_release_dev_list(void *data)
>> +{
>> +	struct coresight_dev_list *dict = data;
>> +
>> +	mutex_lock(&coresight_mutex);
>> +
>> +	if (dict->nr_idx) {
>> +		kfree(dict->fwnode_list);
>> +		dict->nr_idx = 0;
>> +	}
>> +
>> +	mutex_unlock(&coresight_mutex);
>> +}
>> +
>>   /*
>>    * coresight_alloc_device_name - Get an index for a given device in the
>>    * device index list specific to a driver. An index is allocated for a
>> @@ -1766,12 +1780,16 @@ EXPORT_SYMBOL_GPL(coresight_loses_context_with_cpu);
>>   char *coresight_alloc_device_name(struct coresight_dev_list *dict,
>>   				  struct device *dev)
>>   {
>> -	int idx;
>> +	int idx, ret;
>>   	char *name = NULL;
>>   	struct fwnode_handle **list;
>>   
>>   	mutex_lock(&coresight_mutex);
>>   
>> +	ret = devm_add_action_or_reset(dev, coresight_release_dev_list, dict);
>> +	if (ret)
>> +		goto done;
>> +
> 
> Hi Junhao,
> 
> Changing the list allocator to a devm one fixes the issue without having
> to add the callback:
> 
>    -               list = krealloc_array(dict->fwnode_list,
>    +               list = devm_krealloc_array(dev, dict->fwnode_list,

Thats problematic. Please note that the list is "per module/driver" not 
per device. So, you cannot tie it to a single device. Please see my 
response in the thread.

Suzuki


> 
> The callback stands out a bit and would make someone reading it wonder
> why only that one is done that way but all other allocations in
> Coresight avoid it.
> 
> The nr_idx variable doesn't need to be zeroed because its backed by a
> static variable and is zeroed when the module is reloaded as far as I
> can see.
> 
> Thanks
> James
> 
>>   	idx = coresight_search_device_idx(dict, dev_fwnode(dev));
>>   	if (idx < 0) {
>>   		/* Make space for the new entry */

Re: [PATCH 2/2] coresight: core: fix memory leak in dict->fwnode_list

[Suzuki K Poulose]

On 17/08/2023 09:59, Junhao He wrote:
> There are memory leaks reported by kmemleak:
> ...
> unreferenced object 0xffff2020103c3200 (size 256):
>    comm "insmod", pid 4476, jiffies 4294978252 (age 50072.536s)
>    hex dump (first 32 bytes):
>      10 60 40 06 28 20 ff ff 10 c0 59 06 20 20 ff ff  .`@.( ....Y.  ..
>      10 e0 47 06 28 20 ff ff 10 00 49 06 28 20 ff ff  ..G.( ....I.( ..
>    backtrace:
>      [<0000000034ec4724>] __kmem_cache_alloc_node+0x2f8/0x348
>      [<0000000057fbc15d>] __kmalloc_node_track_caller+0x5c/0x110
>      [<00000055d5e34b>] krealloc+0x8c/0x178
>      [<00000000a4635beb>] coresight_alloc_device_name+0x128/0x188 [coresight]
>      [<00000000a92ddfee>] funnel_cs_ops+0x10/0xfffffffffffedaa0 [coresight_funnel]
>      [<00000000449e20f8>] dynamic_funnel_ids+0x80/0xfffffffffffed840 [coresight_funnel]
> ...
> 
> when remove driver, the golab variables defined by the macro
> DEFINE_CORESIGHT_DEVLIST will be released, dict->nr_idx and
> dict->fwnode_list are cleared to 0. The lifetime of the golab
> variable has ended. So the buffer pointer is lost.
> 
> Use the callback of devm_add_action_or_reset() to free memory.

Thanks for the report. But please see below:

> 
> Fixes: 0f5f9b6ba9e1 ("coresight: Use platform agnostic names")
> Signed-off-by: Junhao He <hejunhao3@huawei.com>
> ---
>   drivers/hwtracing/coresight/coresight-core.c | 20 +++++++++++++++++++-
>   1 file changed, 19 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/hwtracing/coresight/coresight-core.c b/drivers/hwtracing/coresight/coresight-core.c
> index 9fabe00a40d6..6849faad697d 100644
> --- a/drivers/hwtracing/coresight/coresight-core.c
> +++ b/drivers/hwtracing/coresight/coresight-core.c
> @@ -1756,6 +1756,20 @@ bool coresight_loses_context_with_cpu(struct device *dev)
>   }
>   EXPORT_SYMBOL_GPL(coresight_loses_context_with_cpu);
>   
> +void coresight_release_dev_list(void *data)
> +{
> +	struct coresight_dev_list *dict = data;
> +
> +	mutex_lock(&coresight_mutex);
> +
> +	if (dict->nr_idx) {
> +		kfree(dict->fwnode_list);
> +		dict->nr_idx = 0;
> +	}
> +
> +	mutex_unlock(&coresight_mutex);
> +}
> +
>   /*
>    * coresight_alloc_device_name - Get an index for a given device in the
>    * device index list specific to a driver. An index is allocated for a
> @@ -1766,12 +1780,16 @@ EXPORT_SYMBOL_GPL(coresight_loses_context_with_cpu);
>   char *coresight_alloc_device_name(struct coresight_dev_list *dict,
>   				  struct device *dev)
>   {
> -	int idx;
> +	int idx, ret;
>   	char *name = NULL;
>   	struct fwnode_handle **list;
>   
>   	mutex_lock(&coresight_mutex);
>   
> +	ret = devm_add_action_or_reset(dev, coresight_release_dev_list, dict);
> +	if (ret)
> +		goto done;

This looks wrong. The devlist should be only released on the "driver" 
unload, not on every device release. The list retains the fwnode to
assign the same name for a device, if it is re-probed (e.g., due to
-EPROBE_DEFER error).

Suzuki

Re: [PATCH 2/2] coresight: core: fix memory leak in dict->fwnode_list

[James Clark]

On 17/08/2023 15:39, Suzuki K Poulose wrote:
> On 17/08/2023 09:59, Junhao He wrote:
>> There are memory leaks reported by kmemleak:
>> ...
>> unreferenced object 0xffff2020103c3200 (size 256):
>>    comm "insmod", pid 4476, jiffies 4294978252 (age 50072.536s)
>>    hex dump (first 32 bytes):
>>      10 60 40 06 28 20 ff ff 10 c0 59 06 20 20 ff ff  .`@.( ....Y.  ..
>>      10 e0 47 06 28 20 ff ff 10 00 49 06 28 20 ff ff  ..G.( ....I.( ..
>>    backtrace:
>>      [<0000000034ec4724>] __kmem_cache_alloc_node+0x2f8/0x348
>>      [<0000000057fbc15d>] __kmalloc_node_track_caller+0x5c/0x110
>>      [<00000055d5e34b>] krealloc+0x8c/0x178
>>      [<00000000a4635beb>] coresight_alloc_device_name+0x128/0x188
>> [coresight]
>>      [<00000000a92ddfee>] funnel_cs_ops+0x10/0xfffffffffffedaa0
>> [coresight_funnel]
>>      [<00000000449e20f8>] dynamic_funnel_ids+0x80/0xfffffffffffed840
>> [coresight_funnel]
>> ...
>>
>> when remove driver, the golab variables defined by the macro
>> DEFINE_CORESIGHT_DEVLIST will be released, dict->nr_idx and
>> dict->fwnode_list are cleared to 0. The lifetime of the golab
>> variable has ended. So the buffer pointer is lost.
>>
>> Use the callback of devm_add_action_or_reset() to free memory.
> 
> Thanks for the report. But please see below:
> 
>>
>> Fixes: 0f5f9b6ba9e1 ("coresight: Use platform agnostic names")
>> Signed-off-by: Junhao He <hejunhao3@huawei.com>
>> ---
>>   drivers/hwtracing/coresight/coresight-core.c | 20 +++++++++++++++++++-
>>   1 file changed, 19 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/hwtracing/coresight/coresight-core.c
>> b/drivers/hwtracing/coresight/coresight-core.c
>> index 9fabe00a40d6..6849faad697d 100644
>> --- a/drivers/hwtracing/coresight/coresight-core.c
>> +++ b/drivers/hwtracing/coresight/coresight-core.c
>> @@ -1756,6 +1756,20 @@ bool coresight_loses_context_with_cpu(struct
>> device *dev)
>>   }
>>   EXPORT_SYMBOL_GPL(coresight_loses_context_with_cpu);
>>   +void coresight_release_dev_list(void *data)
>> +{
>> +    struct coresight_dev_list *dict = data;
>> +
>> +    mutex_lock(&coresight_mutex);
>> +
>> +    if (dict->nr_idx) {
>> +        kfree(dict->fwnode_list);
>> +        dict->nr_idx = 0;
>> +    }
>> +
>> +    mutex_unlock(&coresight_mutex);
>> +}
>> +
>>   /*
>>    * coresight_alloc_device_name - Get an index for a given device in the
>>    * device index list specific to a driver. An index is allocated for a
>> @@ -1766,12 +1780,16 @@
>> EXPORT_SYMBOL_GPL(coresight_loses_context_with_cpu);
>>   char *coresight_alloc_device_name(struct coresight_dev_list *dict,
>>                     struct device *dev)
>>   {
>> -    int idx;
>> +    int idx, ret;
>>       char *name = NULL;
>>       struct fwnode_handle **list;
>>         mutex_lock(&coresight_mutex);
>>   +    ret = devm_add_action_or_reset(dev, coresight_release_dev_list,
>> dict);
>> +    if (ret)
>> +        goto done;
> 
> This looks wrong. The devlist should be only released on the "driver"
> unload, not on every device release. The list retains the fwnode to
> assign the same name for a device, if it is re-probed (e.g., due to
> -EPROBE_DEFER error).
> 
> Suzuki
> 

I think in that case my suggestion to change it to devm_krealloc_array
might be wrong then. Probably worth putting an explicit comment there in
case someone tries to tidy up all the non devm allocators to devm ones.

But how do you release a single device without releasing the whole driver?

James

Re: [PATCH 2/2] coresight: core: fix memory leak in dict->fwnode_list

[Suzuki K Poulose]

On 17/08/2023 15:46, James Clark wrote:
> 
> 
> On 17/08/2023 15:39, Suzuki K Poulose wrote:
>> On 17/08/2023 09:59, Junhao He wrote:
>>> There are memory leaks reported by kmemleak:
>>> ...
>>> unreferenced object 0xffff2020103c3200 (size 256):
>>>     comm "insmod", pid 4476, jiffies 4294978252 (age 50072.536s)
>>>     hex dump (first 32 bytes):
>>>       10 60 40 06 28 20 ff ff 10 c0 59 06 20 20 ff ff  .`@.( ....Y.  ..
>>>       10 e0 47 06 28 20 ff ff 10 00 49 06 28 20 ff ff  ..G.( ....I.( ..
>>>     backtrace:
>>>       [<0000000034ec4724>] __kmem_cache_alloc_node+0x2f8/0x348
>>>       [<0000000057fbc15d>] __kmalloc_node_track_caller+0x5c/0x110
>>>       [<00000055d5e34b>] krealloc+0x8c/0x178
>>>       [<00000000a4635beb>] coresight_alloc_device_name+0x128/0x188
>>> [coresight]
>>>       [<00000000a92ddfee>] funnel_cs_ops+0x10/0xfffffffffffedaa0
>>> [coresight_funnel]
>>>       [<00000000449e20f8>] dynamic_funnel_ids+0x80/0xfffffffffffed840
>>> [coresight_funnel]
>>> ...
>>>
>>> when remove driver, the golab variables defined by the macro
>>> DEFINE_CORESIGHT_DEVLIST will be released, dict->nr_idx and
>>> dict->fwnode_list are cleared to 0. The lifetime of the golab
>>> variable has ended. So the buffer pointer is lost.
>>>
>>> Use the callback of devm_add_action_or_reset() to free memory.
>>
>> Thanks for the report. But please see below:
>>
>>>
>>> Fixes: 0f5f9b6ba9e1 ("coresight: Use platform agnostic names")
>>> Signed-off-by: Junhao He <hejunhao3@huawei.com>
>>> ---
>>>    drivers/hwtracing/coresight/coresight-core.c | 20 +++++++++++++++++++-
>>>    1 file changed, 19 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/hwtracing/coresight/coresight-core.c
>>> b/drivers/hwtracing/coresight/coresight-core.c
>>> index 9fabe00a40d6..6849faad697d 100644
>>> --- a/drivers/hwtracing/coresight/coresight-core.c
>>> +++ b/drivers/hwtracing/coresight/coresight-core.c
>>> @@ -1756,6 +1756,20 @@ bool coresight_loses_context_with_cpu(struct
>>> device *dev)
>>>    }
>>>    EXPORT_SYMBOL_GPL(coresight_loses_context_with_cpu);
>>>    +void coresight_release_dev_list(void *data)
>>> +{
>>> +    struct coresight_dev_list *dict = data;
>>> +
>>> +    mutex_lock(&coresight_mutex);
>>> +
>>> +    if (dict->nr_idx) {
>>> +        kfree(dict->fwnode_list);
>>> +        dict->nr_idx = 0;
>>> +    }
>>> +
>>> +    mutex_unlock(&coresight_mutex);
>>> +}
>>> +
>>>    /*
>>>     * coresight_alloc_device_name - Get an index for a given device in the
>>>     * device index list specific to a driver. An index is allocated for a
>>> @@ -1766,12 +1780,16 @@
>>> EXPORT_SYMBOL_GPL(coresight_loses_context_with_cpu);
>>>    char *coresight_alloc_device_name(struct coresight_dev_list *dict,
>>>                      struct device *dev)
>>>    {
>>> -    int idx;
>>> +    int idx, ret;
>>>        char *name = NULL;
>>>        struct fwnode_handle **list;
>>>          mutex_lock(&coresight_mutex);
>>>    +    ret = devm_add_action_or_reset(dev, coresight_release_dev_list,
>>> dict);
>>> +    if (ret)
>>> +        goto done;
>>
>> This looks wrong. The devlist should be only released on the "driver"
>> unload, not on every device release. The list retains the fwnode to
>> assign the same name for a device, if it is re-probed (e.g., due to
>> -EPROBE_DEFER error).
>>
>> Suzuki
>>
> 
> I think in that case my suggestion to change it to devm_krealloc_array
> might be wrong then. Probably worth putting an explicit comment there in
> case someone tries to tidy up all the non devm allocators to devm ones.
> 
> But how do you release a single device without releasing the whole driver?

You could unbind a device from the driver and that will make another 
driver probe it. I know it works for PCI/platform with VFIO.

Suzuki

> 
> James

Re: [PATCH 2/2] coresight: core: fix memory leak in dict->fwnode_list

[Suzuki K Poulose]

On 17/08/2023 15:39, Suzuki K Poulose wrote:
> On 17/08/2023 09:59, Junhao He wrote:
>> There are memory leaks reported by kmemleak:
>> ...
>> unreferenced object 0xffff2020103c3200 (size 256):
>>    comm "insmod", pid 4476, jiffies 4294978252 (age 50072.536s)
>>    hex dump (first 32 bytes):
>>      10 60 40 06 28 20 ff ff 10 c0 59 06 20 20 ff ff  .`@.( ....Y.  ..
>>      10 e0 47 06 28 20 ff ff 10 00 49 06 28 20 ff ff  ..G.( ....I.( ..
>>    backtrace:
>>      [<0000000034ec4724>] __kmem_cache_alloc_node+0x2f8/0x348
>>      [<0000000057fbc15d>] __kmalloc_node_track_caller+0x5c/0x110
>>      [<00000055d5e34b>] krealloc+0x8c/0x178
>>      [<00000000a4635beb>] coresight_alloc_device_name+0x128/0x188 
>> [coresight]
>>      [<00000000a92ddfee>] funnel_cs_ops+0x10/0xfffffffffffedaa0 
>> [coresight_funnel]
>>      [<00000000449e20f8>] dynamic_funnel_ids+0x80/0xfffffffffffed840 
>> [coresight_funnel]
>> ...
>>
>> when remove driver, the golab variables defined by the macro
>> DEFINE_CORESIGHT_DEVLIST will be released, dict->nr_idx and
>> dict->fwnode_list are cleared to 0. The lifetime of the golab
>> variable has ended. So the buffer pointer is lost.
>>
>> Use the callback of devm_add_action_or_reset() to free memory.
> 
> Thanks for the report. But please see below:
> 
>>
>> Fixes: 0f5f9b6ba9e1 ("coresight: Use platform agnostic names")
>> Signed-off-by: Junhao He <hejunhao3@huawei.com>
>> ---
>>   drivers/hwtracing/coresight/coresight-core.c | 20 +++++++++++++++++++-
>>   1 file changed, 19 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/hwtracing/coresight/coresight-core.c 
>> b/drivers/hwtracing/coresight/coresight-core.c
>> index 9fabe00a40d6..6849faad697d 100644
>> --- a/drivers/hwtracing/coresight/coresight-core.c
>> +++ b/drivers/hwtracing/coresight/coresight-core.c
>> @@ -1756,6 +1756,20 @@ bool coresight_loses_context_with_cpu(struct 
>> device *dev)
>>   }
>>   EXPORT_SYMBOL_GPL(coresight_loses_context_with_cpu);
>> +void coresight_release_dev_list(void *data)
>> +{
>> +    struct coresight_dev_list *dict = data;
>> +
>> +    mutex_lock(&coresight_mutex);
>> +
>> +    if (dict->nr_idx) {
>> +        kfree(dict->fwnode_list);
>> +        dict->nr_idx = 0;
>> +    }
>> +
>> +    mutex_unlock(&coresight_mutex);
>> +}
>> +
>>   /*
>>    * coresight_alloc_device_name - Get an index for a given device in the
>>    * device index list specific to a driver. An index is allocated for a
>> @@ -1766,12 +1780,16 @@ 
>> EXPORT_SYMBOL_GPL(coresight_loses_context_with_cpu);
>>   char *coresight_alloc_device_name(struct coresight_dev_list *dict,
>>                     struct device *dev)
>>   {
>> -    int idx;
>> +    int idx, ret;
>>       char *name = NULL;
>>       struct fwnode_handle **list;
>>       mutex_lock(&coresight_mutex);
>> +    ret = devm_add_action_or_reset(dev, coresight_release_dev_list, 
>> dict);
>> +    if (ret)
>> +        goto done;
> 
> This looks wrong. The devlist should be only released on the "driver" 
> unload, not on every device release. The list retains the fwnode to
> assign the same name for a device, if it is re-probed (e.g., due to
> -EPROBE_DEFER error).

The best way is to free it on module_unload and unfortunately we would
need to do it from all modules using the DEVLIST.

Suzuki


> 
> Suzuki
> 

Re: [PATCH 2/2] coresight: core: fix memory leak in dict->fwnode_list

[James Clark]

On 17/08/2023 15:47, Suzuki K Poulose wrote:
> On 17/08/2023 15:39, Suzuki K Poulose wrote:
>> On 17/08/2023 09:59, Junhao He wrote:
>>> There are memory leaks reported by kmemleak:
>>> ...
>>> unreferenced object 0xffff2020103c3200 (size 256):
>>>    comm "insmod", pid 4476, jiffies 4294978252 (age 50072.536s)
>>>    hex dump (first 32 bytes):
>>>      10 60 40 06 28 20 ff ff 10 c0 59 06 20 20 ff ff  .`@.( ....Y.  ..
>>>      10 e0 47 06 28 20 ff ff 10 00 49 06 28 20 ff ff  ..G.( ....I.( ..
>>>    backtrace:
>>>      [<0000000034ec4724>] __kmem_cache_alloc_node+0x2f8/0x348
>>>      [<0000000057fbc15d>] __kmalloc_node_track_caller+0x5c/0x110
>>>      [<00000055d5e34b>] krealloc+0x8c/0x178
>>>      [<00000000a4635beb>] coresight_alloc_device_name+0x128/0x188
>>> [coresight]
>>>      [<00000000a92ddfee>] funnel_cs_ops+0x10/0xfffffffffffedaa0
>>> [coresight_funnel]
>>>      [<00000000449e20f8>] dynamic_funnel_ids+0x80/0xfffffffffffed840
>>> [coresight_funnel]
>>> ...
>>>
>>> when remove driver, the golab variables defined by the macro
>>> DEFINE_CORESIGHT_DEVLIST will be released, dict->nr_idx and
>>> dict->fwnode_list are cleared to 0. The lifetime of the golab
>>> variable has ended. So the buffer pointer is lost.
>>>
>>> Use the callback of devm_add_action_or_reset() to free memory.
>>
>> Thanks for the report. But please see below:
>>
>>>
>>> Fixes: 0f5f9b6ba9e1 ("coresight: Use platform agnostic names")
>>> Signed-off-by: Junhao He <hejunhao3@huawei.com>
>>> ---
>>>   drivers/hwtracing/coresight/coresight-core.c | 20 +++++++++++++++++++-
>>>   1 file changed, 19 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/hwtracing/coresight/coresight-core.c
>>> b/drivers/hwtracing/coresight/coresight-core.c
>>> index 9fabe00a40d6..6849faad697d 100644
>>> --- a/drivers/hwtracing/coresight/coresight-core.c
>>> +++ b/drivers/hwtracing/coresight/coresight-core.c
>>> @@ -1756,6 +1756,20 @@ bool coresight_loses_context_with_cpu(struct
>>> device *dev)
>>>   }
>>>   EXPORT_SYMBOL_GPL(coresight_loses_context_with_cpu);
>>> +void coresight_release_dev_list(void *data)
>>> +{
>>> +    struct coresight_dev_list *dict = data;
>>> +
>>> +    mutex_lock(&coresight_mutex);
>>> +
>>> +    if (dict->nr_idx) {
>>> +        kfree(dict->fwnode_list);
>>> +        dict->nr_idx = 0;
>>> +    }
>>> +
>>> +    mutex_unlock(&coresight_mutex);
>>> +}
>>> +
>>>   /*
>>>    * coresight_alloc_device_name - Get an index for a given device in
>>> the
>>>    * device index list specific to a driver. An index is allocated for a
>>> @@ -1766,12 +1780,16 @@
>>> EXPORT_SYMBOL_GPL(coresight_loses_context_with_cpu);
>>>   char *coresight_alloc_device_name(struct coresight_dev_list *dict,
>>>                     struct device *dev)
>>>   {
>>> -    int idx;
>>> +    int idx, ret;
>>>       char *name = NULL;
>>>       struct fwnode_handle **list;
>>>       mutex_lock(&coresight_mutex);
>>> +    ret = devm_add_action_or_reset(dev, coresight_release_dev_list,
>>> dict);
>>> +    if (ret)
>>> +        goto done;
>>
>> This looks wrong. The devlist should be only released on the "driver"
>> unload, not on every device release. The list retains the fwnode to
>> assign the same name for a device, if it is re-probed (e.g., due to
>> -EPROBE_DEFER error).
> 
> The best way is to free it on module_unload and unfortunately we would
> need to do it from all modules using the DEVLIST.
> 
> Suzuki
> 

Seems like we might also be able to move the separate lists to be one
big list owned by the main 'coresight' module. If all the other modules
are dependent on that one then it's always loaded first and the list is
available. Then it persists as long as the main module is loaded and can
be freed with the normal devm stuff.

That would avoid the awkward combo of the static variables in each
module plus the non devm kalloced list.

> 
>>
>> Suzuki
>>
> 

Re: [PATCH 2/2] coresight: core: fix memory leak in dict->fwnode_list

[Suzuki K Poulose]

On 17/08/2023 16:01, James Clark wrote:
> 
> 
> On 17/08/2023 15:47, Suzuki K Poulose wrote:
>> On 17/08/2023 15:39, Suzuki K Poulose wrote:
>>> On 17/08/2023 09:59, Junhao He wrote:
>>>> There are memory leaks reported by kmemleak:
>>>> ...
>>>> unreferenced object 0xffff2020103c3200 (size 256):
>>>>     comm "insmod", pid 4476, jiffies 4294978252 (age 50072.536s)
>>>>     hex dump (first 32 bytes):
>>>>       10 60 40 06 28 20 ff ff 10 c0 59 06 20 20 ff ff  .`@.( ....Y.  ..
>>>>       10 e0 47 06 28 20 ff ff 10 00 49 06 28 20 ff ff  ..G.( ....I.( ..
>>>>     backtrace:
>>>>       [<0000000034ec4724>] __kmem_cache_alloc_node+0x2f8/0x348
>>>>       [<0000000057fbc15d>] __kmalloc_node_track_caller+0x5c/0x110
>>>>       [<00000055d5e34b>] krealloc+0x8c/0x178
>>>>       [<00000000a4635beb>] coresight_alloc_device_name+0x128/0x188
>>>> [coresight]
>>>>       [<00000000a92ddfee>] funnel_cs_ops+0x10/0xfffffffffffedaa0
>>>> [coresight_funnel]
>>>>       [<00000000449e20f8>] dynamic_funnel_ids+0x80/0xfffffffffffed840
>>>> [coresight_funnel]
>>>> ...
>>>>
>>>> when remove driver, the golab variables defined by the macro
>>>> DEFINE_CORESIGHT_DEVLIST will be released, dict->nr_idx and
>>>> dict->fwnode_list are cleared to 0. The lifetime of the golab
>>>> variable has ended. So the buffer pointer is lost.
>>>>
>>>> Use the callback of devm_add_action_or_reset() to free memory.
>>>
>>> Thanks for the report. But please see below:
>>>
>>>>
>>>> Fixes: 0f5f9b6ba9e1 ("coresight: Use platform agnostic names")
>>>> Signed-off-by: Junhao He <hejunhao3@huawei.com>
>>>> ---
>>>>    drivers/hwtracing/coresight/coresight-core.c | 20 +++++++++++++++++++-
>>>>    1 file changed, 19 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/hwtracing/coresight/coresight-core.c
>>>> b/drivers/hwtracing/coresight/coresight-core.c
>>>> index 9fabe00a40d6..6849faad697d 100644
>>>> --- a/drivers/hwtracing/coresight/coresight-core.c
>>>> +++ b/drivers/hwtracing/coresight/coresight-core.c
>>>> @@ -1756,6 +1756,20 @@ bool coresight_loses_context_with_cpu(struct
>>>> device *dev)
>>>>    }
>>>>    EXPORT_SYMBOL_GPL(coresight_loses_context_with_cpu);
>>>> +void coresight_release_dev_list(void *data)
>>>> +{
>>>> +    struct coresight_dev_list *dict = data;
>>>> +
>>>> +    mutex_lock(&coresight_mutex);
>>>> +
>>>> +    if (dict->nr_idx) {
>>>> +        kfree(dict->fwnode_list);
>>>> +        dict->nr_idx = 0;
>>>> +    }
>>>> +
>>>> +    mutex_unlock(&coresight_mutex);
>>>> +}
>>>> +
>>>>    /*
>>>>     * coresight_alloc_device_name - Get an index for a given device in
>>>> the
>>>>     * device index list specific to a driver. An index is allocated for a
>>>> @@ -1766,12 +1780,16 @@
>>>> EXPORT_SYMBOL_GPL(coresight_loses_context_with_cpu);
>>>>    char *coresight_alloc_device_name(struct coresight_dev_list *dict,
>>>>                      struct device *dev)
>>>>    {
>>>> -    int idx;
>>>> +    int idx, ret;
>>>>        char *name = NULL;
>>>>        struct fwnode_handle **list;
>>>>        mutex_lock(&coresight_mutex);
>>>> +    ret = devm_add_action_or_reset(dev, coresight_release_dev_list,
>>>> dict);
>>>> +    if (ret)
>>>> +        goto done;
>>>
>>> This looks wrong. The devlist should be only released on the "driver"
>>> unload, not on every device release. The list retains the fwnode to
>>> assign the same name for a device, if it is re-probed (e.g., due to
>>> -EPROBE_DEFER error).
>>
>> The best way is to free it on module_unload and unfortunately we would
>> need to do it from all modules using the DEVLIST.
>>
>> Suzuki
>>
> 
> Seems like we might also be able to move the separate lists to be one
> big list owned by the main 'coresight' module. If all the other modules
> are dependent on that one then it's always loaded first and the list is
> available. Then it persists as long as the main module is loaded and can
> be freed with the normal devm stuff.

That may not work, right ? For the devm stuff to work, you need a 
device. Moving this to the coresight main module, doesn't give us
*a device* where all these lists can be allocated from. Also, we
need a list per device type (e.g., tmc-etf<>, tmc-etb<>, tmc-etr<>
for tmc etc.). So then the individual drivers need to then refer
to the particular (exported!) list for allocations.

> 
> That would avoid the awkward combo of the static variables in each
> module plus the non devm kalloced list.

I think it is not too bad to add a cleanup call to the callers, who use
a devlist.

Suzuki




> 
>>
>>>
>>> Suzuki
>>>
>>