[PATCH 1/3] stack overflow safe kdump (2.6.18-rc1-i386) - safe_smp_processor_id

[PATCH 1/3] stack overflow safe kdump (2.6.18-rc1-i386) - safe_smp_processor_id

[Fernando Luis Vázquez Cao]

On the event of a stack overflow critical data that usually resides at
the bottom of the stack is likely to be stomped and, consequently, its
use should be avoided.

In particular, in the i386 and IA64 architectures the macro
smp_processor_id ultimately makes use of the "cpu" member of struct
thread_info which resides at the bottom of the stack. x86_64, on the
other hand, is not affected by this problem because it benefits from
the use of the PDA infrastructure.

To circumvent this problem I suggest implementing
"safe_smp_processor_id()" (it already exists in x86_64) for i386 and
IA64 and use it as a replacement for smp_processor_id in the reboot path
to the dump capture kernel. This is a possible implementation for i386.

Signed-off-by: Fernando Vazquez <fernando@intellilink.co.jp>
---

diff -urNp linux-2.6.18-rc1/arch/i386/kernel/smp.c linux-2.6.18-rc1-sof/arch/i386/kernel/smp.c
--- linux-2.6.18-rc1/arch/i386/kernel/smp.c	2006-07-10 10:59:58.000000000 +0900
+++ linux-2.6.18-rc1-sof/arch/i386/kernel/smp.c	2006-07-10 11:07:35.000000000 +0900
@@ -634,3 +634,29 @@ fastcall void smp_call_function_interrup
 	}
 }
 
+static int convert_apicid_to_cpu(int apic_id)
+{
+	int i;
+
+	for (i = 0; i < NR_CPUS; i++) {
+		if (x86_cpu_to_apicid[i] == apic_id)
+		return i;
+	}
+	return -1;
+}
+
+int safe_smp_processor_id(void)
+{
+	int apicid, cpuid;
+
+	if (!boot_cpu_has(X86_FEATURE_APIC))
+		return 0;
+
+	apicid = hard_smp_processor_id();
+	if (apicid == BAD_APICID)
+		return 0;
+
+	cpuid = convert_apicid_to_cpu(apicid);
+
+	return cpuid >= 0 ? cpuid : 0;
+}
diff -urNp linux-2.6.18-rc1/include/asm-i386/smp.h linux-2.6.18-rc1-sof/include/asm-i386/smp.h
--- linux-2.6.18-rc1/include/asm-i386/smp.h	2006-07-10 11:00:05.000000000 +0900
+++ linux-2.6.18-rc1-sof/include/asm-i386/smp.h	2006-07-10 15:34:26.000000000 +0900
@@ -89,12 +89,20 @@ static __inline int logical_smp_processo
 
 #endif
 
+#ifdef CONFIG_X86_VOYAGER
+extern int hard_smp_processor_id(void);
+#define safe_smp_processor_id() hard_smp_processor_id()
+#else
+extern int safe_smp_processor_id(void);
+#endif
+
 extern int __cpu_disable(void);
 extern void __cpu_die(unsigned int cpu);
 #endif /* !__ASSEMBLY__ */
 
 #else /* CONFIG_SMP */
 
+#define safe_smp_processor_id()		0
 #define cpu_physical_id(cpu)		boot_cpu_physical_apicid
 
 #define NO_PROC_ID		0xFF		/* No processor magic marker */

Re: [Fastboot] [PATCH 1/3] stack overflow safe kdump (2.6.18-rc1-i386) - safe_smp_processor_id

[Keith Owens]

Fernando Luis =?ISO-8859-1?Q?V=E1zquez?= Cao (on Mon, 10 Jul 2006 16:50:52 +0900) wrote:
>On the event of a stack overflow critical data that usually resides at
>the bottom of the stack is likely to be stomped and, consequently, its
>use should be avoided.
>
>In particular, in the i386 and IA64 architectures the macro
>smp_processor_id ultimately makes use of the "cpu" member of struct
>thread_info which resides at the bottom of the stack. x86_64, on the
>other hand, is not affected by this problem because it benefits from
>the use of the PDA infrastructure.
>
>To circumvent this problem I suggest implementing
>"safe_smp_processor_id()" (it already exists in x86_64) for i386 and
>IA64 and use it as a replacement for smp_processor_id in the reboot path
>to the dump capture kernel. This is a possible implementation for i386.

I agree with avoiding the use of thread_info when the stack might be
corrupt.  However your patch results in reading apic data and scanning
NR_CPU sized tables for each IPI that is sent, which will slow down the
sending of all IPIs, not just dump.  It would be far cheaper to define
a per-cpu variable containing the logical cpu number, set that variable
once as each cpu is brought up and just read it in cases where you
might not trust the integrity of struct thread_info.  safe_smp_processor_id()
resolves to just a read of the per cpu variable.

Re: [Fastboot] [PATCH 1/3] stack overflow safe kdump (2.6.18-rc1-i386) - safe_smp_processor_id

[Fernando Luis Vázquez Cao]

Hi Keith,

Thank you for the comments.

