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

[Steffen Rumler <steffen.rumler.ext@nsn.com>]

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