| Message ID | 20160309173017.GD27913@lst.de (mailing list archive) |
|---|---|
| State | Rejected |
| Headers | show |
On Wed 2016-03-09 18:30:17, Torsten Duwe wrote: > After the mini stack frame is no longer required for TOC storage, it can > be eliminated iff the functionality of klp_return_helper, which required > a stack frame for the extra return address previously, is carried out > by the replacement function now. This requires _every_ live patch replacement > function to execute the following (or similar) sequence of machine instructions > just before every return to the original caller: I have thought about it and it is a nono from my point of view. It is too error prone, especially that there are functions that call return on several locations. Best Regards, Petr
On Thu 2016-03-10 13:25:08, Petr Mladek wrote: > On Wed 2016-03-09 18:30:17, Torsten Duwe wrote: > > After the mini stack frame is no longer required for TOC storage, it can > > be eliminated iff the functionality of klp_return_helper, which required > > a stack frame for the extra return address previously, is carried out > > by the replacement function now. This requires _every_ live patch replacement > > function to execute the following (or similar) sequence of machine instructions > > just before every return to the original caller: > > I have thought about it and it is a nono from my point of view. > It is too error prone, especially that there are functions that > call return on several locations. BTW: How is this solved in kretprobes? Or is it easier there? Best Regards, Petr
On Thu, 10 Mar 2016, Petr Mladek wrote:
> BTW: How is this solved in kretprobes? Or is it easier there?
Is that really a problem? With kretprobes you're never performing the
module->core->module transition in "one" redirection, right?
The 'kretprobe_trampoline' is a global symbol, and hence everybody is
generating 'global call' for it, and that's pretty much it.
On Thu, Mar 10, 2016 at 01:51:16PM +0100, Petr Mladek wrote: > On Thu 2016-03-10 13:25:08, Petr Mladek wrote: > > On Wed 2016-03-09 18:30:17, Torsten Duwe wrote: > > > After the mini stack frame is no longer required for TOC storage, it can > > > be eliminated iff the functionality of klp_return_helper, which required > > > a stack frame for the extra return address previously, is carried out > > > by the replacement function now. This requires _every_ live patch replacement > > > function to execute the following (or similar) sequence of machine instructions > > > just before every return to the original caller: > > > > I have thought about it and it is a nono from my point of view. > > It is too error prone, especially that there are functions that > > call return on several locations. Yes, that's what I think as well when I look at it. > BTW: How is this solved in kretprobes? Or is it easier there? Without any look at the code I assume it uses solution 3. Once you have a probing framework in place, you can remember the real return addresses in a data structure. As I wrote, the function graph tracer does it this way so it would be abvious. Torsten
On Thu, 2016-03-10 at 14:04 +0100, Torsten Duwe wrote: > On Thu, Mar 10, 2016 at 01:51:16PM +0100, Petr Mladek wrote: > > On Thu 2016-03-10 13:25:08, Petr Mladek wrote: > > > On Wed 2016-03-09 18:30:17, Torsten Duwe wrote: > > > > After the mini stack frame is no longer required for TOC storage, it can > > > > be eliminated iff the functionality of klp_return_helper, which required > > > > a stack frame for the extra return address previously, is carried out > > > > by the replacement function now. This requires _every_ live patch replacement > > > > function to execute the following (or similar) sequence of machine instructions > > > > just before every return to the original caller: > > > > > > I have thought about it and it is a nono from my point of view. > > > It is too error prone, especially that there are functions that > > > call return on several locations. > > Yes, that's what I think as well when I look at it. > > BTW: How is this solved in kretprobes? Or is it easier there? > > Without any look at the code I assume it uses solution 3. Once > you have a probing framework in place, you can remember the real > return addresses in a data structure. As I wrote, the function > graph tracer does it this way so it would be abvious. Yeah it has a linked list of struct kretprobe_instance's, each of which stores the real return address. I have some ideas for how to fix livepatch, but this week is a bit busy with merge window prep. cheers
--- a/arch/powerpc/kernel/entry_64.S +++ b/arch/powerpc/kernel/entry_64.S @@ -1277,21 +1277,11 @@ ftrace_call: * For a local call, restore this TOC after calling the patch function. * For a global call, it does not matter what we restore here, * since the global caller does its own restore right afterwards, - * anyway. Just insert a klp_return_helper frame in any case, - * so a patch function can always count on the changed stack offsets. - * The patch introduces a frame such that from the patched function - * we return back to klp_return helper. For ABI compliance r12, - * lr and LRSAVE(r1) contain the address of klp_return_helper. - * We loaded ctr with the address of the patched function earlier + * anyway. Just prepare here the TOC restore in patch functions. + * We loaded ctr with the address of the patched function earlier. */ subf r0, r0, r2 /* Calculate offset from current TOC to LR */ stw r0, 12(r1) /* and save it in CR+4 */ - stdu r1, -32(r1) /* open new mini stack frame */ - bl 5f -5: mflr r12 - addi r12, r12, (klp_return_helper + 4 - .)@l - std r12, LRSAVE(r1) - mtlr r12 mfctr r12 /* allow for TOC calculation in newfunc */ bctr 4: @@ -1313,25 +1303,6 @@ _GLOBAL(ftrace_graph_stub) _GLOBAL(ftrace_stub) blr -#ifdef CONFIG_LIVEPATCH -/* Helper function for local calls that are becoming global - * due to live patching. - * We can't simply patch the NOP after the original call, - * because, depending on the consistency model, some kernel - * threads may still have called the original, local function - * *without* saving their TOC in the respective stack frame slot, - * so the decision is made per-thread during function return by - * maybe inserting a klp_return_helper frame or not. -*/ -klp_return_helper: - addi r1, r1, 32 /* destroy mini stack frame */ - lwa r2, 12(r1) /* Load from CR+4, offset of TOC w.r.t LR */ - ld r0, LRSAVE(r1) /* get the real return address */ - add r2, r2, r0 /* Add the current LR to offset */ - mtlr r0 - blr -#endif - #else _GLOBAL_TOC(_mcount)
After the mini stack frame is no longer required for TOC storage, it can be eliminated iff the functionality of klp_return_helper, which required a stack frame for the extra return address previously, is carried out by the replacement function now. This requires _every_ live patch replacement function to execute the following (or similar) sequence of machine instructions just before every return to the original caller: ld r0, 0(r1) /* use back link to find caller's frame */ lwa r2, 12(r0) /* Load from CR+4, offset of TOC w.r.t LR */ ld r0, LRSAVE(r0) /* get the real return address */ add r2, r2, r0 /* Add the current LR to offset */ Signed-off-by: Torsten Duwe <duwe@suse.de> --- This is solution 1 now. Do we really want that? I don't think so; this is merely to illustrate what the alternative to klp_return_helper and its extra stack frame would look like. Hence, I didn't test yet whether all the details are correct.