kernel panic during kernel module load (powerpc specific part)

kernel panic during kernel module load (powerpc specific part)

[Steffen Rumler]

Hi,

We have seen the following kernel panic, happened during loading a kernel module:

     [  536.107430] Unable to handle kernel paging request for data at address 0xd76a907c
     [  536.114922] Faulting instruction address: 0xc0000770
     [  536.119891] Oops: Kernel access of bad area, sig: 11 [#1]
     [  536.125291] CCEP MPC8541E
     [  536.127908] Modules linked in: pppoe(+) nf_conntrack_ipv6 ...
     [  536.155705] NIP: c0000770 LR: c0000770 CTR: d76ab0d4
     [  536.160674] REGS: d76a8f24 TRAP: 0300   Not tainted  (2.6.33-ccep)
     [  536.166857] MSR: 00021000 <ME,CE>  CR: 24000482  XER: 20000000
     [  536.172718] DEAR: d76a907c, ESR: 00800000
     [  536.176728] TASK = cbd7f9f0[972] 'insmod' THREAD: cbeb2000
     [  536.182041] GPR00: 83cbfff8 d76a8fd4 cbd7f9f0 00000000 83cbfff8 00000000 cbeb3e1e 00000000
     [  536.190438] GPR08: 0000327b d76aa000 24000482 d76a8fd4 cbd7fc08 10019b04 100c2e5c 00000000
     [  536.198836] GPR16: 00000000 1009bafc 100c44a4 100c2ed4 100c0000 100d1e60 100d1ca8 100017ab
     [  536.207235] GPR24: 100017ae 10001936 c0343ae8 10012018 00000000 834bffe8 836bffec 838bfff0
     [  536.215819] NIP [c0000770] InstructionStorage+0xb0/0xc0
     [  536.221048] LR [c0000770] InstructionStorage+0xb0/0xc0
     [  536.226188] Call Trace:
     [  536.228630] Instruction dump:
     [  536.231600] 90eb002c 910b0030 7cbe0aa6 90ab00b8 7d846378 38a00000 39400401 914b00b0
     [  536.239386] 3d400002 614a1002 512a0420 4800d6f5 <c000e43c> c000e65c 60000000 60000000
     [  536.247348] Kernel panic - not syncing: Fatal exception in interrupt
     [  536.253704] Call Trace:
     [  536.256149] Rebooting in 10 seconds..

The system crashes inside the return of the init entry point of the kernel module.

I've found the following root cause:

     (1) The system has a high number of NAT rules configured, which created a bigger vmalloc area.
           I've checked this by looking at /proc/vmallocinfo.

     (2) The kernel module ELF file contains the separate section .init.text for the init entry point,
           which is marked with __init, as usual.

     (3) The kernel module ELF file contains the function prologue and epilogue in the .text section.

     (4) The epilogue is also called from the init entry point, in order to return to the caller.
           It is intended to restore the non-volatile registers from the stack and to jump to the caller.

     (5) Because of (1), it is not more possible to jump by a relative branch instruction. The distance is too big.
           Instead, the trampoline method is applied, which allows longer jumps via register.
           (please see see do_plt_call() in arch/powerpc/kernel/module_32.c)

     (6) Unfortunately, the trampoline code (do_plt_call()) is using register r11 to setup the jump.
           It looks like the prologue and epilogue are using also the register r11, in order to point to the previous stack frame.
           This is a conflict !!! The trampoline code is damaging the content of r11.

According to the current EABI definitions, the register r11 has got a dedicated function (pointer to previous stack frame).
In the following, there are parts of the prologue/epilogue shown, which are generated by the compiler:

    ...
    00000084 <_rest32gpr_28>:
          84:       83 8b ff f0     lwz     r28,-16(r11)
    00000088 <_rest32gpr_29>:
          88:       83 ab ff f4     lwz     r29,-12(r11)
    0000008c <_rest32gpr_30>:
          8c:       83 cb ff f8     lwz     r30,-8(r11)
    00000090 <_rest32gpr_31>:
          90:       83 eb ff fc     lwz     r31,-4(r11)
          94:       4e 80 00 20     blr
    00000098 <_rest32gpr_14_x>:
          98:       81 cb ff b8     lwz     r14,-72(r11)
    ...


I'd suggest to use register r12 instead of r11 in the trampoline generation code, in do_plt_call() (arch/powerpc/kernel/module_32.c).
I'm using kernel 2.6.33, but I think it is also relevant for the current kernel release.

Below, there is the complete debug sessions, showing more the details.

Thanks
Steffen Rumler

--

     0xd54990c0:     addi    r11,r1,48
     0xd54990c4:     mr      r3,r29
     0xd54990c8:     b       0xd5499100 <-- going to return from the init entry point

     (gdb) bt
     #0  0xd5499100 in ?? ()
     #1  0xd54990ac in ?? ()
     #2  0xc0001db0 in do_one_initcall (fn=0, wait=1131130) at init/main.c:719
     #3  0xc0059e50 in sys_init_module (umod=<value optimized out>, ...
     #4  0xc000e038 in syscall_dotrace_cont () at arch/powerpc/kernel/entry_32.S:331
     Backtrace stopped: frame did not save the PC
     (gdb) info reg r1
     r1             0xcbbdbed0    3418210000
     (gdb) x/2x 0xcbbdbed0
     0xcbbdbed0:    0xcbbdbf00    0xd54990ac
     (gdb) x/2x 0xcbbdbf00
     0xcbbdbf00:    0xcbbdbf20    0xc0001db0
     (gdb) info reg r11
     r11            0xcbbdbf00    3418210048

         --> the stack is OK here
         --> r11 is OK, pointing to the previous stack frame

     0xd5499100:     lis        r11,-10381 <-- this is the trampoline code using/changing r11 (do_plt_call()).
     0xd5499104:     addi    r11,r11,-3888
     0xd5499108:     mtctr  r11

     (gdb) info reg r11
     r11            0xd772f0d0    3614634192
<-- r11 is now damaged !!!

     0xd549910c:     bctr

     0xd772f0d0:     lwz     r28,-16(r11) <-- the epilogue, using damaged r11 as stack pointer
     0xd772f0d4:     lwz     r29,-12(r11)
     0xd772f0d8:     lwz     r30,-8(r11)
     0xd772f0dc:     lwz     r0,4(r11)
     0xd772f0e0:     lwz     r31,-4(r11)
     0xd772f0e4:     mtlr    r0
     0xd772f0e8:     mr      r1,r11

     (gdb) info reg lr
     lr             0x83abfff4    0x83abfff4
         0xd772f0ec:     blr
     (gdb) stepi
     Program received signal SIGSTOP, Stopped (signal).

        ---> the kernel panic !!!

Re: kernel panic during kernel module load (powerpc specific part)

[Michael Ellerman]

On Wed, 2012-05-30 at 16:33 +0200, Steffen Rumler wrote:
> Hi,
> 
> The system crashes inside the return of the init entry point of the kernel module.
> 
> I've found the following root cause:
> 
>      (6) Unfortunately, the trampoline code (do_plt_call()) is using register r11 to setup the jump.
>            It looks like the prologue and epilogue are using also the register r11, in order to point to the previous stack frame.
>            This is a conflict !!! The trampoline code is damaging the content of r11.

Hi Steffen,

Great bug report!

I can't quite work out what the standards say, the versions I'm looking
at are probably old anyway.

Have you tried the obvious fix?

cheers


diff --git a/arch/powerpc/kernel/module_32.c b/arch/powerpc/kernel/module_32.c
index 0b6d796..989d79a 100644
--- a/arch/powerpc/kernel/module_32.c
+++ b/arch/powerpc/kernel/module_32.c
@@ -205,9 +205,9 @@ static uint32_t do_plt_call(void *location,
        }
 
        /* Stolen from Paul Mackerras as well... */
-       entry->jump[0] = 0x3d600000+((val+0x8000)>>16); /* lis r11,sym@ha */
-       entry->jump[1] = 0x396b0000 + (val&0xffff);     /* addi r11,r11,sym@l*/
-       entry->jump[2] = 0x7d6903a6;                    /* mtctr r11 */
+       entry->jump[0] = 0x3d800000+((val+0x8000)>>16); /* lis r12,sym@ha */
+       entry->jump[1] = 0x398c0000 + (val&0xffff);     /* addi r12,r12,sym@l*/
+       entry->jump[2] = 0x7d8903a6;                    /* mtctr r12 */
        entry->jump[3] = 0x4e800420;                    /* bctr */
 
        DEBUGP("Initialized plt for 0x%x at %p\n", val, entry);

RE: kernel panic during kernel module load (powerpc specific part)

[Wrobel Heinz-R39252]

Michael,

> On Wed, 2012-05-30 at 16:33 +0200, Steffen Rumler wrote:
> > I've found the following root cause:
> >
> >      (6) Unfortunately, the trampoline code (do_plt_call()) is using
> register r11 to setup the jump.
> >            It looks like the prologue and epilogue are using also the
> register r11, in order to point to the previous stack frame.
> >            This is a conflict !!! The trampoline code is damaging the
> content of r11.
>=20
> Hi Steffen,
>=20
> Great bug report!
>=20
> I can't quite work out what the standards say, the versions I'm looking
> at are probably old anyway.

The ABI supplement from https://www.power.org/resources/downloads/ is expli=
cit about r11 being a requirement for the statically lined save/restore fun=
ctions in section 3.3.4 on page 59.

This means that any trampoline code must not ever use r11 or it can't be us=
ed to get to such save/restore functions safely from far away.

Unfortunately the same doc and predecessors show r11 in all basic examples =
for PLT/trampoline code AFAICS, which is likely why all trampoline code use=
s r11 in any known case.

I would guess that it was never envisioned that compiler generated code wou=
ld be in a different section than save/restore functions, i.e., the Linux m=
odule "__init" assumptions for Power break the ABI. Or does the ABI break t=
he __init concept?!

Using r12 in the trampoline seems to be the obvious solution for module loa=
ding.

But what about other code loading done? If, e.g., a user runs any app from =
bash it gets loaded and relocated and trampolines might get set up somehow.

Wouldn't we have to find fix ANY trampoline code generator remotely related=
 to a basic Power Architecture Linux?
Or is it a basic assumption for anything but modules that compiler generate=
d code may never ever be outside the .text section? I am not sure that woul=
d be a safe assumption.

Isn't this problem going beyond just module loading for Power Architecture =
Linux?

Regards,

Heinz

Re: kernel panic during kernel module load (powerpc specific part)

[Gabriel Paubert]

On Thu, May 31, 2012 at 07:04:42AM +0000, Wrobel Heinz-R39252 wrote:
> Michael,
> 
> > On Wed, 2012-05-30 at 16:33 +0200, Steffen Rumler wrote:
> > > I've found the following root cause:
> > >
> > >      (6) Unfortunately, the trampoline code (do_plt_call()) is using
> > register r11 to setup the jump.
> > >            It looks like the prologue and epilogue are using also the
> > register r11, in order to point to the previous stack frame.
> > >            This is a conflict !!! The trampoline code is damaging the
> > content of r11.
> > 
> > Hi Steffen,
> > 
> > Great bug report!
> > 
> > I can't quite work out what the standards say, the versions I'm looking
> > at are probably old anyway.
> 
> The ABI supplement from https://www.power.org/resources/downloads/ is explicit about r11 being a requirement for the statically lined save/restore functions in section 3.3.4 on page 59.
> 
> This means that any trampoline code must not ever use r11 or it can't be used to get to such save/restore functions safely from far away.

I believe that the basic premise is that you should provide a directly reachable copy 
of the save/rstore functions, even if this means that you need several copies of the functions.

> 
> Unfortunately the same doc and predecessors show r11 in all basic examples for PLT/trampoline code AFAICS, which is likely why all trampoline code uses r11 in any known case.
> 
> I would guess that it was never envisioned that compiler generated code would be in a different section than save/restore functions, i.e., the Linux module "__init" assumptions for Power break the ABI. Or does the ABI break the __init concept?!
> 
> Using r12 in the trampoline seems to be the obvious solution for module loading.
> 
> But what about other code loading done? If, e.g., a user runs any app from bash it gets loaded and relocated and trampolines might get set up somehow.

I don't think so. The linker/whatever should generate a copy of the save/restore functions for every
executable code area (shared library), and probably more than one copy if the text becomes too large.

For 64 bit code, these functions are actually inserted by the linker.

[Ok, I just recompiled my 64 bit kernel with -Os and I see that vmlinux gets one copy
of the save/restore functions and every module also gets its copy.]

This makes sense, really these functions are there for a compromise between locality 
and performance, there should be one per code section, otherwise the cache line 
used by the trampoline negates a large part of their advantage.

> 
> Wouldn't we have to find fix ANY trampoline code generator remotely related to a basic Power Architecture Linux?
> Or is it a basic assumption for anything but modules that compiler generated code may never ever be outside the .text section? I am not sure that would be a safe assumption.
> 
> Isn't this problem going beyond just module loading for Power Architecture Linux?

I don't think so. It really seems to be a 32 bit kernel problem.

	Regards,
	Gabriel

Re: kernel panic during kernel module load (powerpc specific part)

[Benjamin Herrenschmidt]

On Thu, 2012-05-31 at 13:04 +0200, Gabriel Paubert wrote:

> I believe that the basic premise is that you should provide a directly reachable copy 
> of the save/rstore functions, even if this means that you need several copies of the functions.

I just fixed a very similar problem with grub2 in fact. It was using r0
and trashing the saved LR that way.

The real fix is indeed to statically link those gcc "helpers", we
shouldn't generate things like cross-module calls inside function
prologs and epilogues, when stackframes aren't even guaranteed to be
reliable.

However, in the grub2 case, it was easier to just use r12 :-)

