Re: [PATCH AUTOSEL 6.18-6.12] RISC-V: perf: fix resource cleanup on driver probe failure

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] perf: riscv: NULL deref in riscv_pmu_get_hpm_info() after probe failure
- [High] riscv: perf: incomplete snapshot and IRQ cleanup on probe failure
- [Medium] perf: riscv: leak sysctl table registration on pmu_sbi_device_probe() failure
--

commit 45f193117e60b41831289f45f79fab573ecae630
Author: Atish Patra <atishp@meta.com>

RISC-V: perf: fix resource cleanup on driver probe failure

This commit reorganizes the error handling in pmu_sbi_device_probe(),
pmu_sbi_setup_irqs(), and riscv_pmu_destroy() to clean up registered
PMU instances, per-CPU IRQs, pmu_ctr_list, and CPU hotplug state on
probe failure.

Link: https://patch.msgid.link/20260807-counter_delegation-v9-1-58658104e487@meta.com

> diff --git a/drivers/perf/riscv_pmu_sbi.c b/drivers/perf/riscv_pmu_sbi.c
> index 7dd282da67cef..e794a40522fe6 100644
> --- a/drivers/perf/riscv_pmu_sbi.c
> +++ b/drivers/perf/riscv_pmu_sbi.c

[ ... ]

> @@ -1515,9 +1525,20 @@ static int pmu_sbi_device_probe(struct platform_device *pdev)
>  	return 0;
>  
>  out_unregister:
> +	perf_pmu_unregister(&pmu->pmu);

[Severity: Medium]
When cpuhp_state_add_instance() fails and jumps to out_unregister, does
this path leak the sysctl table header registered just beforehand in
pmu_sbi_device_probe()?

drivers/perf/riscv_pmu_sbi.c:pmu_sbi_device_probe() {
	...
	register_sysctl("kernel", sbi_pmu_sysctl_table);

	ret = cpuhp_state_add_instance(CPUHP_AP_PERF_RISCV_STARTING, &pmu->node);
	if (ret)
		goto out_unregister;
}

Because the ctl_table_header pointer returned by register_sysctl() is not
saved or passed to unregister_sysctl_table() in out_unregister, the sysctl
registration remains active after probe failure.

> +
> +out_destroy:
>  	riscv_pmu_destroy(pmu);
> +	if (irq_requested) {
> +		free_percpu_irq(riscv_pmu_irq, pmu->hw_events);
> +		irq_dispose_mapping(riscv_pmu_irq);
> +		riscv_pmu_irq = 0;
> +	}

[Severity: High]
If cpuhp_state_add_instance() fails on a secondary CPU during
pmu_sbi_starting_cpu(), can this cleanup leave the per-CPU IRQ enabled,
snapshot shared memory registered with SBI firmware, and the
sbi_pmu_snapshot_available static branch enabled?

First, in pmu_sbi_starting_cpu(), the per-CPU IRQ is enabled before
setting up the snapshot shared memory:

drivers/perf/riscv_pmu_sbi.c:pmu_sbi_starting_cpu() {
	...
	if (riscv_pmu_use_irq) {
		cpu_hw_evt->irq = riscv_pmu_irq;
		ALT_SBI_PMU_OVF_CLEAR_PENDING(riscv_pmu_irq_mask);
		enable_percpu_irq(riscv_pmu_irq, IRQ_TYPE_NONE);
	}

	if (sbi_pmu_snapshot_available())
		return pmu_sbi_snapshot_setup(pmu, cpu);
	return 0;
}

If pmu_sbi_snapshot_setup() fails on that CPU, pmu_sbi_starting_cpu()
returns an error with riscv_pmu_irq still enabled on the failing CPU, and
cpuhp_rollback_install() only invokes pmu_sbi_dying_cpu() on earlier CPUs.
When out_destroy then calls free_percpu_irq(riscv_pmu_irq, pmu->hw_events),
__free_percpu_irq() sees that desc->percpu_enabled is not empty, triggers a
warning, and returns without removing the irqaction before out_free calls
free_percpu(pmu->hw_events).

Second, pmu_sbi_device_probe() sets up the snapshot shared memory on the
initial probing CPU and enables the static key before calling
cpuhp_state_add_instance():

drivers/perf/riscv_pmu_sbi.c:pmu_sbi_device_probe() {
	...
		cpu = get_cpu();
		ret = pmu_sbi_snapshot_setup(pmu, cpu);
		put_cpu();

		if (ret) {
			/* Snapshot is an optional feature. Continue if not available */
			pmu_sbi_snapshot_free(pmu);
		} else {
			pr_info("SBI PMU snapshot detected\n");
			static_branch_enable(&sbi_pmu_snapshot_available);
		}
}

In the error path, perf_pmu_unregister() can sleep and migrate the task to
another CPU before riscv_pmu_destroy() calls pmu_sbi_snapshot_disable() on
only the current CPU and frees the pages via pmu_sbi_snapshot_free(pmu). If
the initial probing CPU was not rolled back by cpuhp_state_add_instance()
(or if the task migrated), its freed physical page remains registered with
SBI firmware. In addition, riscv_pmu_destroy() does not call
static_branch_disable(&sbi_pmu_snapshot_available).

>  
>  out_free:
> +	free_percpu(pmu->hw_events);
> +	kfree(pmu_ctr_list);
> +	pmu_ctr_list = NULL;
>  	kfree(pmu);
>  	return ret;
>  }

[Severity: High]
Can freeing pmu_ctr_list and setting it to NULL here without clearing the
static global cmask cause a NULL pointer dereference in
riscv_pmu_get_hpm_info()?

In pmu_sbi_device_probe(), pmu_sbi_get_ctrinfo(num_counters, &cmask)
populates the file-scope cmask bitmask alongside allocating pmu_ctr_list.
If a later step in pmu_sbi_device_probe() fails, out_free frees
pmu_ctr_list and sets it to NULL, while cmask stays non-zero.

When kvm_riscv_vcpu_pmu_init() subsequently calls riscv_pmu_get_hpm_info():

drivers/perf/riscv_pmu_sbi.c:riscv_pmu_get_hpm_info() {
	...
	if (!cmask)
		return -EINVAL;

	for_each_set_bit(i, &cmask, RISCV_MAX_COUNTERS) {
		info = &pmu_ctr_list[i];
		if (!info)
			continue;
		if (!hpm_width && info->csr != CSR_CYCLE && info->csr != CSR_INSTRET)
			hpm_width = info->width;
		if (info->type == SBI_PMU_CTR_TYPE_HW)
			hpm_count++;
	}
	...
}

Because cmask is non-zero, &pmu_ctr_list[i] for any set bit i > 0 evaluates
to a non-NULL offset from NULL, bypassing the !info check and dereferencing
info->csr and info->type. Should out_free also reset cmask = 0?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261019135441.1420968-443-sashal@kernel.org?part=1