Undefined behaviour with get_cpu_vendor

Undefined behaviour with get_cpu_vendor

[Christian Ehrhardt]

Hi,

Your Patch at (URL wrapped)

http://www.kernel.org/git/?p=linux/kernel/git/torvalds/old-2.6-bkcvs.git; \
		a=commit;h=99c6e60afff8a7bc6121aeb847dab27c556cf0c9

introduced an additional Parameter (int early) to get_cpu_vendor.
However, the same function is called in arch/i386/kernel/apic.c (via
an explicit extern declaration that doesn't have the new early parameter.

I don't know if this can cause actual problems but I think something like
the patch below is needed for correctness.

   regards    Christian

--- arch/i386/kernel/apic.c     2005-03-26 04:28:38.000000000 +0100
+++ arch/i386/kernel/apic.c.new 2005-08-17 11:54:48.070499352 +0200
@@ -703,14 +703,14 @@
 static int __init detect_init_APIC (void)
 {
        u32 h, l, features;
-       extern void get_cpu_vendor(struct cpuinfo_x86*);
+       extern void get_cpu_vendor(struct cpuinfo_x86*, int);
 
        /* Disabled by kernel option? */
        if (enable_local_apic < 0)
                return -1;
 
        /* Workaround for us being called before identify_cpu(). */
-       get_cpu_vendor(&boot_cpu_data);
+       get_cpu_vendor(&boot_cpu_data, 1);
 
        switch (boot_cpu_data.x86_vendor) {
        case X86_VENDOR_AMD:


-- 
THAT'S ALL FOLKS!

Re: Undefined behaviour with get_cpu_vendor

[Andreas Schwab]

Christian Ehrhardt <ehrhardt@mathematik.uni-ulm.de> writes:

> --- arch/i386/kernel/apic.c     2005-03-26 04:28:38.000000000 +0100
> +++ arch/i386/kernel/apic.c.new 2005-08-17 11:54:48.070499352 +0200
> @@ -703,14 +703,14 @@
>  static int __init detect_init_APIC (void)
>  {
>         u32 h, l, features;
> -       extern void get_cpu_vendor(struct cpuinfo_x86*);
> +       extern void get_cpu_vendor(struct cpuinfo_x86*, int);

Move the declaration to a header and include that here.

Andreas.

-- 
Andreas Schwab, SuSE Labs, schwab@suse.de
SuSE Linux Products GmbH, Maxfeldstraße 5, 90409 Nürnberg, Germany
Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: Undefined behaviour with get_cpu_vendor

[Andi Kleen]

On Wed, Aug 17, 2005 at 11:54:23AM +0200, Christian Ehrhardt wrote:
> 
> Hi,
> 
> Your Patch at (URL wrapped)
> 
> http://www.kernel.org/git/?p=linux/kernel/git/torvalds/old-2.6-bkcvs.git; \
> 		a=commit;h=99c6e60afff8a7bc6121aeb847dab27c556cf0c9
> 
> introduced an additional Parameter (int early) to get_cpu_vendor.
> However, the same function is called in arch/i386/kernel/apic.c (via
> an explicit extern declaration that doesn't have the new early parameter.

Sigh. All people adding externs like this should be ...

But it won't change anything - the only difference with
the flag being 0 is to read less fields, but since the function
has been called earlier and the data has not changed
the output is always the same.

Anyways, the correct change is to just remove this call because it's
not needed anymore because of the early CPU detection.

> I don't know if this can cause actual problems but I think something like
> the patch below is needed for correctness.

It's not needed for correctness. 

-Andi

Remove obsolete get_cpu_vendor call.

Since early CPU identify is in this information is already available

Signed-off-by: Andi Kleen <ak@suse.de>


Index: linux-2.6.13-rc6-misc/arch/i386/kernel/apic.c
===================================================================
--- linux-2.6.13-rc6-misc.orig/arch/i386/kernel/apic.c
+++ linux-2.6.13-rc6-misc/arch/i386/kernel/apic.c
@@ -726,15 +726,11 @@ __setup("apic=", apic_set_verbosity);
 static int __init detect_init_APIC (void)
 {
 	u32 h, l, features;
-	extern void get_cpu_vendor(struct cpuinfo_x86*);
 
 	/* Disabled by kernel option? */
 	if (enable_local_apic < 0)
 		return -1;
 
-	/* Workaround for us being called before identify_cpu(). */
-	get_cpu_vendor(&boot_cpu_data);
-
 	switch (boot_cpu_data.x86_vendor) {
 	case X86_VENDOR_AMD:
 		if ((boot_cpu_data.x86 == 6 && boot_cpu_data.x86_model > 1) ||

Re: Undefined behaviour with get_cpu_vendor

[Horst von Brand]

Andi Kleen <ak@suse.de> wrote:
> On Wed, Aug 17, 2005 at 11:54:23AM +0200, Christian Ehrhardt wrote:
> > Your Patch at (URL wrapped)
> > 
> > http://www.kernel.org/git/?p=linux/kernel/git/torvalds/old-2.6-bkcvs.git; \
> > 		a=commit;h=99c6e60afff8a7bc6121aeb847dab27c556cf0c9
> > 
> > introduced an additional Parameter (int early) to get_cpu_vendor.
> > However, the same function is called in arch/i386/kernel/apic.c (via
> > an explicit extern declaration that doesn't have the new early parameter.
> 
> Sigh. All people adding externs like this should be ...
> 
> But it won't change anything - the only difference with
> the flag being 0 is to read less fields, but since the function
> has been called earlier and the data has not changed
> the output is always the same.

I'm not so sure that "argument not explicitly given" will always turn out
zero...  more like "random" memory contents.
-- 
Dr. Horst H. von Brand                   User #22616 counter.li.org
Departamento de Informatica                     Fono: +56 32 654431
Universidad Tecnica Federico Santa Maria              +56 32 654239
Casilla 110-V, Valparaiso, Chile                Fax:  +56 32 797513