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

[Jie Gan <jie.gan@oss.qualcomm.com>]

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

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