CINXE.COM

<?xml version="1.0" encoding="UTF-8" standalone="yes"?> <!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.0 Transitional//EN" "http://www.w3.org/TR/xhtml1/DTD/xhtml1-transitional.dtd"><html xmlns="http://www.w3.org/1999/xhtml"><head><meta http-equiv="Content-Type" content="text/html; charset=UTF-8" /><title>LKML: Thomas Gleixner: [patch V3 09/18] posix-timers: Rework timer removal</title><link href="/css/message.css" rel="stylesheet" type="text/css" /><link href="/css/wrap.css" rel="alternate stylesheet" type="text/css" title="wrap" /><link href="/css/nowrap.css" rel="stylesheet" type="text/css" title="nowrap" /><link href="/favicon.ico" rel="shortcut icon" /><script src="/js/simple-calendar.js" type="text/javascript"></script><script src="/js/styleswitcher.js" type="text/javascript"></script><link rel="alternate" type="application/rss+xml" title="lkml.org : last 100 messages" href="/rss.php" /><link rel="alternate" type="application/rss+xml" title="lkml.org : last messages by Thomas Gleixner" href="/groupie.php?aid=" /><script> var _paq = window._paq = window._paq || []; /* tracker methods like "setCustomDimension" should be called before "trackPageView" */ _paq.push(["setDoNotTrack", true]); _paq.push(["disableCookies"]); _paq.push(['trackPageView']); _paq.push(['enableLinkTracking']); (function() { var u="//m.lkml.org/"; _paq.push(['setTrackerUrl', u+'matomo.php']); _paq.push(['setSiteId', '1']); var d=document, g=d.createElement('script'), s=d.getElementsByTagName('script')[0]; g.async=true; g.src=u+'matomo.js'; s.parentNode.insertBefore(g,s); })(); </script></head><body onload="es.jasper.simpleCalendar.init();" itemscope="itemscope" itemtype="http://schema.org/BlogPosting"><table border="0" cellpadding="0" cellspacing="0"><tr><td width="180" align="center"><a href="/"><img style="border:0;width:135px;height:32px" src="/images/toprowlk.gif" alt="lkml.org" /></a></td><td width="32">聽</td><td class="nb"><div><a class="nb" href="/lkml"> [lkml]</a> 聽 <a class="nb" href="/lkml/2025"> [2025]</a> 聽 <a class="nb" href="/lkml/2025/3"> [Mar]</a> 聽 <a class="nb" href="/lkml/2025/3/8"> [8]</a> 聽 <a class="nb" href="/lkml/last100"> [last100]</a> 聽 <a href="/rss.php"><img src="/images/rss-or.gif" border="0" alt="RSS Feed" /></a></div><div>Views: <a href="#" class="nowrap" onclick="setActiveStyleSheet('wrap');return false;">[wrap]</a><a href="#" class="wrap" onclick="setActiveStyleSheet('nowrap');return false;">[no wrap]</a> 聽 <a class="nb" href="/lkml/mheaders/2025/3/8/436" onclick="this.href='/lkml/headers'+'/2025/3/8/436';">[headers]</a>聽 <a href="/lkml/bounce/2025/3/8/436">[forward]</a>聽 </div></td><td width="32">聽</td></tr><tr><td valign="top"><div class="es-jasper-simpleCalendar" baseurl="/lkml/"></div><div class="threadlist">Messages in this thread</div><ul class="threadlist"><li class="root"><a href="/lkml/2025/3/8/426">First message in thread</a></li><li><a href="/lkml/2025/3/8/426">Thomas Gleixner</a><ul><li><a href="/lkml/2025/3/8/427">Thomas Gleixner</a><ul><li><a href="/lkml/2025/3/8/616">Frederic Weisbecker</a></li><li><a href="/lkml/2025/3/13/634">"tip-bot2 for Thomas Gleixner"</a></li></ul></li><li><a href="/lkml/2025/3/8/428">Thomas Gleixner</a><ul><li><a href="/lkml/2025/3/11/914">Frederic Weisbecker</a><ul><li><a href="/lkml/2025/3/11/977">Thomas Gleixner</a></li></ul></li><li><a href="/lkml/2025/3/13/633">"tip-bot2 for Eric Dumazet"</a></li></ul></li><li><a href="/lkml/2025/3/8/430">Thomas Gleixner</a><ul><li><a href="/lkml/2025/3/13/631">"tip-bot2 for Eric Dumazet"</a></li></ul></li><li><a href="/lkml/2025/3/8/431">Thomas Gleixner</a><ul><li><a href="/lkml/2025/3/13/630">"tip-bot2 for Thomas Gleixner"</a></li></ul></li><li><a href="/lkml/2025/3/8/432">Thomas Gleixner</a><ul><li><a href="/lkml/2025/3/13/632">"tip-bot2 for Thomas Gleixner"</a></li></ul></li><li><a href="/lkml/2025/3/8/433">Thomas Gleixner</a><ul><li><a href="/lkml/2025/3/13/629">"tip-bot2 for Thomas Gleixner"</a></li></ul></li><li><a href="/lkml/2025/3/8/434">Thomas Gleixner</a><ul><li><a href="/lkml/2025/3/13/627">"tip-bot2 for Thomas Gleixner"</a></li></ul></li><li><a href="/lkml/2025/3/8/435">Thomas Gleixner</a><ul><li><a href="/lkml/2025/3/13/626">"tip-bot2 for Thomas Gleixner"</a></li></ul></li><li class="origin"><a href="/lkml/2025/3/10/13">Thomas Gleixner</a><ul><li><a href="/lkml/2025/3/10/13">Frederic Weisbecker</a><ul><li><a href="/lkml/2025/3/10/148">Thomas Gleixner</a></li></ul></li><li><a href="/lkml/2025/3/10/256">Thomas Gleixner</a><ul><li><a href="/lkml/2025/3/13/628">"tip-bot2 for Thomas Gleixner"</a></li></ul></li></ul></li><li><a href="/lkml/2025/3/8/437">Thomas Gleixner</a><ul><li><a href="/lkml/2025/3/10/634">Frederic Weisbecker</a><ul><li><a href="/lkml/2025/3/10/1329">Thomas Gleixner</a><ul><li><a href="/lkml/2025/3/10/1664">Frederic Weisbecker</a></li></ul></li></ul></li><li><a href="/lkml/2025/3/13/625">"tip-bot2 for Peter Zijlstra"</a></li></ul></li><li><a href="/lkml/2025/3/8/438">Thomas Gleixner</a><ul><li><a href="/lkml/2025/3/10/1716">Frederic Weisbecker</a></li><li><a href="/lkml/2025/3/11/927">Frederic Weisbecker</a></li><li><a href="/lkml/2025/3/13/623">"tip-bot2 for Eric Dumazet"</a></li></ul></li><li><a href="/lkml/2025/3/8/439">Thomas Gleixner</a><ul><li><a href="/lkml/2025/3/11/932">Frederic Weisbecker</a></li><li><a href="/lkml/2025/3/13/624">"tip-bot2 for Thomas Gleixner"</a></li></ul></li><li><a href="/lkml/2025/3/8/440">Thomas Gleixner</a><ul><li><a href="/lkml/2025/3/13/622">"tip-bot2 for Thomas Gleixner"</a></li></ul></li><li><a href="/lkml/2025/3/8/441">Thomas Gleixner</a><ul><li><a href="/lkml/2025/3/11/940">Frederic Weisbecker</a></li><li><a href="/lkml/2025/3/13/621">"tip-bot2 for Thomas Gleixner"</a></li><li><a href="/lkml/2025/3/13/1624">David Laight</a></li><li><a href="/lkml/2025/3/17/208">"Nysal Jan K.A."</a></li></ul></li><li><a href="/lkml/2025/3/8/442">Thomas Gleixner</a><ul><li><a href="/lkml/2025/3/11/1110">Frederic Weisbecker</a></li><li><a href="/lkml/2025/3/13/620">"tip-bot2 for Thomas Gleixner"</a></li></ul></li><li><a href="/lkml/2025/3/8/443">Thomas Gleixner</a><ul><li><a href="/lkml/2025/3/9/14">Cyrill Gorcunov</a></li><li><a href="/lkml/2025/3/11/1103">Frederic Weisbecker</a></li><li><a href="/lkml/2025/3/13/619">"tip-bot2 for Thomas Gleixner"</a></li></ul></li><li><a href="/lkml/2025/3/8/444">Thomas Gleixner</a><ul><li><a href="/lkml/2025/3/9/13">Cyrill Gorcunov</a></li><li><a href="/lkml/2025/3/11/1588">Frederic Weisbecker</a><ul><li><a href="/lkml/2025/3/11/1615">Thomas Gleixner</a><ul><li><a href="/lkml/2025/3/11/1618">Thomas Gleixner</a></li></ul></li><li><a href="/lkml/2025/3/12/649">Cyrill Gorcunov</a></li></ul></li></ul></li><li><a href="/lkml/2025/3/8/445">Thomas Gleixner</a><ul><li><a href="/lkml/2025/3/10/254">Thomas Gleixner</a><ul><li><a href="/lkml/2025/3/11/1599">Frederic Weisbecker</a></li><li><a href="/lkml/2025/3/13/617">"tip-bot2 for Thomas Gleixner"</a></li></ul></li></ul></li></ul></li></ul><div class="threadlist">Patch in this message</div><ul class="threadlist"><li><a href="/lkml/diff/2025/3/8/436/1">Get diff 1</a></li></ul></td><td width="32" rowspan="2" class="c" valign="top"><img src="/images/icornerl.gif" width="32" height="32" alt="/" /></td><td class="c" rowspan="2" valign="top" style="padding-top: 1em"><table><tr><td><table><tr><td class="lp">From</td><td class="rp" itemprop="author">Thomas Gleixner <></td></tr><tr><td class="lp">Subject</td><td class="rp" itemprop="name">[patch V3 09/18] posix-timers: Rework timer removal</td></tr><tr><td class="lp">Date</td><td class="rp" itemprop="datePublished">Sat, 8 Mar 2025 17:48:32 +0100 (CET)</td></tr></table></td><td></td></tr></table><pre itemprop="articleBody">sys_timer_delete() and the do_exit() cleanup function itimer_delete() are doing the same thing, but have needlessly different implementations instead of sharing the code. The other oddity of timer deletion is the fact that the timer is not invalidated before the actual deletion happens, which allows concurrent lookups to succeed. That's wrong because a timer which is in the process of being deleted should not be visible and any actions like signal queueing, delivery and rearming should not happen once the task, which invoked timer_delete(), has the timer locked. Rework the code so that: 1) The signal queueing and delivery code ignore timers which are marked invalid 2) The deletion implementation between sys_timer_delete() and itimer_delete() is shared 3) The timer is invalidated and removed from the linked lists before the deletion callback of the relevant clock is invoked. That requires to rework timer_wait_running() as it does a lookup of the timer when relocking it at the end. In case of deletion this lookup would fail due to the preceding invalidation and the wait loop would terminate prematurely. But due to the preceding invalidation the timer cannot be accessed by other tasks anymore, so there is no way that the timer has been freed after the timer lock has been dropped. Move the re-validation out of timer_wait_running() and handle it at the only other usage site, timer_settime(). Signed-off-by: Thomas Gleixner <tglx@linutronix.de> --- V2: Simplify timer_wait_running() locking - PeterZ --- include/linux/posix-timers.h | 7 + kernel/signal.c | 2 kernel/time/posix-timers.c | 194 ++++++++++++++++++------------------------- 3 files changed, 90 insertions(+), 113 deletions(-) --- a/include/linux/posix-timers.h +++ b/include/linux/posix-timers.h @@ -240,6 +240,13 @@ static inline void posixtimer_sigqueue_p posixtimer_putref(tmr); } + +static inline bool posixtimer_valid(const struct k_itimer *timer) +{ + unsigned long val = (unsigned long)timer->it_signal; + + return !(val & 0x1UL); +} #else /* CONFIG_POSIX_TIMERS */ static inline void posixtimer_sigqueue_getref(struct sigqueue *q) { } static inline void posixtimer_sigqueue_putref(struct sigqueue *q) { } --- a/kernel/signal.c +++ b/kernel/signal.c @@ -2092,7 +2092,7 @@ static inline void posixtimer_sig_ignore * from a non-periodic timer, then just drop the reference * count. Otherwise queue it on the ignored list. */ - if (tmr->it_signal && tmr->it_sig_periodic) + if (posixtimer_valid(tmr) && tmr->it_sig_periodic) hlist_add_head(&tmr->ignored_list, &tsk->signal->ignored_posix_timers); else posixtimer_putref(tmr); --- a/kernel/time/posix-timers.c +++ b/kernel/time/posix-timers.c @@ -279,7 +279,7 @@ static bool __posixtimer_deliver_signal( * since the signal was queued. In either case, don't rearm and * drop the signal. */ - if (timr->it_signal_seq != timr->it_sigqueue_seq || WARN_ON_ONCE(!timr->it_signal)) + if (timr->it_signal_seq != timr->it_sigqueue_seq || !posixtimer_valid(timr)) return false; if (!timr->it_interval || WARN_ON_ONCE(timr->it_status != POSIX_TIMER_REQUEUE_PENDING)) @@ -324,6 +324,9 @@ void posix_timer_queue_signal(struct k_i { lockdep_assert_held(&timr->it_lock); + if (!posixtimer_valid(timr)) + return; + timr->it_status = timr->it_interval ? POSIX_TIMER_REQUEUE_PENDING : POSIX_TIMER_DISARMED; posixtimer_send_sigqueue(timr); } @@ -553,11 +556,11 @@ static struct k_itimer *__lock_timer(tim * The hash lookup and the timers are RCU protected. * * Timers are added to the hash in invalid state where - * timr::it_signal == NULL. timer::it_signal is only set after the - * rest of the initialization succeeded. + * timr::it_signal is marked invalid. timer::it_signal is only set + * after the rest of the initialization succeeded. * * Timer destruction happens in steps: - * 1) Set timr::it_signal to NULL with timr::it_lock held + * 1) Set timr::it_signal marked invalid with timr::it_lock held * 2) Release timr::it_lock * 3) Remove from the hash under hash_lock * 4) Put the reference count. @@ -574,8 +577,8 @@ static struct k_itimer *__lock_timer(tim * * The lookup validates locklessly that timr::it_signal == * current::it_signal and timr::it_id == @timer_id. timr::it_id - * can't change, but timr::it_signal becomes NULL during - * destruction. + * can't change, but timr::it_signal can become invalid during + * destruction, which makes the locked check fail. */ guard(rcu)(); timr = posix_timer_by_id(timer_id); @@ -811,22 +814,13 @@ static void common_timer_wait_running(st * when the task which tries to delete or disarm the timer has preempted * the task which runs the expiry in task work context. */ -static struct k_itimer *timer_wait_running(struct k_itimer *timer) +static void timer_wait_running(struct k_itimer *timer) { - timer_t timer_id = READ_ONCE(timer->it_id); - - /* Prevent kfree(timer) after dropping the lock */ - scoped_guard (rcu) { - unlock_timer(timer); - /* - * kc->timer_wait_running() might drop RCU lock. So @timer - * cannot be touched anymore after the function returns! - */ - timer->kclock->timer_wait_running(timer); - } - - /* Relock the timer. It might be not longer hashed. */ - return lock_timer(timer_id); + /* + * kc->timer_wait_running() might drop RCU lock. So @timer + * cannot be touched anymore after the function returns! + */ + timer->kclock->timer_wait_running(timer); } /* @@ -885,8 +879,7 @@ static int do_timer_settime(timer_t time struct itimerspec64 *new_spec64, struct itimerspec64 *old_spec64) { - struct k_itimer *timr; - int error; + int ret; if (!timespec64_valid(&new_spec64->it_interval) || !timespec64_valid(&new_spec64->it_value)) @@ -895,29 +888,36 @@ static int do_timer_settime(timer_t time if (old_spec64) memset(old_spec64, 0, sizeof(*old_spec64)); - timr = lock_timer(timer_id); -retry: - if (!timr) - return -EINVAL; + for (;;) { + struct k_itimer *timr = lock_timer(timer_id); - if (old_spec64) - old_spec64->it_interval = ktime_to_timespec64(timr->it_interval); + if (!timr) + return -EINVAL; - /* Prevent signal delivery and rearming. */ - timr->it_signal_seq++; + if (old_spec64) + old_spec64->it_interval = ktime_to_timespec64(timr->it_interval); - error = timr->kclock->timer_set(timr, tmr_flags, new_spec64, old_spec64); + /* Prevent signal delivery and rearming. */ + timr->it_signal_seq++; + + ret = timr->kclock->timer_set(timr, tmr_flags, new_spec64, old_spec64); + if (ret != TIMER_RETRY) { + unlock_timer(timr); + break; + } - if (error == TIMER_RETRY) { - // We already got the old time... + /* Read the old time only once */ old_spec64 = NULL; - /* Unlocks and relocks the timer if it still exists */ - timr = timer_wait_running(timr); - goto retry; + /* Protect the timer from being freed after the lock is dropped */ + guard(rcu)(); + unlock_timer(timr); + /* + * timer_wait_running() might drop RCU read side protection + * so the timer has to be looked up again! + */ + timer_wait_running(timr); } - unlock_timer(timr); - - return error; + return ret; } /* Set a POSIX.1b interval timer */ @@ -988,90 +988,56 @@ static inline void posix_timer_cleanup_i } } -/* Delete a POSIX.1b interval timer. */ -SYSCALL_DEFINE1(timer_delete, timer_t, timer_id) +static void posix_timer_delete(struct k_itimer *timer) { - struct k_itimer *timer = lock_timer(timer_id); - -retry_delete: - if (!timer) - return -EINVAL; - - /* Prevent signal delivery and rearming. */ + /* + * Invalidate the timer, remove it from the linked list and remove + * it from the ignored list if pending. + * + * The invalidation must be written with siglock held so that the + * signal code observes timer->it_valid == false in do_sigaction(), + * which prevents it from moving a pending signal of a deleted + * timer to the ignore list. + * + * The invalidation also prevents signal queueing, signal delivery + * and therefore rearming from the signal delivery path. + * + * A concurrent lookup can still find the timer in the hash, but it + * will check timer::it_signal with timer::it_lock held and observe + * bit 0 set, which invalidates it. That also prevents the timer ID + * from being handed out before this timer is completely gone. + */ timer->it_signal_seq++; - if (unlikely(timer->kclock->timer_del(timer) == TIMER_RETRY)) { - /* Unlocks and relocks the timer if it still exists */ - timer = timer_wait_running(timer); - goto retry_delete; - } - scoped_guard (spinlock, &current->sighand->siglock) { + unsigned long sig = (unsigned long)timer->it_signal | 1UL; + + WRITE_ONCE(timer->it_signal, (struct signal_struct *)sig); hlist_del(&timer->list); posix_timer_cleanup_ignored(timer); - /* - * A concurrent lookup could check timer::it_signal lockless. It - * will reevaluate with timer::it_lock held and observe the NULL. - * - * It must be written with siglock held so that the signal code - * observes timer->it_signal == NULL in do_sigaction(SIG_IGN), - * which prevents it from moving a pending signal of a deleted - * timer to the ignore list. - */ - WRITE_ONCE(timer->it_signal, NULL); } - unlock_timer(timer); - posix_timer_unhash_and_free(timer); - return 0; + while (timer->kclock->timer_del(timer) == TIMER_RETRY) { + guard(rcu)(); + spin_unlock_irq(&timer->it_lock); + timer_wait_running(timer); + spin_lock_irq(&timer->it_lock); + } } -/* - * 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) +/* Delete a POSIX.1b interval timer. */ +SYSCALL_DEFINE1(timer_delete, timer_t, timer_id) { - spin_lock_irq(&timer->it_lock); - -retry_delete: - /* - * 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->kclock->timer_del(timer) == TIMER_RETRY) { - /* - * 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) != timer)) - return; - - goto retry_delete; - } - hlist_del(&timer->list); - - posix_timer_cleanup_ignored(timer); + struct k_itimer *timer = lock_timer(timer_id); - /* - * Setting timer::it_signal to NULL is technically not required - * here as nothing can access the timer anymore legitimately via - * the hash table. Set it to NULL nevertheless so that all deletion - * paths are consistent. - */ - WRITE_ONCE(timer->it_signal, NULL); + if (!timer) + return -EINVAL; - spin_unlock_irq(&timer->it_lock); + posix_timer_delete(timer); + unlock_timer(timer); + /* Remove it from the hash, which frees up the timer ID */ posix_timer_unhash_and_free(timer); + return 0; } /* @@ -1082,6 +1048,8 @@ static void itimer_delete(struct k_itime void exit_itimers(struct task_struct *tsk) { struct hlist_head timers; + struct hlist_node *next; + struct k_itimer *timer; if (hlist_empty(&tsk->signal->posix_timers)) return; @@ -1091,8 +1059,10 @@ void exit_itimers(struct task_struct *ts hlist_move_list(&tsk->signal->posix_timers, &timers); /* The timers are not longer accessible via tsk::signal */ - while (!hlist_empty(&timers)) { - itimer_delete(hlist_entry(timers.first, struct k_itimer, list)); + hlist_for_each_entry_safe(timer, next, &timers, list) { + scoped_guard (spinlock_irq, &timer->it_lock) + posix_timer_delete(timer); + posix_timer_unhash_and_free(timer); cond_resched(); } </pre></td><td width="32" rowspan="2" class="c" valign="top"><img src="/images/icornerr.gif" width="32" height="32" alt="\" /></td></tr><tr><td align="right" valign="bottom"> 聽 </td></tr><tr><td align="right" valign="bottom">聽</td><td class="c" valign="bottom" style="padding-bottom: 0px"><img src="/images/bcornerl.gif" width="32" height="32" alt="\" /></td><td class="c">聽</td><td class="c" valign="bottom" style="padding-bottom: 0px"><img src="/images/bcornerr.gif" width="32" height="32" alt="/" /></td></tr><tr><td align="right" valign="top" colspan="2"> 聽 </td><td class="lm">Last update: 2025-03-08 17:51 聽聽 [W:1.386 / U:1.180 seconds] 漏2003-2020 <a href="http://blog.jasper.es/">Jasper Spaans</a>|hosted at <a href="https://www.digitalocean.com/?refcode=9a8e99d24cf9">Digital Ocean</a> and my Meterkast|<a href="http://blog.jasper.es/categories.html#lkml-ref">Read the blog</a></td><td>聽</td></tr></table><script language="javascript" src="/js/styleswitcher.js" type="text/javascript"></script></body></html>