Cheers,
Ben.

> > 
> > Unfortunately the same doc and predecessors show r11 in all basic examples for PLT/trampoline code AFAICS, which is likely why all trampoline code uses r11 in any known case.
> > 
> > I would guess that it was never envisioned that compiler generated code would be in a different section than save/restore functions, i.e., the Linux module "__init" assumptions for Power break the ABI. Or does the ABI break the __init concept?!
> > 
> > Using r12 in the trampoline seems to be the obvious solution for module loading.
> > 
> > But what about other code loading done? If, e.g., a user runs any app from bash it gets loaded and relocated and trampolines might get set up somehow.
> 
> I don't think so. The linker/whatever should generate a copy of the save/restore functions for every
> executable code area (shared library), and probably more than one copy if the text becomes too large.
> 
> For 64 bit code, these functions are actually inserted by the linker.
> 
> [Ok, I just recompiled my 64 bit kernel with -Os and I see that vmlinux gets one copy
> of the save/restore functions and every module also gets its copy.]
> 
> This makes sense, really these functions are there for a compromise between locality 
> and performance, there should be one per code section, otherwise the cache line 
> used by the trampoline negates a large part of their advantage.
> 
> > 
> > Wouldn't we have to find fix ANY trampoline code generator remotely related to a basic Power Architecture Linux?
> > Or is it a basic assumption for anything but modules that compiler generated code may never ever be outside the .text section? I am not sure that would be a safe assumption.
> > 
> > Isn't this problem going beyond just module loading for Power Architecture Linux?
> 
> I don't think so. It really seems to be a 32 bit kernel problem.
> 
> 	Regards,
> 	Gabriel
> _______________________________________________
> Linuxppc-dev mailing list
> Linuxppc-dev@lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/linuxppc-dev