On Mon, 2006-07-10 at 18:27 +1000, Keith Owens wrote:
> Fernando Luis Vazquez Cao (on Mon, 10 Jul 2006 16:50:52 +0900) wrote:
> >On the event of a stack overflow critical data that usually resides at
> >the bottom of the stack is likely to be stomped and, consequently, its
> >use should be avoided.
> >
> >In particular, in the i386 and IA64 architectures the macro
> >smp_processor_id ultimately makes use of the "cpu" member of struct
> >thread_info which resides at the bottom of the stack. x86_64, on the
> >other hand, is not affected by this problem because it benefits from
> >the use of the PDA infrastructure.
> >
> >To circumvent this problem I suggest implementing
> >"safe_smp_processor_id()" (it already exists in x86_64) for i386 and
> >IA64 and use it as a replacement for smp_processor_id in the reboot path
> >to the dump capture kernel. This is a possible implementation for i386.
> 
> I agree with avoiding the use of thread_info when the stack might be
> corrupt.  However your patch results in reading apic data and scanning
> NR_CPU sized tables for each IPI that is sent, which will slow down the
> sending of all IPIs, not just dump.
This patch only affects IPIs sent using send_IPI_allbutself which is
rarely called, so the impact in performance should be negligible.

> It would be far cheaper to define
> a per-cpu variable containing the logical cpu number, set that variable
> once as each cpu is brought up and just read it in cases where you
> might not trust the integrity of struct thread_info.  safe_smp_processor_id()
> resolves to just a read of the per cpu variable.
But to read a per-cpu variable you need to index the corresponding array
with processor id of the current CPU (see code below), but that is
precisely what we are trying to figure out. Anyway as
send_IPI_allbutself is not a fast path (correct if this assumption is
wrong) the current implementation of safe_smp_processor_id should be
fine.

#define get_cpu_var(var) (*({ preempt_disable();
&__get_cpu_var(var); }))
#define __get_cpu_var(var) per_cpu(var, smp_processor_id())

Am I missing something obvious?

Regards,

Fernando

Re: [Fastboot] [PATCH 1/3] stack overflow safe kdump (2.6.18-rc1-i386) - safe_smp_processor_id

[Eric W. Biederman]

Fernando Luis Vázquez Cao <fernando@oss.ntt.co.jp> writes:

> Hi Keith,
>
> Thank you for the comments.
>
> On Mon, 2006-07-10 at 18:27 +1000, Keith Owens wrote:
>> Fernando Luis Vazquez Cao (on Mon, 10 Jul 2006 16:50:52 +0900) wrote:
>> >On the event of a stack overflow critical data that usually resides at
>> >the bottom of the stack is likely to be stomped and, consequently, its
>> >use should be avoided.
>> >
>> >In particular, in the i386 and IA64 architectures the macro
>> >smp_processor_id ultimately makes use of the "cpu" member of struct
>> >thread_info which resides at the bottom of the stack. x86_64, on the
>> >other hand, is not affected by this problem because it benefits from
>> >the use of the PDA infrastructure.
>> >
>> >To circumvent this problem I suggest implementing
>> >"safe_smp_processor_id()" (it already exists in x86_64) for i386 and
>> >IA64 and use it as a replacement for smp_processor_id in the reboot path
>> >to the dump capture kernel. This is a possible implementation for i386.
>> 
>> I agree with avoiding the use of thread_info when the stack might be
>> corrupt.  However your patch results in reading apic data and scanning
>> NR_CPU sized tables for each IPI that is sent, which will slow down the
>> sending of all IPIs, not just dump.
> This patch only affects IPIs sent using send_IPI_allbutself which is
> rarely called, so the impact in performance should be negligible.

Well smp_call_function uses it so I don't know if rarely called applies.

However when called with the NMI vector every instance of send_IPI_allbutself
transforms this into send_IPI_mask.  Which is why we need to know our current
cpu in the first place.

Therefore why don't we just do that explicitly in crash.c
i.e.

static void smp_send_nmi_allbutself(void)
{
	cpumask_t mask = cpu_online_map;
	cpu_clear(safe_smp_processor_id(), mask);
	send_IPI_mask(mask, NMI_VECTOR);
}

That will guarantee that any effects this code paranoia may have
are only seen in the crash dump path.


>> It would be far cheaper to define
>> a per-cpu variable containing the logical cpu number, set that variable
>> once as each cpu is brought up and just read it in cases where you
>> might not trust the integrity of struct thread_info.  safe_smp_processor_id()
>> resolves to just a read of the per cpu variable.
> But to read a per-cpu variable you need to index the corresponding array
> with processor id of the current CPU (see code below), but that is
> precisely what we are trying to figure out. Anyway as
> send_IPI_allbutself is not a fast path (correct if this assumption is
> wrong) the current implementation of safe_smp_processor_id should be
> fine.
>
> #define get_cpu_var(var) (*({ preempt_disable();
> &__get_cpu_var(var); }))
> #define __get_cpu_var(var) per_cpu(var, smp_processor_id())
>
> Am I missing something obvious?

No.  Except that other architectures have cheaper per pointers so they
don't have that problem.

Eric

Re: [Fastboot] [PATCH 1/3] stack overflow safe kdump (2.6.18-rc1-i386) - safe_smp_processor_id

[Keith Owens]

