Re: [PATCH v6 14/21] RISC-V: perf: Skip PMU SBI extension when not implemented

[Atish Patra <atish.patra@linux.dev>]

On 6/8/26 11:33 PM, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
>
> Pre-existing issues:
> - [Critical] Missing perf_pmu_unregister() on error paths leads to Use-After-Free of the pmu struct.
> - [High] Unconditional cpuhp_state_remove_instance() on an uninitialized hlist_node causes a kernel panic.
> --
>
> commit bd51c5655be0902a2fc1fd90b8d692c299e7265b
> Author: Charlie Jenkins <charlie@rivosinc.com>
>
> RISC-V: perf: Skip PMU SBI extension when not implemented
>
> When the PMU SBI extension is not implemented, sbi_v2_available should
> not be set to true. The SBI implementation for counter config matching
> and firmware counter read  should also be skipped when the SBI extension
> is not implemented.
+linux-riscv
>> diff --git a/drivers/perf/riscv_pmu_sbi.c b/drivers/perf/riscv_pmu_sbi.c
>> index 1f16df9d0dd00..5bfcd3821f572 100644
>> --- a/drivers/perf/riscv_pmu_sbi.c
>> +++ b/drivers/perf/riscv_pmu_sbi.c
> [ ... ]
>> @@ -2045,12 +2052,13 @@ static int __init rvpmu_devinit(void)
>>   	int ret;
>>   	struct platform_device *pdev;
>>   
>> -	if (sbi_spec_version >= sbi_mk_version(0, 3) &&
>> -	    sbi_probe_extension(SBI_EXT_PMU))
>> -		static_branch_enable(&riscv_pmu_sbi_available);
>> +	if (sbi_probe_extension(SBI_EXT_PMU)) {
>> +		if (sbi_spec_version >= sbi_mk_version(0, 3))
>> +			static_branch_enable(&riscv_pmu_sbi_available);
>> +		if (sbi_spec_version >= sbi_mk_version(2, 0))
>> +			sbi_v2_available = true;
>> +	}
>>   
>> -	if (sbi_spec_version >= sbi_mk_version(2, 0))
>> -		sbi_v2_available = true;
>>   	/*
>>   	 * We need all three extensions to be present to access the counters
>>   	 * in S-mode via Supervisor Counter delegation.
> [Severity: Critical]
> This is a pre-existing issue, but I noticed a potential use-after-free in the
> error handling path of rvpmu_device_probe() when allocating or registering
> fails after perf_pmu_register() succeeds.
>
> If an error occurs, such as pmu_sbi_snapshot_alloc() failing, the code jumps
> to out_unregister:
>
> out_unregister:
> 	riscv_pmu_destroy(pmu);
>
> out_free:
> 	kfree(pmu);
> 	return ret;
>
> However, neither the error path nor riscv_pmu_destroy() calls
> perf_pmu_unregister(&pmu->pmu).
>
> This leaves the PMU registered in the perf core subsystem while its backing
> memory is freed via kfree(). Any subsequent access to the PMU could result
> in a use-after-free.
>
> Should perf_pmu_unregister() be called during teardown to avoid memory
> corruption?

Yes. Same comment as other patches. Fixed.


> [Severity: High]
> This is a pre-existing issue, but the error teardown path in
> rvpmu_device_probe() seems to have a bug that could trigger a kernel panic.
>
> If rvpmu_device_probe() fails before successfully calling
> cpuhp_state_add_instance(), it jumps to out_unregister and calls
> riscv_pmu_destroy(pmu).
>
> Inside riscv_pmu_destroy(), it unconditionally removes the hotplug instance:
>
> static void riscv_pmu_destroy(struct riscv_pmu *pmu)
> {
> 	...
> 	cpuhp_state_remove_instance(CPUHP_AP_PERF_RISCV_STARTING, &pmu->node);
> }
>
> Because the pmu struct is allocated with kzalloc() and the node hasn't been
> added to the hotplug list yet, its hlist_node is zero-initialized.
>
> When hlist_del() is called internally by cpuhp_state_remove_instance(), it
> attempts to dereference node->pprev (which is NULL), causing a NULL pointer
> dereference.
>
> Would it be better to check if the node is actually hashed before attempting
> to remove the hotplug instance?
Fixed in v7.