Re: [patch 0/4] x86/perf/intel/cstate: Fix cpu hotplug handling and make it modular

[Peter Zijlstra <peterz@infradead.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;
 }