ASSERT() when calling k_start_timer() in a thread

[wcheonq]

Recently I have observed the ASSERT failure at z_add_timeout() in kernel/timeout.c file. It checks if to->node has already been added to timeout_list.

void z_add_timeout(struct _timeout *to, _timeout_func_t fn,
		   k_timeout_t timeout)
{
	if (K_TIMEOUT_EQ(timeout, K_FOREVER)) {
		return;
	}

#ifdef CONFIG_KERNEL_COHERENCE
	__ASSERT_NO_MSG(arch_mem_coherent(to));
#endif

	__ASSERT(!sys_dnode_is_linked(&to->node), "");

I think there is a race condition between ISR and a thread. If a thread is preempted by timer ISR while k_timer_start() is being executed, then I think it hits the case. Specifically, once the thread is preempted after z_abort_timeout() call, then timer ISR can execute z_add_timeout() to add a new timeout for the timer that has been just expired.

void z_impl_k_timer_start(struct k_timer *timer, k_timeout_t duration,
			  k_timeout_t period)
{
	SYS_PORT_TRACING_OBJ_FUNC(k_timer, start, timer, duration, period);

	if (K_TIMEOUT_EQ(duration, K_FOREVER)) {
		return;
	}

	/* z_add_timeout() always adds one to the incoming tick count
	 * to round up to the next tick (by convention it waits for
	 * "at least as long as the specified timeout"), but the
	 * period interval is always guaranteed to be reset from
	 * within the timer ISR, so no round up is desired and 1 is
	 * subtracted in there.
	 *
	 * Note that the duration (!) value gets the same treatment
	 * for backwards compatibility.  This is unfortunate
	 * (i.e. k_timer_start() doesn't treat its initial sleep
	 * argument the same way k_sleep() does), but historical.  The
	 * timer_api test relies on this behavior.
	 */
	if (Z_TICK_ABS(duration.ticks) < 0) {
		duration.ticks = MAX(duration.ticks - 1, 0);
	}

	(void)z_abort_timeout(&timer->timeout);
	timer->period = period;
	timer->status = 0U;

	z_add_timeout(&timer->timeout, z_timer_expiration_handler,
		     duration);
}

From the documentation, I see that "Attempting to start a timer that is already running is permitted." However, I think this is a bug in the kernel.

Has anyone seen the same failure? Any thoughts? Should I fire a bug?

[wcheonq]

Any comments or thoughts?