Re: GIT head no longer boots on x86-64

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

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

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

;)

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

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

------------------>

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;

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

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

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

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?

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

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

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

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

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

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);

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

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

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>