RE: kernel panic during kernel module load (powerpc specific part)

[Wrobel Heinz-R39252]

PiA+IEkgYmVsaWV2ZSB0aGF0IHRoZSBiYXNpYyBwcmVtaXNlIGlzIHRoYXQgeW91IHNob3VsZCBw
cm92aWRlIGEgZGlyZWN0bHkNCj4gPiByZWFjaGFibGUgY29weSBvZiB0aGUgc2F2ZS9yc3RvcmUg
ZnVuY3Rpb25zLCBldmVuIGlmIHRoaXMgbWVhbnMgdGhhdA0KPiB5b3UgbmVlZCBzZXZlcmFsIGNv
cGllcyBvZiB0aGUgZnVuY3Rpb25zLg0KPiANCj4gSSBqdXN0IGZpeGVkIGEgdmVyeSBzaW1pbGFy
IHByb2JsZW0gd2l0aCBncnViMiBpbiBmYWN0LiBJdCB3YXMgdXNpbmcgcjANCj4gYW5kIHRyYXNo
aW5nIHRoZSBzYXZlZCBMUiB0aGF0IHdheS4NCj4gDQo+IFRoZSByZWFsIGZpeCBpcyBpbmRlZWQg
dG8gc3RhdGljYWxseSBsaW5rIHRob3NlIGdjYyAiaGVscGVycyIsIHdlDQo+IHNob3VsZG4ndCBn
ZW5lcmF0ZSB0aGluZ3MgbGlrZSBjcm9zcy1tb2R1bGUgY2FsbHMgaW5zaWRlIGZ1bmN0aW9uIHBy
b2xvZ3MNCj4gYW5kIGVwaWxvZ3Vlcywgd2hlbiBzdGFja2ZyYW1lcyBhcmVuJ3QgZXZlbiBndWFy
YW50ZWVkIHRvIGJlIHJlbGlhYmxlLg0KPiANCj4gSG93ZXZlciwgaW4gdGhlIGdydWIyIGNhc2Us
IGl0IHdhcyBlYXNpZXIgdG8ganVzdCB1c2UgcjEyIDotKQ0KDQpGb3Igbm90IGp1c3QgdGhlIG1v
ZHVsZSBsb2FkaW5nIGNhc2UsIEkgYmVsaWV2ZSByMTIgaXMgdGhlIG9ubHkgcmVhbCBzb2x1dGlv
biBub3cuIEkgY2hlY2tlZCBvbmUgZGVidWdnZXIgY2FwYWJsZSBvZiBkb2luZyBFTEYgZG93bmxv
YWQuIEl0IGFsc28gdXNlcyByMTIgZm9yIHRyYW1wb2xpbmUgY29kZS4gSSBhbSBndWVzc2luZyBm
b3IgdGhlIHJlYXNvbiB0aGF0IHByb21wdGVkIHRoaXMgZGlzY3Vzc2lvbi4NCg0KV2l0aG91dCBy
MTIgd2UnZCBoYXZlIHRvIGNoYW5nZSBzdGFuZGFyZCBsaWJyYXJpZXMgdG8gYXV0b21hZ2ljYWxs
eSBsaW5rIGluIGdjYyBoZWxwZXJzIGZvciBhbnkgY29uY2VpdmFibGUgbm9uLS50ZXh0IHNlY3Rp
b24sIHdoaWNoIEkgYW0gbm90IHN1cmUgaXMgZmVhc2libGUuIEhvdyB3b3VsZCB5b3Ugd3JpdGUg
c2VjdGlvbiBpbmRlcGVuZGVudCBoZWxwZXIgZnVuY3Rpb25zIHdoaWNoIGxpbmsgdG8gYW55IHNl
Y3Rpb24gbmVlZGluZyB0aGVtPyENCkFza2luZyB1c2VycyB0byBjcmVhdGUgdGhlaXIgb3duIHNl
Y3Rpb24gc3BlY2lmaWMgY29weSBvZiBoZWxwZXIgZnVuY3Rpb25zIGlzIGRlZmluaXRlbHkgbm90
IHBvcnRhYmxlIGlmIHRoZSBtb2R1bGUgb3Igb3RoZXIgY29kZSBpcyBub3QgYXJjaGl0ZWN0dXJl
IGRlcGVuZGVudC4NCkl0IGlzIGEgbm9ybWFsIGdjYyBmZWF0dXJlIHRoYXQgeW91IGNhbiBhc3Np
Z24gc3BlY2lmaWMgY29kZSB0byBub24tLnRleHQgc2VjdGlvbnMgYW5kIGl0IGlzIG5vdCBkb2N1
bWVudGVkIHRoYXQgaXQgbWF5IGNyYXNoIGRlcGVuZGluZyBvbiB0aGUgT1MgYXJjaCB0aGUgRUxG
IGlzIGJ1aWx0IGZvciwgc28gYXNraW5nIGZvciBhIFBvd2VyIEFyY2hpdGVjdHVyZSBzcGVjaWZp
YyBjaGFuZ2Ugb24gdG9vbCBsaWJzIHRvIG1ha2UgUG93ZXIgQXJjaGl0ZWN0dXJlIExpbnV4IGhh
cHB5IHNlZW1zIGEgYml0IG11Y2ggdG8gYXNrLg0KDQpVc2luZyByMTIgaW4gYW55IExpbnV4IHJl
bGF0ZWQgdHJhbXBvbGluZSBjb2RlIHNlZW1zIGEgcmVhY2hhYmxlIGdvYWwsIGFuZCBpdCB3b3Vs
ZCBlbGltaW5hdGUgdGhlIGNvbmZsaWN0IHRvIHRoZSBBQkkuDQoNClJlZ2FyZHMsDQoNCkhlaW56
DQo=

