On 20/05/2026 2:55 am, Jie Gan wrote:
On 5/19/2026 9:57 PM, James Clark wrote:
On 13/05/2026 2:32 am, Jie Gan wrote:
Two related leaks when _coresight_build_path() encounters an error after coresight_grab_device() has already incremented the pm_runtime, module, and device references for a node:
- In _coresight_build_path(), if kzalloc_obj() for the path node fails
after coresight_grab_device() succeeds, coresight_drop_device() was never called, permanently leaking all three references.
- In coresight_build_path(), on failure the partial path was freed with
kfree(path) instead of coresight_release_path(path). kfree() only frees the coresight_path struct itself; it does not iterate path_list to call coresight_drop_device() and kfree() for each coresight_node already added by deeper recursive calls, leaking both the pm_runtime, module, and device references and the node memory for every element on the partial path.
Fix both by adding coresight_drop_device() in the OOM unwind of _coresight_build_path(), and replacing kfree(path) with coresight_release_path(path) in coresight_build_path().
Fixes: 32b0707a4182 ("coresight: Add try_get_module() in coresight_grab_device()") Fixes: b3e94405941e ("coresight: associating path with session rather than tracer") Signed-off-by: Jie Gan jie.gan@oss.qualcomm.com
drivers/hwtracing/coresight/coresight-core.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/drivers/hwtracing/coresight/coresight-core.c b/drivers/ hwtracing/coresight/coresight-core.c index 46f247f73cf6..c1354ea8e11d 100644 --- a/drivers/hwtracing/coresight/coresight-core.c +++ b/drivers/hwtracing/coresight/coresight-core.c @@ -825,8 +825,10 @@ static int _coresight_build_path(struct coresight_device *csdev, return ret; node = kzalloc_obj(struct coresight_node); - if (!node) + if (!node) { + coresight_drop_device(csdev); return -ENOMEM; + } node->csdev = csdev; list_add(&node->link, &path->path_list); @@ -851,7 +853,7 @@ struct coresight_path *coresight_build_path(struct coresight_device *source, rc = _coresight_build_path(source, source, sink, path); if (rc) { - kfree(path); + coresight_release_path(path); return ERR_PTR(rc); }
base-commit: e98d21c170b01ddef366f023bbfcf6b31509fa83 change-id: 20260513-fix-memory-leak-issue-034b4a45265e
Best regards,
Looks good to me, but sashiko is complaining: https://sashiko.dev/#/ patchset/20260513-fix-memory-leak-issue- v1-1-49822d7bc7d4%40oss.qualcomm.com
I'm trying to understand why it's saying that, but I think the scenario is that if there are multiple correct paths to a sink, when one path partially fails and a second path succeeds you could get a path_list with some garbage entries in it.
I think the coresight_release_path is added to address this situation. We suffered the path partially failure, and we need release all nodes already added to the path.
It wouldn't call coresight_release_path() in this case though. If one path ends up building to success but a parallel path partially failed then _coresight_build_path() still returns success. During the search it would have still added the nodes from the partially failed path to the path_list. This is only an issue if there are multiple correct paths.
That's kind of a different and existing issue to the one you've fixed, and assumes that multiple paths to one sink are possible, which I'm not sure is supported?
Each path is unique. We only deal with the issue path for balancing the reference count.
Thanks, Jie
I'm not exactly sure what you mean by unique, but the same source and sink could potentially be connected through two different sets of links.
It might be as easy as breaking the loop early for any return value other than -ENODEV, but I'll leave it to you to decide whether to do that here or not.
Reviewed-by: James Clark james.clark@linaro.org
-----Original Message----- From: James Clark james.clark@linaro.org Sent: Wednesday, May 20, 2026 9:38 AM To: Jie Gan jie.gan@oss.qualcomm.com Cc: coresight@lists.linaro.org; linux-arm-kernel@lists.infradead.org; linux- kernel@vger.kernel.org; Suzuki Poulose Suzuki.Poulose@arm.com; Mike Leach Mike.Leach@arm.com; Leo Yan Leo.Yan@arm.com; Alexander Shishkin alexander.shishkin@linux.intel.com; Mathieu Poirier mathieu.poirier@linaro.org; Tingwei Zhang tingwei.zhang@oss.qualcomm.com; Greg Kroah-Hartman gregkh@linuxfoundation.org Subject: Re: [PATCH] coresight: fix resource leaks on path build failure
On 20/05/2026 2:55 am, Jie Gan wrote:
On 5/19/2026 9:57 PM, James Clark wrote:
On 13/05/2026 2:32 am, Jie Gan wrote:
Two related leaks when _coresight_build_path() encounters an error after coresight_grab_device() has already incremented the pm_runtime,
module,
and device references for a node:
- In _coresight_build_path(), if kzalloc_obj() for the path node fails
after coresight_grab_device() succeeds, coresight_drop_device() was never called, permanently leaking all three references.
- In coresight_build_path(), on failure the partial path was freed with
kfree(path) instead of coresight_release_path(path). kfree() only frees the coresight_path struct itself; it does not iterate path_list to call coresight_drop_device() and kfree() for each coresight_node already added by deeper recursive calls, leaking both the pm_runtime, module, and device references and the node memory for every element on the partial path.
Fix both by adding coresight_drop_device() in the OOM unwind of _coresight_build_path(), and replacing kfree(path) with coresight_release_path(path) in coresight_build_path().
Fixes: 32b0707a4182 ("coresight: Add try_get_module() in coresight_grab_device()") Fixes: b3e94405941e ("coresight: associating path with session rather than tracer") Signed-off-by: Jie Gan jie.gan@oss.qualcomm.com
drivers/hwtracing/coresight/coresight-core.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/drivers/hwtracing/coresight/coresight-core.c b/drivers/ hwtracing/coresight/coresight-core.c index 46f247f73cf6..c1354ea8e11d 100644 --- a/drivers/hwtracing/coresight/coresight-core.c +++ b/drivers/hwtracing/coresight/coresight-core.c @@ -825,8 +825,10 @@ static int _coresight_build_path(struct coresight_device *csdev, return ret; node = kzalloc_obj(struct coresight_node); - if (!node) + if (!node) { + coresight_drop_device(csdev); return -ENOMEM; + } node->csdev = csdev; list_add(&node->link, &path->path_list); @@ -851,7 +853,7 @@ struct coresight_path *coresight_build_path(struct coresight_device *source, rc = _coresight_build_path(source, source, sink, path); if (rc) { - kfree(path); + coresight_release_path(path); return ERR_PTR(rc); }
base-commit: e98d21c170b01ddef366f023bbfcf6b31509fa83 change-id: 20260513-fix-memory-leak-issue-034b4a45265e
Best regards,
Looks good to me, but sashiko is complaining: https://sashiko.dev/#/ patchset/20260513-fix-memory-leak-issue- v1-1-49822d7bc7d4%40oss.qualcomm.com
I'm trying to understand why it's saying that, but I think the scenario is that if there are multiple correct paths to a sink, when one path partially fails and a second path succeeds you could get a path_list with some garbage entries in it.
I think the coresight_release_path is added to address this situation. We suffered the path partially failure, and we need release all nodes already added to the path.
It wouldn't call coresight_release_path() in this case though. If one path ends up building to success but a parallel path partially failed then _coresight_build_path() still returns success. During the search it would have still added the nodes from the partially failed path to the path_list. This is only an issue if there are multiple correct paths.
That's kind of a different and existing issue to the one you've fixed, and assumes that multiple paths to one sink are possible, which I'm not sure is supported?
Each path is unique. We only deal with the issue path for balancing the reference count.
Thanks, Jie
I'm not exactly sure what you mean by unique, but the same source and sink could potentially be connected through two different sets of links.
Multiple paths between a source and sink are not permitted under the CoreSight spec.
If such a system was to be built - then a fix would need to be in the declaration of connections - e.g. miss one path out in the device tree for example. Not up to the Coresight drivers to handle out of specification hardware.
Mike
It might be as easy as breaking the loop early for any return value other than -ENODEV, but I'll leave it to you to decide whether to do that here or not.
Reviewed-by: James Clark james.clark@linaro.org
-
James Clark -
Mike Leach