linux-kernel - Re: [patch v2a 01/20] posix-timers: Prevent RT livelock in itimer

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 5 Jun 2023 12:59:54 +0200
From:   Frederic Weisbecker <frederic@...nel.org>
To:     Thomas Gleixner <tglx@...utronix.de>
Cc:     LKML <linux-kernel@...r.kernel.org>,
        Anna-Maria Behnsen <anna-maria@...utronix.de>,
        Peter Zijlstra <peterz@...radead.org>,
        Sebastian Siewior <bigeasy@...utronix.de>,
        syzbot+5c54bd3eb218bb595aa9@...kaller.appspotmail.com,
        Dmitry Vyukov <dvyukov@...gle.com>,
        Michael Kerrisk <mtk.manpages@...il.com>
Subject: Re: [patch v2a 01/20] posix-timers: Prevent RT livelock in
 itimer_delete()

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@...utronix.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@...nel.org>

Thanks.