Re: kernel panic during kernel module load (powerpc specific part)

[Steffen Rumler]

>>> I believe that the basic premise is that you should provide a directly
>>> reachable copy of the save/rstore functions, even if this means that
>> you need several copies of the functions.
>>
>> I just fixed a very similar problem with grub2 in fact. It was using r0
>> and trashing the saved LR that way.
>>
>> The real fix is indeed to statically link those gcc "helpers", we
>> shouldn't generate things like cross-module calls inside function prologs
>> and epilogues, when stackframes aren't even guaranteed to be reliable.
>>
>> However, in the grub2 case, it was easier to just use r12 :-)
> For not just the module loading case, I believe r12 is the only real solution now. I checked one debugger capable of doing ELF download. It also uses r12 for trampoline code. I am guessing for the reason that prompted this discussion.
>
> Without r12 we'd have to change standard libraries to automagically link in gcc helpers for any conceivable non-.text section, which I am not sure is feasible. How would you write section independent helper functions which link to any section needing them?!
> Asking users to create their own section specific copy of helper functions is definitely not portable if the module or other code is not architecture dependent.
> It is a normal gcc feature that you can assign specific code to non-.text sections and it is not documented that it may crash depending on the OS arch the ELF is built for, so asking for a Power Architecture specific change on tool libs to make Power Architecture Linux happy seems a bit much to ask.
>
> Using r12 in any Linux related trampoline code seems a reachable goal, and it would eliminate the conflict to the ABI.
>
> Regards,
>
> Heinz
Hi,

I've tried the fix using register r12 for the trampoline code, instead of register r11.
I'd to adapt entry_matches() too:

--- arch/powerpc/kernel/module_32.c    (revision 1898)
+++ arch/powerpc/kernel/module_32.c    (working copy)
@@ -187,8 +187,8 @@

  static inline int entry_matches(struct ppc_plt_entry *entry, Elf32_Addr val)
  {
-    if (entry->jump[0] == 0x3d600000 + ((val + 0x8000) >> 16)
- && entry->jump[1] == 0x396b0000 + (val & 0xffff))
+    if (entry->jump[0] == 0x3d800000 + ((val + 0x8000) >> 16)
+ && entry->jump[1] == 0x398c0000 + (val & 0xffff))
          return 1;
      return 0;
  }
@@ -215,10 +215,9 @@
          entry++;
      }

-    /* Stolen from Paul Mackerras as well... */
-    entry->jump[0] = 0x3d600000+((val+0x8000)>>16);    /* lis r11,sym@ha */
-    entry->jump[1] = 0x396b0000 + (val&0xffff);    /* addi r11,r11,sym@l*/
-    entry->jump[2] = 0x7d6903a6;            /* mtctr r11 */
+    entry->jump[0] = 0x3d800000+((val+0x8000)>>16); /* lis r12,sym@ha */
+    entry->jump[1] = 0x398c0000 + (val&0xffff);     /* addi r12,r12,sym@l*/
+    entry->jump[2] = 0x7d8903a6;                    /* mtctr r12 */
      entry->jump[3] = 0x4e800420;            /* bctr */

      DEBUGP("Initialized plt for 0x%x at %p\n", val, entry);


It looks working, also in the case the trampoline code is used.
Please see also the debug session below.

Let me summarize the situation:

    + According to Freescale, EABI assigns a dedicated function to register r11.
    + The ELF sections .text and .init.text may trigger the usage of the trampoline code.
    + The trampoline code must not user register r11, too.
    + We must not depend on addresses returned by vmalloc during kernel module loading.

I think the bug must be fixed !!!

Regards
Steffen

--


(gdb) bt
#0  0xd54990f0 in ?? ()
#1  0xd5499088 in ?? ()
#2  0xc0001d9c in do_one_initcall (fn=0xd76e26ac) at init/main.c:716
#3  0xc0059630 in sys_init_module (umod=0x4802f008, len=<value optimized out>, uargs=0x10012008 "") at kernel/module.c:2470
#4  0xc000dff8 in syscall_dotrace_cont () at arch/powerpc/kernel/entry_32.S:331
Backtrace stopped: frame did not save the PC

