| 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
| ||
|
Message-ID: <CABPqkBQkgSeRMtP9F61ckCaikFd_rUTQvF1NxB_DDQRugRRv3g@mail.gmail.com>
Date: Wed, 6 Jun 2012 12:35:12 +0200
From: Stephane Eranian <eranian@...gle.com>
To: linux-kernel@...r.kernel.org
Cc: peterz@...radead.org, zheng.z.yan@...ux.intel.com
Subject: Re: [PATCH] perf, x86: Fix Intel shared extra MSR allocation
Ok, I found the problem. It was in intel_fixup_er().
Unlike in the original code, this routine must update
the event->extra_reg.idx to the idx parameter instead
of trying to swap out from it.
I will repost an updated version (v2).
On Tue, Jun 5, 2012 at 11:35 PM, Stephane Eranian <eranian@...gle.com> wrote:
>
> Zheng Yan reported that event group validation can wreck event state
> when Intel extra_reg allocation changes event state.
>
> Validation shouldn't change any persistent state. Cloning events in
> validate_{event,group}() isn't really pretty either, so add a few
> special cases to avoid modifying the event state.
>
> The code is restructured to minimize the special case impact.
>
> Reported-by: Zheng Yan <zheng.z.yan@...ux.intel.com>
> Cc: Stephane Eranian <eranian@...gle.com>
> Signed-off-by: Peter Zijlstra <a.p.zijlstra@...llo.nl>
> Acked-by: Stephane Eranian <eranian@...gle.com>
> ---
>
> diff --git a/arch/x86/kernel/cpu/perf_event.c b/arch/x86/kernel/cpu/perf_event.c
> index e049d6d..cb60838 100644
> --- a/arch/x86/kernel/cpu/perf_event.c
> +++ b/arch/x86/kernel/cpu/perf_event.c
> @@ -1496,6 +1496,7 @@ static struct cpu_hw_events *allocate_fake_cpuc(void)
> if (!cpuc->shared_regs)
> goto error;
> }
> + cpuc->is_fake = 1;
> return cpuc;
> error:
> free_fake_cpuc(cpuc);
> diff --git a/arch/x86/kernel/cpu/perf_event.h b/arch/x86/kernel/cpu/perf_event.h
> index 6638aaf..83794d8 100644
> --- a/arch/x86/kernel/cpu/perf_event.h
> +++ b/arch/x86/kernel/cpu/perf_event.h
> @@ -117,6 +117,7 @@ struct cpu_hw_events {
> struct perf_event *event_list[X86_PMC_IDX_MAX]; /* in enabled order */
>
> unsigned int group_flag;
> + int is_fake;
>
> /*
> * Intel DebugStore bits
> diff --git a/arch/x86/kernel/cpu/perf_event_intel.c b/arch/x86/kernel/cpu/perf_event_intel.c
> index 166546e..76a2bd2 100644
> --- a/arch/x86/kernel/cpu/perf_event_intel.c
> +++ b/arch/x86/kernel/cpu/perf_event_intel.c
> @@ -1119,27 +1119,33 @@ intel_bts_constraints(struct perf_event *event)
> return NULL;
> }
>
> -static bool intel_try_alt_er(struct perf_event *event, int orig_idx)
> +static int intel_alt_er(int idx)
> {
> if (!(x86_pmu.er_flags & ERF_HAS_RSP_1))
> - return false;
> + return idx;
>
> - if (event->hw.extra_reg.idx == EXTRA_REG_RSP_0) {
> + if (idx == EXTRA_REG_RSP_0)
> + return EXTRA_REG_RSP_1;
> +
> + if (idx == EXTRA_REG_RSP_1)
> + return EXTRA_REG_RSP_0;
> +
> + return idx;
> +}
> +
> +static void intel_fixup_er(struct perf_event *event, int idx)
> +{
> + if (idx == EXTRA_REG_RSP_0) {
> event->hw.config &= ~INTEL_ARCH_EVENT_MASK;
> event->hw.config |= 0x01bb;
> event->hw.extra_reg.idx = EXTRA_REG_RSP_1;
> event->hw.extra_reg.reg = MSR_OFFCORE_RSP_1;
> - } else if (event->hw.extra_reg.idx == EXTRA_REG_RSP_1) {
> + } else if (idx == EXTRA_REG_RSP_1) {
> event->hw.config &= ~INTEL_ARCH_EVENT_MASK;
> event->hw.config |= 0x01b7;
> event->hw.extra_reg.idx = EXTRA_REG_RSP_0;
> event->hw.extra_reg.reg = MSR_OFFCORE_RSP_0;
> }
> -
> - if (event->hw.extra_reg.idx == orig_idx)
> - return false;
> -
> - return true;
> }
>
> /*
> @@ -1157,14 +1163,19 @@ __intel_shared_reg_get_constraints(struct cpu_hw_events *cpuc,
> struct event_constraint *c = &emptyconstraint;
> struct er_account *era;
> unsigned long flags;
> - int orig_idx = reg->idx;
> + int idx = reg->idx;
>
> - /* already allocated shared msr */
> - if (reg->alloc)
> - return NULL; /* call x86_get_event_constraint() */
> + /*
> + * reg->alloc can be set due to existing state, so for fake cpuc
> + * we need to ignore this, otherwise we might fail to allocate
> + * proper fake state for this extra reg constraint. Also see
> + * the comment below.
> + */
> + if (reg->alloc && !cpuc->is_fake)
> + return NULL; /* call x86_get_event_constraints() */
>
> again:
> - era = &cpuc->shared_regs->regs[reg->idx];
> + era = &cpuc->shared_regs->regs[idx];
> /*
> * we use spin_lock_irqsave() to avoid lockdep issues when
> * passing a fake cpuc
> @@ -1172,6 +1183,27 @@ again:
> raw_spin_lock_irqsave(&era->lock, flags);
>
> if (!atomic_read(&era->ref) || era->config == reg->config) {
> + /*
> + * If its a fake cpuc -- as per validate_{group,event}() we
> + * shouldn't touch event state and we can avoid doing so
> + * since both will only call get_event_constraints() once
> + * on each event, this avoids the need for reg->alloc.
> + *
> + * Not doing the ER fixup will only result in era->reg being
> + * wrong, but since we won't actually try and program hardware
> + * this isn't a problem either.
> + */
> + if (!cpuc->is_fake) {
> + if (idx != reg->idx)
> + intel_fixup_er(event, idx);
> + /*
> + * x86_schedule_events() calls get_event_constraints()
> + * multiple times on events in the case of incremental
> + * scheduling(). reg->alloc ensures we only do the ER
> + * allocation once.
> + */
> + reg->alloc = 1;
> + }
>
> /* lock in msr value */
> era->config = reg->config;
> @@ -1180,18 +1212,19 @@ again:
> /* one more user */
> atomic_inc(&era->ref);
>
> - /* no need to reallocate during incremental event scheduling */
> - reg->alloc = 1;
> -
> /*
> * need to call x86_get_event_constraint()
> * to check if associated event has constraints
> */
> c = NULL;
> - } else if (intel_try_alt_er(event, orig_idx)) {
> - raw_spin_unlock_irqrestore(&era->lock, flags);
> - goto again;
> + } else {
> + idx = intel_alt_er(idx);
> + if (idx != reg->idx) {
> + raw_spin_unlock_irqrestore(&era->lock, flags);
> + goto again;
> + }
> }
> +
> raw_spin_unlock_irqrestore(&era->lock, flags);
>
> return c;
> @@ -1204,11 +1237,14 @@ __intel_shared_reg_put_constraints(struct cpu_hw_events *cpuc,
> struct er_account *era;
>
> /*
> - * only put constraint if extra reg was actually
> - * allocated. Also takes care of event which do
> - * not use an extra shared reg
> + * Only put constraint if extra reg was actually allocated. Also takes
> + * care of event which do not use an extra shared reg.
> + *
> + * Also, if this is a fake cpuc we shouldn't touch any event state
> + * (reg->alloc) and we don't care about leaving inconsistent cpuc state
> + * either since it'll be thrown out.
> */
> - if (!reg->alloc)
> + if (!reg->alloc || cpuc->is_fake)
> return;
>
> era = &cpuc->shared_regs->regs[reg->idx];
--
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