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/?...
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(-)
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; }
/*
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);
- 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); /*
*/ if (!graph)
- There are no graph connections, which is fine for some components.
- e.g., ETE
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;
} /*
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
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 */
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 */
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 */
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
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
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
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
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
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
-
James Clark -
Junhao He -
Suzuki K Poulose