From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1756411AbcCUPBe (ORCPT ); Mon, 21 Mar 2016 11:01:34 -0400 Received: from bombadil.infradead.org ([198.137.202.9]:47633 "EHLO bombadil.infradead.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755884AbcCUPBc (ORCPT ); Mon, 21 Mar 2016 11:01:32 -0400 Date: Mon, 21 Mar 2016 16:01:27 +0100 From: Peter Zijlstra To: Thomas Gleixner Cc: LKML , x86@kernel.org, Stephane Eranian , Borislav Petkov , Kan Liang Subject: Re: [patch 0/4] x86/perf/intel/cstate: Fix cpu hotplug handling and make it modular Message-ID: <20160321150127.GY6344@twins.programming.kicks-ass.net> References: <20160320185527.763205536@linutronix.de> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20160320185527.763205536@linutronix.de> User-Agent: Mutt/1.5.21 (2012-12-30) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Sun, Mar 20, 2016 at 06:59:01PM -0000, Thomas Gleixner wrote: > The perf cstate driver is yet another trainwreck vs. cpu hotplug handling. The > hotplug code is not only disfunctional, it's also a uncomprehensible mess. > > The following series fixes the hotplug functionality, sanitizes error handling > and makes the driver modular. > I needed the below to not make it insta explode with perf_fuzzer. At cstate_pmu_event_init() event->cpu can still be -1, that results in funny masks being dereferenced. With the probe thing, we need to clear msr[].attr for all those that are not available, other event_init() will happily still select them (they're just not visible in sysfs, and guess how much the fuzzer cares about that.. :-). --- a/arch/x86/events/intel/cstate.c +++ b/arch/x86/events/intel/cstate.c @@ -267,7 +267,7 @@ static ssize_t cstate_get_attr_cpumask(s static int cstate_pmu_event_init(struct perf_event *event) { u64 cfg = event->attr.config; - unsigned int cpu; + int cpu; if (event->attr.type != event->pmu->type) return -ENOENT; @@ -282,6 +282,9 @@ static int cstate_pmu_event_init(struct event->attr.sample_period) /* no sampling */ return -EINVAL; + if (event->cpu < 0) + return -EINVAL; + if (event->pmu == &cstate_core_pmu) { if (cfg >= PERF_CSTATE_CORE_EVENT_MAX) return -EINVAL; @@ -547,22 +550,24 @@ MODULE_DEVICE_TABLE(x86cpu, intel_cstate * Probe the cstate events and insert the available one into sysfs attrs * Return false if there are no available events. */ -static bool __init cstate_probe_msr(const unsigned long evmsk, - struct perf_cstate_msr *msr, - struct attribute **attrs) +static bool __init cstate_probe_msr(const unsigned long evmsk, int max, + struct perf_cstate_msr *msr, + struct attribute **attrs) { bool found = false; unsigned int bit; u64 val; - for_each_set_bit(bit, &evmsk, sizeof(evmsk) * BITS_PER_BYTE) { - /* Verify whether the MSR is accessible */ - if (!rdmsrl_safe(msr[bit].msr, &val)) { + for (bit = 0; bit < max; bit++) { + if (test_bit(bit, &evmsk) && !rdmsrl_safe(msr[bit].msr, &val)) { *attrs++ = &msr[bit].attr->attr.attr; found = true; + } else { + msr[bit].attr = NULL; } } *attrs = NULL; + return found; } @@ -572,11 +577,13 @@ static int __init cstate_probe(const str if (cm->quirks & SLM_PKG_C6_USE_C7_MSR) pkg_msr[PERF_CSTATE_PKG_C6_RES].msr = MSR_PKG_C7_RESIDENCY; - has_cstate_core = cstate_probe_msr(cm->core_events, core_msr, - core_events_attrs); - - has_cstate_pkg = cstate_probe_msr(cm->pkg_events, pkg_msr, - pkg_events_attrs); + has_cstate_core = cstate_probe_msr(cm->core_events, + PERF_CSTATE_CORE_EVENT_MAX, + core_msr, core_events_attrs); + + has_cstate_pkg = cstate_probe_msr(cm->pkg_events, + PERF_CSTATE_PKG_EVENT_MAX, + pkg_msr, pkg_events_attrs); return (has_cstate_core || has_cstate_pkg) ? 0 : -ENODEV; }