All of lore.kernel.org
 help / color / mirror / Atom feed
From: Jiri Olsa <jolsa@redhat.com>
To: Peter Zijlstra <peterz@infradead.org>
Cc: "Liang, Kan" <kan.liang@intel.com>,
	Andi Kleen <andi@firstfloor.org>, Vince Weaver <vince@deater.net>,
	lkml <linux-kernel@vger.kernel.org>,
	Ingo Molnar <mingo@kernel.org>,
	Thomas Gleixner <tglx@linutronix.de>
Subject: [PATCH] perf/x86/uncore: Allow single pmu/box within events group
Date: Fri, 18 Nov 2016 01:15:28 +0100	[thread overview]
Message-ID: <20161118001528.GA28231@krava> (raw)

hi,
I was regularly hitting bug in uncore_validate_group when running
fuzzer. Attached patch makes it go away.

The problem is that in uncore_validate_group we could access uncore
boxes regs data with event's regs data which was initialized for
another box. Attached patch makes the check fail when it detects
wrong box.

Before I realized that the generic code won't allow for more pmu/box
contexts within events group, I reworked uncore_validate_group logic
to allow that, it's in here:
  https://git.kernel.org/cgit/linux/kernel/git/jolsa/perf.git/log/?h=perf/uncore_fix

I'm not sure wether it's useful to allow different uncore boxes
within events group, but it should not be problem to enable it.

Also I still don't understand how can we touch group sibling events
within event_init callback.. I must be missing something, because
fuzzer does not complain about this ;-)

thanks,
jirka


---
Current uncore_validate_group code expects all events within
the group to have same pmu.

This leads to constraint code using wrong boxes which leads
in my case to touching uninitialized spinlocks, but could
be probably worse.. depends on type and box details.

I get lockdep warning below for following perf stat:
  # perf stat -vv -e '{uncore_cbox_0/config=0x0334/,uncore_qpi_0/event=1/}' -a sleep 1

INFO: trying to register non-static key.
the code is fine but needs lockdep annotation.
turning off the locking correctness validator.
CPU: 12 PID: 3727 Comm: perf_fuzzer Tainted: G        W       4.9.0-rc1+ #10
Hardware name: IBM System x3650 M4 : -[7915E2G]-/00Y7683, BIOS -[VVE124AUS-1.30]- 11/21/2012
 ffffc90002ca3ae0 ffffffff81399565 0000000000000000 0000000000000000
 ffffc90002ca3b50 ffffffff810d0a3a 0000000000000006 ffff880276285eb8
 ffffc90002ca3bc8 ffff8802711a8b98 ffff8802711a8000 ffff8802711a8b98
Call Trace:
 [<ffffffff81399565>] dump_stack+0x68/0x93
 [<ffffffff810d0a3a>] register_lock_class+0x50a/0x510
 [<ffffffff810d2d78>] __lock_acquire+0x88/0x12a0
 [<ffffffff811a2cfc>] ? ___perf_sw_event+0x16c/0x280
 [<ffffffff810f091d>] ? debug_lockdep_rcu_enabled+0x1d/0x20
 [<ffffffff810d43b9>] lock_acquire+0xe9/0x1d0
 [<ffffffffa0186679>] ? uncore_get_constraint+0x59/0x100 [intel_uncore]
 [<ffffffff817075e3>] _raw_spin_lock_irqsave+0x43/0x60
 [<ffffffffa0186679>] ? uncore_get_constraint+0x59/0x100 [intel_uncore]
 [<ffffffffa0186679>] uncore_get_constraint+0x59/0x100 [intel_uncore]
 [<ffffffffa01853fd>] uncore_assign_events+0x6d/0x250 [intel_uncore]
 [<ffffffffa0185df7>] uncore_pmu_event_init+0x187/0x220 [intel_uncore]
 [<ffffffff81197443>] perf_try_init_event+0x43/0x90
 [<ffffffff8119b65e>] perf_event_alloc+0x55e/0xc50
 [<ffffffff8119b568>] ? perf_event_alloc+0x468/0xc50
 [<ffffffff8119f7db>] SYSC_perf_event_open+0x40b/0xf90
 [<ffffffff811a2f69>] SyS_perf_event_open+0x9/0x10
 [<ffffffff81002eb6>] do_syscall_64+0x66/0x1d0
 [<ffffffff81707f24>] entry_SYSCALL64_slow_path+0x25/0x25

The reason is that leader event will get extra reg configuration
which touches regs of second event's box.

This patch makes the uncore_validate_group check fail when it detects
wrong box.

Signed-off-by: Jiri Olsa <jolsa@redhat.com>
---
diff --git a/arch/x86/events/intel/uncore.c b/arch/x86/events/intel/uncore.c
index efca2685d876..a81b5a39b79e 100644
--- a/arch/x86/events/intel/uncore.c
+++ b/arch/x86/events/intel/uncore.c
@@ -597,6 +597,10 @@ static int uncore_validate_group(struct intel_uncore_pmu *pmu,
 	struct intel_uncore_box *fake_box;
 	int ret = -EINVAL, n;
 
+	/* All events in group must be from the same uncore box. */
+	if (leader->pmu->type != pmu->pmu.type)
+		return -EINVAL;
+
 	fake_box = uncore_alloc_box(pmu->type, NUMA_NO_NODE);
 	if (!fake_box)
 		return -ENOMEM;

             reply	other threads:[~2016-11-18  0:15 UTC|newest]

Thread overview: 10+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2016-11-18  0:15 Jiri Olsa [this message]
2016-11-18 11:28 ` [PATCH] perf/x86/uncore: Allow single pmu/box within events group Peter Zijlstra
2016-11-18 12:33   ` Jiri Olsa
2016-11-18 12:53     ` Peter Zijlstra
2016-11-18 13:23       ` Jiri Olsa
2016-11-18 13:48       ` Jiri Olsa
2016-11-18 14:06         ` Peter Zijlstra
2016-11-18 14:30           ` Jiri Olsa
2016-11-18 14:45             ` Peter Zijlstra
2016-11-22 12:30       ` [tip:perf/urgent] perf/x86/intel/uncore: Allow only a single PMU/box within an " tip-bot for Peter Zijlstra

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20161118001528.GA28231@krava \
    --to=jolsa@redhat.com \
    --cc=andi@firstfloor.org \
    --cc=kan.liang@intel.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=mingo@kernel.org \
    --cc=peterz@infradead.org \
    --cc=tglx@linutronix.de \
    --cc=vince@deater.net \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.