Re: [PATCH v6 16/21] RISC-V: perf: Add legacy event encodings via sysfs

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] The globally scoped pmu_ctr_list array is leaked if PMU counter discovery fails, compounded by a flawed error code cast.
--

commit 7d64e724e6eff197f10e62cdd709e07f0a5c9a7d
Author: Atish Patra <atishp@rivosinc.com>

RISC-V: perf: Add legacy event encodings via sysfs

Define sysfs details for the legacy events so that any tool can
parse these to understand the minimum set of legacy events
supported by the platform. The sysfs entry will describe both event
encoding and corresponding counter map so that an perf event can be
programmed accordingly.

> diff --git a/drivers/perf/riscv_pmu_sbi.c b/drivers/perf/riscv_pmu_sbi.c
> index 4b4f151a0744d..00b84b28117a8 100644
> --- a/drivers/perf/riscv_pmu_sbi.c
> +++ b/drivers/perf/riscv_pmu_sbi.c
> @@ -130,7 +130,20 @@ static struct attribute_group riscv_cdeleg_pmu_format_group = {
>  	.attrs = riscv_cdeleg_pmu_formats_attr,
>  };

[ ... ]

[Severity: Medium]
Does this code leak the pmu_ctr_list array if PMU counter discovery fails?

In rvpmu_find_ctrs(), pmu_ctr_list is allocated via kcalloc(), but the function
bails out without freeing it if num_sbi_counters or num_deleg_counters
exceeds RISCV_MAX_COUNTERS:

drivers/perf/riscv_pmu_sbi.c:rvpmu_find_ctrs() {
...
	pmu_ctr_list = kcalloc(RISCV_MAX_COUNTERS, sizeof(*pmu_ctr_list),
			       GFP_KERNEL);
	if (!pmu_ctr_list)
		return -ENOMEM;

	if (riscv_pmu_cdeleg_available_boot())
		num_deleg_counters = rvpmu_deleg_find_ctrs();

	/* This is required for firmware counters even if the above is true */
	if (riscv_pmu_sbi_available_boot())
		num_sbi_counters = rvpmu_sbi_find_num_ctrs();

	if (num_sbi_counters > RISCV_MAX_COUNTERS || num_deleg_counters > RISCV_MAX_COUNTERS)
		return -ENOSPC;
...
}

Also, could num_sbi_counters inadvertently capture negative error codes? 

rvpmu_sbi_find_num_ctrs() returns an int which can be a negative error 
code. Storing it in the u32 num_sbi_counters implicitly casts 
it to a very large positive value. 

This triggers the RISCV_MAX_COUNTERS check and causes the function to abort 
with -ENOSPC, losing the original error.

The caller rvpmu_device_probe() then fails to clean up the leaked globally 
scoped pmu_ctr_list pointer during its error handling:

drivers/perf/riscv_pmu_sbi.c:rvpmu_device_probe() {
...
	num_counters = rvpmu_find_ctrs();
	if (num_counters < 0) {
		pr_err("SBI PMU extension doesn't provide any counters\n");
		goto out_free;
	}
...
out_free:
	kfree(pmu);
	return ret;
}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260608-counter_delegation-v6-0-285b72ed65a9@meta.com?part=16