Re: [PATCH v5 02/14] hwtracing: gtrace: Initial implementation of gtrace framework

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 9 potential issue(s) to consider:
- [High] Use-After-Free due to missing cleanup of upstream connections upon component unregistration.
- [High] Topology validation failure and missing `dest_comp` assignment lead to registration rejection and NULL pointer dereferences.
- [High] Exported lookup APIs return unreferenced device pointers, causing Use-After-Free risks.
- [High] Unbalanced refcounts and memory leak when multiple components bind to the same CPU.
- [High] Strict probe order dependency silently breaks topology building.
- [High] Infinite recursion during graph traversal leads to kernel stack overflow.
- [High] ID matching logic ignores version comparisons and terminates prematurely.
- [High] Unconditional NULL pointer dereference if driver lacks `id_table`.
- [Medium] Deadlock risk by holding subsystem mutex during device_register.
--

--- Patch [2]: [PATCH v5 02/14] hwtracing: gtrace: Initial implementation of gtrace framework ---
commit 9dea7b9c99e06842582e974d2b7070319605798a
Author: Anup Patel <anup.patel@oss.qualcomm.com>

hwtracing: gtrace: Initial implementation of gtrace framework

This patch introduces a generic hardware trace framework (gtrace). It provides
a bus and topology management helpers for trace components to interact across
different architectures.

> --- /dev/null
> +++ b/drivers/hwtracing/gtrace/gtrace-core.c

[ ... ]

> +const struct gtrace_component_id *gtrace_match_id(struct gtrace_component *comp,
> +                                               const struct gtrace_component_id *ids)
> +{
> +     const struct gtrace_component_id *id;
> +
> +     for (id = ids; id->version; id++) {
> +             if (comp->id.type != id->type)
> +                     continue;
> +
> +             return id;
> +     }
> +
> +     return NULL;
> +}

[Severity: High]
Does this loop use the version field as a termination sentinel instead of
evaluating it? If a driver defines a component with version 0, it appears
the loop will terminate prematurely. 

Also, it looks like a version comparison against comp->id.version is missing 
entirely, which might cause it to match any version of a given type.

[ ... ]

> +static int gtrace_match_device(struct device *dev, const struct device_driver *drv)
> +{
> +     const struct gtrace_driver *gtdrv = to_gtrace_driver(drv);
> +     struct gtrace_component *comp = to_gtrace_component(dev);
> +
> +     return gtrace_match_id(comp, gtdrv->id_table) ? 1 : 0;
> +}

[Severity: High]
If a driver is registered on this bus without an id_table (for example, matching
solely by ACPI or OF tables), will passing a NULL id_table here lead to a
NULL pointer dereference when gtrace_match_id() evaluates its loop condition?

[ ... ]

> +struct gtrace_component *gtrace_find_by_fwnode(struct fwnode_handle *fwnode)
> +{
> +     struct gtrace_fwnode_match_data d = { .fwnode = fwnode, .match = NULL };
> +     int ret;
> +
> +     ret = bus_for_each_dev(&gtrace_bustype, NULL, &d, gtrace_match_fwnode);
> +     if (ret < 0)
> +             return ERR_PTR(ret);
> +
> +     return d.match;
> +}

[Severity: High]
Since bus_for_each_dev() drops its temporary reference upon returning, does
returning d.match directly leave the caller with an unreferenced raw pointer?
Could this lead to a use-after-free if the component is unregistered
concurrently?

[ ... ]

> +struct gtrace_component *gtrace_cpu_source(unsigned int cpu)
> +{
> +     if (!cpu_present(cpu))
> +             return NULL;
> +
> +     return per_cpu(gtrace_cpu_source_comp, cpu);
> +}

[Severity: High]
Similar to the fwnode lookup above, should this function return a referenced
device pointer? Exposing the unreferenced component might cause a
use-after-free if the component is suddenly unregistered by another thread.

[ ... ]