Fernando Luis =?ISO-8859-1?Q?V=E1zquez?= Cao (on Mon, 10 Jul 2006 19:15:50 +0900) wrote:
>Hi Keith,
>
>Thank you for the comments.
>
>On Mon, 2006-07-10 at 18:27 +1000, Keith Owens wrote:
>> Fernando Luis Vazquez Cao (on Mon, 10 Jul 2006 16:50:52 +0900) wrote:
>> >On the event of a stack overflow critical data that usually resides at
>> >the bottom of the stack is likely to be stomped and, consequently, its
>> >use should be avoided.
>> >
>> >In particular, in the i386 and IA64 architectures the macro
>> >smp_processor_id ultimately makes use of the "cpu" member of struct
>> >thread_info which resides at the bottom of the stack. x86_64, on the
>> >other hand, is not affected by this problem because it benefits from
>> >the use of the PDA infrastructure.
>> >
>> >To circumvent this problem I suggest implementing
>> >"safe_smp_processor_id()" (it already exists in x86_64) for i386 and
>> >IA64 and use it as a replacement for smp_processor_id in the reboot path
>> >to the dump capture kernel. This is a possible implementation for i386.
>> =
>
>> I agree with avoiding the use of thread_info when the stack might be
>> corrupt.  However your patch results in reading apic data and scanning
>> NR_CPU sized tables for each IPI that is sent, which will slow down the
>> sending of all IPIs, not just dump.
>This patch only affects IPIs sent using send_IPI_allbutself which is
>rarely called, so the impact in performance should be negligible.

The main users of send_IPI_allbutself() are smp_call_function() and
on_each_cpu(), which are used quite often.  My main concern are the
architectures that use IPI to flush TLB entries from other cpus.  For
example, i386 ioremap_page_range() -> flush_tlb_all() -> on_each_cpu().

>> It would be far cheaper to define
>> a per-cpu variable containing the logical cpu number, set that variable
>> once as each cpu is brought up and just read it in cases where you
>> might not trust the integrity of struct thread_info.  safe_smp_processor_=
>id()
>> resolves to just a read of the per cpu variable.
>But to read a per-cpu variable you need to index the corresponding array
>with processor id of the current CPU (see code below), but that is
>precisely what we are trying to figure out.

Ouch, I am so used to ia64 where accessing the local per cpu variables
is a direct read, with no need to use smp_processor_id().

The use of smp_processor_id() in include/asm-generic/percpu.h is
worrying, it means that any RAS code like dump or debuggers cannot
access any per cpu variables.  Corrupt the kernel stack and all per cpu
variables become useless!  That is a hidden bug, just waiting to bite
all the RAS code.

ia64, x86_64, power, s390, sparc64 do not suffer from this problem,
they have efficient implementations of __get_cpu_var().  All other
architectures (including i386) use the generic percpu code and per cpu
variables will not work with corrupt kernel stacks.

Re: [PATCH 1/3] stack overflow safe kdump (2.6.18-rc1-i386) - safe_smp_processor_id

[James Bottomley]

On Mon, 2006-07-10 at 16:50 +0900, Fernando Luis Vázquez Cao wrote:
> diff -urNp linux-2.6.18-rc1/include/asm-i386/smp.h
> linux-2.6.18-rc1-sof/include/asm-i386/smp.h
> --- linux-2.6.18-rc1/include/asm-i386/smp.h     2006-07-10
> 11:00:05.000000000 +0900
> +++ linux-2.6.18-rc1-sof/include/asm-i386/smp.h 2006-07-10
> 15:34:26.000000000 +0900
> @@ -89,12 +89,20 @@ static __inline int logical_smp_processo
>  
>  #endif
>  
> +#ifdef CONFIG_X86_VOYAGER
> +extern int hard_smp_processor_id(void);
> +#define safe_smp_processor_id() hard_smp_processor_id()
> +#else
> +extern int safe_smp_processor_id(void);
> +#endif
> +

Argh, no, don't do this.  The Subarchitecture specific code should never
appear in the standard include directory (that was about the whole
point).  The extern for hard_smp_processor_id should just be global,
since it's common to all architectures, and the voyager specific define
should be in a voyager specific header file.

As an aside, it shows the problems x86 got itself into with it's
inconsistent direction of physical vs logical CPUs.  The idea was that
smp_processor_id() and hard_smp_processor_id() were supposed to return
the same thing, only hard_... went to the actual SMP registers to get
it.

James

Re: [PATCH 1/3] stack overflow safe kdump (2.6.18-rc1-i386) - safe_smp_processor_id

[Eric W. Biederman]

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

> On Mon, 2006-07-10 at 16:50 +0900, Fernando Luis Vázquez Cao wrote:
>> diff -urNp linux-2.6.18-rc1/include/asm-i386/smp.h
>> linux-2.6.18-rc1-sof/include/asm-i386/smp.h
>> --- linux-2.6.18-rc1/include/asm-i386/smp.h     2006-07-10
>> 11:00:05.000000000 +0900
>> +++ linux-2.6.18-rc1-sof/include/asm-i386/smp.h 2006-07-10
>> 15:34:26.000000000 +0900
>> @@ -89,12 +89,20 @@ static __inline int logical_smp_processo
>>  
>>  #endif
>>  
>> +#ifdef CONFIG_X86_VOYAGER
>> +extern int hard_smp_processor_id(void);
>> +#define safe_smp_processor_id() hard_smp_processor_id()
>> +#else
>> +extern int safe_smp_processor_id(void);
>> +#endif
>> +
>
> Argh, no, don't do this.  The Subarchitecture specific code should never
> appear in the standard include directory (that was about the whole
> point).  The extern for hard_smp_processor_id should just be global,
> since it's common to all architectures, and the voyager specific define
> should be in a voyager specific header file.
>
> As an aside, it shows the problems x86 got itself into with it's
> inconsistent direction of physical vs logical CPUs.  The idea was that
> smp_processor_id() and hard_smp_processor_id() were supposed to return
> the same thing, only hard_... went to the actual SMP registers to get
> it.

