the creation of boot_cpu_init() is wrong and accessing uninitialised data

the creation of boot_cpu_init() is wrong and accessing uninitialised data

[James Bottomley]

The basic problem with this function is that on most architectures
smp_processor_id() is an alias for current_thread_info()->cpu which
isn't given its correct value until setup_arch(), so adding a
boot_cpu_init() that uses it *before* setup_arch() is called is plain
wrong.  You manage to get away with it 99% of the time because its
uninitialised value is zero and mostly the id of the booting CPU is
zero ... but guess who's got a box currently booting on CPU 1 with no
CPU 0?

Unfortunately, I can't think of a good solution for what you want to do.
The thing that looks most plausible is hard_smp_processor_id() which
reads the actual register value of the processor id.  However, on x86
(and any other architectures that renumber their CPUs in setup_arch())
this will ultimately turn out wrong again.

James

Re: the creation of boot_cpu_init() is wrong and accessing uninitialised data

[Andrew Morton]

On Mon, 26 Jun 2006 21:45:13 -0500
James Bottomley <James.Bottomley@SteelEye.com> wrote:

> The basic problem with this function is that on most architectures
> smp_processor_id() is an alias for current_thread_info()->cpu which
> isn't given its correct value until setup_arch(), so adding a
> boot_cpu_init() that uses it *before* setup_arch() is called is plain
> wrong.  You manage to get away with it 99% of the time because its
> uninitialised value is zero and mostly the id of the booting CPU is
> zero ... but guess who's got a box currently booting on CPU 1 with no
> CPU 0?

preexisting bug, really.  printk() uses smp_processor_id().  Things like
spin_lock() will probably do so in certain debug modes.  We finally got
caught.

> Unfortunately, I can't think of a good solution for what you want to do.
> The thing that looks most plausible is hard_smp_processor_id() which
> reads the actual register value of the processor id.  However, on x86
> (and any other architectures that renumber their CPUs in setup_arch())
> this will ultimately turn out wrong again.

I think arch code should do it before calling start_kernel(), really.  It's
just such a basic part of the kernel framework.

A less wholesome but perhaps simpler solution would be to call the new
setup_smp_processor_id() on entry to start_kernel().

Re: the creation of boot_cpu_init() is wrong and accessing uninitialised data

[James Bottomley]

On Mon, 2006-06-26 at 20:04 -0700, Andrew Morton wrote:
> I think arch code should do it before calling start_kernel(), really.
> It's
> just such a basic part of the kernel framework.

Hmm ... well, getting at current_thread_info()->cpu is possible, but
nasty to audit, I would have thought (given that we're in assembler
before start_kernel is called).

> A less wholesome but perhaps simpler solution would be to call the new
> setup_smp_processor_id() on entry to start_kernel().

I was wondering about simply replacing boot_cpu_init() with
smp_prepare_boot_cpu().  By and large they do the same thing on most
archs, and mostly they don't seem to depend on setup_arch() having been
called.

However, introducing setup_smp_processor_id() will also work ... I'll
see if I can do it in an easy way.

James

Re: the creation of boot_cpu_init() is wrong and accessing uninitialised data

[Andrew Morton]

On Mon, 26 Jun 2006 22:36:32 -0500
James Bottomley <James.Bottomley@SteelEye.com> wrote:

> On Mon, 2006-06-26 at 20:04 -0700, Andrew Morton wrote:
> > I think arch code should do it before calling start_kernel(), really.
> > It's
> > just such a basic part of the kernel framework.
> 
> Hmm ... well, getting at current_thread_info()->cpu is possible, but
> nasty to audit, I would have thought (given that we're in assembler
> before start_kernel is called).

Well.  It's the assembly code which chose to call start_kernel().  It could
call something else.

> > A less wholesome but perhaps simpler solution would be to call the new
> > setup_smp_processor_id() on entry to start_kernel().
> 
> I was wondering about simply replacing boot_cpu_init() with
> smp_prepare_boot_cpu().  By and large they do the same thing on most
> archs, and mostly they don't seem to depend on setup_arch() having been
> called.

