Re: [PATCH v4 3/4] clocksource/timer-econet-en751221: Disable IRQ until cevt registered

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Critical] The error path for `cpuhp_setup_state()` leaves the clockevent device registered and the per-CPU interrupt enabled, leading to a leaked action and a Use-After-Free/Data Abort.
- [High] The patch attempts to fix a NULL pointer dereference race condition but leaves the `dev->event_handler` uninitialized if the clockevent device is rejected by the core, leading to a kernel panic.
- [High] The driver registers its CPU hotplug state with a `NULL` teardown callback, leaving the per-CPU interrupt enabled when a CPU is offlined.
--

commit b63915756d157ccb863471270a1e61036ce0f8e5
Author: Caleb James DeLisle <cjd@cjdns.fr>
clocksource/timer-econet-en751221: Disable IRQ until cevt registered

This commit reorders the clockevent registration sequence to call
enable_percpu_irq() after clockevents_config_and_register(). This aims
to eliminate a race condition where the timer interrupt could fire before
the event handler is initialized.

> diff --git a/drivers/clocksource/timer-econet-en751221.c b/drivers/clocksource/timer-econet-en751221.c
> index 155471f68e6f8..ed750e39cc4fa 100644
> --- a/drivers/clocksource/timer-econet-en751221.c
> +++ b/drivers/clocksource/timer-econet-en751221.c
> @@ -104,12 +104,11 @@ static int cevt_init_cpu(uint cpu)
>  	reg = ioread32(reg_ctl(cpu)) | ctl_bit_enabled(cpu);
>  	iowrite32(reg, reg_ctl(cpu));
>  
> -	enable_percpu_irq(cd->irq, IRQ_TYPE_NONE);
> -
> -	/* Do this last because it synchronously configures the timer */
>  	clockevents_config_and_register(cd, econet_timer.freq_hz,
>  					ECONET_MIN_DELTA, ECONET_MAX_DELTA);
>  
> +	enable_percpu_irq(cd->irq, IRQ_TYPE_NONE);
> +
>  	return 0;
>  }

If tick_check_new_device() rejects the timer (for instance, because another
higher-rated clockevent device is available), does the device remain in the
DETACHED state without the clockevents core initializing dev->event_handler?

Since the hardware timer is already explicitly enabled before registration
via ioread32(reg_ctl(cpu)) | ctl_bit_enabled(cpu), it will eventually wrap
and trigger an interrupt.

With enable_percpu_irq() unmasking the interrupt line, could
cevt_interrupt() execute and dereference a NULL dev->event_handler?

Should cd->event_handler be explicitly initialized to
clockevents_handle_noop prior to registration to fully resolve this race?

[ ... ]

> @@ -177,6 +176,8 @@ static int __init timer_init(struct device_node *np)
>  		goto out_membase;
>  	}
>  
> +	irq_set_status_flags(econet_timer.irq, IRQ_NOAUTOEN);
> +
>  	ret = request_percpu_irq(econet_timer.irq, cevt_interrupt, np->name,
>  				 &econet_timer_pcpu);
>  

Looking further down in timer_init(), the driver registers its CPU hotplug
state with a NULL teardown callback:

    ret = cpuhp_setup_state(CPUHP_AP_ONLINE_DYN,
                            "clockevents/econet/timer:starting",
                            cevt_init_cpu, NULL);

Since the driver registers its CPU hotplug state with a NULL teardown
callback, what happens when a CPU is hotplugged out?

Because the clockevent device lacks a set_state_shutdown callback, the
clockevents core cannot turn off the hardware timer. This appears to leave
the per-CPU interrupt unmasked at the interrupt controller and the timer
hardware actively running.

When the CPU is later brought back online and unmasks its interrupts, could
it instantly take a pending timer interrupt before initialization is
complete?

Additionally, if cpuhp_setup_state() fails on a subsequent CPU after
succeeding on CPU 0, it attempts to roll back CPU 0. Because of the NULL
teardown callback, disable_percpu_irq() and clockevents_unbind_device()
are never executed for CPU 0.

The error path then jumps to out_irq_free:

out_irq_free:
    free_percpu_irq(econet_timer.irq, &econet_timer_pcpu);
out_irq_mapping:
    irq_dispose_mapping(econet_timer.irq);
out_membase:
    for (int i = 0; i < ARRAY_SIZE(econet_timer.membase); i++) {
        if (econet_timer.membase[i])
            iounmap(econet_timer.membase[i]);
    }

If the interrupt is still enabled on CPU 0, will __free_percpu_irq() safely
warn and return without freeing the action?

Since the hardware timer is still running on CPU 0 and the driver proceeds
to unmap the membase via iounmap(), could the timer fire and cause the
still-registered cevt_interrupt handler to access unmapped MMIO memory,
resulting in a Data Abort?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260516215720.4160831-1-cjd@cjdns.fr?part=3