I agree that it shows the problem, and that voyager is different from the
rest of the x86 implementations. 

At least for things like the cpumask_t density of processor ids
is still an interesting property.  The basic issue is that apicids are
not in general dense on x86.  Not being able compile with support
for only two cpus because your cpus happen to be apicid 0 and apicid
6 by default is an issue.

To some extent this also shows the mess that the x86 subarch code is
because it is never clear if code is implemented in a subarchitecture
or not.

Fernando can you just put a trivial voyager specific definition of
safe_smp_processor_id in mach-voyager/voyager_smp.c.  It isn't a fast
path so the little extra overhead of making two separate functions
is not an issue and then the generic header doesn't have to have
subarch breakage.  Just a definition of safe_smp_processor_id().

Eric

Re: [PATCH 1/3] stack overflow safe kdump (2.6.18-rc1-i386) - safe_smp_processor_id

[James Bottomley]

On Mon, 2006-07-10 at 12:20 -0600, Eric W. Biederman wrote:
> I agree that it shows the problem, and that voyager is different from the
> rest of the x86 implementations. 

As a non-apic based SMP implementation, I don't think there was ever any
dissent about the latter.

> At least for things like the cpumask_t density of processor ids
> is still an interesting property.  The basic issue is that apicids are
> not in general dense on x86.  Not being able compile with support
> for only two cpus because your cpus happen to be apicid 0 and apicid
> 6 by default is an issue.

Density or lack of it is pretty much irrelevant nowadays since the CPU
map iterators are sparse efficient.  Whether x86 PC chooses to avail
itself of this or not is the business of the PC subarch maintainers.
The vast marjority of non-x86 SMP implementations still have sparse (or
at least physical only) CPU maps.

> To some extent this also shows the mess that the x86 subarch code is
> because it is never clear if code is implemented in a subarchitecture
> or not.

Erm, it does?  How?  My statement is that introducing subarch specific
#defines into subarch independent header files is a problem (which it
is).  If you grep for subarch defines in the rest of the arch
independent headers, I don't believe you'll find any.  This would rather
tend to show that for the last seven years, the subarch interface has
been remarkably effective ....

> Fernando can you just put a trivial voyager specific definition of
> safe_smp_processor_id in mach-voyager/voyager_smp.c.  It isn't a fast
> path so the little extra overhead of making two separate functions
> is not an issue and then the generic header doesn't have to have
> subarch breakage.  Just a definition of safe_smp_processor_id().

Yes, that should work.

James

Re: [PATCH 1/3] stack overflow safe kdump (2.6.18-rc1-i386) - safe_smp_processor_id

[Eric W. Biederman]

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

>
>> To some extent this also shows the mess that the x86 subarch code is
>> because it is never clear if code is implemented in a subarchitecture
>> or not.
>
> Erm, it does?  How?  My statement is that introducing subarch specific
> #defines into subarch independent header files is a problem (which it
> is).  If you grep for subarch defines in the rest of the arch
> independent headers, I don't believe you'll find any.  This would rather
> tend to show that for the last seven years, the subarch interface has
> been remarkably effective ....

They are remarkably brittle, because it is very hard to tell which code
is in a subarch and which code is not.  There have been several iterations
where people have broken subarchitectures by mistake.

This whole conversation is funny in one sense because the code under
discussion won't even compile on voyager right now.  Someone else
caught that and the patch that came out of that conversation was sent
to Andrew earlier today.

All I know for certain is that I have submitted patches that have
changed the entire kernel and the only thing that broke was x86
subarches because it is so non-obvious how they are different.

But I do agree the subarch header files are clean.
And no this case except for the fact no one realized that the
code doesn't even compile on voyager does not show how brittle
the x86 subarch code is.    Except for the fact that it seems
obvious that kernel/smp.c is generic code that every smp subarch
would use.


Eric

Re: [Fastboot] [PATCH 1/3] stack overflow safe kdump (2.6.18-rc1-i386) - safe_smp_processor_id

[Fernando Luis Vázquez Cao]

Hi Eric!

