Re: [PATCH v5 11/14] gtrace: Add perf driver for tracing using perf tool

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 10 potential issue(s) to consider:
- [High] Unchecked return value of alloc_percpu() leads to NULL pointer dereference on allocation failure.
- [High] gtrace_setup_aux() checks for !path instead of IS_ERR(path), storing error pointers that cause crashes later.
- [High] Skipped CPUs are not cleared from event_data->mask, causing validation bypass and NULL pointer dereferences in cleanup.
- [High] The virtual memory mapped via vmap() for the AUX buffer is never unmapped, permanently leaking vmalloc space.
- [High] Error paths in gtrace_event_start() and gtrace_event_stop() return directly without calling perf_aux_output_end(), leaking the active AUX buffer handle.
- [High] gtrace_event_stop() passes uninitialized size and format variables to the perf core if gtrace_path_copyto_auxbuf() fails.
- [High] gtrace_path_start and gtrace_path_stop abort on the first component error without rolling back already-modified components.
- [High] Lock ordering violation: gtrace_event_stop() acquires a spinlock_t inside the PMU stop callback, violating wait-context nesting.
- [High] Module unload race in gtrace_perf_exit() with pending asynchronous work leads to use-after-free of module text.
- [Medium] The dynamically allocated pagelist array is permanently leaked on the success path of gtrace_setup_aux().
--

--- Patch [11]: [PATCH v5 11/14] gtrace: Add perf driver for tracing using perf tool ---
commit 7444340c1bfa224d9b35f7fb63e614992d6414e2
Author: Mayuresh Chitale <mayuresh.chitale@oss.qualcomm.com>

gtrace: Add perf driver for tracing using perf tool
    
Add perf driver for the gtrace similar to ARM Coresight and Hisilicon
PTT drivers. The driver adds 'gtrace' event descriptor which can be used
by the perf tool to record the trace data. The actual data format depends
on type of encoder device and optionally the PMU specific trace format
type.

> diff --git a/drivers/hwtracing/gtrace/gtrace-perf.c b/drivers/hwtracing/gtrace/gtrace-perf.c
> new file mode 100644
> index 0000000000000..ed9155bbbdb7a
> --- /dev/null
> +++ b/drivers/hwtracing/gtrace/gtrace-perf.c
> @@ -0,0 +1,347 @@
[ ... ]
> +static void *alloc_event_data(int cpu)
> +{
> +	struct gtrace_event_data *event_data;
> +	cpumask_t *mask;
> +
> +	event_data = kzalloc_obj(*event_data);
> +	if (!event_data)
> +		return NULL;
> +
> +	/* Update mask as per selected CPUs */
> +	mask = &event_data->mask;
> +	if (cpu != -1)
> +		cpumask_set_cpu(cpu, mask);
> +	else
> +		cpumask_copy(mask, cpu_present_mask);
> +
> +	event_data->path = alloc_percpu(struct gtrace_path *);
> +	return event_data;
> +}

[Severity: High]
Can alloc_percpu() fail here? 

If it returns a NULL pointer, event_data->path will be NULL. Later in
gtrace_setup_aux(), this pointer is passed to per_cpu_ptr(), which blindly
adds the CPU offset to the NULL pointer and generates an invalid kernel
address, resulting in an oops on dereference.

[ ... ]
> +static void free_event_data(struct work_struct *work)
> +{
> +	struct gtrace_event_data *event_data;
> +	struct gtrace_path *path;
> +	cpumask_t *mask;
> +	int cpu;
> +
> +	event_data = container_of(work, struct gtrace_event_data, work);
> +	mask = &event_data->mask;
> +	for_each_cpu(cpu, mask) {
> +		path = *gtrace_event_cpu_path_ptr(event_data, cpu);
> +		gtrace_destroy_path(path);
> +	}

[Severity: High]
When gtrace_setup_aux() skips CPUs due to initialization failures (e.g., if
gtrace_cpu_source() returns NULL), it leaves those CPUs in the mask. 

Will this loop blindly pass uninitialized or NULL paths to
gtrace_destroy_path(), causing a NULL pointer dereference when accessing
path->comp_list?

> +	free_percpu(event_data->path);
> +	kfree(event_data);
> +}

[Severity: High]
Looking at the setup path in gtrace_setup_aux(), the buffer is mapped using
vmap(), but this cleanup function does not call vunmap(event_data->buf.base).

Will this permanently leak the vmalloc space each time a session is destroyed?

