Re: [PATCH v9 5/5] clocksource/timer-econet-en751221: Support EN751627 without percpu IRQ

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] clockevents/econet: missing cpuhp teardown causes IRQ storms
- [High] clockevents/econet: UAF on timer registers after partial cpuhp setup failure
--

commit dc93f390b5bb20c598e0e2ea1ef5ba065d8e2acd
Author: Caleb James DeLisle <cjd@cjdns.fr>

clocksource/timer-econet-en751221: Support EN751627 without percpu IRQ

EN751627 is based on the 1004Kc which uses a different interrupt number
for each CPU timer. Support both this and the EN751221 which uses a
single percpu interrupt.

> diff --git a/drivers/clocksource/timer-econet-en751221.c b/drivers/clocksource/timer-econet-en751221.c
> --- a/drivers/clocksource/timer-econet-en751221.c
> +++ b/drivers/clocksource/timer-econet-en751221.c

[ ... ]

> @@ -102,6 +104,25 @@ static int cevt_init_cpu(uint cpu)
>  	struct clock_event_device *cd = &per_cpu(econet_timer_pcpu, cpu);
>  	u32 reg;
>  
> +	if (!reg_ctl(cpu)) {
> +		pr_err("%s: missing address resource for CPU %d\n", cd->name,
> +		       cpu);
> +		return -EINVAL;
> +	}
> +	if (cd->irq <= 0) {
> +		pr_err("%s: missing IRQ for CPU %d\n", cd->name, cpu);
> +		return -EINVAL;
> +	}
> +	if (!econet_timer.is_percpu) {
> +		int ret = irq_force_affinity(cd->irq, cpumask_of(cpu));
> +
> +		if (ret) {
> +			pr_err("%s: failed to set IRQ affinity to CPU %d: %pe\n",
> +			       cd->name, cpu, ERR_PTR(ret));
> +			return ret;
> +		}
> +	}
> +

[Severity: High]
If this initialization fails on a subsequent CPU (for example, if
irq_force_affinity() returns an error on CPU 1), the cpuhp subsystem will try
to roll back CPU 0.

Because timer_init() registers cpuhp_setup_state() with a NULL teardown
callback, does this mean CPU 0's clockevent device and timer hardware will
remain active?

If timer_init() then jumps to the out_irq_free error path and unmaps the
timer MMIO registers via iounmap(), will CPU 0's active timer interrupt
handler trigger a panic when it accesses the unmapped memory?

>  	pr_debug("%s: Setting up clockevent for CPU %d\n", cd->name, cpu);
>  
>  	reg = ioread32(reg_ctl(cpu)) | ctl_bit_enabled(cpu);
> @@ -110,7 +131,10 @@ static int cevt_init_cpu(uint cpu)
>  	clockevents_config_and_register(cd, econet_timer.freq_hz,
>  					ECONET_MIN_DELTA, ECONET_MAX_DELTA);
>  
> -	enable_percpu_irq(cd->irq, IRQ_TYPE_NONE);
> +	if (econet_timer.is_percpu)
> +		enable_percpu_irq(cd->irq, IRQ_TYPE_NONE);
> +	else
> +		enable_irq(cd->irq);

[Severity: High]
When a CPU is offlined on a platform using standard global IRQs (EN751627),
the missing cpuhp teardown callback means the timer and its IRQ are never
disabled.

Because standard IRQs migrate when a CPU goes offline, will the still-active
IRQ migrate to another CPU?

If the timer hardware fires, cevt_interrupt() handles it on the new CPU,
reads the new CPU's pending bit via this_cpu_ptr(), finds it 0, and returns
IRQ_NONE without clearing the offlined CPU's timer interrupt.

Does this result in an unhandled IRQ storm?

Additionally, when the CPU is brought back online, will calling enable_irq()
on the already-enabled IRQ trigger an unbalanced enable warning?

>  
>  	return 0;
>  }

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