<-- we are returning from the driver init entry point

(gdb) info reg r11
r11            0xcbb57f00    3417669376
(gdb) x/2x 0xcbb57f00
0xcbb57f00:    0xcbb57f20    0xc0001d9c

<-- register r11 is fine here

0xd54990f0:     lis     r12,-10386
0xd54990f4:     addi    r12,r12,-20268
0xd54990f8:     mtctr   r12

<-- this is the trampoline code using now r12, instead of r11

(gdb) info reg r12
r12            0xd76db0d4    3614290132
(gdb) info reg ctr
ctr            0xd76db0d4    3614290132
0xd54990fc:     bctr

<-- we are jumping to the epilogue

0xd76db0d4:     lwz     r29,-12(r11)
0xd76db0d8:     lwz     r30,-8(r11)
0xd76db0dc:     lwz     r0,4(r11)
0xd76db0e0:     lwz     r31,-4(r11)
0xd76db0e4:     mtlr    r0
0xd76db0e8:     mr      r1,r11

<- the epilogue

(gdb) info reg lr
lr             0xc0001d9c    0xc0001d9c <do_one_initcall+100>
0xc0001d9c <do_one_initcall+100>:       lis     r9,-16330
0xc0001da0 <do_one_initcall+104>:       lwz     r0,12296(r9)
0xc0001da4 <do_one_initcall+108>:       lis     r9,-16330
0xd76db0ec:     blr

<-- the jump back to do_one_initcall()
<-- no crash

Re: kernel panic during kernel module load (powerpc specific part)

[Paul Mackerras]

On Mon, Jun 04, 2012 at 09:43:01AM +0200, Steffen Rumler wrote:
> Let me summarize the situation:
> 
>    + According to Freescale, EABI assigns a dedicated function to register r11.
>    + The ELF sections .text and .init.text may trigger the usage of the trampoline code.
>    + The trampoline code must not user register r11, too.
>    + We must not depend on addresses returned by vmalloc during kernel module loading.
> 
> I think the bug must be fixed !!!

I agree.  If you submit your patch with a nice description of the
problem and how the patch fixes it, along with your Signed-off-by,
I'll send it upstream.

Paul.

Re: kernel panic during kernel module load (powerpc specific part)

[Gabriel Paubert]

On Fri, Jun 01, 2012 at 11:33:37AM +0000, Wrobel Heinz-R39252 wrote:
> > > I believe that the basic premise is that you should provide a directly
> > > reachable copy of the save/rstore functions, even if this means that
> > you need several copies of the functions.
> > 
> > I just fixed a very similar problem with grub2 in fact. It was using r0
> > and trashing the saved LR that way.
> > 
> > The real fix is indeed to statically link those gcc "helpers", we
> > shouldn't generate things like cross-module calls inside function prologs
> > and epilogues, when stackframes aren't even guaranteed to be reliable.
> > 
> > However, in the grub2 case, it was easier to just use r12 :-)
> 
> For not just the module loading case, I believe r12 is the only real solution now. I checked one debugger capable of doing ELF download. It also uses r12 for trampoline code. I am guessing for the reason that prompted this discussion.
> 

I disagree. Look carefully at Be's answer: cross-module calls
are intrinsically dangerous when stack frames are in a transient
state.

> Without r12 we'd have to change standard libraries to automagically link in gcc helpers for any conceivable non-.text section, which I am not sure is feasible. How would you write section independent helper functions which link to any section needing them?!

I don't thnk that it is tha bad: the helpers should be linked to the default .text section
when needed, typically the init code and so on are mapped within the reach of that 
section (otherwise you'll end up with the linker complaining that it finds overflowing
branch offsets between .text and .init.text).

> Asking users to create their own section specific copy of helper functions is definitely not portable if the module or other code is not architecture dependent.

Well, it automagically works on 64 bit. There is is performed by magic built into the linker.

> It is a normal gcc feature that you can assign specific code to non-.text sections and it is not documented that it may crash depending on the OS arch the ELF is built for, so asking for a Power Architecture specific change on tool libs to make Power Architecture Linux happy seems a bit much to ask.
> 

Once again I disagree.

> Using r12 in any Linux related trampoline code seems a reachable goal, and it would eliminate the conflict to the ABI.
> 

There is no conflict to the ABI. These functions are supposed to be directly reachable from whatever code
section may need them.

Now I have a question: how did you get the need for this?

None of my kernels uses them:
- if I compile with -O2, the compiler simply expands epilogue and prologue to series of lwz and stw
- if I compile with -Os, the compiler generates lmw/stmw which give the smallest possible cache footprint

Neither did I find a single reference to these functions in several systems that I grepped for.

	Regards,
	Gabriel

Re: kernel panic during kernel module load (powerpc specific part)

[Benjamin Herrenschmidt]

On Mon, 2012-06-04 at 13:03 +0200, Gabriel Paubert wrote:
> There is no conflict to the ABI. These functions are supposed to be
> directly reachable from whatever code
> section may need them.
> 
> Now I have a question: how did you get the need for this?
> 
> None of my kernels uses them:
> - if I compile with -O2, the compiler simply expands epilogue and
> prologue to series of lwz and stw
> - if I compile with -Os, the compiler generates lmw/stmw which give
> the smallest possible cache footprint
> 
> Neither did I find a single reference to these functions in several
> systems that I grepped for.

Newer gcc's ... even worse, with -Os, it does it for a single register
spill afaik. At least that's how I hit it with grub2 using whatever gcc
is in fc17.

Cheers,
Ben.

Re: kernel panic during kernel module load (powerpc specific part)

[Gabriel Paubert]

On Tue, Jun 05, 2012 at 08:00:42AM +1000, Benjamin Herrenschmidt wrote:
> On Mon, 2012-06-04 at 13:03 +0200, Gabriel Paubert wrote:
> > There is no conflict to the ABI. These functions are supposed to be
> > directly reachable from whatever code
> > section may need them.
> > 
> > Now I have a question: how did you get the need for this?
> > 
> > None of my kernels uses them:
> > - if I compile with -O2, the compiler simply expands epilogue and
> > prologue to series of lwz and stw
> > - if I compile with -Os, the compiler generates lmw/stmw which give
> > the smallest possible cache footprint
> > 
> > Neither did I find a single reference to these functions in several
> > systems that I grepped for.
> 
> Newer gcc's ... even worse, with -Os, it does it for a single register
> spill afaik. At least that's how I hit it with grub2 using whatever gcc
> is in fc17.

