GIT head no longer boots on x86-64

GIT head no longer boots on x86-64

[Alan Cox]

The boot ends on the first module load with a crash

	BUG: mm/vmalloc.c 118

From
	text_poke
	alternatives_smp_unlock
	alternatives_smp_module_add
	module_finalize
	load_module

while loading the EHCI driver

Re: GIT head no longer boots on x86-64

[Jiri Slaby]

On 10/13/2008 11:23 AM, Alan Cox wrote:
> The boot ends on the first module load with a crash
> 
> 	BUG: mm/vmalloc.c 118

Not 187? 118 is currently a line with `{'. Which git SHA? Is DEBUG_VIRTUAL=y?

> From
> 	text_poke
> 	alternatives_smp_unlock
> 	alternatives_smp_module_add
> 	module_finalize
> 	load_module
> 
> while loading the EHCI driver

Re: GIT head no longer boots on x86-64

[Alan Cox]

On Mon, 13 Oct 2008 12:13:24 +0200
Jiri Slaby <jirislaby@gmail.com> wrote:

> On 10/13/2008 11:23 AM, Alan Cox wrote:
> > The boot ends on the first module load with a crash
> > 
> > 	BUG: mm/vmalloc.c 118
> 
> Not 187? 118 is currently a line with `{'. Which git SHA? Is DEBUG_VIRTUAL=y?

Typo sorry - was copying them from across the room - line 188

Re: GIT head no longer boots on x86-64

[Jiri Slaby]

Could you try the debug patch below to see what address is text_poke trying
to translate?

---
 mm/vmalloc.c |    8 ++++++--
 1 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/mm/vmalloc.c b/mm/vmalloc.c
index bba06c4..8b8d0a4 100644
--- a/mm/vmalloc.c
+++ b/mm/vmalloc.c
@@ -179,13 +179,17 @@ struct page *vmalloc_to_page(const void *vmalloc_addr)
 	pud_t *pud;
 	pmd_t *pmd;
 	pte_t *ptep, pte;
+	char buf[KSYM_NAME_LEN];
 
 	/*
 	 * XXX we might need to change this if we add VIRTUAL_BUG_ON for
 	 * architectures that do not vmalloc module space
 	 */
-	VIRTUAL_BUG_ON(!is_vmalloc_addr(vmalloc_addr) &&
-			!is_module_address(addr));
+/*	VIRTUAL_BUG_ON(!is_vmalloc_addr(vmalloc_addr) &&
+			!is_module_address(addr));*/
+	sprint_symbol(buf, (unsigned long)__builtin_return_address(0));
+	if (!is_vmalloc_addr(vmalloc_addr) && !is_module_address(addr))
+		printk("BUG? %s (from %s): %p\n", __func__, buf, vmalloc_addr);
 
 	if (!pgd_none(*pgd)) {
 		pud = pud_offset(pgd, addr);
-- 
1.6.0.2

Re: GIT head no longer boots on x86-64

[Alan Cox]

On Mon, 13 Oct 2008 12:56:54 +0200
Jiri Slaby <jirislaby@gmail.com> wrote:

> Could you try the debug patch below to see what address is text_poke trying
> to translate?


BUG? vmalloc_to_page (from text_poke+0x30/0x14a): ffffffffa01e40b1
BUG? vmalloc_to_page (from text_poke+0x40/0x14a): ffffffffa01e50b1
BUG? vmalloc_to_page (from text_poke+0x30/0x14a): ffffffffa01e40da
BUG? vmalloc_to_page (from text_poke+0x40/0x14a): ffffffffa01e50da
BUG? vmalloc_to_page (from text_poke+0x30/0x14a): ffffffffa01e41eb
BUG? vmalloc_to_page (from text_poke+0x40/0x14a): ffffffffa01e51eb
BUG? vmalloc_to_page (from text_poke+0x30/0x14a): ffffffffa01e4446
BUG? vmalloc_to_page (from text_poke+0x40/0x14a): ffffffffa01e5446
BUG? vmalloc_to_page (from text_poke+0x30/0x14a): ffffffffa01e458d
BUG? vmalloc_to_page (from text_poke+0x40/0x14a): ffffffffa01e558d
BUG? vmalloc_to_page (from text_poke+0x30/0x14a): ffffffffa01e4621
BUG? vmalloc_to_page (from text_poke+0x40/0x14a): ffffffffa01e5621
BUG? vmalloc_to_page (from text_poke+0x30/0x14a): ffffffffa01e49a3
BUG? vmalloc_to_page (from text_poke+0x40/0x14a): ffffffffa01e59a3
BUG? vmalloc_to_page (from text_poke+0x30/0x14a): ffffffffa01e4a21
BUG? vmalloc_to_page (from text_poke+0x40/0x14a): ffffffffa01e5a21
BUG? vmalloc_to_page (from text_poke+0x30/0x14a): ffffffffa01e4a28
BUG? vmalloc_to_page (from text_poke+0x40/0x14a): ffffffffa01e5a28
BUG? vmalloc_to_page (from text_poke+0x30/0x14a): ffffffffa01e4a7a
BUG? vmalloc_to_page (from text_poke+0x40/0x14a): ffffffffa01e5a7a

and lots more

Re: GIT head no longer boots on x86-64

[Linus Torvalds]

On Mon, 13 Oct 2008, Alan Cox wrote:

> On Mon, 13 Oct 2008 12:56:54 +0200
> Jiri Slaby <jirislaby@gmail.com> wrote:
> 
> > Could you try the debug patch below to see what address is text_poke trying
> > to translate?
> 
> BUG? vmalloc_to_page (from text_poke+0x30/0x14a): ffffffffa01e40b1

Hmm. Last page of code being fixed up, perhaps?

Does this fix it?

(Totally untested. Of course.)

			Linus

---
 arch/x86/kernel/alternative.c |   13 ++++++-------
 1 files changed, 6 insertions(+), 7 deletions(-)

diff --git a/arch/x86/kernel/alternative.c b/arch/x86/kernel/alternative.c
index fb04e49..f4a8870 100644
--- a/arch/x86/kernel/alternative.c
+++ b/arch/x86/kernel/alternative.c
@@ -498,23 +498,22 @@ void *text_poke_early(void *addr, const void *opcode, size_t len)
  */
 void *__kprobes text_poke(void *addr, const void *opcode, size_t len)
 {
-	unsigned long flags;
+	unsigned long flags, offset;
 	char *vaddr;
-	int nr_pages = 2;
 	struct page *pages[2];
-	int i;
+	int i, nr_pages;
 
+	offset = (unsigned long) addr & ~PAGE_MASK;
+	nr_pages = (offset + len) > PAGE_SIZE ? 2 : 1;
 	if (!core_kernel_text((unsigned long)addr)) {
 		pages[0] = vmalloc_to_page(addr);
-		pages[1] = vmalloc_to_page(addr + PAGE_SIZE);
+		pages[1] = vmalloc_to_page(addr + len - 1);
 	} else {
 		pages[0] = virt_to_page(addr);
 		WARN_ON(!PageReserved(pages[0]));
-		pages[1] = virt_to_page(addr + PAGE_SIZE);
+		pages[1] = pages[0]+1;
 	}
 	BUG_ON(!pages[0]);
-	if (!pages[1])
-		nr_pages = 1;
 	vaddr = vmap(pages, nr_pages, VM_MAP, PAGE_KERNEL);
 	BUG_ON(!vaddr);
 	local_irq_save(flags);

Re: GIT head no longer boots on x86-64

[Jiri Slaby]

On 10/13/2008 05:03 PM, Linus Torvalds wrote:
> 
> On Mon, 13 Oct 2008, Alan Cox wrote:
> 
>> On Mon, 13 Oct 2008 12:56:54 +0200
>> Jiri Slaby <jirislaby@gmail.com> wrote:
>>
>>> Could you try the debug patch below to see what address is text_poke trying
>>> to translate?
>> BUG? vmalloc_to_page (from text_poke+0x30/0x14a): ffffffffa01e40b1
> 
> Hmm. Last page of code being fixed up, perhaps?
> 
> Does this fix it?

I don't think so. The patch below should.

More background:
I guess SMP kernel running on UP? In such a case the module .text
is patched to use UP locks before the module is added to the modules
list and it thinks there are no valid data at that place while
patching.

Could you test it? The bug disappeared here in qemu. I've checked
callers of the function, and it should not matter for them.

Also the !is_module_address(addr) test is useless now.

---
 include/linux/mm.h |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/include/linux/mm.h b/include/linux/mm.h
index c61ba10..45772fd 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -267,6 +267,10 @@ static inline int is_vmalloc_addr(const void *x)
 #ifdef CONFIG_MMU
 	unsigned long addr = (unsigned long)x;
 
+#ifdef CONFIG_X86_64
+	if (addr >= MODULES_VADDR && addr < MODULES_END)
+		return 1;
+#endif
 	return addr >= VMALLOC_START && addr < VMALLOC_END;
 #else
 	return 0;
-- 
1.6.0.2

Re: GIT head no longer boots on x86-64

[Alan Cox]

> I guess SMP kernel running on UP? In such a case the module .text

Yep

> is patched to use UP locks before the module is added to the modules
> list and it thinks there are no valid data at that place while
> patching.
> 
> Could you test it? The bug disappeared here in qemu. I've checked
> callers of the function, and it should not matter for them.

Seems to do the job.

Jiri 1 Linus 0

;)

Re: GIT head no longer boots on x86-64

[Ingo Molnar]

* Alan Cox <alan@lxorguk.ukuu.org.uk> wrote:

> > I guess SMP kernel running on UP? In such a case the module .text
> 
> Yep
> 
> > is patched to use UP locks before the module is added to the modules
> > list and it thinks there are no valid data at that place while
> > patching.
> > 
> > Could you test it? The bug disappeared here in qemu. I've checked
> > callers of the function, and it should not matter for them.
> 
> Seems to do the job.

Queued the fix below up in tip/x86/urgent for a merge to Linus later 
today. Thanks!

	Ingo

------------------>
>From 5870942537422066655816e971629aa729c023d8 Mon Sep 17 00:00:00 2001
From: Jiri Slaby <jirislaby@gmail.com>
Date: Mon, 13 Oct 2008 17:11:33 +0200
Subject: [PATCH] x86: fix CONFIG_DEBUG_VIRTUAL=y boot crash on x86-64

Alan reported a bootup crash in the module loader:

> BUG? vmalloc_to_page (from text_poke+0x30/0x14a): ffffffffa01e40b1

SMP kernel is running on UP, in such a case the module .text
is patched to use UP locks before the module is added to the modules
list and it thinks there are no valid data at that place while
patching.

Also the !is_module_address(addr) test is useless now.

Reported-by: Alan Cox <alan@lxorguk.ukuu.org.uk>
Signed-off-by: Ingo Molnar <mingo@elte.hu>
Tested-by: Alan Cox <alan@lxorguk.ukuu.org.uk>
---
 include/linux/mm.h |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/include/linux/mm.h b/include/linux/mm.h
index c61ba10..45772fd 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -267,6 +267,10 @@ static inline int is_vmalloc_addr(const void *x)
 #ifdef CONFIG_MMU
 	unsigned long addr = (unsigned long)x;
 
+#ifdef CONFIG_X86_64
+	if (addr >= MODULES_VADDR && addr < MODULES_END)
+		return 1;
+#endif
 	return addr >= VMALLOC_START && addr < VMALLOC_END;
 #else
 	return 0;

Re: GIT head no longer boots on x86-64

[Jiri Slaby]

On 10/15/2008 01:51 PM, Ingo Molnar wrote:
> Queued the fix below up in tip/x86/urgent for a merge to Linus later 
> today. Thanks!

Thanks. Omitted S-O-B below.

> From 5870942537422066655816e971629aa729c023d8 Mon Sep 17 00:00:00 2001
> From: Jiri Slaby <jirislaby@gmail.com>
> Date: Mon, 13 Oct 2008 17:11:33 +0200
> Subject: [PATCH] x86: fix CONFIG_DEBUG_VIRTUAL=y boot crash on x86-64
> 
> Alan reported a bootup crash in the module loader:
> 
>> BUG? vmalloc_to_page (from text_poke+0x30/0x14a): ffffffffa01e40b1
> 
> SMP kernel is running on UP, in such a case the module .text
> is patched to use UP locks before the module is added to the modules
> list and it thinks there are no valid data at that place while
> patching.
> 
> Also the !is_module_address(addr) test is useless now.
> 
> Reported-by: Alan Cox <alan@lxorguk.ukuu.org.uk>

Signed-off-by: Jiri Slaby <jirislaby@gmail.com>

> Signed-off-by: Ingo Molnar <mingo@elte.hu>
> Tested-by: Alan Cox <alan@lxorguk.ukuu.org.uk>
> ---
>  include/linux/mm.h |    4 ++++
>  1 files changed, 4 insertions(+), 0 deletions(-)
> 
> diff --git a/include/linux/mm.h b/include/linux/mm.h
> index c61ba10..45772fd 100644
> --- a/include/linux/mm.h
> +++ b/include/linux/mm.h
> @@ -267,6 +267,10 @@ static inline int is_vmalloc_addr(const void *x)
>  #ifdef CONFIG_MMU
>  	unsigned long addr = (unsigned long)x;
>  
> +#ifdef CONFIG_X86_64
> +	if (addr >= MODULES_VADDR && addr < MODULES_END)
> +		return 1;
> +#endif
>  	return addr >= VMALLOC_START && addr < VMALLOC_END;
>  #else
>  	return 0;

Re: GIT head no longer boots on x86-64

[Linus Torvalds]

On Wed, 15 Oct 2008, Ingo Molnar wrote:
> 
> Queued the fix below up in tip/x86/urgent for a merge to Linus later 
> today. Thanks!

Please don't send this crap to me.

Guys, _look_ at the patch for one second. And then tell me it isn't crap. 

The question is: "Is this a vmalloc'ed area?". That's the name of the 
function. AND YOU JUST BROKE IT!

Fix the damn caller instead. Don't make x86-64-specific changes to a 
generic function that breaks the whole meaning of the function. I don't 
understand what the hell is wrong with you people - we don't fix bugs by 
introducing idiocies, we fix them by fixing the code.

EVEN YOUR COMMIT MESSAGE should have made this obvious.

The code in question already does

	VIRTUAL_BUG_ON(!is_vmalloc_addr(vmalloc_addr) &&
                       !is_module_address(addr));

and look at that thing and ask yourself: where was the bug again.

And dammit, if you say it was in "is_vmalloc_addr()", I can only shake my 
head.

Please guys. Use some taste. 

		Linus

Re: GIT head no longer boots on x86-64

[Jiri Slaby]

On 10/15/2008 05:06 PM, Linus Torvalds wrote:
> On Wed, 15 Oct 2008, Ingo Molnar wrote:
>> Queued the fix below up in tip/x86/urgent for a merge to Linus later 
>> today. Thanks!
> 
> Please don't send this crap to me.
> 
> Guys, _look_ at the patch for one second. And then tell me it isn't crap. 

Not in my eyes.

> The question is: "Is this a vmalloc'ed area?". That's the name of the 
> function. AND YOU JUST BROKE IT!

Modules area is vmalloc'ed on x86; on x86_64 only in different virtual address
space area. So returning true from is_vmalloc_addr() for this space looks very
sane to me, as it was on x86_32 for years.

Users usually do
is_vmalloc_addr(a) ? vfree(a) : kfree(a);
Even there it makes more sense to me.

However I'm fine with introducing is_module_addr() alike function for x86 to
check the general modules space bounds on x86_64 and return is_vmalloc_addr() on
x86_32. Does this look better?

Re: GIT head no longer boots on x86-64

[Linus Torvalds]

On Wed, 15 Oct 2008, Jiri Slaby wrote:
> 
> Users usually do
> is_vmalloc_addr(a) ? vfree(a) : kfree(a);
> Even there it makes more sense to me.

Umm. No it doesn't.

That is exactly _wh7y_ "is_vmalloc_addr()" exists. But we sure as hell 
don't ever want to trigger on modules for that.

If you think that "is_vmalloc_addr()" should trigger for any kernel 
virtual address, why not just make it do so, then? And _name_ it so.

Names are important. In fact, naming is often _more_ important than the 
implementation is. And that means that the implementation should follow 
the naming, or the implementation is wrong.

			Linus

Re: GIT head no longer boots on x86-64

[Arjan van de Ven]

On Wed, 15 Oct 2008 17:33:42 +0200
Jiri Slaby <jirislaby@gmail.com> wrote:

> Users usually do
> is_vmalloc_addr(a) ? vfree(a) : kfree(a);
> Even there it makes more sense to me.
> 

I would like to point out that I greatly dislike any and all such
abuses. Either you vmalloc something or you kmalloc it. Doing it dynamic
with no way to tell? Horrible.

(in fact I might do a patch in the opposite direction; have vmalloc()
be fancy and internally try kmalloc first, if it fails, then do the
expensive stuff)

but really, you need to know what you allocated. 


-- 
Arjan van de Ven 	Intel Open Source Technology Centre
For development, discussion and tips for power savings, 
visit http://www.lesswatts.org

Re: GIT head no longer boots on x86-64

[Linus Torvalds]

On Wed, 15 Oct 2008, Linus Torvalds wrote:
> 
> The code in question already does
> 
> 	VIRTUAL_BUG_ON(!is_vmalloc_addr(vmalloc_addr) &&
>                        !is_module_address(addr));
> 
> and look at that thing and ask yourself: where was the bug again.

Btw, I don't even know if this is a sparc64 issue too, but it sounds 
possible. Sparc64 seems similar to x86-64 in that there is a special range 
for module addresses.

I'm too lazy to check everybody's "module_alloc()", and maybe others do 
too, but use a different symbol, so grepping for it doesn't trigger.

But regardless, a much more correct fix appears to just screw this, and 
make it explicit on the symbol. And only do it if modules are even 
supported.

And if you *really* want to change "is_vmalloc_addr()", then

 (a) do it right, not some crappy x86-64-specific sh*t
 (b) do it like I do it, and make it dependent on modules even being 
     enabled
 (c) and rename it to match what it does.

not the horrible patch I've seen.

Oh, btw. This patch is *totally* untested. I don't even enable modules. So 
if it doesn't compile, it isn't perfect. But while it may not _work_, at 
least it's not _ugly_.

(Quite frankly, I think an even more correct fix is to rename the whole 
"vmalloc_to_page()" function, since it's clearly used for other things 
than vmalloc. Maybe "kernel_virtual_to_page()". Whatever. This is trying 
to be minimal without being totally disgusting).

			Linus

---
 mm/vmalloc.c |   18 ++++++++++++++++--
 1 files changed, 16 insertions(+), 2 deletions(-)

diff --git a/mm/vmalloc.c b/mm/vmalloc.c
index bba06c4..f018d7e 100644
--- a/mm/vmalloc.c
+++ b/mm/vmalloc.c
@@ -168,6 +168,21 @@ int map_vm_area(struct vm_struct *area, pgprot_t prot, struct page ***pages)
 }
 EXPORT_SYMBOL_GPL(map_vm_area);
 
+static inline int is_vmalloc_or_module_addr(const void *x)
+{
+	/*
+	 * x86-64 and sparc64 put modules in a special place,
+	 * and fall back on vmalloc() if that fails. Others
+	 * just put it in the vmalloc space.
+	 */
+#if defined(CONFIG_MODULES) && defined(MODULES_VADDR)
+	unsigned long addr = (unsigned long)x;
+	if (addr >= MODULES_VADDR && addr < MODULES_END)
+		return 1;
+#endif
+	return is_vmalloc_addr(x);
+}
+
 /*
  * Map a vmalloc()-space virtual address to the physical page.
  */
@@ -184,8 +199,7 @@ struct page *vmalloc_to_page(const void *vmalloc_addr)
 	 * XXX we might need to change this if we add VIRTUAL_BUG_ON for
 	 * architectures that do not vmalloc module space
 	 */
-	VIRTUAL_BUG_ON(!is_vmalloc_addr(vmalloc_addr) &&
-			!is_module_address(addr));
+	VIRTUAL_BUG_ON(!is_vmalloc_or_module_addr(vmalloc_addr));
 
 	if (!pgd_none(*pgd)) {
 		pud = pud_offset(pgd, addr);

Re: GIT head no longer boots on x86-64

[H. Peter Anvin]

Linus Torvalds wrote:
> 
> Oh, btw. This patch is *totally* untested. I don't even enable modules. So 
> if it doesn't compile, it isn't perfect. But while it may not _work_, at 
> least it's not _ugly_.
> 
> (Quite frankly, I think an even more correct fix is to rename the whole 
> "vmalloc_to_page()" function, since it's clearly used for other things 
> than vmalloc. Maybe "kernel_virtual_to_page()". Whatever. This is trying 
> to be minimal without being totally disgusting).
> 

I have verified that this patch fixes the problem, at least in my test 
rig, and has queued it up for tip:x86/urgent.  It should be in the next 
pull request.

Note that this bug only bites when CONFIG_DEBUG_VIRTUAL=y and we're 
running an SMP kernel on UP.  Not that that is any excuse.

	-hpa

Re: GIT head no longer boots on x86-64

[Linus Torvalds]

On Mon, 13 Oct 2008, Jiri Slaby wrote:
> +	sprint_symbol(buf, (unsigned long)__builtin_return_address(0));
> +	if (!is_vmalloc_addr(vmalloc_addr) && !is_module_address(addr))
> +		printk("BUG? %s (from %s): %p\n", __func__, buf, vmalloc_addr);

Just use

	printk("BUG? %s (from %pS): %p\n",
		__func__,
		__builtin_return_address(0),
		vmalloc_addr);

and don't ever sprint_symbol() any more. 

The rules:
 - %pS for symbolic names of real pointers off the stack etc (ie something 
   that is approximately type "void *" and points directly to the function 
   code)
 - %pF for symbolic names of a C function pointer (ie of type (*fn)(...) 
   and actually has a real C function pointer type)
where the difference doesn't matter for x86 (or most sane architectures), 
but does matter for architectures that use function descriptors rather 
than direct pointers (ia64, hppa, ppc64, maybe others).

			Linus

Re: GIT head no longer boots on x86-64

[Jiri Slaby]

On 10/13/2008 04:45 PM, Linus Torvalds wrote:
> 
> On Mon, 13 Oct 2008, Jiri Slaby wrote:
>> +	sprint_symbol(buf, (unsigned long)__builtin_return_address(0));
>> +	if (!is_vmalloc_addr(vmalloc_addr) && !is_module_address(addr))
>> +		printk("BUG? %s (from %s): %p\n", __func__, buf, vmalloc_addr);
> 
> Just use
> 
> 	printk("BUG? %s (from %pS): %p\n",
> 		__func__,
> 		__builtin_return_address(0),
> 		vmalloc_addr);
> 
> and don't ever sprint_symbol() any more. 

Ok, thanks, I knew about it, but didn't remember it while I was hacking that
debug patch.