| Message ID | 1338420242.27402.2.camel@concordia (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
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. 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. 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? Regards, Heinz
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
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
> > 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
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
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.
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
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
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.
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.
> 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
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.
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);