| 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: Tue, 23 Aug 2016 11:54:32 +0300
From: Alexander Shishkin <alexander.shishkin@...ux.intel.com>
To: Peter Zijlstra <a.p.zijlstra@...llo.nl>
Cc: Ingo Molnar <mingo@...hat.com>, linux-kernel@...r.kernel.org,
vince@...ter.net, eranian@...gle.com,
Arnaldo Carvalho de Melo <acme@...radead.org>,
Alexander Shishkin <alexander.shishkin@...ux.intel.com>
Subject: [PATCH 1/3] perf/x86/intel/bts: Fix confused ordering of PMU callbacks
The intel_bts driver is using a cpu-local 'started' variable to order
callbacks and PMIs and make sure that AUX transactions don't get messed
up. However, the ordering rules in regard to this variable is a complete
mess, which recently resulted in perf_fuzzer-triggered warnings and
panics.
The general ordering rule that is patch is enforcing is that this
cpu-local variable be set only when the cpu-local AUX transaction is
active; consequently, this variable is to be checked before the AUX
related bits can be touched.
Signed-off-by: Alexander Shishkin <alexander.shishkin@...ux.intel.com>
---
arch/x86/events/intel/bts.c | 85 ++++++++++++++++++++++++++++++++++-----------
1 file changed, 64 insertions(+), 21 deletions(-)
diff --git a/arch/x86/events/intel/bts.c b/arch/x86/events/intel/bts.c
index 0a6e393a2e..0b8ccff564 100644
--- a/arch/x86/events/intel/bts.c
+++ b/arch/x86/events/intel/bts.c
@@ -204,6 +204,16 @@ static void bts_update(struct bts_ctx *bts)
static int
bts_buffer_reset(struct bts_buffer *buf, struct perf_output_handle *handle);
+/*
+ * Ordering PMU callbacks wrt themselves and the PMI is done by means
+ * of bts::started, which:
+ * - is set when bts::handle::event is valid, that is, between
+ * perf_aux_output_begin() and perf_aux_output_end();
+ * - is zero otherwise;
+ * - is ordered against bts::handle::event with a local barrier to
+ * enforce CPU ordering.
+ */
+
static void __bts_event_start(struct perf_event *event)
{
struct bts_ctx *bts = this_cpu_ptr(&bts_ctx);
@@ -221,10 +231,13 @@ static void __bts_event_start(struct perf_event *event)
/*
* local barrier to make sure that ds configuration made it
- * before we enable BTS
+ * before we enable BTS and bts::started is set
*/
wmb();
+ /* PMI handler: this counter is running and likely generating PMIs */
+ WRITE_ONCE(bts->started, 1);
+
intel_pmu_enable_bts(config);
}
@@ -251,9 +264,6 @@ static void bts_event_start(struct perf_event *event, int flags)
__bts_event_start(event);
- /* PMI handler: this counter is running and likely generating PMIs */
- ACCESS_ONCE(bts->started) = 1;
-
return;
fail_end_stop:
@@ -265,28 +275,34 @@ fail_stop:
static void __bts_event_stop(struct perf_event *event)
{
+ struct bts_ctx *bts = this_cpu_ptr(&bts_ctx);
+
+ /* PMI handler: don't restart this counter */
+ WRITE_ONCE(bts->started, 0);
+
/*
* No extra synchronization is mandated by the documentation to have
* BTS data stores globally visible.
*/
intel_pmu_disable_bts();
-
- if (event->hw.state & PERF_HES_STOPPED)
- return;
-
- ACCESS_ONCE(event->hw.state) |= PERF_HES_STOPPED;
}
static void bts_event_stop(struct perf_event *event, int flags)
{
struct cpu_hw_events *cpuc = this_cpu_ptr(&cpu_hw_events);
struct bts_ctx *bts = this_cpu_ptr(&bts_ctx);
- struct bts_buffer *buf = perf_get_aux(&bts->handle);
+ struct bts_buffer *buf;
- /* PMI handler: don't restart this counter */
- ACCESS_ONCE(bts->started) = 0;
+ if (READ_ONCE(bts->started)) {
+ __bts_event_stop(event);
+
+ /* order buf (handle::event load) against bts::started store */
+ mb();
+ }
+
+ buf = perf_get_aux(&bts->handle);
- __bts_event_stop(event);
+ event->hw.state |= PERF_HES_STOPPED;
if (flags & PERF_EF_UPDATE) {
bts_update(bts);
@@ -311,7 +327,13 @@ void intel_bts_enable_local(void)
{
struct bts_ctx *bts = this_cpu_ptr(&bts_ctx);
- if (bts->handle.event && bts->started)
+ if (!READ_ONCE(bts->started))
+ return;
+
+ /* matches wmb() ordering bts::started store against handle::event */
+ rmb();
+
+ if (bts->handle.event)
__bts_event_start(bts->handle.event);
}
@@ -319,6 +341,12 @@ void intel_bts_disable_local(void)
{
struct bts_ctx *bts = this_cpu_ptr(&bts_ctx);
+ if (!READ_ONCE(bts->started))
+ return;
+
+ /* matches wmb() ordering bts::started store against handle::event */
+ rmb();
+
if (bts->handle.event)
__bts_event_stop(bts->handle.event);
}
@@ -407,9 +435,15 @@ int intel_bts_interrupt(void)
struct perf_event *event = bts->handle.event;
struct bts_buffer *buf;
s64 old_head;
- int err;
+ int err = -ENOSPC;
- if (!event || !bts->started)
+ if (!READ_ONCE(bts->started))
+ return 0;
+
+ /* matches wmb() ordering bts::started store against handle::event */
+ rmb();
+
+ if (!event)
return 0;
buf = perf_get_aux(&bts->handle);
@@ -432,12 +466,21 @@ int intel_bts_interrupt(void)
!!local_xchg(&buf->lost, 0));
buf = perf_aux_output_begin(&bts->handle, event);
- if (!buf)
- return 1;
+ if (buf)
+ err = bts_buffer_reset(buf, &bts->handle);
- err = bts_buffer_reset(buf, &bts->handle);
- if (err)
- perf_aux_output_end(&bts->handle, 0, false);
+ if (err) {
+ WRITE_ONCE(bts->started, 0);
+
+ if (buf) {
+ /*
+ * cleared bts::started should be visible before
+ * cleared handle::event
+ */
+ wmb();
+ perf_aux_output_end(&bts->handle, 0, false);
+ }
+ }
return 1;
}
--
2.8.1
Powered by blists - more mailing lists