Message ID | 20160323155858.GB27110@lst.de (mailing list archive) |
---|---|
State | Rejected, archived |
Headers | show |
On 24/03/16 02:58, Torsten Duwe wrote: > Since nobody liked the extra stack frame nor its workarounds, here is > the next attempt. Assumptions: > > 1. Heuristics are bad. The better they are, the more subtly the > way they might fail. > > 2. The TOC pointer is usually dividable by 4, if not by 8. An odd > value never occurs. > > Conclusively, this patch unambiguously creates an odd TOC value when > an ftraced function's global entry point is used. Ftrace_caller will > then immediately fix it, and alongside gather the information whether > the made call was local or global. > > In case of live patching this information is furthermore used to decide > whether a klp_return_helper needs to be inserted or not. > CAVEAT: any frameless klp_return_helper does not play well with > sibling calls! There's an emergency exit that might work, at worst > it will cause an oops, but it surely avoids a lockup. > At least the live patching modules on ppc64le will need to be compiled > using the -fno-optimize-sibling-calls compiler flag! > > Thanks go to Michael Matz and Richard Biener for reassurance about > heuristics and pointers to the compiler flag. > > Signed-off-by: Torsten Duwe <duwe@suse.de> > > I missed this yesterday, not on cc, but caught it on the list today Thanks for working on this. I did a quick look, so the CR+4 code plus heuristics for global/local call detection? I'll review this soon - hopefully tonight, but we have a long weekend coming up, so there might be delays. In the meanwhile feel free to add my signed-off-by for the CR+4 code. I am also looking at a different approach -- per thread lr0 stack. Balbir Singh.
On Thu, Mar 24, 2016 at 01:23:01PM +1100, Balbir Singh wrote: > On 24/03/16 02:58, Torsten Duwe wrote: > > > > 1. Heuristics are bad. The better they are, the more subtly the > > way they might fail. [...] > I missed this yesterday, not on cc, but caught it on the list today I replied to Michael's last post and removed the back reference to start a new thread. The list is rather long... sorry I didn't notice. > Thanks for working on this. I did a quick look, so the CR+4 code Yes. It's only a proof of concept. That idea is yours, no doubt, and should be mentioned in the final submission. I already wrote it in my previous version, where I also have changed the arithmetic to produce small positive deltas. > plus heuristics for global/local call detection? Nope, definitely not! I flag global entries unambiguously. I had a version with R12/LR heuristics on Monday which I dumped. > I'll review this soon - hopefully tonight, but we have a long weekend coming up, so there might be delays. In the meanwhile feel free to add my signed-off-by for the CR+4 code. I am also looking at a different approach -- per thread lr0 stack. This CR+4 code leaves only 1 slot, so sibling calls are extremely dangerous, as I mentioned. But with a little attention, this patch works very well. I mostly wanted to hear opinions about a transient odd TOC value before I start polishing. Torsten
* Torsten Duwe <duwe@lst.de> [2016-03-23 16:58:58]: > > Since nobody liked the extra stack frame nor its workarounds, here is > the next attempt. Assumptions: > > 1. Heuristics are bad. The better they are, the more subtly the > way they might fail. > > 2. The TOC pointer is usually dividable by 4, if not by 8. An odd > value never occurs. > > Conclusively, this patch unambiguously creates an odd TOC value when > an ftraced function's global entry point is used. Ftrace_caller will > then immediately fix it, and alongside gather the information whether > the made call was local or global. > > In case of live patching this information is furthermore used to decide > whether a klp_return_helper needs to be inserted or not. > CAVEAT: any frameless klp_return_helper does not play well with > sibling calls! There's an emergency exit that might work, at worst > it will cause an oops, but it surely avoids a lockup. > At least the live patching modules on ppc64le will need to be compiled > using the -fno-optimize-sibling-calls compiler flag! > > Thanks go to Michael Matz and Richard Biener for reassurance about > heuristics and pointers to the compiler flag. > > Signed-off-by: Torsten Duwe <duwe@suse.de> Hi Torsten, Should this patch be applied over Petr Mladek's v4 ? Thanks, Kamalesh.
On Thu, Mar 24, 2016 at 03:44:55PM +0530, Kamalesh Babulal wrote: > * Torsten Duwe <duwe@lst.de> [2016-03-23 16:58:58]: > > > > > Since nobody liked the extra stack frame nor its workarounds, here is > > the next attempt. Assumptions: > > > > 1. Heuristics are bad. The better they are, the more subtly the > > way they might fail. > > > > 2. The TOC pointer is usually dividable by 4, if not by 8. An odd > > value never occurs. > > > > Conclusively, this patch unambiguously creates an odd TOC value when > > an ftraced function's global entry point is used. Ftrace_caller will > > then immediately fix it, and alongside gather the information whether > > the made call was local or global. > > > > In case of live patching this information is furthermore used to decide > > whether a klp_return_helper needs to be inserted or not. > > CAVEAT: any frameless klp_return_helper does not play well with > > sibling calls! There's an emergency exit that might work, at worst > > it will cause an oops, but it surely avoids a lockup. > > At least the live patching modules on ppc64le will need to be compiled > > using the -fno-optimize-sibling-calls compiler flag! > > > > Thanks go to Michael Matz and Richard Biener for reassurance about > > heuristics and pointers to the compiler flag. > > > > Signed-off-by: Torsten Duwe <duwe@suse.de> > > Hi Torsten, > > Should this patch be applied over Petr Mladek's v4 ? Yes. Just omit the changes it makes to entry_64.S and use this instead. Torsten
On Thu, 2016-03-24 at 09:04 +0100, Torsten Duwe wrote: > On Thu, Mar 24, 2016 at 01:23:01PM +1100, Balbir Singh wrote: > > On 24/03/16 02:58, Torsten Duwe wrote: > > > > > > 1. Heuristics are bad. The better they are, the more subtly the > > > way they might fail. > [...] > > This CR+4 code leaves only 1 slot, so sibling calls are extremely dangerous, > as I mentioned. But with a little attention, this patch works very well. > > I mostly wanted to hear opinions about a transient odd TOC value > before I start polishing. Hi Torsten, I just posted the version I've been working on. I prefer it, but you'll probably tell me that there's some horrible case it doesn't handle correctly :) cheers
* Torsten Duwe <duwe@lst.de> [2016-03-24 11:27:57]: > On Thu, Mar 24, 2016 at 03:44:55PM +0530, Kamalesh Babulal wrote: > > * Torsten Duwe <duwe@lst.de> [2016-03-23 16:58:58]: > > > > > > > > Since nobody liked the extra stack frame nor its workarounds, here is > > > the next attempt. Assumptions: > > > > > > 1. Heuristics are bad. The better they are, the more subtly the > > > way they might fail. > > > > > > 2. The TOC pointer is usually dividable by 4, if not by 8. An odd > > > value never occurs. > > > > > > Conclusively, this patch unambiguously creates an odd TOC value when > > > an ftraced function's global entry point is used. Ftrace_caller will > > > then immediately fix it, and alongside gather the information whether > > > the made call was local or global. > > > > > > In case of live patching this information is furthermore used to decide > > > whether a klp_return_helper needs to be inserted or not. > > > CAVEAT: any frameless klp_return_helper does not play well with > > > sibling calls! There's an emergency exit that might work, at worst > > > it will cause an oops, but it surely avoids a lockup. > > > At least the live patching modules on ppc64le will need to be compiled > > > using the -fno-optimize-sibling-calls compiler flag! > > > > > > Thanks go to Michael Matz and Richard Biener for reassurance about > > > heuristics and pointers to the compiler flag. > > > > > > Signed-off-by: Torsten Duwe <duwe@suse.de> > > > > Hi Torsten, > > > > Should this patch be applied over Petr Mladek's v4 ? > > Yes. Just omit the changes it makes to entry_64.S and use this instead. Thanks, I was able to successfully execute basic livepatch sample module test. Thanks, Kamalesh.
diff --git a/arch/powerpc/kernel/entry_64.S b/arch/powerpc/kernel/entry_64.S index 9916d15..449c22a 100644 --- a/arch/powerpc/kernel/entry_64.S +++ b/arch/powerpc/kernel/entry_64.S @@ -1240,6 +1240,16 @@ _GLOBAL(ftrace_caller) std r7, _NIP(r1) std r7, _LINK(r1) +#ifdef CONFIG_LIVEPATCH + mr r14,r7 /* remember old NIP */ + + /* Test whether R2 is odd */ + andi. r3,r2,1 + xor r2,r2,r3 + mfcr r3 /* remember CR0 */ + stw r3,8(r1) +#endif + /* Save callee's TOC in the ABI compliant location */ std r2, 24(r1) ld r2,PACATOC(r13) /* get kernel TOC in r2 */ @@ -1273,8 +1283,16 @@ ftrace_call: ld r3, _NIP(r1) mtctr r3 +#ifdef CONFIG_LIVEPATCH + lwa r0,8(r1) + mtcr r0 /* recall "global call" info in CR0 */ + + cmp 5,1,r14,r3 /* has NIP been altered? */ +#endif + /* Restore gprs */ - REST_8GPRS(0,r1) + REST_GPR(3,r1) /* 0:scratch, 1:SP, 2:manually below */ + REST_4GPRS(4,r1) REST_8GPRS(8,r1) REST_8GPRS(16,r1) REST_8GPRS(24,r1) @@ -1289,6 +1307,30 @@ ftrace_call: ld r0, LRSAVE(r1) mtlr r0 +#ifdef CONFIG_LIVEPATCH + beq+ cr5,4f /* likely(old_NIP == new_NIP) */ + bne 6f /* It was a global call already */ + + /* find klp_return_helper's address from here */ + bl 5f +5: mflr r12 + addi r12, r12, (klp_return_helper + 4 - .)@l + + cmpd r12,r0 /* sibling call? bail out! */ + beq- 6f + + subf r0,r0,r2 /* calculate TOC - LR */ + stw r0,12(r1) /* and save the delta in the reserved field */ + std r12, LRSAVE(r1) + std r2, 24(r1) /* save TOC now, unconditionally. */ + mtlr r12 + mr r0,r12 +6: + mfctr r12 /* allow for TOC calculation in newfunc */ + bctr +4: +#endif + #ifdef CONFIG_FUNCTION_GRAPH_TRACER stdu r1, -112(r1) .globl ftrace_graph_call @@ -1305,6 +1347,24 @@ _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 or not. +*/ +klp_return_helper: + ld r2, 24(r1) /* restore TOC (saved by ftrace_caller) */ + lwa r0,12(r1) /* get TOC - LR */ + subf r0,r0,r2 /* and calculate the real return address */ + mtlr r0 + blr +#endif + #else _GLOBAL_TOC(_mcount) /* Taken from output of objdump from lib64/glibc */ diff --git a/arch/powerpc/kernel/ftrace.c b/arch/powerpc/kernel/ftrace.c index 9dac18d..30b550a 100644 --- a/arch/powerpc/kernel/ftrace.c +++ b/arch/powerpc/kernel/ftrace.c @@ -41,6 +41,59 @@ ftrace_call_replace(unsigned long ip, unsigned long addr, int link) return op; } +#ifdef CONFIG_LIVEPATCH +static int +odd_TOC(unsigned long ip, bool enable) +{ + unsigned int *addi_loc = 0; + unsigned int new, replaced; + + /* In case of DYNAMIC_FTRACE, mark the global entry with an odd TOC + * value for live patching. 2 instructions later (what we just patched) + * we'll be in ftrace_caller anyway, which will straighten it up again. + */ + + /* This test should only succeed when a "leaf+" function + * is at the beginning of a page. Those functions need no global + * call indicator anyway. All (other) functions are aligned 16 bytes. + */ + if ( (ip & 0xFFFF) < 12 ) /* ip too close to beginning of a page? */ + return 0; + + addi_loc = (unsigned int *)ip; + addi_loc -= 2; /* ip - 8 */ + + if (probe_kernel_read(&replaced, addi_loc, MCOUNT_INSN_SIZE)) + return -EFAULT; + + /* look for the 2nd 16-bit add that calculates the TOC */ + if ( (replaced & 0xFFFF0000) != 0x38420000) { /* "addi r2,r2,#UI" */ + addi_loc--; /* ip - 12, older GCCs */ + + if (probe_kernel_read(&replaced, addi_loc, MCOUNT_INSN_SIZE)) + return -EFAULT; + + /* So is this a -mprofile-kernel _mcount site at all? */ + if ( (replaced & 0xFFFF0000) != 0x38420000) + return 0; + } + + /* Did we enable or disable ftracing here? clear or set the odd bit + * accordingly. + */ + if (enable) + new = replaced | 1; + else + new = replaced & ~1; + + if (patch_instruction(addi_loc, new)) + return -EPERM; + return 0; +} +#else +static int odd_TOC(unsigned long ip, bool enable) {return 0;} +#endif + static int ftrace_modify_code(unsigned long ip, unsigned int old, unsigned int new) { @@ -71,7 +124,7 @@ ftrace_modify_code(unsigned long ip, unsigned int old, unsigned int new) if (patch_instruction((unsigned int *)ip, new)) return -EPERM; - return 0; + return odd_TOC(ip, (new != PPC_INST_NOP)); } /* @@ -194,7 +247,7 @@ __ftrace_make_nop(struct module *mod, return -EPERM; } - return 0; + return odd_TOC(ip, false); } #else /* !PPC64 */ @@ -384,7 +437,7 @@ __ftrace_make_call(struct dyn_ftrace *rec, unsigned long addr) return -EINVAL; } - return 0; + return odd_TOC((unsigned long)ip, true); } #ifdef CONFIG_DYNAMIC_FTRACE_WITH_REGS
Since nobody liked the extra stack frame nor its workarounds, here is the next attempt. Assumptions: 1. Heuristics are bad. The better they are, the more subtly the way they might fail. 2. The TOC pointer is usually dividable by 4, if not by 8. An odd value never occurs. Conclusively, this patch unambiguously creates an odd TOC value when an ftraced function's global entry point is used. Ftrace_caller will then immediately fix it, and alongside gather the information whether the made call was local or global. In case of live patching this information is furthermore used to decide whether a klp_return_helper needs to be inserted or not. CAVEAT: any frameless klp_return_helper does not play well with sibling calls! There's an emergency exit that might work, at worst it will cause an oops, but it surely avoids a lockup. At least the live patching modules on ppc64le will need to be compiled using the -fno-optimize-sibling-calls compiler flag! Thanks go to Michael Matz and Richard Biener for reassurance about heuristics and pointers to the compiler flag. Signed-off-by: Torsten Duwe <duwe@suse.de>