Re: [PATCH] coresight: fix resource leaks on path build failure

[James Clark <james.clark@linaro.org>]

On 20/05/2026 11:13 am, Jie Gan wrote:
> 
> 
> On 5/20/2026 5:27 PM, Mike Leach wrote:
>>
>>
>>> -----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:
>>>>>>
>>>>>> 1. 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.
>>>>>>
>>>>>> 2. 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.
>>>
> 
> The point here is there are multiple routes from the same source device 
> to the same sink device, am right?
> 
> I have no experience on this scenario. So with the scenario, the 
> build_path may succeeded in one route and failed in another route, but 
> finally, the _coresight_build_path still returns success, is that correct?
> 
>>>>>
>>>>> 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.
>>
> 
> As Mike mentioned, my understanding is that a source device is only 
> allowed to be added to one valid path—this is what I mean by “unique.”
> 
> Thanks,
> Jie
> 

That's ok then we can ignore this for this patch. But it would be good 
to enforce that in _coresight_build_path() with some kind of assert. Or 
at least add a comment to appease the AI reviewers.

>> 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>
>>>>>
>>>>
>>
>