Re: PROBLEM: CPU hotplug leads to NULL pointer dereference with RAPL enabled on AMD 2990WX

[Borislav Petkov]

On Mon, Jan 04, 2021 at 10:50:10AM +0100, Borislav Petkov wrote:
> On Mon, Jan 04, 2021 at 12:02:44AM +0100, Johnathan Smithinovic wrote:
> > CPU hotplug leads to NULL pointer dereference with RAPL enabled on AMD 2990WX
> > 
> > 
> > When hot-plugging CPUs (e.g. manually or on suspend) I get a NULL
> > pointer dereference in rapl_cpu_offline() for CPUs 16 and 24.
> > It *seems* to me that this has to do with commit
> > 700d098acec5271161606f3c0086b71695ea2ef8
> > ("x86/CPU/AMD: Save AMD NodeId as cpu_die_id").
> > When reverting said commit hotplug works again.
> 
> Yeah, known issue and I'm working on it.

I can't get my box to generate the topology config yours has so can you
run the debug patch below on your system on latest Linus tree, offline
cores (it should prevent the oops so that you can catch dmesg) and then
send me a full dmesg, private mail's fine too.

Thx.

---
diff --git a/arch/x86/events/rapl.c b/arch/x86/events/rapl.c
index 7dbbeaacd995..19563faa58ae 100644
--- a/arch/x86/events/rapl.c
+++ b/arch/x86/events/rapl.c
@@ -139,10 +139,13 @@ static unsigned int rapl_cntr_mask;
 static u64 rapl_timer_ms;
 static struct perf_msr *rapl_msrs;
 
-static inline struct rapl_pmu *cpu_to_rapl_pmu(unsigned int cpu)
+static inline struct rapl_pmu *cpu_to_rapl_pmu(unsigned int cpu, bool dbg)
 {
 	unsigned int dieid = topology_logical_die_id(cpu);
 
+	if (dbg)
+		pr_info("%s: CPU%d, dieid: %d\n", __func__, cpu, dieid);
+
 	/*
 	 * The unsigned check also catches the '-1' return value for non
 	 * existent mappings in the topology map.
@@ -360,7 +363,7 @@ static int rapl_pmu_event_init(struct perf_event *event)
 		return -EINVAL;
 
 	/* must be done before validate_group */
-	pmu = cpu_to_rapl_pmu(event->cpu);
+	pmu = cpu_to_rapl_pmu(event->cpu, false);
 	if (!pmu)
 		return -EINVAL;
 	event->cpu = pmu->cpu;
@@ -543,13 +546,16 @@ static struct perf_msr amd_rapl_msrs[PERF_RAPL_MAX] = {
 
 static int rapl_cpu_offline(unsigned int cpu)
 {
-	struct rapl_pmu *pmu = cpu_to_rapl_pmu(cpu);
+	struct rapl_pmu *pmu = cpu_to_rapl_pmu(cpu, true);
 	int target;
 
 	/* Check if exiting cpu is used for collecting rapl events */
 	if (!cpumask_test_and_clear_cpu(cpu, &rapl_cpu_mask))
 		return 0;
 
+	if (WARN_ON(!pmu))
+		return -1;
+
 	pmu->cpu = -1;
 	/* Find a new cpu to collect rapl events */
 	target = cpumask_any_but(topology_die_cpumask(cpu), cpu);
@@ -565,7 +571,7 @@ static int rapl_cpu_offline(unsigned int cpu)
 
 static int rapl_cpu_online(unsigned int cpu)
 {
-	struct rapl_pmu *pmu = cpu_to_rapl_pmu(cpu);
+	struct rapl_pmu *pmu = cpu_to_rapl_pmu(cpu, true);
 	int target;
 
 	if (!pmu) {
@@ -682,6 +688,8 @@ static int __init init_rapl_pmus(void)
 	int maxdie = topology_max_packages() * topology_max_die_per_package();
 	size_t size;
 
+	pr_info("%s: maxdie: %d\n", __func__, maxdie);
+
 	size = sizeof(*rapl_pmus) + maxdie * sizeof(struct rapl_pmu *);
 	rapl_pmus = kzalloc(size, GFP_KERNEL);
 	if (!rapl_pmus)
diff --git a/arch/x86/kernel/smpboot.c b/arch/x86/kernel/smpboot.c
index 8ca66af96a54..20343682aace 100644
--- a/arch/x86/kernel/smpboot.c
+++ b/arch/x86/kernel/smpboot.c
@@ -319,6 +319,11 @@ int topology_phys_to_logical_die(unsigned int die_id, unsigned int cur_cpu)
 	for_each_possible_cpu(cpu) {
 		struct cpuinfo_x86 *c = &cpu_data(cpu);
 
+		pr_info("%s: init: %d, cpu %d, cur_cpu: %d, cpu_die_id: %d, die_id: %d, "
+			"phys_proc_id: %d, proc_id: %d, logical_die_id: %d\n",
+			__func__, c->initialized, cpu, cur_cpu, c->cpu_die_id, die_id,
+			c->phys_proc_id, proc_id, c->logical_die_id);
+
 		if (c->initialized && c->cpu_die_id == die_id &&
 		    c->phys_proc_id == proc_id)
 			return c->logical_die_id;

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette