Problem with module_init?

Problem with module_init?

[Sean MacLennan]

With the latest from Linus' tree, I get the following oops:

Oops: Kernel access of bad area, sig: 11 [#1]
Warp
Modules linked in: simple(+)
NIP: d14b8000 LR: c0001420 CTR: 00000000
REGS: cf2a1df0 TRAP: 0400   Not tainted  (2.6.29-pika)
MSR: 00029000 <EE,ME,CE>  CR: 24000022  XER: 00000000
TASK = cf896100[1673] 'insmod' THREAD: cf2a0000
GPR00: 00000000 cf2a1ea0 cf896100 d14b8000 00000001 d14b5360 ffffffff
00000000 GPR08: c03312fc c03312fc c0546018 c0546018 24000022 100872cc
0ffa7d00 007fff99 GPR16: 00400450 00800000 007fff00 0ffa7cec 00000000
ffffffff 00000000 1008323c GPR24: 10083254 00000000 c0330000 c0310000
c03110a4 d14b8000 cf2a0000 d14b5360 NIP [d14b8000] simple_init+0x0/0x8
[simple] LR [c0001420] do_one_initcall+0x34/0x1a0
Call Trace:
[cf2a1f20] [c0045e18] sys_init_module+0xb0/0x1b8
[cf2a1f40] [c000dbbc] ret_from_syscall+0x0/0x3c
Instruction dump:
XXXXXXXX XXXXXXXX XXXXXXXX XXXXXXXX XXXXXXXX XXXXXXXX XXXXXXXX XXXXXXXX 
XXXXXXXX XXXXXXXX XXXXXXXX XXXXXXXX <38600000> 4e800020 XXXXXXXX
XXXXXXXX ---[ end trace 9ac3b092cfc75a75 ]---

This is the entire driver:

#include <linux/module.h>

static int __init simple_init(void)
{
	return 0;
}
module_init(simple_init);

static void __exit simple_exit(void)
{
}
module_exit(simple_exit);

MODULE_DESCRIPTION("simple driver");
MODULE_LICENSE("GPL");

Has something changed for module?

Cheers,
   Sean

Re: Problem with module_init?

[Sean MacLennan]

8d1cf34e7ad5c7738ce20d20bd7f002f562cb8b5 is first bad commit
commit 8d1cf34e7ad5c7738ce20d20bd7f002f562cb8b5
Author: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Date:   Thu Mar 19 19:34:08 2009 +0000

    powerpc/mm: Tweak PTE bit combination definitions
    
    This patch tweaks the way some PTE bit combinations are defined, in
    such a way that the 32 and 64-bit variant become almost identical
    and that will make it easier to bring in a new common pte-* file
    for the new variant of the Book3-E support.
    
    The combination of bits defining access to kernel pages are now
    clearly separated from the combination used by userspace and the
    core VM. The resulting generated code should remain identical
    unless I made a mistake. 
    Note: While at it, I removed a non-sensical statement related to
    CONFIG_KGDB in ppc_mmu_32.c which could cause kernel mappings to be
    user accessible when that option is enabled. Probably something
    that bitrot. 
    Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>

:040000 040000 c105fb9641b078f1a8a60c2a7900ca2dd088b364
e90678246962d7a455aa27ba12814c11cd9b3c22 M      arch

Re: Problem with module_init?

[Josh Boyer]

On Tue, Mar 31, 2009 at 11:11:53PM -0400, Sean MacLennan wrote:
>8d1cf34e7ad5c7738ce20d20bd7f002f562cb8b5 is first bad commit
>commit 8d1cf34e7ad5c7738ce20d20bd7f002f562cb8b5
>Author: Benjamin Herrenschmidt <benh@kernel.crashing.org>
>Date:   Thu Mar 19 19:34:08 2009 +0000
>
>    powerpc/mm: Tweak PTE bit combination definitions
>    
>    This patch tweaks the way some PTE bit combinations are defined, in
>    such a way that the 32 and 64-bit variant become almost identical
>    and that will make it easier to bring in a new common pte-* file
>    for the new variant of the Book3-E support.
>    
>    The combination of bits defining access to kernel pages are now
>    clearly separated from the combination used by userspace and the
>    core VM. The resulting generated code should remain identical
>    unless I made a mistake. 
>    Note: While at it, I removed a non-sensical statement related to
>    CONFIG_KGDB in ppc_mmu_32.c which could cause kernel mappings to be
>    user accessible when that option is enabled. Probably something
>    that bitrot. 
>    Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
>
>:040000 040000 c105fb9641b078f1a8a60c2a7900ca2dd088b364
>e90678246962d7a455aa27ba12814c11cd9b3c22 M      arch

I'm assuming this is the result of a git-bisect run?

Can I also assume you were loading the module on your Warp board?

Ben, have you tested module loads on 4xx since you redid this?

josh

Re: Problem with module_init?

[Sean MacLennan]

On Wed, 1 Apr 2009 07:27:56 -0400
Josh Boyer <jwboyer@linux.vnet.ibm.com> wrote:

> I'm assuming this is the result of a git-bisect run?

Correct.

> Can I also assume you were loading the module on your Warp board?

Correct again.

Cheers,
  Sean

Re: Problem with module_init?

[Benjamin Herrenschmidt]

On Wed, 2009-04-01 at 07:27 -0400, Josh Boyer wrote:
> 
> I'm assuming this is the result of a git-bisect run?
> 
> Can I also assume you were loading the module on your Warp board?
> 
> Ben, have you tested module loads on 4xx since you redid this?
> 
I must shamefully admit almost never testing modules ... I always
netboot zImages with all the drivers I need built-in.

I suspect I just screwed up the definition of PAGE_KERNEL_EXEC or
something like that. I'll have a look.

Cheers,
Ben.

Re: Problem with module_init?

[Sean MacLennan]

On Thu, 02 Apr 2009 09:24:43 +1100
"Benjamin Herrenschmidt" <benh@kernel.crashing.org> wrote:

> I suspect I just screwed up the definition of PAGE_KERNEL_EXEC or
> something like that.

Yup, that is exactly what you did ;) You left out _PAGE_HWEXEC. The
following patch fixes the problem for me.