On Mon, 2006-07-10 at 05:37 -0600, Eric W. Biederman wrote:
> Fernando Luis Vázquez Cao <fernando@oss.ntt.co.jp> writes:
> 
> > Hi Keith,
> >
> > Thank you for the comments.
> >
> > On Mon, 2006-07-10 at 18:27 +1000, Keith Owens wrote:
> >> Fernando Luis Vazquez Cao (on Mon, 10 Jul 2006 16:50:52 +0900) wrote:
> >> >On the event of a stack overflow critical data that usually resides at
> >> >the bottom of the stack is likely to be stomped and, consequently, its
> >> >use should be avoided.
> >> >
> >> >In particular, in the i386 and IA64 architectures the macro
> >> >smp_processor_id ultimately makes use of the "cpu" member of struct
> >> >thread_info which resides at the bottom of the stack. x86_64, on the
> >> >other hand, is not affected by this problem because it benefits from
> >> >the use of the PDA infrastructure.
> >> >
> >> >To circumvent this problem I suggest implementing
> >> >"safe_smp_processor_id()" (it already exists in x86_64) for i386 and
> >> >IA64 and use it as a replacement for smp_processor_id in the reboot path
> >> >to the dump capture kernel. This is a possible implementation for i386.
> >> 
> >> I agree with avoiding the use of thread_info when the stack might be
> >> corrupt.  However your patch results in reading apic data and scanning
> >> NR_CPU sized tables for each IPI that is sent, which will slow down the
> >> sending of all IPIs, not just dump.
> > This patch only affects IPIs sent using send_IPI_allbutself which is
> > rarely called, so the impact in performance should be negligible.
> 
> Well smp_call_function uses it so I don't know if rarely called applies.
> 
> However when called with the NMI vector every instance of send_IPI_allbutself
> transforms this into send_IPI_mask.  Which is why we need to know our current
> cpu in the first place.
> 
> Therefore why don't we just do that explicitly in crash.c
> i.e.
> 
> static void smp_send_nmi_allbutself(void)
> {
> 	cpumask_t mask = cpu_online_map;
> 	cpu_clear(safe_smp_processor_id(), mask);
> 	send_IPI_mask(mask, NMI_VECTOR);
> }
> 
> That will guarantee that any effects this code paranoia may have
> are only seen in the crash dump path.

That is a good idea, but I have on concern. In mach-default by default
we use __send_IPI_shortcut (no_broadcast==0) instead of send_IPI_mask.
Is it always safe to ignore the no_broadcast setting? In other words,
can __send_IPI_shortcut be replaced by send_IPI_mask safely?

The implementation of send_IPI_allbutself in the different architectures
follows:

smp_send_nmi_allbutself
  send_IPI_allbutself

* mach-bigsmp
send_IPI_allbutself
  cpu_clear(smp_processor_id(), mask)
  send_IPI_mask
    send_IPI_mask_sequence
      apic_wait_icr_idle

* mach-default
send_IPI_allbutself
  __local_send_IPI_allbutself
    if (no_broadcast) {
      cpu_clear(smp_processor_id(), mask)
      send_IPI_mask(mask, vector)
        send_IPI_mask_bitmask
          apic_wait_icr_idle
    } else {
      __send_IPI_shortcut(APIC_DEST_ALLBUT, vector)
        apic_wait_icr_idle
    }

* mach-es7000
send_IPI_allbutself
  cpu_clear(smp_processor_id(), mask);
  send_IPI_mask
    send_IPI_mask_sequence
      apic_wait_icr_idle

* mach-numaq
send_IPI_allbutself
  cpu_clear(smp_processor_id(), mask)
  send_IPI_mask
    send_IPI_mask_sequence
      apic_wait_icr_idle

* mach-summit
send_IPI_allbutself
  cpu_clear(smp_processor_id(), mask)
  send_IPI_mask
    send_IPI_mask_sequence
      apic_wait_icr_idle

Regards,

Fernando

> 
> 
> >> It would be far cheaper to define
> >> a per-cpu variable containing the logical cpu number, set that variable
> >> once as each cpu is brought up and just read it in cases where you
> >> might not trust the integrity of struct thread_info.  safe_smp_processor_id()
> >> resolves to just a read of the per cpu variable.
> > But to read a per-cpu variable you need to index the corresponding array
> > with processor id of the current CPU (see code below), but that is
> > precisely what we are trying to figure out. Anyway as
> > send_IPI_allbutself is not a fast path (correct if this assumption is
> > wrong) the current implementation of safe_smp_processor_id should be
> > fine.
> >
> > #define get_cpu_var(var) (*({ preempt_disable();
> > &__get_cpu_var(var); }))
> > #define __get_cpu_var(var) per_cpu(var, smp_processor_id())
> >
> > Am I missing something obvious?
> 
> No.  Except that other architectures have cheaper per pointers so they
> don't have that problem.
> 
> Eric

Re: [Fastboot] [PATCH 1/3] stack overflow safe kdump (2.6.18-rc1-i386) - safe_smp_processor_id

[Fernando Luis Vázquez Cao]

