OOPS in identify_cpu() on CPUs without CPUID

PATCH Superseded Ondrej Zary 3 participants 6 messages Aug 8, 2009 – Aug 9, 2009 x86

lore

Share & tools

lore

Ondrej Zary

author

2009-08-08 17:08 UTC #0 lore raw

PATCH 1/1 OOPS in identify_cpu() on CPUs without CPUID

Kernel is broken for x86 CPUs without CPUID since 2.6.28. It crashes with NULL pointer dereference in identify_cpu():

766 generic_identify(c);
767
768--> if (this_cpu->c_identify)
769 this_cpu->c_identify(c);

this_cpu is NULL. This is because it's only initialized in get_cpu_vendor() function, which is not called if the CPU has no CPUID instruction.

Signed-off-by Ondrej Zary <[email protected]>

arch/x86/kernel/cpu/common.c +1 -0

@@ -699,6 +699,7 @@
 
 static void __cpuinit generic_identify(struct cpuinfo_x86 *c)
 {
+	this_cpu = &default_cpu;
 	c->extended_cpuid_level = 0;
 
 	if (!have_cpuid_p())

Signature · 1 line

Ondrej Zary

Ingo Molnar

2009-08-08 17:54 UTC #1 lore raw

Quoted 11 lines · Ondrej Zary

Kernel is broken for x86 CPUs without CPUID since 2.6.28. It crashes with NULL pointer dereference in identify_cpu():
766 generic_identify(c);
767
768--> if (this_cpu->c_identify)
769 this_cpu->c_identify(c);
this_cpu is NULL. This is because it's only initialized in get_cpu_vendor() function, which is not called if the CPU has no CPUID instruction.

indeed ...

arch/x86/kernel/cpu/common.c line 705

Quoted 12 lines

Signed-off-by Ondrej Zary <[email protected]>

arch/x86/kernel/cpu/common.c +1 -0

@@ -699,6 +699,7 @@
 
 static void __cpuinit generic_identify(struct cpuinfo_x86 *c)
 {
+	this_cpu = &default_cpu;
 	c->extended_cpuid_level = 0;
 
 	if (!have_cpuid_p())

How about initializing this_cpu instead, via:

static const struct cpu_dev *this_cpu __cpuinitdata = &default_cpu;

?
Ingo

Ondrej Zary

author

2009-08-08 18:19 UTC #2 lore raw

Quoted 27 lines · On Saturday 08 August 2009 19:53:44 Ingo Molnar wrote:

Quoted 11 lines · Ondrej Zary
Kernel is broken for x86 CPUs without CPUID since 2.6.28. It crashes with NULL pointer dereference in identify_cpu():
766 generic_identify(c);
767
768--> if (this_cpu->c_identify)
769 this_cpu->c_identify(c);
this_cpu is NULL. This is because it's only initialized in get_cpu_vendor() function, which is not called if the CPU has no CPUID instruction.
indeed ...
Quoted 12 lines
Signed-off-by Ondrej Zary <[email protected]>
--- linux-2.6.30.4-orig/arch/x86/kernel/cpu/common.c 2009-06-10
05:05:27.000000000 +0200 +++
linux-2.6.30.4-router/arch/x86/kernel/cpu/common.c 2009-08-08
18:00:21.000000000 +0200 @@ -699,6 +699,7 @@
static void __cpuinit generic_identify(struct cpuinfo_x86 *c) {
arch/x86/kernel/cpu/common.c +1 -0
+	this_cpu = &default_cpu;
 	c->extended_cpuid_level = 0;

Quoted 7 lines

Quoted 1 line
if (!have_cpuid_p())
How about initializing this_cpu instead, via:
static const struct cpu_dev *this_cpu __cpuinitdata = &default_cpu;
?

That might be even better - looks cleaner. I can't test it right now as I don't have the hardware here. I'll test it tomorrow (unless someone will be faster).

Signature · 1 line

Ondrej Zary

H. Peter Anvin

reviewer

2009-08-09 01:30 UTC #3 lore raw

In reply to Ingo Molnar

Quoted 14 lines · Ingo Molnar

indeed ...
Quoted 11 lines
Signed-off-by Ondrej Zary <[email protected]>
arch/x86/kernel/cpu/common.c +1 -0
@@ -699,6 +699,7 @@
 
 static void __cpuinit generic_identify(struct cpuinfo_x86 *c)
 {
+	this_cpu = &default_cpu;
 	c->extended_cpuid_level = 0;
 

Quoted 6 lines

arch/x86/kernel/cpu/common.c line 705
Quoted 1 line
arch/x86/kernel/cpu/common.c +0 -0
 	if (!have_cpuid_p())
How about initializing this_cpu instead, via:
static const struct cpu_dev *this_cpu __cpuinitdata = &default_cpu;

The whole this_cpu hack is scary as all hell... although it's probably OK on a technicality, it takes what is properly a per-cpu attribute and turns it into a static global.

We *should* be able to initialize the APs (at least) in parallel, and although there probably aren't any systems in the field which don't have duplicate vendors, it is at least theoretically possible to have combinations of CPUID and non-CPUID processors in the same systems.

As such, it really would be better if this_cpu was changed to be passed as return values and on the stack (as appropriate). However, that is not 2.6.31 material, and as such doing the static initialization would be okay.

Ondrej, would you be interested in doing a "fullblown" patch for this?

-hpa

Signature · 2 lines

H. Peter Anvin, Intel Open Source Technology Center
I work for Intel.  I don't speak on their behalf.

Ondrej Zary

author

2009-08-09 17:45 UTC #5 lore raw

PATCH v2 1/1 OOPS in identify_cpu() on CPUs without CPUID

In reply to Ingo Molnar

Quoted 27 lines · On Saturday 08 August 2009 19:53:44 Ingo Molnar wrote:

Quoted 11 lines · Ondrej Zary
Kernel is broken for x86 CPUs without CPUID since 2.6.28. It crashes with NULL pointer dereference in identify_cpu():
766 generic_identify(c);
767
768--> if (this_cpu->c_identify)
769 this_cpu->c_identify(c);
this_cpu is NULL. This is because it's only initialized in get_cpu_vendor() function, which is not called if the CPU has no CPUID instruction.
indeed ...
Quoted 12 lines
Signed-off-by Ondrej Zary <[email protected]>
--- linux-2.6.30.4-orig/arch/x86/kernel/cpu/common.c 2009-06-10
05:05:27.000000000 +0200 +++
linux-2.6.30.4-router/arch/x86/kernel/cpu/common.c 2009-08-08
18:00:21.000000000 +0200 @@ -699,6 +699,7 @@
static void __cpuinit generic_identify(struct cpuinfo_x86 *c) {
(quoted diff) +1 -0
+	this_cpu = &default_cpu;
 	c->extended_cpuid_level = 0;

Quoted 8 lines

Quoted 1 line
if (!have_cpuid_p())
How about initializing this_cpu instead, via:
static const struct cpu_dev *this_cpu __cpuinitdata = &default_cpu;
?
Ingo

The new patch is not very nice as default_cpu has to be moved. Works fine.

Kernel is broken for x86 CPUs without CPUID since 2.6.28. It crashes with NULL pointer dereference in identify_cpu():

766 generic_identify(c);
767
768--> if (this_cpu->c_identify)
769 this_cpu->c_identify(c);

this_cpu is NULL. This is because it's only initialized in get_cpu_vendor() function, which is not called if the CPU has no CPUID instruction.

Signed-off-by Ondrej Zary <[email protected]>

arch/x86/kernel/cpu/common.c +24 ~~-24~~

@@ -58,7 +58,30 @@
 	alloc_bootmem_cpumask_var(&cpu_sibling_setup_mask);
 }
 
-static const struct cpu_dev *this_cpu __cpuinitdata;
+static void __cpuinit default_init(struct cpuinfo_x86 *c)
+{
+#ifdef CONFIG_X86_64
+	display_cacheinfo(c);
+#else
+	/* Not much we can do here... */
+	/* Check if at least it has cpuid */
+	if (c->cpuid_level == -1) {
+		/* No cpuid. It must be an ancient CPU */
+		if (c->x86 == 4)
+			strcpy(c->x86_model_id, "486");
+		else if (c->x86 == 3)
+			strcpy(c->x86_model_id, "386");
+	}
+#endif
+}
+
+static const struct cpu_dev __cpuinitconst default_cpu = {
+	.c_init	= default_init,
+	.c_vendor = "Unknown",
+	.c_x86_vendor = X86_VENDOR_UNKNOWN,
+};
+
+static const struct cpu_dev *this_cpu __cpuinitdata = &default_cpu;
 
 DEFINE_PER_CPU_PAGE_ALIGNED(struct gdt_page, gdt_page) = { .gdt = {
 #ifdef CONFIG_X86_64

@@ -330,29 +353,6 @@
 
 static const struct cpu_dev *__cpuinitdata cpu_devs[X86_VENDOR_NUM] = {};
 
-static void __cpuinit default_init(struct cpuinfo_x86 *c)
-{
-#ifdef CONFIG_X86_64
-	display_cacheinfo(c);
-#else
-	/* Not much we can do here... */
-	/* Check if at least it has cpuid */
-	if (c->cpuid_level == -1) {
-		/* No cpuid. It must be an ancient CPU */
-		if (c->x86 == 4)
-			strcpy(c->x86_model_id, "486");
-		else if (c->x86 == 3)
-			strcpy(c->x86_model_id, "386");
-	}
-#endif
-}
-
-static const struct cpu_dev __cpuinitconst default_cpu = {
-	.c_init	= default_init,
-	.c_vendor = "Unknown",
-	.c_x86_vendor = X86_VENDOR_UNKNOWN,
-};
-
 static void __cpuinit get_model_name(struct cpuinfo_x86 *c)
 {
 	unsigned int *v;

Signature · 1 line

Ondrej Zary

Ondrej Zary

author

2009-08-09 21:12 UTC #4 lore raw

In reply to H. Peter Anvin

Quoted 30 lines · On Sunday 09 August 2009 03:28:10 H. Peter Anvin wrote:

Quoted 14 lines · Ingo Molnar
indeed ...
Quoted 12 lines
Signed-off-by Ondrej Zary <[email protected]>
--- linux-2.6.30.4-orig/arch/x86/kernel/cpu/common.c 2009-06-10
05:05:27.000000000 +0200 +++
linux-2.6.30.4-router/arch/x86/kernel/cpu/common.c 2009-08-08
18:00:21.000000000 +0200 @@ -699,6 +699,7 @@
static void __cpuinit generic_identify(struct cpuinfo_x86 *c) {
arch/x86/kernel/cpu/common.c +1 -0
+	this_cpu = &default_cpu;
 	c->extended_cpuid_level = 0;
Quoted 5 lines
Quoted 1 line
if (!have_cpuid_p())
How about initializing this_cpu instead, via:
static const struct cpu_dev *this_cpu __cpuinitdata = &default_cpu;
The whole this_cpu hack is scary as all hell... although it's probably OK on a technicality, it takes what is properly a per-cpu attribute and turns it into a static global.
We *should* be able to initialize the APs (at least) in parallel, and although there probably aren't any systems in the field which don't have duplicate vendors, it is at least theoretically possible to have combinations of CPUID and non-CPUID processors in the same systems.

Quoted 6 lines

As such, it really would be better if this_cpu was changed to be passed as return values and on the stack (as appropriate). However, that is not 2.6.31 material, and as such doing the static initialization would be okay.
Ondrej, would you be interested in doing a "fullblown" patch for this?

That would be too much for me. I know basically nothing about this initialization code.

Quoted 1 line

-hpa

Signature · 1 line

Ondrej Zary