Re: [PATCH v3 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 2 potential issue(s) to consider:
- [High] The error teardown path for `cpuhp_setup_state()` failure unmaps MMIO memory without unregistering active clock event devices, creating a latent use-after-unmap bug.
- [High] `cevt_dev_init()` accesses memory blocks using absolute CPU IDs, causing a NULL pointer dereference if the CPU mask is sparse or the device tree is incomplete.
--

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

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

This commit converts the initialization process into two clearly delineated
phases. It moves all IRQ and address resource mapping before the point of no
return and adds teardown logic in case of error.

> 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

[ ... ]

> @@ -126,22 +127,9 @@ static void __init cevt_dev_init(uint cpu)
>  	iowrite32(U32_MAX, reg_compare(cpu));
>  }
>  
> -static int __init cevt_init(struct device_node *np)
> +static void __init cevt_init(struct device_node *np)
>  {
> -	int i, irq, ret;
> -
> -	irq = irq_of_parse_and_map(np, 0);
> -	if (irq <= 0) {
> -		pr_err("%pOFn: irq_of_parse_and_map failed", np);
> -		return -EINVAL;
> -	}
> -
> -	ret = request_percpu_irq(irq, cevt_interrupt, np->name, &econet_timer_pcpu);
> -
> -	if (ret < 0) {
> -		pr_err("%pOFn: IRQ %d setup failed (%d)\n", np, irq, ret);
> -		goto err_unmap_irq;
> -	}
> +	int i;
>  
>  	for_each_possible_cpu(i) {
>  		struct clock_event_device *cd = &per_cpu(econet_timer_pcpu, i);
> @@ -151,21 +139,12 @@ static int __init cevt_init(struct device_node *np)
>  					  CLOCK_EVT_FEAT_C3STOP |
>  					  CLOCK_EVT_FEAT_PERCPU;
>  		cd->set_next_event	= cevt_set_next_event;
> -		cd->irq			= irq;
> +		cd->irq			= econet_timer.irq;
>  		cd->cpumask		= cpumask_of(i);
>  		cd->name		= np->name;
>  
>  		cevt_dev_init(i);

Could this iteration cause a NULL pointer dereference if the CPU mask is
sparse?

timer_init() allocates memory blocks based on num_possible_cpus(). If
cpu_possible_mask is sparse (for example, if only CPUs 0 and 3 are present),
num_possible_cpus() is 2, and only 1 block is mapped.

When this loop reaches CPU 3, cevt_dev_init(3) might try to access an
unmapped membase pointer.

A later patch in this series changes the allocation to use of_address_count()
and adds a check in cevt_init_cpu(), but since cevt_dev_init() is called
unconditionally here, the issue appears to remain reachable during early
initialization.

>  	}

[ ... ]

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

[ ... ]

> +	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, does this error path leave active clock event
devices registered with unmapped MMIO memory?

cpuhp_setup_state() invokes its startup callback (cevt_init_cpu) on all
online CPUs, which activates the clock events. If it fails on a subsequent
CPU, the NULL teardown callback means no rollback occurs.

When the error path jumps to out_irq_free and unmaps the MMIO memory, any
subsequent timer interrupt could dereference the unmapped memory.

A later patch in this series modifies cevt_init_cpu() to return errors,
which makes this teardown path reachable.

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