On Tue, 2006-07-11 at 13:21 +0900, Fernando Luis Vázquez Cao wrote:
> Hi Eric!
> 
> On Mon, 2006-07-10 at 05:37 -0600, Eric W. Biederman wrote:
> > Fernando Luis Vázquez Cao <fernando@oss.ntt.co.jp> writes:
> > 
> > > Hi Keith,
> > >
> > > Thank you for the comments.
> > >
> > > On Mon, 2006-07-10 at 18:27 +1000, Keith Owens wrote:
> > >> Fernando Luis Vazquez Cao (on Mon, 10 Jul 2006 16:50:52 +0900) wrote:
> > >> >On the event of a stack overflow critical data that usually resides at
> > >> >the bottom of the stack is likely to be stomped and, consequently, its
> > >> >use should be avoided.
> > >> >
> > >> >In particular, in the i386 and IA64 architectures the macro
> > >> >smp_processor_id ultimately makes use of the "cpu" member of struct
> > >> >thread_info which resides at the bottom of the stack. x86_64, on the
> > >> >other hand, is not affected by this problem because it benefits from
> > >> >the use of the PDA infrastructure.
> > >> >
> > >> >To circumvent this problem I suggest implementing
> > >> >"safe_smp_processor_id()" (it already exists in x86_64) for i386 and
> > >> >IA64 and use it as a replacement for smp_processor_id in the reboot path
> > >> >to the dump capture kernel. This is a possible implementation for i386.
> > >> 
> > >> I agree with avoiding the use of thread_info when the stack might be
> > >> corrupt.  However your patch results in reading apic data and scanning
> > >> NR_CPU sized tables for each IPI that is sent, which will slow down the
> > >> sending of all IPIs, not just dump.
> > > This patch only affects IPIs sent using send_IPI_allbutself which is
> > > rarely called, so the impact in performance should be negligible.
> > 
> > Well smp_call_function uses it so I don't know if rarely called applies.
> > 
> > However when called with the NMI vector every instance of send_IPI_allbutself
> > transforms this into send_IPI_mask.  Which is why we need to know our current
> > cpu in the first place.
> > 
> > Therefore why don't we just do that explicitly in crash.c
> > i.e.
> > 
> > static void smp_send_nmi_allbutself(void)
> > {
> > 	cpumask_t mask = cpu_online_map;
> > 	cpu_clear(safe_smp_processor_id(), mask);
> > 	send_IPI_mask(mask, NMI_VECTOR);
> > }
> > 
> > That will guarantee that any effects this code paranoia may have
> > are only seen in the crash dump path.
> 
> That is a good idea, but I have on concern. In mach-default by default
> we use __send_IPI_shortcut (no_broadcast==0) instead of send_IPI_mask.
> Is it always safe to ignore the no_broadcast setting? In other words,
> can __send_IPI_shortcut be replaced by send_IPI_mask safely?
>From reading the code, it seems that send_IPI_mask is always safer (we
avoid the risk of sending an IPI to an offline CPU) and with it we can
certainly accomplish what we want. I will prepare new patches taking all
your comments and advices.

Thank you,

Fernando

P.S.: Sorry for replying to myself...

> 
> The implementation of send_IPI_allbutself in the different architectures
> follows:
> 
> smp_send_nmi_allbutself
>   send_IPI_allbutself
> 
> * mach-bigsmp
> send_IPI_allbutself
>   cpu_clear(smp_processor_id(), mask)
>   send_IPI_mask
>     send_IPI_mask_sequence
>       apic_wait_icr_idle
> 
> * mach-default
> send_IPI_allbutself
>   __local_send_IPI_allbutself
>     if (no_broadcast) {
>       cpu_clear(smp_processor_id(), mask)
>       send_IPI_mask(mask, vector)
>         send_IPI_mask_bitmask
>           apic_wait_icr_idle
>     } else {
>       __send_IPI_shortcut(APIC_DEST_ALLBUT, vector)
>         apic_wait_icr_idle
>     }
> 
> * mach-es7000
> send_IPI_allbutself
>   cpu_clear(smp_processor_id(), mask);
>   send_IPI_mask
>     send_IPI_mask_sequence
>       apic_wait_icr_idle
> 
> * mach-numaq
> send_IPI_allbutself
>   cpu_clear(smp_processor_id(), mask)
>   send_IPI_mask
>     send_IPI_mask_sequence
>       apic_wait_icr_idle
> 
> * mach-summit
> send_IPI_allbutself
>   cpu_clear(smp_processor_id(), mask)
>   send_IPI_mask
>     send_IPI_mask_sequence
>       apic_wait_icr_idle
> 
> Regards,
> 
> Fernando
> 
> > 
> > 
> > >> It would be far cheaper to define
> > >> a per-cpu variable containing the logical cpu number, set that variable
> > >> once as each cpu is brought up and just read it in cases where you
> > >> might not trust the integrity of struct thread_info.  safe_smp_processor_id()
> > >> resolves to just a read of the per cpu variable.
> > > But to read a per-cpu variable you need to index the corresponding array
> > > with processor id of the current CPU (see code below), but that is
> > > precisely what we are trying to figure out. Anyway as
> > > send_IPI_allbutself is not a fast path (correct if this assumption is
> > > wrong) the current implementation of safe_smp_processor_id should be
> > > fine.
> > >
> > > #define get_cpu_var(var) (*({ preempt_disable();
> > > &__get_cpu_var(var); }))
> > > #define __get_cpu_var(var) per_cpu(var, smp_processor_id())
> > >
> > > Am I missing something obvious?
> > 
> > No.  Except that other architectures have cheaper per pointers so they
> > don't have that problem.
> > 
> > Eric
> 
> _______________________________________________
> fastboot mailing list
> fastboot@lists.osdl.org
> https://lists.osdl.org/mailman/listinfo/fastboot

Re: [Fastboot] [PATCH 1/3] stack overflow safe kdump (2.6.18-rc1-i386) - safe_smp_processor_id

[Keith Owens]

Fernando Luis =?ISO-8859-1?Q?V=E1zquez?= Cao (on Tue, 11 Jul 2006 13:21:01 +0900) wrote:
>That is a good idea, but I have on concern. In mach-default by default
>we use __send_IPI_shortcut (no_broadcast==0) instead of send_IPI_mask.
>Is it always safe to ignore the no_broadcast setting? In other words,
>can __send_IPI_shortcut be replaced by send_IPI_mask safely?

