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

[James Clark]

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@xxxxxxxxxxxxxxxx>
> > > ---
> > >   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@xxxxxxxxxx>