| 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, 12 Sep 2012 18:42:30 +0200
From: Stephane Eranian <eranian@...gle.com>
To: Peter Zijlstra <peterz@...radead.org>
Cc: Sebastian Andrzej Siewior <bigeasy@...utronix.de>,
Oleg Nesterov <oleg@...hat.com>,
linux-kernel <linux-kernel@...r.kernel.org>,
Ingo Molnar <mingo@...nel.org>
Subject: Re: [RFC][PATCH] perf, intel: Don't touch MSR_IA32_DEBUGCTLMSR from
NMI context
On Wed, Sep 12, 2012 at 6:22 PM, Peter Zijlstra <peterz@...radead.org> wrote:
> Oleg and Sebastian found that touching MSR_IA32_DEBUGCTLMSR from NMI
> context is problematic since the only way to change the various
> unrelated bits in there is:
>
> debugctl = get_debugctlmsr()
> /* frob flags in debugctl */
> update_debugctlmsr(debugctl);
>
> Which is entirely unsafe if we prod at the MSR from NMI context.
>
> In particular the path that is responsible is:
>
> x86_pmu_handle_irq() (NMI handler)
> x86_pmu_stop()
> x86_pmu.disable -> intel_pmu_disable_event()
> intel_pmu_lbr_disable()
> __intel_pmu_lbr_disable()
> wrmsrl(MSR_IA32_DEBUGCTLMSR,... );
>
> So remove intel_pmu_lbr_{en,dis}able() from
> intel_pmu_{en,dis}able_event() and stick them in
> intel_{get,put}_event_constraints() which are only ever called from
> regular contexts.
>
Not sure this is going to work for LBR.
We use FREEZE_LBR_ON_PMI to sync LBR data with counter overflows.
That means, LBR is already frozen by the time we get to the handler. But
that means we need to re-enable LBR when we leave the handler. I don't
think EOI is going to magically re-enable it. So we need to touch DEBUGCTL
in the irq handler.
> We combine intel_pmu_needs_lbr_smpl(), which tells us if the events
> wants LBR data, with event->hw.branch_reg.alloc, which given
> intel_shared_regs_constraints() is set if our event got the shared
> resource, to give us a correctly balanced condition in
> {get,put}_event_constraints() for intel_pmu_lbr_{en,dis}able().
>
> Also change core_pmu to only use x86_get_event_constraints since it
> doesn't support any of the fancy DS (BTS,PEBS), LBR and OFFCORE features
> that make up intel_{get,put}_event_constraints.
>
> Signed-off-by: Peter Zijlstra <a.p.zijlstra@...llo.nl>
> ---
> arch/x86/kernel/cpu/perf_event_intel.c | 48 ++++++++++++++++++++--------------
> 1 file changed, 29 insertions(+), 19 deletions(-)
>
> diff --git a/arch/x86/kernel/cpu/perf_event_intel.c b/arch/x86/kernel/cpu/perf_event_intel.c
> index 0d3d63a..ef4cd36 100644
> --- a/arch/x86/kernel/cpu/perf_event_intel.c
> +++ b/arch/x86/kernel/cpu/perf_event_intel.c
> @@ -821,6 +821,11 @@ static inline bool intel_pmu_needs_lbr_smpl(struct perf_event *event)
> return false;
> }
>
> +static inline bool intel_pmu_has_lbr(struct perf_event *event)
> +{
> + return intel_pmu_needs_lbr_smpl(event) && event->hw.branch_reg.alloc;
> +}
> +
> static void intel_pmu_disable_all(void)
> {
> struct cpu_hw_events *cpuc = &__get_cpu_var(cpu_hw_events);
> @@ -975,13 +980,6 @@ static void intel_pmu_disable_event(struct perf_event *event)
> cpuc->intel_ctrl_guest_mask &= ~(1ull << hwc->idx);
> cpuc->intel_ctrl_host_mask &= ~(1ull << hwc->idx);
>
> - /*
> - * must disable before any actual event
> - * because any event may be combined with LBR
> - */
> - if (intel_pmu_needs_lbr_smpl(event))
> - intel_pmu_lbr_disable(event);
> -
> if (unlikely(hwc->config_base == MSR_ARCH_PERFMON_FIXED_CTR_CTRL)) {
> intel_pmu_disable_fixed(hwc);
> return;
> @@ -1036,12 +1034,6 @@ static void intel_pmu_enable_event(struct perf_event *event)
> intel_pmu_enable_bts(hwc->config);
> return;
> }
> - /*
> - * must enabled before any actual event
> - * because any event may be combined with LBR
> - */
> - if (intel_pmu_needs_lbr_smpl(event))
> - intel_pmu_lbr_enable(event);
>
> if (event->attr.exclude_host)
> cpuc->intel_ctrl_guest_mask |= (1ull << hwc->idx);
> @@ -1382,17 +1374,28 @@ intel_get_event_constraints(struct cpu_hw_events *cpuc, struct perf_event *event
>
> c = intel_bts_constraints(event);
> if (c)
> - return c;
> + goto got_constraint;
>
> c = intel_pebs_constraints(event);
> if (c)
> - return c;
> + goto got_constraint;
>
> c = intel_shared_regs_constraints(cpuc, event);
> if (c)
> - return c;
> + goto got_constraint;
> +
> + c = x86_get_event_constraints(cpuc, event);
> +
> +got_constraint:
>
> - return x86_get_event_constraints(cpuc, event);
> + /*
> + * Must enabled before any actual event because any event may be
> + * combined with LBR.
> + */
> + if (intel_pmu_has_lbr(event))
> + intel_pmu_lbr_enable(event);
> +
> + return c;
> }
>
> static void
> @@ -1413,6 +1416,14 @@ intel_put_shared_regs_event_constraints(struct cpu_hw_events *cpuc,
> static void intel_put_event_constraints(struct cpu_hw_events *cpuc,
> struct perf_event *event)
> {
> + /*
> + * Must disabled after any actual event because any event may be
> + * combined with LBR, but must be done before releasing the shared
> + * regs resource since that protects the LBR state.
> + */
> + if (intel_pmu_has_lbr(event))
> + intel_pmu_lbr_disable(event);
> +
> intel_put_shared_regs_event_constraints(cpuc, event);
> }
>
> @@ -1623,8 +1634,7 @@ static __initconst const struct x86_pmu core_pmu = {
> * the generic event period:
> */
> .max_period = (1ULL << 31) - 1,
> - .get_event_constraints = intel_get_event_constraints,
> - .put_event_constraints = intel_put_event_constraints,
> + .get_event_constraints = x86_get_event_constraints,
> .event_constraints = intel_core_event_constraints,
> .guest_get_msrs = core_guest_get_msrs,
> .format_attrs = intel_arch_formats_attr,
>
--
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