Re: [PATCH v3 2/4] clocksource/drivers: Add support for the Renesas RZ/N1 timers

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [High] Off-by-one error in CPU bounds check allows out-of-bounds array access in the hotplug callback.
- [High] A 16-bit high-frequency clocksource is incorrectly flagged with CLOCK_SOURCE_SUSPEND_NONSTOP, breaking system suspend timekeeping.
- [High] Probe incorrectly returns a positive IRQ number on devm_request_irq failure.
- [High] Missing teardown callback in cpuhp_setup_state causes a Use-After-Free of clockevent devices.
- [Medium] Down-counting timer values are potentially returned directly, breaking the requirement for monotonic increasing counters.
- [Medium] Failing CPU bringup by returning an error from a CPU hotplug starting callback.
--

--- Patch [2]: [PATCH v3 2/4] clocksource/drivers: Add support for the Renesas RZ/N1 timers ---
commit 67eeb3e62e1eb246f2738740b9b5319a484b8d50
Author: Herve Codina (Schneider Electric) <herve.codina@bootlin.com>

clocksource/drivers: Add support for the Renesas RZ/N1 timers

The Renesas RZ/N1 timer block controller is the controller in charge of
timers available in the Renesas RZ/N1 SoCs family. This controller handles
8 timers: 6 16-bit timers and 2 32-bit timers.

> diff --git a/drivers/clocksource/timer-rzn1.c b/drivers/clocksource/timer-rzn1.c
> new file mode 100644
> index 0000000000000..024f01bb0227b
> --- /dev/null
> +++ b/drivers/clocksource/timer-rzn1.c

[ ... ]

> +static u64 notrace rzn1_sched_read(void)
> +{
> +	return readl_relaxed(rzn1_sched_clock->base + RZN1_TIMER_CURRENT_COUNT_REG);
> +}
> +
> +static u64 rzn1_clksrc_read(struct clocksource *cs)
> +{
> +	return readl_relaxed(rzn1_sched_clock->base + RZN1_TIMER_CURRENT_COUNT_REG);
> +}

[Severity: Medium]
Since RZN1_TIMER_LOAD_COUNT_REG is used earlier to set the timer period,
does this hardware act as a down-counter?

If the hardware counts downward, will returning the raw value here break the
timekeeping requirement for monotonically increasing values?

> +static struct clocksource rzn1_clocksource = {
> +	.name = "rzn1_timer_clocksource",
> +	.rating = 200,
> +	.flags = CLOCK_SOURCE_IS_CONTINUOUS | CLOCK_SOURCE_SUSPEND_NONSTOP,

[Severity: High]
Is it safe to flag a 16-bit clocksource with CLOCK_SOURCE_SUSPEND_NONSTOP?

Since a 16-bit counter fed by a high-frequency clock will wrap around very
quickly, will this cause massive wall time jumps if the system suspends for
longer than the wrap interval?

> +	.read = rzn1_clksrc_read,
> +};
> +
> +/* Timers table where 32-bit timers will be assigned per-cpu */
> +static struct rzn1_timer *rzn1_tab_timers;
> +
> +static int rzn1_local_timer_starting_cpu(unsigned int cpu)
> +{
> +	struct rzn1_timer *timer;
> +
> +	if (cpu > RZN1_TIMER_NB_32BIT_TIMERS)
> +		return -EINVAL;

[Severity: Medium]
Will returning an error like -EINVAL completely abort the hotplug process
for any CPU beyond the first two?

Should this return 0 instead, allowing additional CPUs to still boot online
and rely on a global broadcast timer?

[Severity: High]
Also, does the greater-than condition here have an off-by-one error?

If RZN1_TIMER_NB_32BIT_TIMERS is 2, a cpu value of 2 bypasses this check.

> +
> +	timer = &rzn1_tab_timers[cpu + RZN1_TIMER_BASE_INDEX_32BIT_TIMERS];

[Severity: High]
If a cpu value of 2 bypassed the check above, will this access out of bounds?

The array is only sized for 8 elements total. Adding a cpu value of 2 to
RZN1_TIMER_BASE_INDEX_32BIT_TIMERS (which is 6) computes an index of 8.

[ ... ]

> +static int rzn1_timer_setup_timers(struct platform_device *pdev,
> +				   struct rzn1_timer *tab_timers,
> +				   unsigned int first_timer_index,
> +				   unsigned int last_timer_index,
> +				   void __iomem *base, unsigned long clock_rate)
> +{

[ ... ]

> +		ret = devm_request_irq(dev, timer->ced.irq, rzn1_timer_interrupt,
> +				       IRQF_TIMER, timer->ced.name, timer);
> +		if (ret < 0)
> +			return dev_err_probe(dev, irq, "timer%u: Failed to request IRQ\n", i);

[Severity: High]
Will calling dev_err_probe() with irq return the positive IRQ number back to
the caller instead of the negative ret code?

If the driver probe returns a positive integer, won't the driver core assume
the probe succeeded despite the failure?

[ ... ]

> +static int rzn1_timer_probe_first(struct platform_device *pdev,
> +				  struct rzn1_timer *tab_timers,
> +				  void __iomem *base, unsigned long clock_rate)
> +{

[ ... ]

> +	/*
> +	 * Second step, almost all operations that can fail have been called.
> +	 * Timers are ready to work. Start with the last operation that can fail,
> +	 * installing and invoking hotplug callbacks
> +	 */
> +	rzn1_tab_timers = tab_timers;
> +	ret = cpuhp_setup_state(CPUHP_AP_ONLINE_DYN,
> +				"clockevents/rzn1/global_timer:starting",
> +				rzn1_local_timer_starting_cpu, NULL);

[Severity: High]
Does passing a NULL teardown callback here leave registered clockevent
devices dangling if a subsequent step fails during probe?

If the driver fails and frees its timer structures via devres, or if a CPU
is dynamically offlined later, will this lack of teardown cause a
use-after-free in the clockevents core?

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260717095552.767475-1-herve.codina@bootlin.com?part=2