Cheers,
   Sean

diff --git a/arch/powerpc/include/asm/pte-common.h b/arch/powerpc/include/asm/pte-common.h
index d9740e8..a84f248 100644
--- a/arch/powerpc/include/asm/pte-common.h
+++ b/arch/powerpc/include/asm/pte-common.h
@@ -167,7 +167,8 @@ extern unsigned long bad_call_to_PMD_PAGE_SIZE(void);
 #endif
 
 /* Make modules code happy. We don't set RO yet */
-#define PAGE_KERNEL_EXEC	PAGE_KERNEL_X
+// #define PAGE_KERNEL_EXEC	PAGE_KERNEL_X
+#define PAGE_KERNEL_EXEC __pgprot(_PAGE_BASE | _PAGE_KERNEL_RW | _PAGE_EXEC | _PAGE_HWEXEC)
 
 /* Advertise special mapping type for AGP */
 #define PAGE_AGP		(PAGE_KERNEL_NC)

Re: Problem with module_init?

[Benjamin Herrenschmidt]

On Thu, 2009-04-02 at 00:08 -0400, Sean MacLennan wrote:
> On Thu, 02 Apr 2009 09:24:43 +1100
> "Benjamin Herrenschmidt" <benh@kernel.crashing.org> wrote:
> 
> > I suspect I just screwed up the definition of PAGE_KERNEL_EXEC or
> > something like that.
> 
> Yup, that is exactly what you did ;) You left out _PAGE_HWEXEC. The
> following patch fixes the problem for me.

The proper fix is for PAGE_KERNEL_X to have _PAGE_HWEXEC. I'll fix that.

Cheers,
Ben.

> Cheers,
>    Sean
> 
> diff --git a/arch/powerpc/include/asm/pte-common.h b/arch/powerpc/include/asm/pte-common.h
> index d9740e8..a84f248 100644
> --- a/arch/powerpc/include/asm/pte-common.h
> +++ b/arch/powerpc/include/asm/pte-common.h
> @@ -167,7 +167,8 @@ extern unsigned long bad_call_to_PMD_PAGE_SIZE(void);
>  #endif
>  
>  /* Make modules code happy. We don't set RO yet */
> -#define PAGE_KERNEL_EXEC	PAGE_KERNEL_X
> +// #define PAGE_KERNEL_EXEC	PAGE_KERNEL_X
> +#define PAGE_KERNEL_EXEC __pgprot(_PAGE_BASE | _PAGE_KERNEL_RW | _PAGE_EXEC | _PAGE_HWEXEC)
>  
>  /* Advertise special mapping type for AGP */
>  #define PAGE_AGP		(PAGE_KERNEL_NC)

Re: Problem with module_init?

[Sean MacLennan]

On Thu, 02 Apr 2009 15:26:02 +1100
Benjamin Herrenschmidt <benh@kernel.crashing.org> wrote:

> The proper fix is for PAGE_KERNEL_X to have _PAGE_HWEXEC. I'll fix
> that.

That may not be enough. I made that change (adding _PAGE_HWEXEC to
PAGE_KERNEL_X) and it works for some drivers, but I am still crashing in
one driver. Although, that *could* be unrelated bug.

Cheers,
   Sean

Re: Problem with module_init?

[Sean MacLennan]

On Thu, 2 Apr 2009 00:39:05 -0400
"Sean MacLennan" <sean.maclennan@ottawa.kanatek.ca> wrote:

> Although, that *could* be unrelated bug.

It *is* an unrelated bug. dma_alloc_coherent now requires a device
where before it was optional. Carry on.

Cheers,
   Sean

Re: Problem with module_init?

[Benjamin Herrenschmidt]

On Thu, 2009-04-02 at 00:51 -0400, Sean MacLennan wrote:
> On Thu, 2 Apr 2009 00:39:05 -0400
> "Sean MacLennan" <sean.maclennan@ottawa.kanatek.ca> wrote:
> 
> > Although, that *could* be unrelated bug.
> 
> It *is* an unrelated bug. dma_alloc_coherent now requires a device
> where before it was optional. Carry on.

Right, that's new in .30

Cheers,
Ben.

> Cheers,
>    Sean