ppc64le live patch: clear out storage location(s) in mini stack frame
PATCH 1/2 ppc64le live patch: clear out storage location(s) in mini stack frame
This can be applied on top of Petr Mladek's v4 rework of the ppc64le live patching. Inspired by Balbir Singh's v5, information about the callee's r2 is stored in a "reserved" 32 bit location in the caller's stack frame, instead of 64 bits in the newly created mini frame 24(r1).
It only needs to work for a local call, when caller's TOC == callee's TOC, and along with the return address (LR) it's all within a 4GiB range (+-31 bits). If the original call already was global, we are allowed to restore any nonsense into r2, because the global caller will restore its TOC anyway from the ABI compliant location 24(r1) right after return.
---
This is only the preparation for dumping the mini stack frame. It shouldn't break anything, bisecting-wise.
arch/powerpc/kernel/entry_64.S +4 -2
@@ -1284,8 +1284,9 @@ ftrace_call: * lr and LRSAVE(r1) contain the address of klp_return_helper. * 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 */ - std r2, 24(r1) /* save TOC now, unconditionally. */ bl 5f 5: mflr r12 addi r12, r12, (klp_return_helper + 4 - .)@l
@@ -1323,9 +1324,10 @@ _GLOBAL(ftrace_stub) * maybe inserting a klp_return_helper frame or not. */ klp_return_helper: - ld r2, 24(r1) /* restore TOC (saved by ftrace_caller) */ 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
PATCH 2/2 ppc64le live patch: get rid of mini stack frame
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 */
---
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.
arch/powerpc/kernel/entry_64.S +2 -31
@@ -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)
Re: [PATCH 2/2] ppc64le live patch: get rid of mini stack frame
Quoted 6 lines · Torsten Duwe
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
Re: [PATCH 2/2] ppc64le live patch: get rid of mini stack frame
Quoted 11 lines · Petr Mladek
Quoted 6 lines · Torsten Duwe
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
Re: [PATCH 2/2] ppc64le live patch: get rid of mini stack frame
Quoted 1 line · Petr Mladek
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.
Signature · 2 lines
Jiri Kosina SUSE Labs
Re: [PATCH 2/2] ppc64le live patch: get rid of mini stack frame
In reply to Petr MladekQuoted 9 lines · Petr Mladek
Quoted 8 lines · Petr Mladek
Quoted 6 lines · Torsten Duwe
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:
Quoted 3 lines
Quoted 3 lines
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.
Quoted 1 line
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
Michael Ellerman
reviewerRe: [PATCH 2/2] ppc64le live patch: get rid of mini stack frame
Quoted 14 lines · Torsten Duwe
Quoted 9 lines · Petr Mladek
Quoted 8 lines · Petr Mladek
Quoted 6 lines · Torsten Duwe
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:Quoted 3 lines
Quoted 3 lines
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.
Quoted 8 lines
Yes, that's what I think as well when I look at it.
Quoted 1 line
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
Quoted 5 lines · Torsten Duwe
This can be applied on top of Petr Mladek's v4 rework of the ppc64le live patching. Inspired by Balbir Singh's v5, information about the callee's r2 is stored in a "reserved" 32 bit location in the caller's stack frame, instead of 64 bits in the newly created mini frame 24(r1).
Quoted 8 lines
It only needs to work for a local call, when caller's TOC == callee's TOC, and along with the return address (LR) it's all within a 4GiB range (+-31 bits). If the original call already was global, we are allowed to restore any nonsense into r2, because the global caller will restore its TOC anyway from the ABI compliant location 24(r1) right after return.
Hi, Torsten
Sorry, I've had no time to test this. Caught up with something else for the moment. Hopefully I'll get a chance over the weekend.
Have you tested this against Petr's sample changes to patch printk?
Balbir Singh.