stack overflow safe kdump (2.6.18-rc1-i386) - safe_smp_processor_id

PATCH In review Fernando Luis Vázquez Cao 4 participants 18 messages Jul 10, 2006 – Jul 11, 2006

Share & tools

Fernando Luis Vázquez Cao

author

2006-07-10 07:50 UTC #0 lore raw

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

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 <[email protected]>

---

arch/i386/kernel/smp.c +26 -0

diff -urNp linux-2.6.18-rc1/arch/i386/kernel/smp.c linux-2.6.18-rc1-sof/arch/i386/kernel/smp.c
@@ -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;
+}

include/asm-i386/smp.h +8 -0

diff -urNp linux-2.6.18-rc1/include/asm-i386/smp.h linux-2.6.18-rc1-sof/include/asm-i386/smp.h
@@ -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 */

Keith Owens

2006-07-10 08:28 UTC #1 lore raw

Quoted 10 lines · Fernando Luis =?ISO-8859-1?Q?V=E1zquez?= Cao (on Mon, 10 Jul 2006 16:50:52…

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.

Quoted 4 lines

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.

Fernando Luis Vázquez Cao

author

2006-07-10 10:15 UTC #2 lore raw

Hi Keith,

Thank you for the comments.

Quoted 16 lines · Keith Owens

Quoted 10 lines · 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.
Quoted 4 lines
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.

Quoted 4 lines

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.

Quoted 5 lines

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

Eric W. Biederman

2006-07-10 11:38 UTC #3 lore raw

Quoted 21 lines · Fernando Luis Vázquez Cao

Hi Keith,
Thank you for the comments.
Quoted 16 lines · Keith Owens
Quoted 14 lines · 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.

Quoted 6 lines

Quoted 4 lines
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.

Quoted 12 lines

Quoted 5 lines
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.

Quoted 5 lines

#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

Fernando Luis Vázquez Cao

author

2006-07-11 04:21 UTC #4 lore raw

Hi Eric!

Quoted 40 lines · Eric W. Biederman

Quoted 21 lines · Fernando Luis Vázquez Cao
Hi Keith,
Thank you for the comments.
Quoted 16 lines · Keith Owens
Quoted 14 lines · 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.
Quoted 6 lines
Quoted 4 lines
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)

Quoted 8 lines

{
	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

Quoted 24 lines

Quoted 14 lines
Quoted 7 lines
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.
Quoted 5 lines
#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

Fernando Luis Vázquez Cao

author

2006-07-11 04:44 UTC #5 lore raw

Quoted 49 lines · Fernando Luis Vázquez Cao

Hi Eric!
Quoted 46 lines · Eric W. Biederman
Quoted 21 lines · Fernando Luis Vázquez Cao
Hi Keith,
Thank you for the comments.
Quoted 16 lines · Keith Owens
Quoted 14 lines · 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.
Quoted 6 lines
Quoted 4 lines
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);
}

Quoted 8 lines

Quoted 2 lines
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...

Quoted 81 lines

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
Quoted 18 lines
Quoted 18 lines
Quoted 7 lines
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())
Quoted 6 lines
Quoted 1 line
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
[email protected]
https://lists.osdl.org/mailman/listinfo/fastboot

Keith Owens

2006-07-11 04:56 UTC #6 lore raw

In reply to Fernando Luis Vázquez Cao

Quoted 4 lines · Fernando Luis =?ISO-8859-1?Q?V=E1zquez?= Cao (on Tue, 11 Jul 2006 13:21:01…

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

Fernando Luis Vázquez Cao

author

2006-07-11 06:15 UTC #7 lore raw

Hi Keith!

Quoted 14 lines · Keith Owens

Quoted 4 lines · Fernando Luis =?ISO-8859-1?Q?V=E1zquez?= Cao (on Tue, 11 Jul 2006 13:21:01…
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].

Quoted 3 lines

[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

Keith Owens

2006-07-11 06:25 UTC #8 lore raw

Quoted 2 lines · Fernando Luis =?ISO-8859-1?Q?V=E1zquez?= Cao (on Tue, 11 Jul 2006 15:15:14…

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.

Keith Owens

2006-07-10 12:04 UTC #9 lore raw

In reply to Fernando Luis Vázquez Cao

Quoted 20 lines · Fernando Luis =?ISO-8859-1?Q?V=E1zquez?= Cao (on Mon, 10 Jul 2006 19:15:50…

Hi Keith,
Thank you for the comments.
Quoted 15 lines · Keith Owens
Quoted 14 lines · 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.

Quoted 8 lines

Quoted 6 lines
=
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().

Quoted 9 lines

Quoted 4 lines
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()
Quoted 1 line
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.

Fernando Luis Vázquez Cao

author

2006-07-11 06:40 UTC #10 lore raw

Quoted 45 lines · Keith Owens

Quoted 20 lines · Fernando Luis =?ISO-8859-1?Q?V=E1zquez?= Cao (on Mon, 10 Jul 2006 19:15:50…
Hi Keith,
Thank you for the comments.
Quoted 15 lines · Keith Owens
Quoted 14 lines · 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.
Quoted 8 lines
Quoted 6 lines
=
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().
Quoted 9 lines
Quoted 4 lines
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()
Quoted 1 line
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.

Quoted 8 lines

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

Quoted 5 lines

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.

James Bottomley

2006-07-10 14:17 UTC #11 lore raw

In reply to Fernando Luis Vázquez Cao

Quoted 7 lines · Fernando Luis Vázquez Cao

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

include/asm-i386/smp.h static __inline int logical_smp_processo · line 98

Quoted 10 lines

include/asm-i386/smp.h +7 -0

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

Eric W. Biederman

2006-07-10 18:21 UTC #12 lore raw

Quoted 25 lines · James Bottomley

Quoted 7 lines · Fernando Luis Vázquez Cao
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
include/asm-i386/smp.h static __inline int logical_smp_processo · line 98
Quoted 10 lines
include/asm-i386/smp.h +7 -0
@@ -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.

Quoted 5 lines

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

James Bottomley

2006-07-10 22:48 UTC #13 lore raw

Quoted 2 lines · Eric W. Biederman

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.

Quoted 5 lines

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.

Quoted 3 lines

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

Quoted 5 lines

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

Eric W. Biederman

2006-07-11 03:43 UTC #14 lore raw

Quoted 11 lines · James Bottomley

Quoted 4 lines
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

James Bottomley

2006-07-11 12:36 UTC #15 lore raw

Quoted 6 lines · Eric W. Biederman

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

Eric W. Biederman

2006-07-11 19:42 UTC #16 lore raw

Quoted 8 lines · James Bottomley

Quoted 6 lines · Eric W. Biederman
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.

Quoted 5 lines

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

Fernando Luis Vázquez Cao

author

2006-07-11 06:21 UTC #17 lore raw

In reply to James Bottomley

Hi James,

Thank you for taking the time to review the code!

Quoted 30 lines · James Bottomley

Quoted 2 lines · Eric W. Biederman
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.
Quoted 5 lines
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.
Quoted 3 lines
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 ....

Quoted 7 lines

Quoted 5 lines
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