Ok, it's beyond stupid, and at this point must be considered a gcc bug.

	Gabriel

Re: kernel panic during kernel module load (powerpc specific part)

[Benjamin Herrenschmidt]

On Tue, 2012-06-05 at 12:44 +0200, Gabriel Paubert wrote:
> On Tue, Jun 05, 2012 at 08:00:42AM +1000, Benjamin Herrenschmidt wrote:
> > On Mon, 2012-06-04 at 13:03 +0200, Gabriel Paubert wrote:
> > > There is no conflict to the ABI. These functions are supposed to be
> > > directly reachable from whatever code
> > > section may need them.
> > > 
> > > Now I have a question: how did you get the need for this?
> > > 
> > > None of my kernels uses them:
> > > - if I compile with -O2, the compiler simply expands epilogue and
> > > prologue to series of lwz and stw
> > > - if I compile with -Os, the compiler generates lmw/stmw which give
> > > the smallest possible cache footprint
> > > 
> > > Neither did I find a single reference to these functions in several
> > > systems that I grepped for.
> > 
> > Newer gcc's ... even worse, with -Os, it does it for a single register
> > spill afaik. At least that's how I hit it with grub2 using whatever gcc
> > is in fc17.
> 
> Ok, it's beyond stupid, and at this point must be considered a gcc bug.

Probably, I haven't had a chance to report it...

It would make more sense if the out of line functions also handled
creating the stack frame & disposing of it (ie, maybe a tail call for
return) but they don't even do that so yes it's quite gross.

Cheers,
Ben.

Re: kernel panic during kernel module load (powerpc specific part)

[Gabriel Paubert]

On Tue, Jun 05, 2012 at 08:00:42AM +1000, Benjamin Herrenschmidt wrote:
> On Mon, 2012-06-04 at 13:03 +0200, Gabriel Paubert wrote:
> > There is no conflict to the ABI. These functions are supposed to be
> > directly reachable from whatever code
> > section may need them.
> > 
> > Now I have a question: how did you get the need for this?
> > 
> > None of my kernels uses them:
> > - if I compile with -O2, the compiler simply expands epilogue and
> > prologue to series of lwz and stw
> > - if I compile with -Os, the compiler generates lmw/stmw which give
> > the smallest possible cache footprint
> > 
> > Neither did I find a single reference to these functions in several
> > systems that I grepped for.
> 
> Newer gcc's ... even worse, with -Os, it does it for a single register
> spill afaik. At least that's how I hit it with grub2 using whatever gcc
> is in fc17.

Well, I've not yet been able to make it call the save/restore routines,
but here on a machine with Debian testing and several gcc installed:

- gcc-4.4 never generates lmw/stmw, it always uses individual
 lwz/stw whatever options are set (-Os -mmultiple).

- gcc-4.6 and gcc-4.7 behave identically, if -Os is set, they
  generate by default lmw/stmw. But if I combine -Os with 
  -mno-multiple, they call the helper functions.

In other words, on this system, gcc-4.4 is broken but should not
cause any harm. gcc-4.6 and gcc-4.7 look correct, but are there
any processors on which -mno-multiple is worth setting?

	Regards,
	Gabriel

Re: kernel panic during kernel module load (powerpc specific part)

[Benjamin Herrenschmidt]

On Tue, 2012-06-05 at 13:32 +0200, Gabriel Paubert wrote:
> - gcc-4.6 and gcc-4.7 behave identically, if -Os is set, they
>   generate by default lmw/stmw. But if I combine -Os with 
>   -mno-multiple, they call the helper functions.
> 
> In other words, on this system, gcc-4.4 is broken but should not
> cause any harm. gcc-4.6 and gcc-4.7 look correct, but are there
> any processors on which -mno-multiple is worth setting?

It could be an artifact of the -mtune we use, dunno. And yes, I'm pretty
sure some embedded processors don't like multiple load/store.

Cheers,
Ben.

Re: kernel panic during kernel module load (powerpc specific part)

[Steffen Rumler]

> On Fri, Jun 01, 2012 at 11:33:37AM +0000, Wrobel Heinz-R39252 wrote:
>>>> I believe that the basic premise is that you should provide a directly
>>>> reachable copy of the save/rstore functions, even if this means that
>>> you need several copies of the functions.
>>>
>>> I just fixed a very similar problem with grub2 in fact. It was using r0
>>> and trashing the saved LR that way.
>>>
>>> The real fix is indeed to statically link those gcc "helpers", we
>>> shouldn't generate things like cross-module calls inside function prologs
>>> and epilogues, when stackframes aren't even guaranteed to be reliable.
>>>
>>> However, in the grub2 case, it was easier to just use r12 :-)
>> For not just the module loading case, I believe r12 is the only real solution now. I checked one debugger capable of doing ELF download. It also uses r12 for trampoline code. I am guessing for the reason that prompted this discussion.
>>
> I disagree. Look carefully at Be's answer: cross-module calls
> are intrinsically dangerous when stack frames are in a transient
> state.
>
>> Without r12 we'd have to change standard libraries to automagically link in gcc helpers for any conceivable non-.text section, which I am not sure is feasible. How would you write section independent helper functions which link to any section needing them?!
> I don't thnk that it is tha bad: the helpers should be linked to the default .text section
> when needed, typically the init code and so on are mapped within the reach of that
> section (otherwise you'll end up with the linker complaining that it finds overflowing
> branch offsets between .text and .init.text).
>
>> Asking users to create their own section specific copy of helper functions is definitely not portable if the module or other code is not architecture dependent.
> Well, it automagically works on 64 bit. There is is performed by magic built into the linker.
>
>> It is a normal gcc feature that you can assign specific code to non-.text sections and it is not documented that it may crash depending on the OS arch the ELF is built for, so asking for a Power Architecture specific change on tool libs to make Power Architecture Linux happy seems a bit much to ask.
>>
> Once again I disagree.
>
>> Using r12 in any Linux related trampoline code seems a reachable goal, and it would eliminate the conflict to the ABI.
>>
> There is no conflict to the ABI. These functions are supposed to be directly reachable from whatever code
> section may need them.
>
> Now I have a question: how did you get the need for this?
>
> None of my kernels uses them:
> - if I compile with -O2, the compiler simply expands epilogue and prologue to series of lwz and stw
> - if I compile with -Os, the compiler generates lmw/stmw which give the smallest possible cache footprint
>
> Neither did I find a single reference to these functions in several systems that I grepped for.
>
> 	Regards,
> 	Gabriel
>
Hi,

