Re: [PATCH v5 05/14] gtrace: Add functions to start/stop tracing on a component path

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Missing rollback in gtrace_path_start() leaves components in a partially started state on error.
- [High] Early abort in gtrace_path_stop() terminates teardown on the first error, leaving remaining components active.
- [High] Unsafe dereference of gtdrv without NULL check and lockless access to dev->driver can cause a kernel panic or memory corruption.
--

--- Patch [5]: [PATCH v5 05/14] gtrace: Add functions to start/stop tracing on a component path ---
commit 24fa9f699451874fed23109ef92badb57913e17b
Author: Mayuresh Chitale <mayuresh.chitale@oss.qualcomm.com>

gtrace: Add functions to start/stop tracing on a component path

The perf driver framework needs to be able to start / stop all components
in a trace component path during its operation. Add gtrace_path_start()
and gtrace_path_stop() functions for this purpose.

> diff --git a/drivers/hwtracing/gtrace/gtrace-core.c b/drivers/hwtracing/gtrace/gtrace-core.c
> index a89d195ab5b89..6ff1b30879472 100644
> --- a/drivers/hwtracing/gtrace/gtrace-core.c
> +++ b/drivers/hwtracing/gtrace/gtrace-core.c
> @@ -582,6 +582,50 @@ static void gtrace_release_path_nodes(struct gtrace_path *path)
>  	}
>  }
>  
> +int gtrace_path_start(struct gtrace_path *path)
> +{
> +	const struct gtrace_driver *gtdrv;
> +	struct gtrace_component *comp;
> +	struct gtrace_path_node *node;
> +	int ret;
> +
> +	list_for_each_entry_reverse(node, &path->comp_list, head) {
> +		comp = node->comp;
> +		gtdrv = to_gtrace_driver(comp->dev.driver);

[Severity: High]
Does this code risk a NULL pointer dereference or memory corruption?

If a driver is unbound concurrently, comp->dev.driver could become NULL.
The to_gtrace_driver() macro evaluates its argument twice locklessly:

#define to_gtrace_driver(__drv)   \
    ((__drv) ? container_of_const((__drv), struct gtrace_driver, driver) : NULL)

If comp->dev.driver is set to NULL between the two evaluations,
container_of_const(NULL) returns a bogus negative offset instead of NULL.

Even if it safely returns NULL, there is no NULL check for gtdrv before
dereferencing it on the next line.

> +		if (!gtdrv->start)
> +			continue;
> +
> +		ret = gtdrv->start(comp);
> +		if (ret)
> +			return ret;

[Severity: High]
If a component fails to start and returns an error here, do we need to
unwind and stop the components that were already successfully started
in previous loop iterations? 

Returning immediately leaves the hardware components in a partially started 
state, which could lead to hardware inconsistencies, resource leaks, or 
unexpected DMA operations.

> +	}
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(gtrace_path_start);
> +
> +int gtrace_path_stop(struct gtrace_path *path)
> +{
> +	const struct gtrace_driver *gtdrv;
> +	struct gtrace_component *comp;
> +	struct gtrace_path_node *node;
> +	int ret;
> +
> +	list_for_each_entry(node, &path->comp_list, head) {
> +		comp = node->comp;
> +		gtdrv = to_gtrace_driver(comp->dev.driver);

[Severity: High]
This shares the same risk as gtrace_path_start(). A concurrent driver unbind 
or a missing driver could result in gtdrv being NULL or an invalid pointer,
leading to a crash when gtdrv->stop is checked below.

> +		if (!gtdrv->stop)
> +			continue;
> +
> +		ret = gtdrv->stop(comp);
> +		if (ret)
> +			return ret;

[Severity: High]
If a component fails to stop, is it intentional to abort the entire teardown
sequence early?

Returning on the first error prevents the loop from stopping the remaining 
components in the path, leaving downstream components actively running and 
consuming hardware resources indefinitely. Should teardown paths use a 
best-effort approach to disable as many components as possible?

> +	}
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(gtrace_path_stop);

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