In etm_setup_aux(), when a user sink is obtained via coresight_get_sink_by_id(), it increments the reference count of the sink device. However, if the sink is used in path building, the path holds a reference, but the initial reference from coresight_get_sink_by_id() is not released, causing a reference count leak. We should release the initial reference after the path is built.
Found by code review.
Cc: stable@vger.kernel.org Fixes: 0e6c20517596 ("coresight: etm-perf: Allow an event to use different sinks") Signed-off-by: Ma Ke make24@iscas.ac.cn --- Changes in v2: - modified the patch as suggestions. --- drivers/hwtracing/coresight/coresight-etm-perf.c | 5 +++++ 1 file changed, 5 insertions(+)
diff --git a/drivers/hwtracing/coresight/coresight-etm-perf.c b/drivers/hwtracing/coresight/coresight-etm-perf.c index 17afa0f4cdee..56d012ab6d3a 100644 --- a/drivers/hwtracing/coresight/coresight-etm-perf.c +++ b/drivers/hwtracing/coresight/coresight-etm-perf.c @@ -454,6 +454,11 @@ static void *etm_setup_aux(struct perf_event *event, void **pages, goto err;
out: + if (user_sink) { + put_device(&user_sink->dev); + user_sink = NULL; + } + return event_data;
err:
On 15/12/2025 04:27, Ma Ke wrote:
In etm_setup_aux(), when a user sink is obtained via coresight_get_sink_by_id(), it increments the reference count of the sink device. However, if the sink is used in path building, the path holds a reference, but the initial reference from coresight_get_sink_by_id() is not released, causing a reference count leak. We should release the initial reference after the path is built.
Found by code review.
Cc: stable@vger.kernel.org Fixes: 0e6c20517596 ("coresight: etm-perf: Allow an event to use different sinks") Signed-off-by: Ma Ke make24@iscas.ac.cn
Changes in v2:
- modified the patch as suggestions.
I think Leo's comment on the previous v2 is still unaddressed. But releasing it in coresight_get_sink_by_id() would make it consistent with coresight_find_csdev_by_fwnode() and prevent further mistakes.
It also leads me to see that users of coresight_find_device_by_fwnode() should also release it, but only one out of two appears to.
James
drivers/hwtracing/coresight/coresight-etm-perf.c | 5 +++++ 1 file changed, 5 insertions(+)
diff --git a/drivers/hwtracing/coresight/coresight-etm-perf.c b/drivers/hwtracing/coresight/coresight-etm-perf.c index 17afa0f4cdee..56d012ab6d3a 100644 --- a/drivers/hwtracing/coresight/coresight-etm-perf.c +++ b/drivers/hwtracing/coresight/coresight-etm-perf.c @@ -454,6 +454,11 @@ static void *etm_setup_aux(struct perf_event *event, void **pages, goto err; out:
- if (user_sink) {
put_device(&user_sink->dev);user_sink = NULL;- }
- return event_data;
err:
On Mon, Dec 15, 2025 at 11:02:08AM +0200, James Clark wrote:
On 15/12/2025 04:27, Ma Ke wrote:
In etm_setup_aux(), when a user sink is obtained via coresight_get_sink_by_id(), it increments the reference count of the sink device. However, if the sink is used in path building, the path holds a reference, but the initial reference from coresight_get_sink_by_id() is not released, causing a reference count leak. We should release the initial reference after the path is built.
Found by code review.
Cc: stable@vger.kernel.org Fixes: 0e6c20517596 ("coresight: etm-perf: Allow an event to use different sinks") Signed-off-by: Ma Ke make24@iscas.ac.cn
Changes in v2:
- modified the patch as suggestions.
I think Leo's comment on the previous v2 is still unaddressed. But releasing it in coresight_get_sink_by_id() would make it consistent with coresight_find_csdev_by_fwnode() and prevent further mistakes.
The point is the coresight core layer uses coresight_grab_device() to increase the device's refcnt. This is why we don't need to grab a device when setup AUX.
It also leads me to see that users of coresight_find_device_by_fwnode() should also release it, but only one out of two appears to.
Good finding!
Thanks, Leo
On 12/15/2025 5:51 PM, Leo Yan wrote:
On Mon, Dec 15, 2025 at 11:02:08AM +0200, James Clark wrote:
On 15/12/2025 04:27, Ma Ke wrote:
In etm_setup_aux(), when a user sink is obtained via coresight_get_sink_by_id(), it increments the reference count of the sink device. However, if the sink is used in path building, the path holds a reference, but the initial reference from coresight_get_sink_by_id() is not released, causing a reference count leak. We should release the initial reference after the path is built.
Found by code review.
Cc: stable@vger.kernel.org Fixes: 0e6c20517596 ("coresight: etm-perf: Allow an event to use different sinks") Signed-off-by: Ma Ke make24@iscas.ac.cn
Changes in v2:
- modified the patch as suggestions.
I think Leo's comment on the previous v2 is still unaddressed. But releasing it in coresight_get_sink_by_id() would make it consistent with coresight_find_csdev_by_fwnode() and prevent further mistakes.
The point is the coresight core layer uses coresight_grab_device() to increase the device's refcnt. This is why we don't need to grab a device when setup AUX.
That make sense. We dont need to hold the refcnt for a while and it should be released immediately after locating the required device.
Thanks, Jie
It also leads me to see that users of coresight_find_device_by_fwnode() should also release it, but only one out of two appears to.
Good finding!
Thanks, Leo
On 12/15/2025 10:09 AM, Jie Gan wrote:
On 12/15/2025 5:51 PM, Leo Yan wrote:
On Mon, Dec 15, 2025 at 11:02:08AM +0200, James Clark wrote:
On 15/12/2025 04:27, Ma Ke wrote:
In etm_setup_aux(), when a user sink is obtained via coresight_get_sink_by_id(), it increments the reference count of the sink device. However, if the sink is used in path building, the path holds a reference, but the initial reference from coresight_get_sink_by_id() is not released, causing a reference count leak. We should release the initial reference after the path is built.
Found by code review.
Cc: stable@vger.kernel.org Fixes: 0e6c20517596 ("coresight: etm-perf: Allow an event to use different sinks") Signed-off-by: Ma Ke make24@iscas.ac.cn
Changes in v2:
- modified the patch as suggestions.
I think Leo's comment on the previous v2 is still unaddressed. But releasing it in coresight_get_sink_by_id() would make it consistent with coresight_find_csdev_by_fwnode() and prevent further mistakes.
The point is the coresight core layer uses coresight_grab_device() to increase the device's refcnt. This is why we don't need to grab a device when setup AUX.
That make sense. We dont need to hold the refcnt for a while and it should be released immediately after locating the required device.
Thanks, Jie
It also leads me to see that users of coresight_find_device_by_fwnode() should also release it, but only one out of two appears to.
Good finding!
Thanks, Leo
Hi all,
Thank you for the insightful discussion. I've carefully read the feedback from Leo, James, and Jie, and now have a clear understanding of the reference count management.
The core issue: coresight_get_sink_by_id() internally calls bus_find_device(), which increases reference count via get_device().
From the discussion, I note two possible fix directions:
1. Release the initial reference in etm_setup_aux() (current v2 patch) 2. Modify the behavior of coresight_get_sink_by_id() itself so it doesn't increase the reference count.
Leo mentioned referencing how acpi_dev_present() does it, and James also pointed out that APIs should be consistent. I think it makes sense that following the principle like "lookup doesn't hold a reference" could prevent similar leaks in the future.
To ensure the correctness of the v3 patch, I'd like to confirm which patch is preferred. If option 2 is the consensus, I'm happy to modify the implementation of coresight_get_sink_by_id() as suggested.
Looking forward to your further guidance.
Thanks!
Hi,
On Fri, Dec 19, 2025 at 10:39:49AM +0800, Ma Ke wrote:
[...]
From the discussion, I note two possible fix directions:
- Release the initial reference in etm_setup_aux() (current v2 patch)
- Modify the behavior of coresight_get_sink_by_id() itself so it
doesn't increase the reference count.
The option 2 is the right way to go.
To ensure the correctness of the v3 patch, I'd like to confirm which patch is preferred. If option 2 is the consensus, I'm happy to modify the implementation of coresight_get_sink_by_id() as suggested.
It is good to use a separate patch to fix coresight_find_device_by_fwnode() mentioned by James:
diff --git a/drivers/hwtracing/coresight/coresight-platform.c b/drivers/hwtracing/coresight/coresight-platform.c index 0db64c5f4995..2b34f818ba88 100644 --- a/drivers/hwtracing/coresight/coresight-platform.c +++ b/drivers/hwtracing/coresight/coresight-platform.c @@ -107,14 +107,16 @@ coresight_find_device_by_fwnode(struct fwnode_handle *fwnode) * platform bus. */ dev = bus_find_device_by_fwnode(&platform_bus_type, fwnode); - if (dev) - return dev;
/* * We have a configurable component - circle through the AMBA bus * looking for the device that matches the endpoint node. */ - return bus_find_device_by_fwnode(&amba_bustype, fwnode); + if (!dev) + dev = bus_find_device_by_fwnode(&amba_bustype, fwnode); + + put_device(dev); + return dev; }
/* @@ -274,7 +276,6 @@ static int of_coresight_parse_endpoint(struct device *dev,
of_node_put(rparent); of_node_put(rep); - put_device(rdev);
return ret; }
Thanks for working on this.
On 19/12/2025 09:41, Leo Yan wrote:
Hi,
On Fri, Dec 19, 2025 at 10:39:49AM +0800, Ma Ke wrote:
[...]
From the discussion, I note two possible fix directions:
- Release the initial reference in etm_setup_aux() (current v2 patch)
- Modify the behavior of coresight_get_sink_by_id() itself so it
doesn't increase the reference count.
The option 2 is the right way to go.
To ensure the correctness of the v3 patch, I'd like to confirm which patch is preferred. If option 2 is the consensus, I'm happy to modify the implementation of coresight_get_sink_by_id() as suggested.
It is good to use a separate patch to fix coresight_find_device_by_fwnode() mentioned by James:
diff --git a/drivers/hwtracing/coresight/coresight-platform.c b/drivers/hwtracing/coresight/coresight-platform.c index 0db64c5f4995..2b34f818ba88 100644 --- a/drivers/hwtracing/coresight/coresight-platform.c +++ b/drivers/hwtracing/coresight/coresight-platform.c @@ -107,14 +107,16 @@ coresight_find_device_by_fwnode(struct fwnode_handle *fwnode) * platform bus. */ dev = bus_find_device_by_fwnode(&platform_bus_type, fwnode);
- if (dev)
return dev;/* * We have a configurable component - circle through the AMBA bus * looking for the device that matches the endpoint node. */
- return bus_find_device_by_fwnode(&amba_bustype, fwnode);
- if (!dev)
dev = bus_find_device_by_fwnode(&amba_bustype, fwnode);- put_device(dev);
^^ NAK, see below.
- return dev; }
/* @@ -274,7 +276,6 @@ static int of_coresight_parse_endpoint(struct device *dev, of_node_put(rparent); of_node_put(rep);
- put_device(rdev);
This doesn't look good. We can't use the "dev" reliably without the reference count. We are opening up use-after-free.
NAK for this.
Suzuki
return ret; }
Thanks for working on this.
On Fri, Dec 19, 2025 at 09:59:54AM +0000, Suzuki K Poulose wrote:
[...]
diff --git a/drivers/hwtracing/coresight/coresight-platform.c b/drivers/hwtracing/coresight/coresight-platform.c index 0db64c5f4995..2b34f818ba88 100644 --- a/drivers/hwtracing/coresight/coresight-platform.c +++ b/drivers/hwtracing/coresight/coresight-platform.c @@ -107,14 +107,16 @@ coresight_find_device_by_fwnode(struct fwnode_handle *fwnode) * platform bus. */ dev = bus_find_device_by_fwnode(&platform_bus_type, fwnode);
- if (dev)
return dev;*/
- We have a configurable component - circle through the AMBA bus
- looking for the device that matches the endpoint node.
- return bus_find_device_by_fwnode(&amba_bustype, fwnode);
- if (!dev)
dev = bus_find_device_by_fwnode(&amba_bustype, fwnode);- put_device(dev);
^^ NAK, see below.
- return dev; } /*
@@ -274,7 +276,6 @@ static int of_coresight_parse_endpoint(struct device *dev, of_node_put(rparent); of_node_put(rep);
- put_device(rdev);
This doesn't look good. We can't use the "dev" reliably without the reference count. We are opening up use-after-free.
My understanding is we don't grab a device from coresight_find_device_by_fwnode(). The callers only check whether the device is present on the bus; if it isn't, the driver defers probe.
This is similiar to coresight_find_csdev_by_fwnode(), which calls put_device(dev) to release refcnt immediately. This is why I suggested the change, so the two functions behave consistently.
Thanks, Leo
On 19/12/2025 11:38, Leo Yan wrote:
On Fri, Dec 19, 2025 at 09:59:54AM +0000, Suzuki K Poulose wrote:
[...]
diff --git a/drivers/hwtracing/coresight/coresight-platform.c b/drivers/hwtracing/coresight/coresight-platform.c index 0db64c5f4995..2b34f818ba88 100644 --- a/drivers/hwtracing/coresight/coresight-platform.c +++ b/drivers/hwtracing/coresight/coresight-platform.c @@ -107,14 +107,16 @@ coresight_find_device_by_fwnode(struct fwnode_handle *fwnode) * platform bus. */ dev = bus_find_device_by_fwnode(&platform_bus_type, fwnode);
- if (dev)
return dev;*/
- We have a configurable component - circle through the AMBA bus
- looking for the device that matches the endpoint node.
- return bus_find_device_by_fwnode(&amba_bustype, fwnode);
- if (!dev)
dev = bus_find_device_by_fwnode(&amba_bustype, fwnode);- put_device(dev);
^^ NAK, see below.
- return dev; } /*
@@ -274,7 +276,6 @@ static int of_coresight_parse_endpoint(struct device *dev, of_node_put(rparent); of_node_put(rep);
- put_device(rdev);
This doesn't look good. We can't use the "dev" reliably without the reference count. We are opening up use-after-free.
My understanding is we don't grab a device from coresight_find_device_by_fwnode(). The callers only check whether the device is present on the bus; if it isn't, the driver defers probe.
This is similiar to coresight_find_csdev_by_fwnode(), which calls put_device(dev) to release refcnt immediately. This is why I suggested the change, so the two functions behave consistently.
I see, sorry. I saw some other uses of the device, but clearly I was wrong. May be we should simply re-structure the function to :
bool coresight_fwnode_device_present(fwnode) {
// find and drop the ref if required. return true/false; }
The name "find_device_by_fwnode" and returning a freed reference doesn't look good to me.
Suzuki
Thanks, Leo
On Fri, Dec 19, 2025 at 11:48:35AM +0000, Suzuki K Poulose wrote:
[...]
My understanding is we don't grab a device from coresight_find_device_by_fwnode(). The callers only check whether the device is present on the bus; if it isn't, the driver defers probe.
This is similiar to coresight_find_csdev_by_fwnode(), which calls put_device(dev) to release refcnt immediately. This is why I suggested the change, so the two functions behave consistently.
I see, sorry. I saw some other uses of the device, but clearly I was wrong. May be we should simply re-structure the function to :
No worries and thanks for confirmation.
bool coresight_fwnode_device_present(fwnode) {
// find and drop the ref if required. return true/false; }
The name "find_device_by_fwnode" and returning a freed reference doesn't look good to me.
Renaming is good. Maybe use a separate patch to rename:
coresight_find_csdev_by_fwnode() -> coresight_fwnode_csdev_present()
Thanks, Leo
linux-stable-mirror@lists.linaro.org
-
James Clark -
Jie Gan -
Leo Yan -
Ma Ke -
Suzuki K Poulose