[2.6.23-rc1 REGRESSION] CPU hotplug totally broken on HPC nx6325 (x86_64)

[2.6.23-rc1 REGRESSION] CPU hotplug totally broken on HPC nx6325 (x86_64)

[Rafael J. Wysocki]

Hi,

On my Turion64-based HPC nx6325 with the 2.6.23-rc1 x86_64 kernel doing

# echo 0 > /sys/devices/system/cpu/cpu1/online

causes the system to crash in a spectacular fashion (call traces going
continuously on the console, no reaction to anything except for the power
button).  For this reason, suspend and hibernation don't work as well.

Bisection has shown that your commit 19d36ccdc34f5ed444f8a6af0cbfdb6790eb1177
"x86: Fix alternatives and kprobes to remap write-protected kernel text" causes
that to happen and indeed after applying the appended patch the problem doesn't
show up any more.

[In the first version of the appended patch I forgot to add the chunk in
mark_rodata_ro() and thus I got an oops from the line added to nop_out().
I wonder if that might be related to the $subject problem.]

Strangely enough, everything works fine on my second test box based on a
dual-core Athlon 64.

Unfortunately, I can't get any debugging information from the box after the
failure.  I can try to set up netconsole, but I don't really expect it to work.

Greetings,
Rafael


---
 arch/i386/kernel/alternative.c |    8 ++++++--
 arch/i386/kernel/kprobes.c     |    8 ++++++--
 arch/x86_64/kernel/kprobes.c   |    9 +++++++--
 arch/x86_64/mm/init.c          |   10 ++++++++++
 4 files changed, 29 insertions(+), 6 deletions(-)

Index: linux-2.6.23-rc1/arch/i386/kernel/alternative.c
===================================================================
--- linux-2.6.23-rc1.orig/arch/i386/kernel/alternative.c
+++ linux-2.6.23-rc1/arch/i386/kernel/alternative.c
@@ -156,7 +156,7 @@ static void nop_out(void *insns, unsigne
 		unsigned int noplen = len;
 		if (noplen > ASM_NOP_MAX)
 			noplen = ASM_NOP_MAX;
-		text_poke(insns, noptable[noplen], noplen);
+		memcpy(insns, noptable[noplen], noplen);
 		insns += noplen;
 		len -= noplen;
 	}
@@ -208,7 +208,7 @@ static void alternatives_smp_lock(u8 **s
 			continue;
 		if (*ptr > text_end)
 			continue;
-		text_poke(*ptr, ((unsigned char []){0xf0}), 1); /* add lock prefix */
+		**ptr = 0xf0; /* lock prefix */
 	};
 }
 
@@ -366,6 +366,10 @@ void apply_paravirt(struct paravirt_patc
 		/* Pad the rest with nops */
 		nop_out(p->instr + used, p->len - used);
 	}
+
+	/* Sync to be conservative, in case we patched following
+	 * instructions */
+	sync_core();
 }
 extern struct paravirt_patch_site __start_parainstructions[],
 	__stop_parainstructions[];