how should we continue here ?
There is the kernel panic, I've described.

Technically, there is an conflict between the code generated by the compiler and the loader in module_32.c, at least by using -Os.
Because the prologue/epilogue is part of the .text and init_module() is part of .init.text (in the case __init is applied, as usual),
a directly reachable call is not always possible.

Thanks
Steffen

Re: kernel panic during kernel module load (powerpc specific part)

[Benjamin Herrenschmidt]

On Wed, 2012-06-06 at 09:36 +0200, Steffen Rumler wrote:
> 
> how should we continue here ?
> There is the kernel panic, I've described.
> 
> Technically, there is an conflict between the code generated by the
> compiler and the loader in module_32.c, at least by using -Os.
> Because the prologue/epilogue is part of the .text and init_module()
> is part of .init.text (in the case __init is applied, as usual),
> a directly reachable call is not always possible. 

As we discussed earlier, if you could submit a patch to use r12 instead,
we should merge that.

Cheers,
Ben.

[PATCH] kernel panic during kernel module load (powerpc specific part)

[Steffen Rumler]

Hi,

The patch below is intended to fix the following problem.

According to the PowerPC EABI specification, the GPR r11 is assigned
the dedicated function to point to the previous stack frame.
In the powerpc-specific kernel module loader, do_plt_call()
(in arch/powerpc/kernel/module_32.c), the GPR r11 is also used
to generate trampoline code.

This combination crashes the kernel, in the following case:

   + The compiler has been generated the prologue and epilogue,
     which is part of the .text section.
   + The compiler has been generated the code for the module init entry point,
     part of the .init.text section (in the case it is marked with __init).
   + By returning from the module init entry point, the epilogue is called by doing
     a branch instruction.
   + If the epilogue is too far away, a relative branch instruction cannot be applied.
     Instead trampoline code is generated in do_plt_call(), in order to jump via register.
     Unfortunately the code generated by do_plt_call() destroys the content of GPR r11.
   + Because GPR r11 does not more keep the right stack frame pointer,
     the kernel crashes right after the epilogue.

The fix just uses GPR r12 instead of GPR r11 for generating the trampoline code.
According to the statements from Freescale, this is also save from EABI perspective.

I've tested the fix for kernel 2.6.33 on MPC8541.

Signed-off-by: Steffen Rumler <steffen.rumler.ext@nsn.com>
---

--- orig/arch/powerpc/kernel/module_32.c	2012-06-06 16:04:28.956446788 +0200
+++ new/arch/powerpc/kernel/module_32.c		2012-06-06 16:04:17.746290683 +0200
@@ -187,8 +187,8 @@

  static inline int entry_matches(struct ppc_plt_entry *entry, Elf32_Addr val)
  {
-	if (entry->jump[0] == 0x3d600000 + ((val + 0x8000) >> 16)
-	    && entry->jump[1] == 0x396b0000 + (val & 0xffff))
+	if (entry->jump[0] == 0x3d800000 + ((val + 0x8000) >> 16)
+	    && entry->jump[1] == 0x398c0000 + (val & 0xffff))
  		return 1;
  	return 0;
  }
@@ -215,10 +215,9 @@
  		entry++;
  	}

-	/* Stolen from Paul Mackerras as well... */
-	entry->jump[0] = 0x3d600000+((val+0x8000)>>16);	/* lis r11,sym@ha */
-	entry->jump[1] = 0x396b0000 + (val&0xffff);	/* addi r11,r11,sym@l*/
-	entry->jump[2] = 0x7d6903a6;			/* mtctr r11 */
+	entry->jump[0] = 0x3d800000+((val+0x8000)>>16); /* lis r12,sym@ha */
+	entry->jump[1] = 0x398c0000 + (val&0xffff);     /* addi r12,r12,sym@l*/
+	entry->jump[2] = 0x7d8903a6;                    /* mtctr r12 */
  	entry->jump[3] = 0x4e800420;			/* bctr */

  	DEBUGP("Initialized plt for 0x%x at %p\n", val, entry);

Re: [PATCH] kernel panic during kernel module load (powerpc specific part)

[roger blofeld]

--- On Wed, 6/6/12, Steffen Rumler <steffen.rumler.ext@nsn.com> wrote:=0A=
=0A> From: Steffen Rumler <steffen.rumler.ext@nsn.com>=0A> Subject: [PATCH]=
 kernel panic during kernel module load (powerpc specific part)=0A> To: "ex=
t Benjamin Herrenschmidt" <benh@kernel.crashing.org>, paulus@samba.org=0A> =
Cc: "Wrobel Heinz-R39252" <r39252@freescale.com>, "Michael Ellerman" <micha=
el@ellerman.id.au>, "linuxppc-dev@lists.ozlabs.org" <linuxppc-dev@lists.ozl=
abs.org>=0A> Date: Wednesday, June 6, 2012, 7:37 AM=0A> Hi,=0A> =0A> The pa=
tch below is intended to fix the following problem.=0A> =0A> According to t=
he PowerPC EABI specification, the GPR r11 is=0A> assigned=0A> the dedicate=
d function to point to the previous stack=0A> frame.=0A> In the powerpc-spe=
cific kernel module loader, do_plt_call()=0A> (in arch/powerpc/kernel/modul=
e_32.c), the GPR r11 is also=0A> used=0A> to generate trampoline code.=0A> =
=0A> This combination crashes the kernel, in the following case:=0A> =0A> =
=A0 + The compiler has been generated the prologue and=0A> epilogue,=0A> =
=A0 =A0 which is part of the .text section.=0A> =A0 + The compiler has been=
 generated the code for the=0A> module init entry point,=0A> =A0 =A0 part o=
f the .init.text section (in the case it=0A> is marked with __init).=0A> =
=A0 + By returning from the module init entry point, the=0A> epilogue is ca=
lled by doing=0A> =A0 =A0 a branch instruction.=0A> =A0 + If the epilogue i=
s too far away, a relative branch=0A> instruction cannot be applied.=0A> =
=A0 =A0 Instead trampoline code is generated in=0A> do_plt_call(), in order=
 to jump via register.=0A> =A0 =A0 Unfortunately the code generated by=0A> =
