[v2a,01/20] posix-timers: Prevent RT livelock in itimer_delete()
Commit Message
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.
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(-)
Comments
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.
@@ -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.
+ *
+ * timer_wait_running() drops timer::it_lock, which opens
+ * the possibility for another task to delete the timer.
+ *
+ * That's not possible here because this is invoked from
+ * do_exit() only for the last thread of the thread group.
+ * So no other task can access and delete that timer.
+ */
+ if (WARN_ON_ONCE(timer_wait_running(timer, &flags) != timer))
+ return;
+
goto retry_delete;
}
list_del(&timer->list);
- spin_unlock_irq(&timer->it_lock);
+ spin_unlock_irqrestore(&timer->it_lock, flags);
release_posix_timer(timer, IT_ID_SET);
}
/*
- * This is called by do_exit or de_thread, only when nobody else can
- * modify the signal->posix_timers list. Yet we need sighand->siglock
- * to prevent the race with /proc/pid/timers.
+ * Invoked from do_exit() when the last thread of a thread group exits.
+ * At that point no other task can access the timers of the dying
+ * task anymore.
*/
void exit_itimers(struct task_struct *tsk)
{
@@ -1067,10 +1092,12 @@ void exit_itimers(struct task_struct *ts
if (list_empty(&tsk->signal->posix_timers))
return;
+ /* Protect against concurrent read via /proc/$PID/timers */
spin_lock_irq(&tsk->sighand->siglock);
list_replace_init(&tsk->signal->posix_timers, &timers);
spin_unlock_irq(&tsk->sighand->siglock);
+ /* The timers are not longer accessible via tsk::signal */
while (!list_empty(&timers)) {
tmr = list_first_entry(&timers, struct k_itimer, list);
itimer_delete(tmr);