It is always safe to use send_IPI_mask.  It is not used by default
because of concerns that send_IPI_mask may be slower than using a
broadcast, although I do not know if anybody has measurements to back
up that concern.  OTOH I can guarantee that sending NMI as a broadcast
has problems, it breaks some Dell Xeon servers[1].  My fix was to never
broadcast NMI, from 2.6.18-rc1 NMI_VECTOR always uses a mask[2] and
crash was changed accordingly[3].

[1] http://marc.theaimsgroup.com/?t=114828920800003&r=1&w=2
[2] http://marc.theaimsgroup.com/?t=115103727400006&r=1&w=2
[3] http://marc.theaimsgroup.com/?t=115096703800003&r=1&w=2

Re: [Fastboot] [PATCH 1/3] stack overflow safe kdump (2.6.18-rc1-i386) - safe_smp_processor_id

[Fernando Luis Vázquez Cao]

Hi Keith!

On Tue, 2006-07-11 at 14:55 +1000, Keith Owens wrote:
> Fernando Luis =?ISO-8859-1?Q?V=E1zquez?= Cao (on Tue, 11 Jul 2006 13:21:01 +0900) wrote:
> >That is a good idea, but I have on concern. In mach-default by default
> >we use __send_IPI_shortcut (no_broadcast==0) instead of send_IPI_mask.
> >Is it always safe to ignore the no_broadcast setting? In other words,
> >can __send_IPI_shortcut be replaced by send_IPI_mask safely?
> 
> It is always safe to use send_IPI_mask.  It is not used by default
> because of concerns that send_IPI_mask may be slower than using a
> broadcast, although I do not know if anybody has measurements to back
> up that concern.  OTOH I can guarantee that sending NMI as a broadcast
> has problems, it breaks some Dell Xeon servers[1].  My fix was to never
> broadcast NMI, from 2.6.18-rc1 NMI_VECTOR always uses a mask[2] and
> crash was changed accordingly[3].
> 
> [1] http://marc.theaimsgroup.com/?t=114828920800003&r=1&w=2
> [2] http://marc.theaimsgroup.com/?t=115103727400006&r=1&w=2
> [3] http://marc.theaimsgroup.com/?t=115096703800003&r=1&w=2

Thank you for the links (I had forgotten about that thread) and
comments!
I prepared new patches and hopefully I got it right this time, Do they
look good this time (PATCH 4/4 in particular)?

Thank you in advance,

Fernando

Re: [Fastboot] [PATCH 1/3] stack overflow safe kdump (2.6.18-rc1-i386) - safe_smp_processor_id

[Fernando Luis Vázquez Cao]

Hi James,

Thank you for taking the time to review the code!

On Mon, 2006-07-10 at 15:58 -0500, James Bottomley wrote:
> On Mon, 2006-07-10 at 12:20 -0600, Eric W. Biederman wrote:
> > I agree that it shows the problem, and that voyager is different from the
> > rest of the x86 implementations. 
> 
> As a non-apic based SMP implementation, I don't think there was ever any
> dissent about the latter.
> 
> > At least for things like the cpumask_t density of processor ids
> > is still an interesting property.  The basic issue is that apicids are
> > not in general dense on x86.  Not being able compile with support
> > for only two cpus because your cpus happen to be apicid 0 and apicid
> > 6 by default is an issue.
> 
> Density or lack of it is pretty much irrelevant nowadays since the CPU
> map iterators are sparse efficient.  Whether x86 PC chooses to avail
> itself of this or not is the business of the PC subarch maintainers.
> The vast marjority of non-x86 SMP implementations still have sparse (or
> at least physical only) CPU maps.
> 
> > To some extent this also shows the mess that the x86 subarch code is
> > because it is never clear if code is implemented in a subarchitecture
> > or not.
> 
> Erm, it does?  How?  My statement is that introducing subarch specific
> #defines into subarch independent header files is a problem (which it
> is).  If you grep for subarch defines in the rest of the arch
> independent headers, I don't believe you'll find any.  This would rather
> tend to show that for the last seven years, the subarch interface has
> been remarkably effective ....
> 
> > Fernando can you just put a trivial voyager specific definition of
> > safe_smp_processor_id in mach-voyager/voyager_smp.c.  It isn't a fast
> > path so the little extra overhead of making two separate functions
> > is not an issue and then the generic header doesn't have to have
> > subarch breakage.  Just a definition of safe_smp_processor_id().
> 
> Yes, that should work.
Done. I hope I got it right this time. Anyway, if there is something
incorrect in the new patches (1/4 and 2/4 in particular) let me know.

Regards,

Fernando

Re: [Fastboot] [PATCH 1/3] stack overflow safe kdump (2.6.18-rc1-i386) - safe_smp_processor_id

[Keith Owens]

Fernando Luis =?ISO-8859-1?Q?V=E1zquez?= Cao (on Tue, 11 Jul 2006 15:15:14 +0900) wrote:
>I prepared new patches and hopefully I got it right this time, Do they
>look good this time (PATCH 4/4 in particular)?

Looks good.

Re: [Fastboot] [PATCH 1/3] stack overflow safe kdump (2.6.18-rc1-i386) - safe_smp_processor_id

[Fernando Luis Vázquez Cao]