That won't fix the other bugs - we're presently calling printk() prior to
setup_arch(), and printk uses smp_procesor_id().

> However, introducing setup_smp_processor_id() will also work ... I'll
> see if I can do it in an easy way.

It's a bit odd - I think non-zero BSPs happen a bit more often than
only-on-voyager.

Re: the creation of boot_cpu_init() is wrong and accessing uninitialised data

[Keith Owens]

Andrew Morton (on Mon, 26 Jun 2006 22:03:37 -0700) wrote:
>It's a bit odd - I think non-zero BSPs happen a bit more often than
>only-on-voyager.

AFAICR, the BSP is supposed to be logical cpu 0 on all architectures.
Most architectures assign logical cpu 0 to the BSP, even if the BSP has
a non-zero hard_smp_processor_id.  ia64 even has this

int __cpu_disable(void)
{
        int cpu = smp_processor_id();

	/*
	 * dont permit boot processor for now
	 */
	if (cpu == 0 && !bsp_remove_ok) {

Re: the creation of boot_cpu_init() is wrong and accessing uninitialised data

[James Bottomley]

On Tue, 2006-06-27 at 23:00 +1000, Keith Owens wrote:
> AFAICR, the BSP is supposed to be logical cpu 0 on all architectures.
> Most architectures assign logical cpu 0 to the BSP, even if the BSP
> has
> a non-zero hard_smp_processor_id.  ia64 even has this

That's definitely not a requirement.  For systems like voyager whose
CPUs cannot be renumbered it makes no sense.  The original reason for
renumbering was that most CPU loops ran from 1 to max_cpu and therefore
worked better if the CPU map was dense.  However, I fixed that up long
ago so sparse CPU map traversal should be fine now.

James

Re: the creation of boot_cpu_init() is wrong and accessing uninitialised data

[James Bottomley]

On Mon, 2006-06-26 at 22:03 -0700, Andrew Morton wrote:
> Well.  It's the assembly code which chose to call start_kernel().  It could
> call something else.

arch_start_kernel maybe?  I still think that's a bit of a non-obvious
alteration to a very well ingrained existing procedure.

> > > A less wholesome but perhaps simpler solution would be to call the new
> > > setup_smp_processor_id() on entry to start_kernel().
> > 
> > I was wondering about simply replacing boot_cpu_init() with
> > smp_prepare_boot_cpu().  By and large they do the same thing on most
> > archs, and mostly they don't seem to depend on setup_arch() having been
> > called.
> 
> That won't fix the other bugs - we're presently calling printk() prior to
> setup_arch(), and printk uses smp_procesor_id().

No, I mean call smp_prepare_boot_cpu() where boot_cpu_init() is now
(i.e. *before* setup_arch).  On all the arch's I know (parisc, i386 and
voyager) this function does nothing but set up the cpu map as
boot_cpu_init() does.  Of course, this change would break any arch that
relied on setup_arch() being called before smp_prepare_boot_cpu() ...

> > However, introducing setup_smp_processor_id() will also work ... I'll
> > see if I can do it in an easy way.
> 
> It's a bit odd - I think non-zero BSPs happen a bit more often than
> only-on-voyager.

OK, here's the patch that adds this API then.

James

Index: voyager-init-2.6/include/linux/smp.h
===================================================================
--- voyager-init-2.6.orig/include/linux/smp.h	2006-06-27 09:20:42.000000000 -0500
+++ voyager-init-2.6/include/linux/smp.h	2006-06-27 09:23:23.000000000 -0500
@@ -74,6 +74,15 @@
  */
 void smp_prepare_boot_cpu(void);
 
+/*
+ * Some architectures use current_thread_info()->cpu to get the
+ * CPU, so for the boot CPU this has to be set up really early.  Thus
+ * this hook is used to initialise the value if necessary
+ */
+#ifndef ARCH_HAS_SMP_SETUP_PROCESSOR_ID
+void smp_setup_processor_id(void) { }
+#endif
+
 #else /* !SMP */
 
 /*
@@ -96,6 +105,7 @@
 static inline void smp_send_reschedule(int cpu) { }
 #define num_booting_cpus()			1
 #define smp_prepare_boot_cpu()			do {} while (0)
+#define smp_setup_processor_id()		do {} while (0)
 
 #endif /* !SMP */
 
Index: voyager-init-2.6/init/main.c
===================================================================
--- voyager-init-2.6.orig/init/main.c	2006-06-27 09:19:12.000000000 -0500
+++ voyager-init-2.6/init/main.c	2006-06-27 09:20:30.000000000 -0500
@@ -455,6 +455,7 @@
  * enable them
  */
 	lock_kernel();
+	smp_setup_processor_id();
 	boot_cpu_init();
 	page_address_init();
 	printk(KERN_NOTICE);
Index: voyager-init-2.6/include/asm-i386/smp.h
===================================================================
--- voyager-init-2.6.orig/include/asm-i386/smp.h	2006-06-27 09:23:38.000000000 -0500
+++ voyager-init-2.6/include/asm-i386/smp.h	2006-06-27 09:24:29.000000000 -0500
@@ -8,6 +8,7 @@
 #include <linux/kernel.h>
 #include <linux/threads.h>
 #include <linux/cpumask.h>
+#include "mach_smp.h"
 #endif
 
 #ifdef CONFIG_X86_LOCAL_APIC
Index: voyager-init-2.6/include/asm-i386/mach-default/mach_smp.h
===================================================================
--- /dev/null	1970-01-01 00:00:00.000000000 +0000
+++ voyager-init-2.6/include/asm-i386/mach-default/mach_smp.h	2006-06-27 09:42:16.000000000 -0500
@@ -0,0 +1,13 @@
+/*
+ * include/asm-i386/mach-default/mach_smp.h
+ *
+ * Machine specific SMP definitions
+ */
+#ifndef _MACH_SMP_H
+/*
+ * The boot CPU is always zero for apic based systems
+ */
+#define ARCH_HAS_SMP_SETUP_PROCESSOR_ID
+#define smp_setup_processor_id() \
+	do { current_thread_info()->cpu = 0; } while (0)
+#endif
Index: voyager-init-2.6/include/asm-i386/mach-voyager/mach_smp.h
===================================================================
--- /dev/null	1970-01-01 00:00:00.000000000 +0000
+++ voyager-init-2.6/include/asm-i386/mach-voyager/mach_smp.h	2006-06-27 09:30:30.000000000 -0500
@@ -0,0 +1,9 @@
+/*
+ * include/asm-i386/mach-voyager/mach_smp.h
+ *
+ * Machine specific SMP definitions
+ */
+#ifndef _MACH_SMP_H
+#define ARCH_HAS_SMP_SETUP_PROCESSOR_ID
+extern void smp_setup_processor_id(void);
+#endif
Index: voyager-init-2.6/arch/i386/mach-voyager/voyager_smp.c
===================================================================
--- voyager-init-2.6.orig/arch/i386/mach-voyager/voyager_smp.c	2006-06-27 09:30:46.000000000 -0500
+++ voyager-init-2.6/arch/i386/mach-voyager/voyager_smp.c	2006-06-27 09:31:44.000000000 -0500
@@ -1916,6 +1916,12 @@
 	cpu_set(smp_processor_id(), cpu_present_map);
 }
 
