From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752897AbcKRAPf (ORCPT ); Thu, 17 Nov 2016 19:15:35 -0500 Received: from mx1.redhat.com ([209.132.183.28]:43080 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752123AbcKRAPd (ORCPT ); Thu, 17 Nov 2016 19:15:33 -0500 Date: Fri, 18 Nov 2016 01:15:28 +0100 From: Jiri Olsa To: Peter Zijlstra Cc: "Liang, Kan" , Andi Kleen , Vince Weaver , lkml , Ingo Molnar , Thomas Gleixner Subject: [PATCH] perf/x86/uncore: Allow single pmu/box within events group Message-ID: <20161118001528.GA28231@krava> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline User-Agent: Mutt/1.7.1 (2016-10-04) X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.31]); Fri, 18 Nov 2016 00:15:32 +0000 (UTC) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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: [] dump_stack+0x68/0x93 [] register_lock_class+0x50a/0x510 [] __lock_acquire+0x88/0x12a0 [] ? ___perf_sw_event+0x16c/0x280 [] ? debug_lockdep_rcu_enabled+0x1d/0x20 [] lock_acquire+0xe9/0x1d0 [] ? uncore_get_constraint+0x59/0x100 [intel_uncore] [] _raw_spin_lock_irqsave+0x43/0x60 [] ? uncore_get_constraint+0x59/0x100 [intel_uncore] [] uncore_get_constraint+0x59/0x100 [intel_uncore] [] uncore_assign_events+0x6d/0x250 [intel_uncore] [] uncore_pmu_event_init+0x187/0x220 [intel_uncore] [] perf_try_init_event+0x43/0x90 [] perf_event_alloc+0x55e/0xc50 [] ? perf_event_alloc+0x468/0xc50 [] SYSC_perf_event_open+0x40b/0xf90 [] SyS_perf_event_open+0x9/0x10 [] do_syscall_64+0x66/0x1d0 [] 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 --- 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;