On Mon, 2006-07-10 at 22:04 +1000, Keith Owens wrote:
> Fernando Luis =?ISO-8859-1?Q?V=E1zquez?= Cao (on Mon, 10 Jul 2006 19:15:50 +0900) wrote:
> >Hi Keith,
> >
> >Thank you for the comments.
> >
> >On Mon, 2006-07-10 at 18:27 +1000, Keith Owens wrote:
> >> Fernando Luis Vazquez Cao (on Mon, 10 Jul 2006 16:50:52 +0900) wrote:
> >> >On the event of a stack overflow critical data that usually resides at
> >> >the bottom of the stack is likely to be stomped and, consequently, its
> >> >use should be avoided.
> >> >
> >> >In particular, in the i386 and IA64 architectures the macro
> >> >smp_processor_id ultimately makes use of the "cpu" member of struct
> >> >thread_info which resides at the bottom of the stack. x86_64, on the
> >> >other hand, is not affected by this problem because it benefits from
> >> >the use of the PDA infrastructure.
> >> >
> >> >To circumvent this problem I suggest implementing
> >> >"safe_smp_processor_id()" (it already exists in x86_64) for i386 and
> >> >IA64 and use it as a replacement for smp_processor_id in the reboot path
> >> >to the dump capture kernel. This is a possible implementation for i386.
> >> =
> >
> >> I agree with avoiding the use of thread_info when the stack might be
> >> corrupt.  However your patch results in reading apic data and scanning
> >> NR_CPU sized tables for each IPI that is sent, which will slow down the
> >> sending of all IPIs, not just dump.
> >This patch only affects IPIs sent using send_IPI_allbutself which is
> >rarely called, so the impact in performance should be negligible.
> 
> The main users of send_IPI_allbutself() are smp_call_function() and
> on_each_cpu(), which are used quite often.  My main concern are the
> architectures that use IPI to flush TLB entries from other cpus.  For
> example, i386 ioremap_page_range() -> flush_tlb_all() -> on_each_cpu().
> 
> >> It would be far cheaper to define
> >> a per-cpu variable containing the logical cpu number, set that variable
> >> once as each cpu is brought up and just read it in cases where you
> >> might not trust the integrity of struct thread_info.  safe_smp_processor_=
> >id()
> >> resolves to just a read of the per cpu variable.
> >But to read a per-cpu variable you need to index the corresponding array
> >with processor id of the current CPU (see code below), but that is
> >precisely what we are trying to figure out.
> 
> Ouch, I am so used to ia64 where accessing the local per cpu variables
> is a direct read, with no need to use smp_processor_id().
> 
> The use of smp_processor_id() in include/asm-generic/percpu.h is
> worrying, it means that any RAS code like dump or debuggers cannot
> access any per cpu variables.  Corrupt the kernel stack and all per cpu
> variables become useless!  That is a hidden bug, just waiting to bite
> all the RAS code.
Agreed. Some time ago I considered implementing something similar to
x86_64's PDA for i386 using the gs register. However, I discovered that
a similar approach had been discussed before and finally discarded,
because access to this segment register in i386 architectures was deemed
too slow.

Regards,

Fernando

> 
> ia64, x86_64, power, s390, sparc64 do not suffer from this problem,
> they have efficient implementations of __get_cpu_var().  All other
> architectures (including i386) use the generic percpu code and per cpu
> variables will not work with corrupt kernel stacks.

Re: [PATCH 1/3] stack overflow safe kdump (2.6.18-rc1-i386) - safe_smp_processor_id

[James Bottomley]

On Mon, 2006-07-10 at 21:42 -0600, Eric W. Biederman wrote:
> But I do agree the subarch header files are clean.
> And no this case except for the fact no one realized that the
> code doesn't even compile on voyager does not show how brittle
> the x86 subarch code is.    Except for the fact that it seems
> obvious that kernel/smp.c is generic code that every smp subarch
> would use.

OK ... that's the mistaken assumption.  kernel/smp.c is not subarch
generic, it's APIC specific.  So all apic using subarchs, which is
pretty much everything except voyager, use it.  Since voyager uses
vic/qic based smp harness, it has its own version of this file (in fact
voyager has a completely separate SMP HAL).

James

Re: [PATCH 1/3] stack overflow safe kdump (2.6.18-rc1-i386) - safe_smp_processor_id

[Eric W. Biederman]

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

> On Mon, 2006-07-10 at 21:42 -0600, Eric W. Biederman wrote:
>> But I do agree the subarch header files are clean.
>> And no this case except for the fact no one realized that the
>> code doesn't even compile on voyager does not show how brittle
>> the x86 subarch code is.    Except for the fact that it seems
>> obvious that kernel/smp.c is generic code that every smp subarch
>> would use.
>
> OK ... that's the mistaken assumption.  kernel/smp.c is not subarch
> generic, it's APIC specific.  So all apic using subarchs, which is
> pretty much everything except voyager, use it.  Since voyager uses
> vic/qic based smp harness, it has its own version of this file (in fact
> voyager has a completely separate SMP HAL).

Yep.  My point is that with the current subarch structure on x86 it is
really easy to make mistaken assumptions like kernel/smp.c applies to
all x86 subarchitectures, because the lines are not clear.  The
architectures where I have seen that the lines are clear generally
allow for building a single kernel that can boot on any subarch.

My hope is that we can recognized how non-obvious the x86 subarch code
is so that future work will be able to improve the situation.

To give credit I do think the division of labor between the subarch's
appears sound.  I just don't like how the subarches are glued together
into the x86 arch.

Eric