linux-kernel - Re: [PATCH] timers: fix synchronization rules in comments of del_timer

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] [day] [month] [year] [list]

Message-ID: <4a6d3b57.7607e.182801fb63f.Coremail.duoming@zju.edu.cn>
Date:   Tue, 9 Aug 2022 09:02:18 +0800 (GMT+08:00)
From:   duoming@....edu.cn
To:     "Thomas Gleixner" <tglx@...utronix.de>
Cc:     linux-kernel@...r.kernel.org, jstultz@...gle.com, sboyd@...nel.org,
        edumazet@...gle.com
Subject: Re: [PATCH] timers: fix synchronization rules in comments of
 del_timer_sync

Hello,

On Mon, 08 Aug 2022 16:01:57 +0200 Thomas Gleixner wrote:

> On Fri, Jul 01 2022 at 16:55, Duoming Zhou wrote:
> > - * Synchronization rules: Callers must prevent restarting of the timer,
> > - * otherwise this function is meaningless. It must not be called from
> > - * interrupt contexts unless the timer is an irqsafe one. The caller must
> > - * not hold locks which would prevent completion of the timer's
> > - * handler. The timer's handler must not call add_timer_on(). Upon exit the
> > - * timer is not queued and the handler is not running on any CPU.
> > + * Synchronization rules: Callers must prevent restarting of the timer in
> > + * other places except for the timer's handler, otherwise this function is
> > + * meaningless. It must not be called from interrupt contexts unless the
> > + * timer is an irqsafe one. The caller must not hold locks which would
> > + * prevent completion of the timer's handler. The timer's handler must
> > + * not call add_timer_on().
> 
> If we are making this correct: What's so special about add_timer_on()?

The del_timer_sync() could also stop add_timer_on(), there is nothing special
about add_timer_on(). I think change the annotation to the following is better.

diff --git a/kernel/time/timer.c b/kernel/time/timer.c
index 717fcb9fb14..dd623018dbc 100644
--- a/kernel/time/timer.c
+++ b/kernel/time/timer.c
@@ -1375,11 +1375,11 @@ static inline void del_timer_wait_running(struct timer_list *timer) { }
  * CPUs.
  *
  * Synchronization rules: Callers must prevent restarting of the timer,
- * otherwise this function is meaningless. It must not be called from
- * interrupt contexts unless the timer is an irqsafe one. The caller must
- * not hold locks which would prevent completion of the timer's
- * handler. The timer's handler must not call add_timer_on(). Upon exit the
- * timer is not queued and the handler is not running on any CPU.
+ * otherwise this function is meaningless. It could also stop periodic timer.
+ * It must not be called from interrupt contexts unless the timer is an irqsafe
+ * one. The caller must not hold locks which would prevent completion of the
+ * timer's handler. Upon exit the timer is not queued and the handler is not
+ * running on any CPU.

Best regards,
Duoming Zhou