Re: [patch v2a 01/20] posix-timers: Prevent RT livelock in itimer_delete()

[Frederic Weisbecker <frederic@kernel.org>]

Le Thu, Jun 01, 2023 at 10:16:34PM +0200, Thomas Gleixner a écrit :
> itimer_delete() has a retry loop when the timer is concurrently expired. On
> non-RT kernels this just spin-waits until the timer callback has completed.
> On RT kernels this is a potential livelock when the exiting task preempted
> the hrtimer soft interrupt.

It's not just RT but also archs supporting HAVE_POSIX_CPU_TIMERS_TASK_WORK

> 
> Replace spin_unlock() with an invocation of timer_wait_running() to handle
> it the same way as the other retry loops in the posix timer code.
> 
> Fixes: ec8f954a40da ("posix-timers: Use a callback for cancel synchronization on PREEMPT_RT")
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> ---
> V2:  Remove bogus comments vs. posix CPU timers - Frederic
> V2a: Send the real fixed up version
> ---
>  kernel/time/posix-timers.c |   43 +++++++++++++++++++++++++++++++++++--------
>  1 file changed, 35 insertions(+), 8 deletions(-)
> 
> --- a/kernel/time/posix-timers.c
> +++ b/kernel/time/posix-timers.c
> @@ -1037,27 +1037,52 @@ SYSCALL_DEFINE1(timer_delete, timer_t, t
>  }
>  
>  /*
> - * return timer owned by the process, used by exit_itimers
> + * Delete a timer if it is armed, remove it from the hash and schedule it
> + * for RCU freeing.
>   */
>  static void itimer_delete(struct k_itimer *timer)
>  {
> -retry_delete:
> -	spin_lock_irq(&timer->it_lock);
> +	unsigned long flags;
>  
> +retry_delete:
> +	/*
> +	 * irqsave is required to make timer_wait_running() work.
> +	 */
> +	spin_lock_irqsave(&timer->it_lock, flags);
> +
> +	/*
> +	 * Even if the timer is not longer accessible from other tasks
> +	 * it still might be armed and queued in the underlying timer
> +	 * mechanism. Worse, that timer mechanism might run the expiry
> +	 * function concurrently.
> +	 */
>  	if (timer_delete_hook(timer) == TIMER_RETRY) {
> -		spin_unlock_irq(&timer->it_lock);
> +		/*
> +		 * Timer is expired concurrently, prevent livelocks
> +		 * and pointless spinning on RT.

Ditto.

Reviewed-by: Frederic Weisbecker <frederic@kernel.org>

Thanks.