| 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
| ||
|
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/
Powered by blists - more mailing lists