Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Mon, 5 Jun 2023 12:59:54 +0200
From:   Frederic Weisbecker <frederic@kernel.org>
To:     Thomas Gleixner <tglx@linutronix.de>
Cc:     LKML <linux-kernel@vger.kernel.org>,
        Anna-Maria Behnsen <anna-maria@linutronix.de>,
        Peter Zijlstra <peterz@infradead.org>,
        Sebastian Siewior <bigeasy@linutronix.de>,
        syzbot+5c54bd3eb218bb595aa9@syzkaller.appspotmail.com,
        Dmitry Vyukov <dvyukov@google.com>,
        Michael Kerrisk <mtk.manpages@gmail.com>
Subject: Re: [patch v2a 01/20] posix-timers: Prevent RT livelock in
 itimer_delete()
Message-ID: <ZH3AKuUlZnZjoQo7@localhost.localdomain>
References: <20230425181827.219128101@linutronix.de>
 <20230425183312.862346341@linutronix.de>
 <ZFPmKtFGPUQkeDEd@localhost.localdomain>
 <87bkj03qmq.ffs@tglx>
 <875y9743dt.ffs@tglx>
 <877csndn49.ffs@tglx>
 <87v8g7c50d.ffs@tglx>
MIME-Version: 1.0
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <87v8g7c50d.ffs@tglx>
Precedence: bulk

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.