Re: [PATCH v5 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] Failing cpuhp_setup_state causes I/O memory unmapping while clock event devices are still registered and active, leading to a fatal page fault.
- [High] Race condition during initialization leads to NULL pointer dereference.
- [High] Mapping memory blocks based on num_possible_cpus() leads to out-of-bounds accesses on systems with sparse CPU masks.
- [Medium] Error teardown path retains dangling pointers in the global econet_timer.membase array.
--

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

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

This commit splits the initialization of the en751221 econet timer into two
phases to support a clean exit on error. It moves IRQ setup and memory
mapping before the point of no return. A teardown path is added to free
IRQs and unmap memory if initialization fails early.

> 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;
>  		}
>  	}

Does calculating the number of memory blocks using num_possible_cpus() cause
issues on systems with sparse CPU masks?

If a system only has CPUs 0 and 4, num_possible_cpus() evaluates to 2, mapping
only one block. When hardware access paths use the absolute CPU index
(cpu >> 1), could reading econet_timer.membase[2] lead to a NULL pointer
dereference?

(Note: It is understood this issue was present before this patch).

> +	econet_timer.irq = irq_of_parse_and_map(np, 0);
> +	if (econet_timer.irq <= 0) {
> +		pr_err("%pOFn: irq_of_parse_and_map failed\n", np);
> +		ret = -EINVAL;
> +		goto out_membase;
> +	}
> +
> +	ret = request_percpu_irq(econet_timer.irq, cevt_interrupt, np->name,
> +				 &econet_timer_pcpu);
> +
> +	if (ret < 0) {
> +		pr_err("%pOFn: IRQ %d setup failed (%d)\n", np,
> +		       econet_timer.irq, ret);
> +		goto out_irq_mapping;
> +	}
> +
> +	cevt_init(np);

Is there a potential race condition here if an interrupt fires immediately
after request_percpu_irq() is called, before cevt_init() populates
cd->cpumask?

If the handler executes, could it result in a NULL pointer dereference
when reading dev->cpumask?

(Note: It is understood this might be fixed later in the patch series by
commit clocksource/timer-econet-en751221: Disable IRQ until cevt registered).

> +	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 on a subsequent CPU, could the already-registered
clock events on previous CPUs continue to be invoked by the timer core?

Since the teardown path unmaps the IO memory without unregistering these
clock event devices, would calling cd->set_next_event dereference unmapped
memory?

Should cpuhp_setup_state() be placed after the point of no return, or should
a teardown callback be provided to unregister the devices?

[ ... ]

> +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]);
> +	}
> +
> +	return ret;
>  }

When iounmap() is called in the out_membase path, should the pointers in
econet_timer.membase be set to NULL?

Since econet_timer is a global static structure, if the driver's init
function is invoked a second time (such as from a secondary matching device
tree node), could a subsequent failure cause iounmap() to be called on
already-unmapped pointers?

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