[PATCH] OOPS in identify_cpu() on CPUs without CPUID

[PATCH] OOPS in identify_cpu() on CPUs without CPUID

[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.

Signed-off-by: Ondrej Zary <linux@rainbow-software.org>

--- 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)
 {
+	this_cpu = &default_cpu;
 	c->extended_cpuid_level = 0;
 
 	if (!have_cpuid_p())


-- 
Ondrej Zary

Re: [PATCH] OOPS in identify_cpu() on CPUs without CPUID

[Ingo Molnar]

* Ondrej Zary <linux@rainbow-software.org> wrote:

> 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 ...

> Signed-off-by: Ondrej Zary <linux@rainbow-software.org>
> 
> --- 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)
>  {
> +	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

Re: [PATCH] OOPS in identify_cpu() on CPUs without CPUID

[Ondrej Zary]

On Saturday 08 August 2009 19:53:44 Ingo Molnar wrote:
> * Ondrej Zary <linux@rainbow-software.org> wrote:
> > 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 ...
>
> > Signed-off-by: Ondrej Zary <linux@rainbow-software.org>
> >
> > --- 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)
> >  {
> > +	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;
>
> ?

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).

-- 
Ondrej Zary

Re: [PATCH] OOPS in identify_cpu() on CPUs without CPUID

[H. Peter Anvin]

On 08/08/2009 10:53 AM, Ingo Molnar wrote:
> 
> indeed ...
> 
>> Signed-off-by: Ondrej Zary <linux@rainbow-software.org>
>>
>> --- 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)
>>  {
>> +	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;
> 

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

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

[PATCH v2] OOPS in identify_cpu() on CPUs without CPUID

[Ondrej Zary]

On Saturday 08 August 2009 19:53:44 Ingo Molnar wrote:
> * Ondrej Zary <linux@rainbow-software.org> wrote:
> > 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 ...
>
> > Signed-off-by: Ondrej Zary <linux@rainbow-software.org>
> >
> > --- 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)
> >  {
> > +	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

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 <linux@rainbow-software.org>
--- 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-09 13:33:04.000000000 +0200
@@ -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;


-- 
Ondrej Zary

Re: [PATCH] OOPS in identify_cpu() on CPUs without CPUID

[Ondrej Zary]

On Sunday 09 August 2009 03:28:10 H. Peter Anvin wrote:
> On 08/08/2009 10:53 AM, Ingo Molnar wrote:
> > indeed ...
> >
> >> Signed-off-by: Ondrej Zary <linux@rainbow-software.org>
> >>
> >> --- 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)
> >>  {
> >> +	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;
>
> 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?

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

> 	-hpa



-- 
Ondrej Zary