Index: linux-2.6.23-rc1/arch/i386/kernel/kprobes.c
===================================================================
--- linux-2.6.23-rc1.orig/arch/i386/kernel/kprobes.c
+++ linux-2.6.23-rc1/arch/i386/kernel/kprobes.c
@@ -170,12 +170,16 @@ int __kprobes arch_prepare_kprobe(struct
 
 void __kprobes arch_arm_kprobe(struct kprobe *p)
 {
-	text_poke(p->addr, ((unsigned char []){BREAKPOINT_INSTRUCTION}), 1);
+	*p->addr = BREAKPOINT_INSTRUCTION;
+	flush_icache_range((unsigned long) p->addr,
+			   (unsigned long) p->addr + sizeof(kprobe_opcode_t));
 }
 
 void __kprobes arch_disarm_kprobe(struct kprobe *p)
 {
-	text_poke(p->addr, &p->opcode, 1);
+	*p->addr = p->opcode;
+	flush_icache_range((unsigned long) p->addr,
+			   (unsigned long) p->addr + sizeof(kprobe_opcode_t));
 }
 
 void __kprobes arch_remove_kprobe(struct kprobe *p)
Index: linux-2.6.23-rc1/arch/x86_64/kernel/kprobes.c
===================================================================
--- linux-2.6.23-rc1.orig/arch/x86_64/kernel/kprobes.c
+++ linux-2.6.23-rc1/arch/x86_64/kernel/kprobes.c
@@ -39,6 +39,7 @@
 #include <linux/module.h>
 #include <linux/kdebug.h>
 
+#include <asm/cacheflush.h>
 #include <asm/pgtable.h>
 #include <asm/uaccess.h>
 #include <asm/alternative.h>
@@ -209,12 +210,16 @@ static void __kprobes arch_copy_kprobe(s
 
 void __kprobes arch_arm_kprobe(struct kprobe *p)
 {
-	text_poke(p->addr, ((unsigned char []){BREAKPOINT_INSTRUCTION}), 1);
+	*p->addr = BREAKPOINT_INSTRUCTION;
+	flush_icache_range((unsigned long) p->addr,
+			   (unsigned long) p->addr + sizeof(kprobe_opcode_t));
 }
 
 void __kprobes arch_disarm_kprobe(struct kprobe *p)
 {
-	text_poke(p->addr, &p->opcode, 1);
+	*p->addr = p->opcode;
+	flush_icache_range((unsigned long) p->addr,
+			   (unsigned long) p->addr + sizeof(kprobe_opcode_t));
 }
 
 void __kprobes arch_remove_kprobe(struct kprobe *p)
Index: linux-2.6.23-rc1/arch/x86_64/mm/init.c
===================================================================
--- linux-2.6.23-rc1.orig/arch/x86_64/mm/init.c
+++ linux-2.6.23-rc1/arch/x86_64/mm/init.c
@@ -600,6 +600,16 @@ void mark_rodata_ro(void)
 {
 	unsigned long start = (unsigned long)_stext, end;
 
+#ifdef CONFIG_HOTPLUG_CPU
+	/* It must still be possible to apply SMP alternatives. */
+	if (num_possible_cpus() > 1)
+		start = (unsigned long)_etext;
+#endif
+
+#ifdef CONFIG_KPROBES
+	start = (unsigned long)__start_rodata;
+#endif
+	
 	end = (unsigned long)__end_rodata;
 	start = (start + PAGE_SIZE - 1) & PAGE_MASK;
 	end &= PAGE_MASK;

Re: [2.6.23-rc1 REGRESSION] CPU hotplug totally broken on HPC nx6325 (x86_64)

[Linus Torvalds]

On Thu, 26 Jul 2007, Rafael J. Wysocki wrote:
>
> On my Turion64-based HPC nx6325 with the 2.6.23-rc1 x86_64 kernel doing
> 
> # echo 0 > /sys/devices/system/cpu/cpu1/online
> 
> causes the system to crash in a spectacular fashion (call traces going
> continuously on the console, no reaction to anything except for the power
> button).  For this reason, suspend and hibernation don't work as well.

Yeah, I really shouldn't have applied that patch. I didn't notice that it 
not only cleaned up the direct memcpy's, it also re-introduced the damn 
broken code that we fixed once already.

Dammit, that read-only debug support IS NOT WORTH THIS CRAP.

I absolutely *detest* it when "debugging features" end up being the thing 
that causes crashes. I think it shows a total lack of taste and 
understanding, and I'm totally tired of it. This has happened too many 
times.

Andi: please don't send this patch *ever* again. If a patch that is 
supposed to help debugging just causes problems, that patch should be 
thrown away FOREVER.

Andrew - on that same note: please throw away the dwarf traceback crap 
from your tree that Andi is still apparently pushing. Exact same issue. 

Debugging "helper" code that has historically only caused problems. We 
could equally well just enable frame pointers for debugging and add the 
trivial code to follow that instead, and it would work (the way it has 
worked on x86 basically forever).

Rafael, does reverting just this part (and leaving the "text_poke()" 
cleanups) work for you?

			Linus

---
 arch/i386/kernel/alternative.c |   10 ----------
 arch/i386/mm/init.c            |   14 +++++++++++---
 arch/x86_64/mm/init.c          |   10 ++++++++++
 3 files changed, 21 insertions(+), 13 deletions(-)

diff --git a/arch/i386/kernel/alternative.c b/arch/i386/kernel/alternative.c
index c3750c2..c0d0a89 100644
--- a/arch/i386/kernel/alternative.c
+++ b/arch/i386/kernel/alternative.c
@@ -432,20 +432,10 @@ void __init alternative_instructions(void)
  */
 void __kprobes text_poke(void *oaddr, unsigned char *opcode, int len)
 {
-        u8 *addr = oaddr;
-	if (!pte_write(*lookup_address((unsigned long)addr))) {
-		struct page *p[2] = { virt_to_page(addr), virt_to_page(addr+PAGE_SIZE) };
-		addr = vmap(p, 2, VM_MAP, PAGE_KERNEL);
-		if (!addr)
-			return;
-		addr += ((unsigned long)oaddr) % PAGE_SIZE;
-	}
 	memcpy(addr, opcode, len);
 	sync_core();
 	/* Not strictly needed, but can speed CPU recovery up. Ignore cross cacheline
 	   case. */
 	if (cpu_has_clflush)
 		asm("clflush (%0) " :: "r" (oaddr) : "memory");
-	if (addr != oaddr)
-		vunmap(addr);
 }
diff --git a/arch/i386/mm/init.c b/arch/i386/mm/init.c
index 1b1a1e6..4c4809f 100644
--- a/arch/i386/mm/init.c
+++ b/arch/i386/mm/init.c
@@ -800,9 +800,17 @@ void mark_rodata_ro(void)
 	unsigned long start = PFN_ALIGN(_text);
 	unsigned long size = PFN_ALIGN(_etext) - start;
 
-	change_page_attr(virt_to_page(start),
-	                 size >> PAGE_SHIFT, PAGE_KERNEL_RX);
-	printk("Write protecting the kernel text: %luk\n", size >> 10);
+#ifndef CONFIG_KPROBES
+#ifdef CONFIG_HOTPLUG_CPU
+	/* It must still be possible to apply SMP alternatives. */
+	if (num_possible_cpus() <= 1)
+#endif
+	{
+		change_page_attr(virt_to_page(start),
+		                 size >> PAGE_SHIFT, PAGE_KERNEL_RX);
+		printk("Write protecting the kernel text: %luk\n", size >> 10);
+	}
+#endif
 	start += size;
 	size = (unsigned long)__end_rodata - start;
 	change_page_attr(virt_to_page(start),
diff --git a/arch/x86_64/mm/init.c b/arch/x86_64/mm/init.c
index 38f5d63..458893b 100644
--- a/arch/x86_64/mm/init.c
+++ b/arch/x86_64/mm/init.c
@@ -600,6 +600,16 @@ void mark_rodata_ro(void)
 {
 	unsigned long start = (unsigned long)_stext, end;
 
+#ifdef CONFIG_HOTPLUG_CPU
+	/* It must still be possible to apply SMP alternatives. */
+	if (num_possible_cpus() > 1)
+		start = (unsigned long)_etext;
+#endif
+
+#ifdef CONFIG_KPROBES
+	start = (unsigned long)__start_rodata;
+#endif
+	
 	end = (unsigned long)__end_rodata;
 	start = (start + PAGE_SIZE - 1) & PAGE_MASK;
 	end &= PAGE_MASK;

Re: [2.6.23-rc1 REGRESSION] CPU hotplug totally broken on HPC nx6325 (x86_64)

[Rafael J. Wysocki]

On Thursday, 26 July 2007 18:43, Linus Torvalds wrote:
> 
> On Thu, 26 Jul 2007, Rafael J. Wysocki wrote:
> >
> > On my Turion64-based HPC nx6325 with the 2.6.23-rc1 x86_64 kernel doing
> > 
> > # echo 0 > /sys/devices/system/cpu/cpu1/online
> > 
> > causes the system to crash in a spectacular fashion (call traces going
> > continuously on the console, no reaction to anything except for the power
> > button).  For this reason, suspend and hibernation don't work as well.
> 
> Yeah, I really shouldn't have applied that patch. I didn't notice that it 
> not only cleaned up the direct memcpy's, it also re-introduced the damn 
> broken code that we fixed once already.
> 
> Dammit, that read-only debug support IS NOT WORTH THIS CRAP.
> 
> I absolutely *detest* it when "debugging features" end up being the thing 
> that causes crashes. I think it shows a total lack of taste and 
> understanding, and I'm totally tired of it. This has happened too many 
> times.
> 
> Andi: please don't send this patch *ever* again. If a patch that is 
> supposed to help debugging just causes problems, that patch should be 
> thrown away FOREVER.
> 
> Andrew - on that same note: please throw away the dwarf traceback crap 
> from your tree that Andi is still apparently pushing. Exact same issue. 
> 
> Debugging "helper" code that has historically only caused problems. We 
> could equally well just enable frame pointers for debugging and add the 
> trivial code to follow that instead, and it would work (the way it has 
> worked on x86 basically forever).
> 
> Rafael, does reverting just this part (and leaving the "text_poke()" 
> cleanups) work for you?

Yes, it does, with the appended fix on top. :-)

Greetings,
Rafael


---
 arch/i386/kernel/alternative.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Index: linux-2.6.23-rc1/arch/i386/kernel/alternative.c
===================================================================
--- linux-2.6.23-rc1.orig/arch/i386/kernel/alternative.c
+++ linux-2.6.23-rc1/arch/i386/kernel/alternative.c
@@ -432,7 +432,7 @@ void __init alternative_instructions(voi
  */
 void __kprobes text_poke(void *oaddr, unsigned char *opcode, int len)
 {
-	memcpy(addr, opcode, len);
+	memcpy(oaddr, opcode, len);
 	sync_core();
 	/* Not strictly needed, but can speed CPU recovery up. Ignore cross cacheline
 	   case. */

Re: [2.6.23-rc1 REGRESSION] CPU hotplug totally broken on HPC nx6325 (x86_64)

[Linus Torvalds]

On Thu, 26 Jul 2007, Rafael J. Wysocki wrote:
> > 
> > Rafael, does reverting just this part (and leaving the "text_poke()" 
> > cleanups) work for you?
> 
> Yes, it does, with the appended fix on top. :-)

Heh, I noticed that myself, but assumed you'd figure it out. 

I ended up renaming "oaddr" as "addr", so the thing I committed is 
slightly different from the patch (+ your fix), but it should be otherwise 
100% equivalent.

		Linus

Re: [2.6.23-rc1 REGRESSION] CPU hotplug totally broken on HPC nx6325 (x86_64)

[Chris Wright]

* Linus Torvalds (torvalds@linux-foundation.org) wrote:
> On Thu, 26 Jul 2007, Rafael J. Wysocki wrote:
> > On my Turion64-based HPC nx6325 with the 2.6.23-rc1 x86_64 kernel doing
> > 
> > # echo 0 > /sys/devices/system/cpu/cpu1/online
> > 
> > causes the system to crash in a spectacular fashion (call traces going
> > continuously on the console, no reaction to anything except for the power
> > button).  For this reason, suspend and hibernation don't work as well.
> 
> Yeah, I really shouldn't have applied that patch. I didn't notice that it 
> not only cleaned up the direct memcpy's, it also re-introduced the damn 
> broken code that we fixed once already.

This also fixes paravirt patching which was broken when text_poke()
tried to patch the various pv ops in lookup_address.

thanks,
-chris

Re: [2.6.23-rc1 REGRESSION] CPU hotplug totally broken on HPC nx6325 (x86_64)

[Linus Torvalds]

On Mon, 30 Jul 2007, Chris Wright wrote:
> 
> This also fixes paravirt patching which was broken when text_poke()
> tried to patch the various pv ops in lookup_address.

Hmm. What is "this"? The revert? 

That said, I do wonder whether virtualization still has problems with 
CONFIG_RODATA, though. We limit the RODATA memory ranges based on KPROBES 
and HOTPLUG_CPU, but not based on VIRTUALIZATION.

I'd expect any virtualization fixups to hit the same problems that the SMP 
alternatives hit. No?

		Linus

Re: [2.6.23-rc1 REGRESSION] CPU hotplug totally broken on HPC nx6325 (x86_64)

[Chris Wright]

* Linus Torvalds (torvalds@linux-foundation.org) wrote:
> On Mon, 30 Jul 2007, Chris Wright wrote:
> > This also fixes paravirt patching which was broken when text_poke()
> > tried to patch the various pv ops in lookup_address.
> 
> Hmm. What is "this"? The revert? 

Yes, sorry, your revert also fixes paravirt patching.

> That said, I do wonder whether virtualization still has problems with 
> CONFIG_RODATA, though. We limit the RODATA memory ranges based on KPROBES 
> and HOTPLUG_CPU, but not based on VIRTUALIZATION.
> 
> I'd expect any virtualization fixups to hit the same problems that the SMP 
> alternatives hit. No?

Hmm, patching should've already happened, and aside of module loading,
isn't typically done again.  I think it's OK as it is.

thanks,
-chris