Re: [PATCH v4 2/4] clocksource/timer-econet-en751221: Init teardown on error if possible

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] Teardown path unmaps memory while clockevents remain registered (Use-After-Free)
- [High] Missing CPU hotplug teardown callback leaves per-CPU IRQs enabled on offline CPUs
- [High] Pre-existing out-of-bounds mapping access with sparse CPU possible mask
- [Medium] Ignored error code from clocksource_mmio_init causes false success reporting
--

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

clocksource/timer-econet-en751221: Init teardown on error if possible

This commit reorganizes the driver initialization process into two
distinct phases, providing a rollback path for errors that occur prior to
the point of no return. It adds logic to unmap memory and release interrupts
if the setup steps fail.

> diff --git a/drivers/clocksource/timer-econet-en751221.c b/drivers/clocksource/timer-econet-en751221.c
> index 4008076b1a210..155471f68e6f8 100644
> --- a/drivers/clocksource/timer-econet-en751221.c
> +++ b/drivers/clocksource/timer-econet-en751221.c
> @@ -186,22 +165,45 @@ static int __init timer_init(struct device_node *np)
>  		econet_timer.membase[i] = of_iomap(np, i);
>  		if (!econet_timer.membase[i]) {
>  			pr_err("%pOFn: failed to map register [%d]\n", np, i);
> -			return -ENXIO;
> +			ret = -ENXIO;
> +			goto out_membase;
>  		}
>  	}

This is a pre-existing issue that is resolved later in this patch series
by commit clocksource/timer-econet-en751221: Support EN751627 without
percpu IRQ, but I am mentioning it here for completeness.

Can this loop cause an out-of-bounds access with a sparse CPU mask?

The loop iterates up to num_blocks, which is calculated based on
num_possible_cpus(). If the system has a sparse CPU topology where
CPUs 0 and 4 are possible, num_possible_cpus() is 2, so the loop only
maps membase[0].

However, cevt_init() iterates over for_each_possible_cpu(i). For CPU 4,
cevt_dev_init(4) accesses membase[2], which remains null, potentially
causing a kernel panic when dereferenced.

[ ... ]

> +	cevt_init(np);
> +
> +	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;
> +	}

If cpuhp_setup_state() fails here, can the teardown path cause a
use-after-free?

Because the teardown callback in cpuhp_setup_state() is null, the CPU
hotplug core's attempt to roll back the state on previously initialized
CPUs acts as a no-op. This leaves the clock event devices and hardware
timers fully active on those CPUs.

The error path then proceeds to out_irq_free and subsequently unmaps the
memory regions via iounmap(). When the clockevents core later tries to
access the active timer on those CPUs, it will read from and write to
unmapped memory.

Additionally, while this is a pre-existing issue, does providing a null
teardown callback leave per-CPU interrupts enabled on offline CPUs?

When a CPU is dynamically offlined, the teardown callback should stop the
hardware timer and call disable_percpu_irq(). Without it, the timer remains
free-running and the interrupt unmasked for the offline CPU.

> +
> +	/* Point of no return, do not attempt to tear down after this. */
>  
>  	/* For clocksource purposes always read clock zero, whatever the CPU */
>  	ret = clocksource_mmio_init(reg_count(0), np->name,
>  				    econet_timer.freq_hz, 301, ECONET_BITS,
>  				    clocksource_mmio_readl_up);
> -	if (ret) {
> -		pr_err("%pOFn: clocksource_mmio_init failed: %d", np, ret);
> -		return ret;
> -	}
> -
> -	ret = cevt_init(np);
> -	if (ret < 0)
> -		return ret;
> +	if (ret)
> +		pr_err("%pOFn: clocksource_mmio_init failed: %d\n", np, ret);

Does removing the return on error here cause the driver to report success
when initialization fails?

The failure code is captured in ret, but execution falls through and the
function unconditionally returns 0 at the end. This allows the driver to
print success messages and report a successful initialization even if the
clocksource registration failed.

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