> +static int gtrace_setup_inconn(struct device *dev, void *data)
> +{
> +     struct gtrace_component *comp = to_gtrace_component(dev);
> +     struct gtrace_platform_data *pdata = comp->pdata;
> +     struct gtrace_connection *conn = data;
> +     int i;
> +
> +     if (device_match_fwnode(&comp->dev, conn->dest_fwnode)) {
> +             for (i = 0; i < pdata->nr_inconns; i++) {
> +                     if (pdata->inconns[i])
> +                             continue;
> +                     pdata->inconns[i] = conn;
> +                     return 1;
> +             }
> +     }

[Severity: High]
While this properly links the connection into the destination's inconns array,
should it also assign conn->dest_comp = comp? If conn->dest_comp remains
unassigned (NULL), won't later graph traversals encounter a NULL pointer
dereference?

[ ... ]

> +static int gtrace_setup_inconns_from_outconns(struct gtrace_component *comp)
> +{
> +     struct gtrace_platform_data *pdata = comp->pdata;
> +     struct gtrace_connection *conn;
> +     int i, ret;
> +
> +     lockdep_assert_held(&gtrace_mutex);
> +
> +     for (i = 0; i < pdata->nr_outconns; i++) {
> +             conn = pdata->outconns[i];
> +             ret = bus_for_each_dev(&gtrace_bustype, NULL, conn, gtrace_setup_inconn);
> +             if (ret < 0) {
> +                     gtrace_cleanup_inconns_from_outconns(comp);
> +                     return ret;
> +             }
> +     }

[Severity: High]
If a destination component has not probed yet, bus_for_each_dev() will return 0.
Will treating 0 as a success leave this connection permanently incomplete,
since the framework does not appear to re-evaluate bidirectional matches or
request probe deferrals?

[ ... ]

> +struct gtrace_component *gtrace_register_component(struct gtrace_component_id *id,
> +                                                const char *name,
> +                                                struct gtrace_platform_data *pdata)
> +{
> +     struct gtrace_connection *conn;
> +     struct gtrace_component *comp;
> +     int i, ret = 0;
> +
> +     if (!id || id->type >= GTRACE_COMPONENT_TYPE_MAX) {
> +             ret = -EINVAL;
> +             goto err_out;
> +     }
> +
> +     if (!pdata || !pdata->dev) {
> +             ret = -EINVAL;
> +             goto err_out;
> +     }
> +
> +     for (i = 0; i < pdata->nr_inconns; i++) {
> +             if (pdata->inconns[i]) {
> +                     ret = -EINVAL;
> +                     goto err_out;
> +             }
> +     }
> +
> +     for (i = 0; i < pdata->nr_outconns; i++) {
> +             conn = pdata->outconns[i];
> +             if (!conn || conn->src_port < 0 || conn->src_comp ||
> +                 !device_match_fwnode(pdata->dev, conn->src_fwnode) ||
> +                 conn->dest_port < 0 || !conn->dest_fwnode || !conn->dest_comp) {
> +                     ret = -EINVAL;
> +                     goto err_out;
> +             }
> +     }

[Severity: High]
During component registration, won't the !conn->dest_comp check always fail
here since the dynamic discovery hasn't had a chance to run yet? This appears
to reject registration for any component that has output connections.

[ ... ]

> +     mutex_lock(&gtrace_mutex);
> +
> +     ret = device_register(&comp->dev);

[Severity: Medium]
Calling device_register() triggers a synchronous driver probe. If the driver's
probe routine calls back into an exported framework API that also attempts to
acquire gtrace_mutex, could this result in a deadlock?

[ ... ]

> +     if (comp->pdata->bound_cpu >= 0) {
> +             gtrace_get_component(comp);
> +             per_cpu(gtrace_cpu_source_comp, comp->pdata->bound_cpu) = comp;
> +     }

[Severity: High]
If multiple components bind to the same CPU, does blindly overwriting this
per-CPU pointer without releasing the previously bound component lead to a
memory leak?

[ ... ]

> +void gtrace_unregister_component(struct gtrace_component *comp)
> +{
> +     struct gtrace_component *c;
> +
> +     mutex_lock(&gtrace_mutex);
> +
> +     if (comp->pdata->bound_cpu >= 0) {
> +             c = per_cpu(gtrace_cpu_source_comp, comp->pdata->bound_cpu);
> +             per_cpu(gtrace_cpu_source_comp, comp->pdata->bound_cpu) = NULL;
> +             gtrace_put_component(c);
> +     }

[Severity: High]
If an old component's CPU slot was overwritten by a newer component (leaking
the original reference as mentioned above), could putting the current occupant
here cause us to prematurely free the newer component?

> +
> +     gtrace_cleanup_inconns_from_outconns(comp);
> +     device_unregister(&comp->dev);

[Severity: High]
This cleans up downstream outconns, but what about upstream components that
connect to this component? If upstream nodes retain dangling pointers to this
freed component, could it cause a use-after-free during graph traversal?

One additional question regarding graph traversals: when recursively walking the
graph topology across connections, it doesn't appear that visited nodes are
tracked. If hardware topology tables are misconfigured with a cycle
(e.g., A -> B -> A), could this trigger infinite recursion and overflow the
kernel stack?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260810152223.3946743-1-mayuresh.chitale@oss.qualcomm.com?part=2