[ ... ]
> +static void *gtrace_setup_aux(struct perf_event *event, void **pages,
> +			       int nr_pages, bool overwrite)
> +{
[ ... ]
> +	/*
> +	 * Create the path for each CPU in the mask. In case of any failure skip the CPU
> +	 */
> +	for_each_cpu(cpu, mask) {
> +		struct gtrace_component *src;
> +		struct gtrace_path *path;
> +
> +		src = gtrace_cpu_source(cpu);
> +		if (!src)
> +			continue;
> +
> +		path = gtrace_create_path(src, NULL, GTRACE_COMPONENT_MODE_PERF);
> +		if (!path)
> +			continue;

[Severity: High]
When skipping a CPU due to these error paths, should the CPU be cleared from
event_data->mask? 

Leaving it in the mask bypasses the check for valid CPUs below and causes
the cleanup function to operate on uninitialized pointers.

[Severity: High]
Does gtrace_create_path() return NULL on failure? Looking at its
implementation, it returns an error pointer like ERR_PTR(-ENOMEM).

Using !path here evaluates to false for error pointers, storing an invalid
pointer that will later cause a kernel panic when dereferenced.

> +
> +		*gtrace_event_cpu_path_ptr(event_data, cpu) = path;
> +	}
[ ... ]
> +	pagelist = kcalloc(nr_pages, sizeof(*pagelist), GFP_KERNEL);
> +	if (!pagelist)
> +		goto err;
> +
> +	for (i = 0; i < nr_pages; i++)
> +		pagelist[i] = virt_to_page(pages[i]);
> +
> +	event_data->buf.base = vmap(pagelist, nr_pages, VM_MAP, PAGE_KERNEL);
> +	if (!event_data->buf.base) {
> +		kfree(pagelist);
> +		goto err;
> +	}
> +
> +	event_data->buf.nr_pages = nr_pages;
> +	event_data->buf.length = nr_pages * PAGE_SIZE;
> +	event_data->buf.pos = 0;
> +	return event_data;

[Severity: Medium]
Is the pagelist array leaked on this success path? 

The error path correctly frees it, but it appears vmap() only maps the pages
without taking ownership of the array itself.

[ ... ]
> +static void gtrace_event_start(struct perf_event *event, int flags)
> +{
[ ... ]
> +	event_data = perf_aux_output_begin(handle, event);
> +	if (!event_data)
> +		goto fail;
> +
> +	if (!cpumask_test_cpu(cpu, &event_data->mask))
> +		goto out;
> +
> +	event_data->buf.pos = handle->head % event_data->buf.length;
> +	path = *gtrace_event_cpu_path_ptr(event_data, cpu);
> +	if (!path) {
> +		pr_err("Error. Path not found\n");
> +		return;
> +	}

[Severity: High]
Because perf_aux_output_begin() successfully acquired a handle to the AUX
ring buffer, does this error path need to call perf_aux_output_end() before
returning? Returning directly leaves the ring buffer permanently locked.

> +
> +	if (gtrace_path_start(path)) {
> +		pr_err("Error. Tracing not started\n");
> +		return;
> +	}

[Severity: High]
Looking at gtrace_path_start(), if a component fails to start in the middle
of its initialization loop, it returns an error immediately. 

Does this lack of rollback strand previously started hardware components in an
active state?

[ ... ]
> +static void gtrace_event_stop(struct perf_event *event, int mode)
> +{
[ ... ]
> +	/* stop tracing */
> +	path = *gtrace_event_cpu_path_ptr(event_data, cpu);
> +	if (!path) {
> +		pr_err("Error. Path not found\n");
> +		return;
> +	}

[Severity: High]
If we hit this error path after an active handle is established, do we need
to call perf_aux_output_end() to properly close the session?

> +
> +	if (gtrace_path_stop(path)) {
> +		pr_err("Error. Tracing not stopped\n");
> +		return;
> +	}

[Severity: High]
Similar to gtrace_path_start(), gtrace_path_stop() aborts its loop early
if a single component fails to stop. Could this leave hardware components
running indefinitely?

> +
> +	event->hw.state = PERF_HES_STOPPED;
> +	if (handle->event && (mode & PERF_EF_UPDATE)) {
> +		if (WARN_ON_ONCE(handle->event != event))
> +			return;
> +		spin_lock(&perf_buf_lock);

[Severity: High]
Is it safe to acquire perf_buf_lock here? 

The perf events core invokes PMU stop callbacks with a raw_spinlock_t held.
Acquiring a regular spinlock_t while holding a raw lock is an invalid
wait-context nesting order and will trigger a BUG on PREEMPT_RT kernels.

> +		ret = gtrace_path_copyto_auxbuf(path, &event_data->buf, &size, &format);
> +		spin_unlock(&perf_buf_lock);
> +		WARN_ON_ONCE(ret);
> +		if (READ_ONCE(handle->event)) {
> +			/* Tag the AUX data with the format reported by the sink. */
> +			perf_aux_output_flag(handle, format);
> +			if (size > handle->size) {
> +				size = handle->size;
> +				perf_aux_output_flag(handle, PERF_AUX_FLAG_TRUNCATED);
> +			}
> +			perf_aux_output_end(handle, size);

[Severity: High]
If gtrace_path_copyto_auxbuf() returns an error, size and format might
remain uninitialized. 

Since this code emits a warning but continues execution, will it pass
stack-garbage values to perf_aux_output_flag() and perf_aux_output_end(),
corrupting the ring buffer head pointer?

[ ... ]
> +void __exit gtrace_perf_exit(void)
> +{
> +	perf_pmu_unregister(&gtrace_pmu);
> +}

[Severity: High]
The driver schedules asynchronous work items via schedule_work(&event_data->work)
to clean up event data. 

Does this exit function need to call flush_work() or flush_scheduled_work()
before returning? If the module unloads while work is pending, the workqueue
thread will execute freed memory.

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