linux-kernel - Re: [PATCH 8/9] hrtimer: Allow hrtimer::function() to free the 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] [thread-next>] [day] [month] [year] [list]

Message-ID: <20150603211324.GC3644@twins.programming.kicks-ass.net>
Date:	Wed, 3 Jun 2015 23:13:24 +0200
From:	Peter Zijlstra <peterz@...radead.org>
To:	Kirill Tkhai <tkhai@...dex.ru>
Cc:	"umgwanakikbuti@...il.com" <umgwanakikbuti@...il.com>,
	"mingo@...e.hu" <mingo@...e.hu>,
	"ktkhai@...allels.com" <ktkhai@...allels.com>,
	"rostedt@...dmis.org" <rostedt@...dmis.org>,
	"tglx@...utronix.de" <tglx@...utronix.de>,
	"juri.lelli@...il.com" <juri.lelli@...il.com>,
	"pang.xunlei@...aro.org" <pang.xunlei@...aro.org>,
	"oleg@...hat.com" <oleg@...hat.com>,
	"wanpeng.li@...ux.intel.com" <wanpeng.li@...ux.intel.com>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 8/9] hrtimer: Allow hrtimer::function() to free the timer

On Wed, Jun 03, 2015 at 07:26:00PM +0300, Kirill Tkhai wrote:
> > @@ -402,7 +394,8 @@ extern u64 hrtimer_get_next_event(void);
> >   */
> >  static inline int hrtimer_active(const struct hrtimer *timer)
> >  {
> > - return timer->state != HRTIMER_STATE_INACTIVE;
> > + return timer->state != HRTIMER_STATE_INACTIVE ||
> > + timer->base->running == timer;
> >  }
> 
> It seems to be not good, because hrtimer_active() check stops
> to be atomic. So the things like hrtimer_try_to_cancel() race
> with a callback of self-rearming timer and may return a false
> positive result.

Hurm.. the below isn't really pretty either I suppose. The best I can
say about it is that's its not too expensive on x86.

I should probably go sleep..

--- a/include/linux/hrtimer.h
+++ b/include/linux/hrtimer.h
@@ -391,11 +391,25 @@ extern u64 hrtimer_get_next_event(void);
  * A timer is active, when it is enqueued into the rbtree or the
  * callback function is running or it's in the state of being migrated
  * to another cpu.
+ *
+ * See __run_hrtimer().
  */
-static inline int hrtimer_active(const struct hrtimer *timer)
+static inline bool hrtimer_active(const struct hrtimer *timer)
 {
-	return timer->state != HRTIMER_STATE_INACTIVE ||
-		timer->base->running == timer;
+	if (timer->state != HRTIMER_STATE_INACTIVE)
+		return true;
+
+	smp_rmb(); /* C matches A */
+
+	if (timer->base->running == timer)
+		return true;
+
+	smp_rmb(); /* D matches B */
+
+	if (timer->state != HRTIMER_STATE_INACTIVE)
+		return true;
+
+	return false;
 }
 
 /*
--- a/kernel/time/hrtimer.c
+++ b/kernel/time/hrtimer.c
@@ -1122,6 +1122,20 @@ static void __run_hrtimer(struct hrtimer
 
 	debug_deactivate(timer);
 	base->running = timer;
+
+	/*
+	 * Pairs with hrtimer_active().
+	 *
+	 *	[S] base->running = timer	[L] timer->state
+	 *	    WMB 			    RMB
+	 *	[S] timer->state = INACTIVE	[L] base->running
+	 *
+	 * BUG_ON(base->running != timer && timer->state != INACTIVE)
+	 *
+	 * If we observe INACTIVE we must observe base->running == timer.
+	 */
+	smp_wmb(); /* A matches C */
+
 	__remove_hrtimer(timer, base, HRTIMER_STATE_INACTIVE, 0);
 	timer_stats_account_hrtimer(timer);
 	fn = timer->function;
@@ -1150,6 +1164,20 @@ static void __run_hrtimer(struct hrtimer
 	    !(timer->state & HRTIMER_STATE_ENQUEUED))
 		enqueue_hrtimer(timer, base);
 
+	/*
+	 * Pairs with hrtimer_active().
+	 *
+	 *	[S] timer->state = ENQUEUED	[L] base->running
+	 *	    WMB 			    RMB
+	 *	[S] base->running = NULL	[L] timer->state
+	 *
+	 * BUG_ON(base->running == NULL && timer->state == INACTIVE)
+	 *
+	 * If we observe base->running == NULL, we must observe any preceding
+	 * enqueue.
+	 */
+	smp_wmb(); /* B matches D */
+
 	WARN_ON_ONCE(base->running != timer);
 	base->running = NULL;
 }
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/