Re: [PATCH v5 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:
- [High] Uninitialized `dev->event_handler` causes a kernel panic if the clock event device is rejected by the timekeeping core.
- [High] The error path after `cpuhp_setup_state()` fails to roll back the hardware state and unregister the clockevent device, resulting in a Use-After-Free/Page fault.
- [Low] Redundant `IRQ_NOAUTOEN` flag setting on a per-CPU interrupt.
--

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

clocksource/timer-econet-en751221: Disable IRQ until cevt registered

This commit reorders interrupt enablement to occur after clockevent
registration to avoid a race condition with uninitialized event handlers,
and explicitly sets the no-auto-enable flag on the IRQ.

> 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);
> +

The commit message notes this eliminates a race where cevt_interrupt triggers
before the event_handler is set. Does this reordering fully resolve the issue?

If the timekeeping core rejects the new clockevent device in
tick_check_new_device(), dev->event_handler will remain uninitialized (NULL).
Since the hardware timer is unconditionally started and the IRQ is enabled
here, could the timer eventually wrap around, assert the interrupt, and cause
a NULL pointer dereference in cevt_interrupt()?

Additionally, looking at the error path in timer_init() where cevt_init_cpu()
is registered as a hotplug callback:

timer_init() {
    ...
    ret = cpuhp_setup_state(CPUHP_AP_ONLINE_DYN,
                            "clockevents/econet/timer:starting",
                            cevt_init_cpu, NULL);
    if (ret < 0) {
        pr_err("%pOFn: cpuhp setup failed (%d)\n", np, ret);
        goto out_irq_free;
    }
    ...
}

Because cpuhp_setup_state() is called with a NULL teardown callback, if the
overall hotplug state setup fails, there is no rollback for CPUs that already
successfully executed cevt_init_cpu().

If the out_irq_free error path then unmaps the MMIO base while the clockevent
devices remain registered and active in the core timer subsystem, will a later
access to the device (such as invoking cevt_set_next_event()) dereference the
unmapped memory and cause a fatal page fault?

>  	return 0;
>  }

[ ... ]

> @@ -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);
> +

This isn't a bug, but is the manual setting of IRQ_NOAUTOEN necessary here?

Per-CPU interrupts requested via request_percpu_irq() mandate that the
interrupt descriptor has the IRQ_PER_CPU_DEVID flag set. The mechanism that
applies this flag, irq_set_percpu_devid_flags(), automatically sets the
IRQ_NOAUTOEN flag on the interrupt descriptor at the same time.

>  	ret = request_percpu_irq(econet_timer.irq, cevt_interrupt, np->name,
>  				 &econet_timer_pcpu);
>

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