+void __init
+smp_setup_processor_id(void)
+{
+	current_thread_info()->cpu = hard_smp_processor_id();
+}
+
 int __devinit
 __cpu_up(unsigned int cpu)
 {

Re: the creation of boot_cpu_init() is wrong and accessing uninitialised data

[Andrew Morton]

James Bottomley <James.Bottomley@SteelEye.com> wrote:
>
> > > However, introducing setup_smp_processor_id() will also work ... I'll
> > > see if I can do it in an easy way.
> > 
> > It's a bit odd - I think non-zero BSPs happen a bit more often than
> > only-on-voyager.
> 
> OK, here's the patch that adds this API then.
> 
> James
> 
> Index: voyager-init-2.6/include/linux/smp.h
> ===================================================================
> --- voyager-init-2.6.orig/include/linux/smp.h	2006-06-27 09:20:42.000000000 -0500
> +++ voyager-init-2.6/include/linux/smp.h	2006-06-27 09:23:23.000000000 -0500
> @@ -74,6 +74,15 @@
>   */
>  void smp_prepare_boot_cpu(void);
>  
> +/*
> + * Some architectures use current_thread_info()->cpu to get the
> + * CPU, so for the boot CPU this has to be set up really early.  Thus
> + * this hook is used to initialise the value if necessary
> + */
> +#ifndef ARCH_HAS_SMP_SETUP_PROCESSOR_ID
> +void smp_setup_processor_id(void) { }
> +#endif

That wants to be `static inline'.


But still.  __attribute__((weak)) is wonderful for this sort of thing. 
Methinks it's much neater to do:


diff -puN init/main.c~add-smp_setup_processor_id init/main.c
--- a/init/main.c~add-smp_setup_processor_id
+++ a/init/main.c
@@ -454,10 +454,17 @@ static void __init boot_cpu_init(void)
 	cpu_set(cpu, cpu_possible_map);
 }
 
+static void __init __attribute__((weak)) smp_setup_processor_id(void)
+{
+}
+
 asmlinkage void __init start_kernel(void)
 {
 	char * command_line;
 	extern struct kernel_param __start___param[], __stop___param[];
+
+	smp_setup_processor_id();
+
 /*
  * Interrupts are still disabled. Do necessary setups, then
  * enable them
diff -puN arch/i386/mach-voyager/voyager_smp.c~add-smp_setup_processor_id arch/i386/mach-voyager/voyager_smp.c
--- a/arch/i386/mach-voyager/voyager_smp.c~add-smp_setup_processor_id
+++ a/arch/i386/mach-voyager/voyager_smp.c
@@ -1938,3 +1938,9 @@ smp_cpus_done(unsigned int max_cpus)
 {
 	zap_low_mappings();
 }
+
+void __init
+smp_setup_processor_id(void)
+{
+	current_thread_info()->cpu = hard_smp_processor_id();
+}
diff -puN include/linux/smp.h~add-smp_setup_processor_id include/linux/smp.h
--- a/include/linux/smp.h~add-smp_setup_processor_id
+++ a/include/linux/smp.h
@@ -125,4 +125,6 @@ static inline void smp_send_reschedule(i
 #define put_cpu()		preempt_enable()
 #define put_cpu_no_resched()	preempt_enable_no_resched()
 
+void smp_setup_processor_id(void);
+
 #endif /* __LINUX_SMP_H */
_


Note that I moved the call to smp_setup_processor_id() to be super-early. 
Before the lock_kernel(), before everything.  It seems better that way. 
And there's lockdep init stuff in -mm which is called immediately on entry
to start_kernel() which might want to use smp_processor_id().

Is that all OK?

Re: the creation of boot_cpu_init() is wrong and accessing uninitialised data

[James Bottomley]

On Tue, 2006-06-27 at 17:04 -0700, Andrew Morton wrote:
> +static void __init __attribute__((weak)) smp_setup_processor_id(void)

If you're really sure this works .. then it looks OK.  However, I
thought weak was a linker attribute, not a compiler one.  How does the
compiler know if the static inline needs to be incorporated or if a
strong symbol is going to override it when the whole thing is linked
together at the time it just compiles main.c?

> +{
> +}
> +
>  asmlinkage void __init start_kernel(void)
>  {
>  	char * command_line;
>  	extern struct kernel_param __start___param[], __stop___param[];
> +
> +	smp_setup_processor_id();
> +
>  /*
>   * Interrupts are still disabled. Do necessary setups, then
>   * enable them
> diff -puN arch/i386/mach-voyager/voyager_smp.c~add-smp_setup_processor_id arch/i386/mach-voyager/voyager_smp.c
> --- a/arch/i386/mach-voyager/voyager_smp.c~add-smp_setup_processor_id
> +++ a/arch/i386/mach-voyager/voyager_smp.c
> @@ -1938,3 +1938,9 @@ smp_cpus_done(unsigned int max_cpus)
>  {
>  	zap_low_mappings();
>  }
> +
> +void __init
> +smp_setup_processor_id(void)
> +{
> +	current_thread_info()->cpu = hard_smp_processor_id();
> +}
> diff -puN include/linux/smp.h~add-smp_setup_processor_id include/linux/smp.h
> --- a/include/linux/smp.h~add-smp_setup_processor_id
> +++ a/include/linux/smp.h
> @@ -125,4 +125,6 @@ static inline void smp_send_reschedule(i
>  #define put_cpu()		preempt_enable()
>  #define put_cpu_no_resched()	preempt_enable_no_resched()
>  
> +void smp_setup_processor_id(void);
> +
>  #endif /* __LINUX_SMP_H */
> _
> 
> 
> Note that I moved the call to smp_setup_processor_id() to be super-early. 
> Before the lock_kernel(), before everything.  It seems better that way. 
> And there's lockdep init stuff in -mm which is called immediately on entry
> to start_kernel() which might want to use smp_processor_id().
> 
> Is that all OK?

I'll give it a whirl tomorrow when I get access to one of the voyager
systems in Columbia.

James

Re: the creation of boot_cpu_init() is wrong and accessing uninitialised data

[Andrew Morton]

On Tue, 27 Jun 2006 22:45:34 -0400
James Bottomley <James.Bottomley@SteelEye.com> wrote:

> On Tue, 2006-06-27 at 17:04 -0700, Andrew Morton wrote:
> > +static void __init __attribute__((weak)) smp_setup_processor_id(void)
> 
> If you're really sure this works .. then it looks OK.

We do it quite a lot actually.
box:/usr/src/linux-2.6.17> grep -r 'attribute.*weak' . | wc -l
91

>  However, I
> thought weak was a linker attribute, not a compiler one.

Yes, it is mainly a linker thing.  But the compiler has to emit the ".weak"
pseudo-op and not inline the function.

>  How does the
> compiler know if the static inline needs to be incorporated or if a
> strong symbol is going to override it when the whole thing is linked
> together at the time it just compiles main.c?

I didn't mark it inline.

If the compiler chose to inline it then that would be rather dumb of it,
given that it had the weak attribute.  But yes, paranoia says we should be
tagging this noinline too.

> > Is that all OK?
> 
> I'll give it a whirl tomorrow when I get access to one of the voyager
> systems in Columbia.

Thanks.

Re: the creation of boot_cpu_init() is wrong and accessing uninitialised data

[James Bottomley]

On Tue, 2006-06-27 at 19:57 -0700, Andrew Morton wrote:
> I didn't mark it inline.
> 
> If the compiler chose to inline it then that would be rather dumb of
> it,
> given that it had the weak attribute.  But yes, paranoia says we
> should be
> tagging this noinline too.

Sorry, I was thinking static means "I may inline". And indeed, my
compiler says this:

init/main.c: In function 'smp_init':
init/main.c: At top level:
init/main.c:457: error: weak declaration of 'smp_setup_processor_id' must be public
init/main.c:457: error: static declaration of 'smp_setup_processor_id' follows non-static declaration
include/linux/smp.h:128: error: previous declaration of 'smp_setup_processor_id' was here

Making it non-static in init/main.c fixes this

I'm still compiling, so might have the results later this evening.

James

Re: the creation of boot_cpu_init() is wrong and accessing uninitialised data

[James Bottomley]

On Wed, 2006-06-28 at 19:10 -0400, James Bottomley wrote:
> I'm still compiling, so might have the results later this evening.

Actually, ran into a 53c700 driver problem, but I can now verify that
this patch works on voyager when booting with a non-zero CPU.

James

Re: the creation of boot_cpu_init() is wrong and accessing uninitialised data

[Eric W. Biederman]

James Bottomley <James.Bottomley@SteelEye.com> writes:

> On Wed, 2006-06-28 at 19:10 -0400, James Bottomley wrote:
>> I'm still compiling, so might have the results later this evening.
>
> Actually, ran into a 53c700 driver problem, but I can now verify that
> this patch works on voyager when booting with a non-zero CPU.

What is the point of using a non-zero logical cpu id?
I don't care about the apic id or the equivalent.

There are cases like machine_shutdown where we care about who
the boot cpu is so we can reboot on that cpu.  As far as I know 
the kernel has not abstraction to describe the boot cpu
except for giving it logical cpu id 0.  Has an abstraction
been added that I'm not aware of?

Eric

Re: the creation of boot_cpu_init() is wrong and accessing uninitialised data

[James Bottomley]

On Sun, 2006-07-02 at 08:52 -0600, Eric W. Biederman wrote:
> What is the point of using a non-zero logical cpu id?
> I don't care about the apic id or the equivalent.

Logical CPUs were just an artifact to get dense CPU maps, which is now
no longer necessary.  There are architectures which have never used
logical CPUs.  For them, the boot CPU is whatever the BIOS says it is.

> There are cases like machine_shutdown where we care about who
> the boot cpu is so we can reboot on that cpu.

That's not at all safe:  one of the standard times for CPUs to be
deconfigured is on boot.  It's really not a good idea to rely on finding
the same CPU back again to boot on.

> As far as I know 
> the kernel has not abstraction to describe the boot cpu
> except for giving it logical cpu id 0.  Has an abstraction
> been added that I'm not aware of?

The kernel has never had an abstraction requiring logical CPUs, let
alone one requiring that the boot CPU be numbered zero.

James

Re: the creation of boot_cpu_init() is wrong and accessing uninitialised data

[Eric W. Biederman]

James Bottomley <James.Bottomley@SteelEye.com> writes:

> On Sun, 2006-07-02 at 08:52 -0600, Eric W. Biederman wrote:
>> What is the point of using a non-zero logical cpu id?
>> I don't care about the apic id or the equivalent.
>
> Logical CPUs were just an artifact to get dense CPU maps, which is now
> no longer necessary.  There are architectures which have never used
> logical CPUs.  For them, the boot CPU is whatever the BIOS says it is.

So for the patch mentioned earlier in the thread I have no problems.
And I think it makes sense for arch independent code not to care.

But for x86 I am much less convinced.

>> There are cases like machine_shutdown where we care about who
>> the boot cpu is so we can reboot on that cpu.
>
> That's not at all safe:  one of the standard times for CPUs to be
> deconfigured is on boot.  It's really not a good idea to rely on finding
> the same CPU back again to boot on.

You mean besides the fact that the MPspec requires it and reboots
become much more reliable when you do because you don't tickle BIOS bugs?
It is safe because we don't do it if the cpu is not online.

>> As far as I know 
>> the kernel has not abstraction to describe the boot cpu
>> except for giving it logical cpu id 0.  Has an abstraction
>> been added that I'm not aware of?
>
> The kernel has never had an abstraction requiring logical CPUs, let
> alone one requiring that the boot CPU be numbered zero.

x86 cares, and has such code.  Now possibly that is just the generic
subarch, and voyager does not share that code.  But since the
subarch/arch code division is almost invisible on x86 it's hard to say.

Eric