do_plt_call() destroys the content of GPR r11.=0A> =A0 + Because GPR r11 do=
es not more keep the right stack=0A> frame pointer,=0A> =A0 =A0 the kernel =
crashes right after the epilogue.=0A> =0A> The fix just uses GPR r12 instea=
d of GPR r11 for generating=0A> the trampoline code.=0A> According to the s=
tatements from Freescale, this is also=0A> save from EABI perspective.=0A> =
=0A> I've tested the fix for kernel 2.6.33 on MPC8541.=0A> =0A> Signed-off-=
by: Steffen Rumler <steffen.rumler.ext@nsn.com>=0A> ---=0A> =0A> --- orig/a=
rch/powerpc/kernel/module_32.c=A0=A0=A0=0A> 2012-06-06 16:04:28.956446788 +=
0200=0A> +++ new/arch/powerpc/kernel/module_32.c=A0=A0=A0=0A> =A0=A0=A0 201=
2-06-06 16:04:17.746290683 +0200=0A> @@ -187,8 +187,8 @@=0A> =0A>  static i=
nline int entry_matches(struct ppc_plt_entry=0A> *entry, Elf32_Addr val)=0A=
>  {=0A> -=A0=A0=A0 if (entry->jump[0] =3D=3D 0x3d600000 +=0A> ((val + 0x80=
00) >> 16)=0A> -=A0=A0=A0 =A0 =A0 &&=0A> entry->jump[1] =3D=3D 0x396b0000 +=
 (val & 0xffff))=0A> +=A0=A0=A0 if (entry->jump[0] =3D=3D 0x3d800000 +=0A> =
((val + 0x8000) >> 16)=0A> +=A0=A0=A0 =A0 =A0 &&=0A> entry->jump[1] =3D=3D =
0x398c0000 + (val & 0xffff))=0A>  =A0=A0=A0 =A0=A0=A0 return 1;=0A>  =A0=A0=
=A0 return 0;=0A>  }=0A> @@ -215,10 +215,9 @@=0A>  =A0=A0=A0 =A0=A0=A0 entr=
y++;=0A>  =A0=A0=A0 }=0A> =0A> -=A0=A0=A0 /* Stolen from Paul Mackerras as =
well...=0A> */=0A> -=A0=A0=A0 entry->jump[0] =3D=0A> 0x3d600000+((val+0x800=
0)>>16);=A0=A0=A0 /*=0A> lis r11,sym@ha */=0A> -=A0=A0=A0 entry->jump[1] =
=3D 0x396b0000 +=0A> (val&0xffff);=A0=A0=A0 /* addi r11,r11,sym@l*/=0A> -=
=A0=A0=A0 entry->jump[2] =3D=0A> 0x7d6903a6;=A0=A0=A0 =A0=A0=A0=0A> =A0=A0=
=A0 /* mtctr r11 */=0A> +=A0=A0=A0 entry->jump[0] =3D=0A> 0x3d800000+((val+=
0x8000)>>16); /* lis r12,sym@ha */=0A> +=A0=A0=A0 entry->jump[1] =3D 0x398c=
0000 +=0A> (val&0xffff);=A0 =A0=A0=A0/* addi=0A> r12,r12,sym@l*/=0A> +=A0=
=A0=A0 entry->jump[2] =3D 0x7d8903a6;=A0=0A> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =
=A0=0A> =A0 /* mtctr r12 */=0A>  =A0=A0=A0 entry->jump[3] =3D=0A> 0x4e80042=
0;=A0=A0=A0 =A0=A0=A0=0A> =A0=A0=A0 /* bctr */=0A> =0A>  =A0=A0=A0 DEBUGP("=
Initialized plt for 0x%x at=0A> %p\n", val, entry);=0A> ___________________=
____________________________=0A> Linuxppc-dev mailing list=0A> Linuxppc-dev=
@lists.ozlabs.org=0A> https://lists.ozlabs.org/listinfo/linuxppc-dev=0A> =
=0A=0AHi,=0A=0AShouldn't the corresponding ftrace code be updated to match?=
 Perhaps like the following (untested) patch:=0A=0ASigned-off-by: Roger Blo=
feld <blofeldus@yahoo.com>=0A---=0A=0A--- a/arch/powerpc/kernel/ftrace.c=0A=
+++ b/arch/powerpc/kernel/ftrace.c=0A@@ -245,9 +245,9 @@ __ftrace_make_nop(=
struct module *mod,=0A =0A =09/*=0A =09 * On PPC32 the trampoline looks lik=
e:=0A-=09 *  0x3d, 0x60, 0x00, 0x00  lis r11,sym@ha=0A-=09 *  0x39, 0x6b, 0=
x00, 0x00  addi r11,r11,sym@l=0A-=09 *  0x7d, 0x69, 0x03, 0xa6  mtctr r11=
=0A+=09 *  0x3d, 0x80, 0x00, 0x00  lis r12,sym@ha=0A+=09 *  0x39, 0x8c, 0x0=
0, 0x00  addi r12,r12,sym@l=0A+=09 *  0x7d, 0x89, 0x03, 0xa6  mtctr r12=0A =
=09 *  0x4e, 0x80, 0x04, 0x20  bctr=0A =09 */=0A =0A@@ -262,9 +262,9 @@ __f=
trace_make_nop(struct module *mod,=0A =09pr_devel(" %08x %08x ", jmp[0], jm=
p[1]);=0A =0A =09/* verify that this is what we expect it to be */=0A-=09if=
 (((jmp[0] & 0xffff0000) !=3D 0x3d600000) ||=0A-=09    ((jmp[1] & 0xffff000=
0) !=3D 0x396b0000) ||=0A-=09    (jmp[2] !=3D 0x7d6903a6) ||=0A+=09if (((jm=
p[0] & 0xffff0000) !=3D 0x3d800000) ||=0A+=09    ((jmp[1] & 0xffff0000) !=
=3D 0x398c0000) ||=0A+=09    (jmp[2] !=3D 0x7d8903a6) ||=0A =09    (jmp[3] =
!=3D 0x4e800420)) {=0A =09=09printk(KERN_ERR "Not a trampoline\n");=0A =09=
=09return -EINVAL;=0A