Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753234AbbHDUuH (ORCPT ); Tue, 4 Aug 2015 16:50:07 -0400 Received: from mga11.intel.com ([192.55.52.93]:61685 "EHLO mga11.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752274AbbHDUuE (ORCPT ); Tue, 4 Aug 2015 16:50:04 -0400 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.15,611,1432623600"; d="scan'208";a="619291324" From: "Liang, Kan" To: Andy Lutomirski CC: Thomas Gleixner , "linux-kernel@vger.kernel.org" , "H. Peter Anvin" , "Andrew Lutomirski" , Linus Torvalds , Peter Zijlstra , "Ingo Molnar" , "linux-tip-commits@vger.kernel.org" Subject: RE: [tip:perf/core] perf/x86: Add an MSR PMU driver Thread-Topic: [tip:perf/core] perf/x86: Add an MSR PMU driver Thread-Index: AQHQzpQ65l/Dz0xJ8EeCrMA8u7d3Np37ZoGAgACIDlD//60vAIAAhnPA//99DICAAI7+8A== Date: Tue, 4 Aug 2015 20:39:27 +0000 Message-ID: <37D7C6CF3E00A74B8858931C1DB2F077018D2103@SHSMSX103.ccr.corp.intel.com> References: <1437407346-31186-1-git-send-email-kan.liang@intel.com> <37D7C6CF3E00A74B8858931C1DB2F077018D16F1@SHSMSX103.ccr.corp.intel.com> <37D7C6CF3E00A74B8858931C1DB2F077018D206A@SHSMSX103.ccr.corp.intel.com> In-Reply-To: Accept-Language: zh-CN, en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [10.239.127.40] Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Transfer-Encoding: 8bit X-MIME-Autoconverted: from base64 to 8bit by mail.home.local id t74KoNVh004088 Content-Length: 5173 Lines: 154 > On Tue, Aug 4, 2015 at 11:11 AM, Liang, Kan wrote: > > > > > >> > >> On Tue, Aug 4, 2015 at 8:03 AM, Liang, Kan > wrote: > >> > > >> >> > + > >> >> > +enum perf_msr_id { > >> >> > + PERF_MSR_TSC = 0, > >> >> > + PERF_MSR_APERF = 1, > >> >> > + PERF_MSR_MPERF = 2, > >> >> > + PERF_MSR_PPERF = 3, > >> >> > + PERF_MSR_SMI = 4, > >> >> > + > >> >> > + PERF_MSR_EVENT_MAX, > >> >> > +}; > >> >> > + > >> >> > +struct perf_msr { > >> >> > + int id; > >> >> > + u64 msr; > >> >> > +}; > >> >> > + > >> >> > +static struct perf_msr msr[] = { > >> >> > + { PERF_MSR_TSC, 0 }, > >> >> > + { PERF_MSR_APERF, MSR_IA32_APERF }, > >> >> > + { PERF_MSR_MPERF, MSR_IA32_MPERF }, > >> >> > + { PERF_MSR_PPERF, MSR_PPERF }, > >> >> > + { PERF_MSR_SMI, MSR_SMI_COUNT }, }; > >> >> > >> >> I think this could be easier to work with if it were > >> >> [PERF_MSR_TSC] = {...}, etc. No big deal, though, until the list > >> >> gets long. However, it might make fixing the apparent issue below > easier... > >> >> > >> >> > +static int msr_event_init(struct perf_event *event) { > >> >> > + u64 cfg = event->attr.config; > >> >> > >> >> ... > >> >> > >> >> > + event->hw.event_base = msr[cfg].msr; > >> >> > >> >> Shouldn't this verify that the fancy enumeration code actually > >> >> believes that msr[cfg] exists on this system? Otherwise we might > >> >> have a very short wait until the perf fuzzer oopses this thing :) > >> >> > >> > > >> > I think we already did the check before using msr[cfg]. > >> > >> Where? All I see is: > >> > >> + if (cfg >= PERF_MSR_EVENT_MAX) > >> + return -EINVAL; > > > > Yes, we check cfg here. So msr[cfg] should be always available. > > > > PERF_MSR_EVENT_MAX is a constant. If I run this thing on an AMD CPU > that supports TSC, APERF, MPERF, and nothing else, and someone asks for > PPERF, then the check will succeed and we'll oops, right? > Right, it could be a problem. How about the patch as below? --- >From 0217ffc9a0d2fac6417552b9f66fafc538ef9068 Mon Sep 17 00:00:00 2001 Date: Tue, 4 Aug 2015 08:27:19 -0400 Subject: [PATCH 1/1] perf/x86/msr: Fix issue of accessing unsupported MSR events Most of platforms only support part of MSR events. If unsupported MSR events are mistakenly accessed, it may cause oops. Introducing .available to mark the supported MSR events, and check it in event init. Reported-by: Andy Lutomirski Signed-off-by: Kan Liang --- arch/x86/kernel/cpu/perf_event_msr.c | 21 ++++++++++++++++----- 1 file changed, 16 insertions(+), 5 deletions(-) diff --git a/arch/x86/kernel/cpu/perf_event_msr.c b/arch/x86/kernel/cpu/perf_event_msr.c index af216e9..c174242 100644 --- a/arch/x86/kernel/cpu/perf_event_msr.c +++ b/arch/x86/kernel/cpu/perf_event_msr.c @@ -13,14 +13,15 @@ enum perf_msr_id { struct perf_msr { int id; u64 msr; + bool available; }; static struct perf_msr msr[] = { - { PERF_MSR_TSC, 0 }, - { PERF_MSR_APERF, MSR_IA32_APERF }, - { PERF_MSR_MPERF, MSR_IA32_MPERF }, - { PERF_MSR_PPERF, MSR_PPERF }, - { PERF_MSR_SMI, MSR_SMI_COUNT }, + { PERF_MSR_TSC, 0, true }, + { PERF_MSR_APERF, MSR_IA32_APERF, false }, + { PERF_MSR_MPERF, MSR_IA32_MPERF, false }, + { PERF_MSR_PPERF, MSR_PPERF, false }, + { PERF_MSR_SMI, MSR_SMI_COUNT, false }, }; PMU_EVENT_ATTR_STRING(tsc, evattr_tsc, "event=0x00"); @@ -74,6 +75,10 @@ static int msr_event_init(struct perf_event *event) event->attr.sample_period) /* no sampling */ return -EINVAL; + /* The platform may not support all events */ + if (!msr[cfg].available) + return -EINVAL; + event->hw.idx = -1; event->hw.event_base = msr[cfg].msr; event->hw.config = cfg; @@ -181,18 +186,22 @@ static int __init intel_msr_init(int idx) case 71: /* 14nm Broadwell + GT3e (Intel Iris Pro graphics) */ case 79: /* 14nm Broadwell Server */ events_attrs[idx++] = &evattr_smi.attr.attr; + msr[PERF_MSR_SMI].available = true; break; case 78: /* 14nm Skylake Mobile */ case 94: /* 14nm Skylake Desktop */ events_attrs[idx++] = &evattr_pperf.attr.attr; events_attrs[idx++] = &evattr_smi.attr.attr; + msr[PERF_MSR_PPERF].available = true; + msr[PERF_MSR_SMI].available = true; break; case 55: /* 22nm Atom "Silvermont" */ case 76: /* 14nm Atom "Airmont" */ case 77: /* 22nm Atom "Silvermont Avoton/Rangely" */ events_attrs[idx++] = &evattr_smi.attr.attr; + msr[PERF_MSR_SMI].available = true; break; } @@ -215,6 +224,8 @@ static int __init msr_init(void) events_attrs[idx++] = &evattr_aperf.attr.attr; events_attrs[idx++] = &evattr_mperf.attr.attr; events_attrs[idx] = NULL; + msr[PERF_MSR_APERF].available = true; + msr[PERF_MSR_MPERF].available = true; } switch (boot_cpu_data.x86_vendor) { -- ????{.n?+???????+%?????ݶ??w??{.n?+????{??G?????{ay?ʇڙ?,j??f???h?????????z_??(?階?ݢj"???m??????G????????????&???~???iO???z??v?^?m???? ????????I?