Re: [PATCH] perf: xgene: initialize lock before requesting IRQ

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:

Pre-existing issues:
- [High] Concurrent traversal and lockless modification of PMU device lists during initialization.
- [High] If ACPI or FDT PMU device discovery fails, successfully registered perf PMUs are not unregistered, leading to a Use-After-Free.
- [High] Early interrupt delivery before PMU list population causes an unrecoverable interrupt storm.
- [High] Registering a static CPU hotplug state globally without proper teardown causes subsequent probes and driver rebinds to fail permanently.
--

commit c30a076393fbb32657447ba93f8e3397354b3ffa
Author: Runyu Xiao <runyu.xiao@seu.edu.cn>

perf: xgene: initialize lock before requesting IRQ

This commit fixes an issue in xgene_pmu_probe() where the PMU IRQ is requested
before the xgene_pmu->lock is initialized. By moving the raw_spin_lock_init()
call before devm_request_irq(), it ensures that xgene_pmu_isr() will always see
a valid lock if it runs immediately.

> diff --git a/drivers/perf/xgene_pmu.c b/drivers/perf/xgene_pmu.c
> index ab3623fab914c..0aa2b605fd53d 100644
> --- a/drivers/perf/xgene_pmu.c
> +++ b/drivers/perf/xgene_pmu.c
> @@ -1873,6 +1873,8 @@ static int xgene_pmu_probe(struct platform_device *pdev)
>  	if (irq < 0)
>  		return -EINVAL;
>  
> +	raw_spin_lock_init(&xgene_pmu->lock);
> +
>  	rc = devm_request_irq(&pdev->dev, irq, xgene_pmu_isr,
>  				IRQF_NOBALANCING | IRQF_NO_THREAD,
>  				dev_name(&pdev->dev), xgene_pmu);

[Severity: High]
This is a pre-existing issue, but is there a data race when modifying PMU device
lists during initialization?

The IRQ is requested and enabled here, and the CPU hotplug instance is
registered shortly after, before the PMU lists are fully initialized:

drivers/perf/xgene_pmu.c:fdt_pmu_probe_pmu_dev() {
    ...
    list_add(&ctx->next, &xgene_pmu->l3cpmus);
    ...
}

If an interrupt fires immediately or a CPU offline event occurs, xgene_pmu_isr()
or xgene_pmu_offline_cpu() could traverse the lists concurrently without locks:

drivers/perf/xgene_pmu.c:xgene_pmu_isr() {
    ...
    list_for_each_entry(ctx, &xgene_pmu->mcpmus, next) {
    ...
}

Can this lead to list pointer corruption and a kernel panic?

[Severity: High]
This is a pre-existing issue, but can early interrupt delivery before PMU list
population cause an unrecoverable interrupt storm?

The IRQ is requested and enabled at the interrupt controller level here, before
child PMU devices are added to internal lists. Additionally, the PMU is exposed
to userspace, allowing events to be scheduled before list population:

drivers/perf/xgene_pmu.c:xgene_init_perf() {
    ...
    return perf_pmu_register(&pmu_dev->pmu, name, -1);
    ...
}

Hardware interrupts are cleared in _xgene_pmu_isr(), but this is unreachable if
the internal lists are empty:

drivers/perf/xgene_pmu.c:_xgene_pmu_isr() {
    ...
    writel(pmovsr, csr + PMU_PMOVSCLR);
    ...
}

If an interrupt is pending or triggered early by userspace, xgene_pmu_isr() will
iterate over empty lists, fail to clear the hardware interrupt source, and
unconditionally return IRQ_HANDLED:

drivers/perf/xgene_pmu.c:xgene_pmu_isr() {
    ...
    return IRQ_HANDLED;
}

Does this bypass the core kernel spurious IRQ protection and cause a hard lockup?

>  	if (rc)
>  		return rc;
>  
>  	xgene_pmu->irq = irq;
>  
> -	raw_spin_lock_init(&xgene_pmu->lock);
> -
>  	/* Check for active MCBs and MCUs */
>  	rc = xgene_pmu_probe_active_mcb_mcu_l3c(xgene_pmu, pdev);

[Severity: High]
This is a pre-existing issue, but if ACPI or FDT PMU device discovery fails, are
successfully registered perf PMUs leaked, leading to a Use-After-Free?

If acpi_pmu_probe_pmu_dev() returns an error after potentially registering
several PMUs:

drivers/perf/xgene_pmu.c:acpi_pmu_probe_pmu_dev() {
    ...
    if (ACPI_FAILURE(status)) {
        dev_err(dev, "failed to probe PMU devices\n");
        return -ENODEV;
    }
    ...
}

The error path fails to unregister the PMUs from the perf subsystem before returning:

drivers/perf/xgene_pmu.c:xgene_pmu_probe() {
    ...
out_unregister:
    cpuhp_state_remove_instance(CPUHP_AP_PERF_ARM_APM_XGENE_ONLINE,
                                &xgene_pmu->node);
    return rc;
}

Since the device contexts are devm-allocated, will they be freed upon probe failure
while leaving dangling pointers in the perf core?

[Severity: High]
This is a pre-existing issue, but does registering a static CPU hotplug state
globally without proper teardown cause subsequent probes and driver rebinds to
fail permanently?

xgene_pmu_probe() registers the static CPU hotplug state earlier during probe:

drivers/perf/xgene_pmu.c:xgene_pmu_probe() {
    ...
    rc = cpuhp_setup_state_multi(CPUHP_AP_PERF_ARM_APM_XGENE_ONLINE,
                                 "CPUHP_AP_PERF_ARM_APM_XGENE_ONLINE",
    ...
}

However, xgene_pmu_remove() only calls cpuhp_state_remove_instance(), and does not
call cpuhp_remove_multi_state():

drivers/perf/xgene_pmu.c:xgene_pmu_remove() {
    ...
    cpuhp_state_remove_instance(CPUHP_AP_PERF_ARM_APM_XGENE_ONLINE,
                                &xgene_pmu->node);
    ...
}

If the driver is unbound and rebound, will a subsequent probe attempt try to
register the same static state and fail with -EBUSY permanently?

>  	if (rc) {

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260902072241.3489222-1-runyu.xiao@seu.edu.cn?part=1