On 01/03/16 15:38, Michael Ellerman wrote: > Hi Balbir, > > Some comments inline ... > > On Thu, 2016-02-25 at 23:11 +1100, Balbir Singh wrote: > >> This applies on top of the patches posted by Michael today >> Enable livepatching. This takes patch 6/8 and 7/8 of v8 as the base. >> Removes the extra strict check in gcc-profile-kernel-notrace.sh >> and adds logic for checking offsets in livepatch. The patch >> for HAVE_C_RECORDMCOUNT is not required and not used here. >> >> Depending on whether or not a TOC is generated, the offset >> for _mcount can be +16 or +8. The changes are such that the >> offset checks are specific to powerpc. >> >> Comments? Testing? I tested the sample in the livepatch >> directory > You forgot to CC linuxppc-dev :) Yes, I did bounce the message later - sorry! >> References >> >> 1. https://patchwork.ozlabs.org/patch/581521/ >> 2. https://patchwork.ozlabs.org/patch/587464/ >> >> Signed-off-by: Torsten Duwe <duwe@xxxxxxx> >> Signed-off-by: Balbir Singh <bsingharora@xxxxxxxxx> >> --- >> arch/powerpc/Kconfig | 3 ++ >> arch/powerpc/gcc-mprofile-kernel-notrace.sh | 7 ---- >> arch/powerpc/include/asm/livepatch.h | 61 +++++++++++++++++++++++++++++ >> arch/powerpc/kernel/Makefile | 1 + >> arch/powerpc/kernel/entry_64.S | 46 ++++++++++++++++++++++ >> arch/powerpc/kernel/livepatch.c | 38 ++++++++++++++++++ >> include/linux/livepatch.h | 13 ++++++ >> kernel/livepatch/core.c | 4 +- >> 8 files changed, 164 insertions(+), 9 deletions(-) >> >> diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig >> index 9f72565..72e46b0 100644 >> --- a/arch/powerpc/Kconfig >> +++ b/arch/powerpc/Kconfig >> @@ -160,6 +160,7 @@ config PPC >> select ARCH_HAS_DEVMEM_IS_ALLOWED >> select HAVE_ARCH_SECCOMP_FILTER >> select ARCH_HAS_UBSAN_SANITIZE_ALL >> + select HAVE_LIVEPATCH if PPC64 && CPU_LITTLE_ENDIAN > I think this should be "if HAVE_DYNAMIC_FTRACE_WITH_REGS", that way if/when we > add support for that on BE/32-bit etc. we won't need to update this line. OK.. I just took it from the patches, will change it >> diff --git a/arch/powerpc/gcc-mprofile-kernel-notrace.sh b/arch/powerpc/gcc-mprofile-kernel-notrace.sh >> index 68d6482..6dafff6 100755 >> --- a/arch/powerpc/gcc-mprofile-kernel-notrace.sh >> +++ b/arch/powerpc/gcc-mprofile-kernel-notrace.sh >> @@ -12,12 +12,6 @@ echo "int func() { return 0; }" | \ >> >> trace_result=$? >> >> -echo "int func() { return 0; }" | \ >> - $* -S -x c -O2 -p -mprofile-kernel - -o - 2> /dev/null | \ >> - sed -n -e '/func:/,/bl _mcount/p' | grep -q TOC >> - >> -leaf_toc_result=$? >> - >> /bin/echo -e "#include <linux/compiler.h>\nnotrace int func() { return 0; }" | \ >> $* -S -x c -O2 -p -mprofile-kernel - -o - 2> /dev/null | \ >> grep -q "mcount" >> @@ -25,7 +19,6 @@ leaf_toc_result=$? >> notrace_result=$? >> >> if [ "$trace_result" -eq "0" -a \ >> - "$leaf_toc_result" -eq "0" -a \ >> "$notrace_result" -eq "1" ]; then >> echo y >> else > You can drop that hunk as I've reworked that script completely in my version. Thanks! >> diff --git a/arch/powerpc/include/asm/livepatch.h b/arch/powerpc/include/asm/livepatch.h >> new file mode 100644 >> index 0000000..6abb69c >> --- /dev/null >> +++ b/arch/powerpc/include/asm/livepatch.h >> @@ -0,0 +1,61 @@ >> +/* >> + * livepatch.h - powerpc-specific Kernel Live Patching Core >> + * >> + * Copyright (C) 2015 SUSE >> + * >> + * This program is free software; you can redistribute it and/or >> + * modify it under the terms of the GNU General Public License >> + * as published by the Free Software Foundation; either version 2 >> + * of the License, or (at your option) any later version. >> + * >> + * This program is distributed in the hope that it will be useful, >> + * but WITHOUT ANY WARRANTY; without even the implied warranty of >> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the >> + * GNU General Public License for more details. >> + * >> + * You should have received a copy of the GNU General Public License >> + * along with this program; if not, see <http://www.gnu.org/licenses/>. >> + */ >> +#ifndef _ASM_POWERPC64_LIVEPATCH_H >> +#define _ASM_POWERPC64_LIVEPATCH_H >> + >> +#include <linux/module.h> >> +#include <linux/ftrace.h> >> + >> +#ifdef CONFIG_LIVEPATCH >> +static inline int klp_check_compiler_support(void) >> +{ >> +#if !defined(_CALL_ELF) || _CALL_ELF != 2 || !defined(CC_USING_MPROFILE_KERNEL) >> + return 1; >> +#endif >> + return 0; >> +} > I don't understand why we need that. If your compiler is not supported then you > can't even compile the live patch code. I guess we have to implement it to keep > the livepatch core happy, but it should just return 1. It's been around from the previous patches, we can definitely refactor it > >> +#define ARCH_HAVE_KLP_MATCHADDR > I think the consensus these days is that we do that either via Kconfig, or > using the #define foo foo pattern. > > Or just make it a weak function. Done! > >> +static inline int klp_matchaddr(struct ftrace_ops *ops, unsigned long ip, >> + int remove, int reset) >> +{ >> + int offsets[] = {8, 16}; > Because of the two versions of mprofile-kernel (2 or 3 instruction sequence) > and the presence or absense of the global entry point, the full set of offsets > is: 4, 8, 12, 16. Petr wanted to do a version where we lookup things in ftrace(). Steve suggested using ftrace_location(). We can refactor these bits. > >> + int i; >> + int ret = 1; >> + >> + for (i = 0; i < ARRAY_SIZE(offsets); i++) { >> + ret = ftrace_set_filter_ip(ops, ip+offsets[i], remove, reset); >> + if (!ret) >> + break; >> + } >> + return ret; >> +} >> + >> +extern int klp_write_module_reloc(struct module *mod, unsigned long type, >> + unsigned long loc, unsigned long value); > That could be static inline for the moment, all it does is return ENOSYS. Done! >> + >> +static inline void klp_arch_set_pc(struct pt_regs *regs, unsigned long ip) >> +{ >> + regs->nip = ip; >> +} > ptrace already defines instruction_pointer_set() to do this. But I guess that's > a cleanup for later. > > >> +#else >> +#error Live patching support is disabled; check CONFIG_LIVEPATCH >> +#endif > I don't think you need that here in the header, the kconfig logic already > handles this for us. Yes >> + >> +#endif /* _ASM_POWERPC64_LIVEPATCH_H */ >> diff --git a/arch/powerpc/kernel/Makefile b/arch/powerpc/kernel/Makefile >> index 44667fd..405efce 100644 >> --- a/arch/powerpc/kernel/Makefile >> +++ b/arch/powerpc/kernel/Makefile >> @@ -119,6 +119,7 @@ obj-$(CONFIG_DYNAMIC_FTRACE) += ftrace.o >> obj-$(CONFIG_FUNCTION_GRAPH_TRACER) += ftrace.o >> obj-$(CONFIG_FTRACE_SYSCALLS) += ftrace.o >> obj-$(CONFIG_TRACING) += trace_clock.o >> +obj-$(CONFIG_LIVEPATCH) += livepatch.o >> >> ifneq ($(CONFIG_PPC_INDIRECT_PIO),y) >> obj-y += iomap.o >> diff --git a/arch/powerpc/kernel/entry_64.S b/arch/powerpc/kernel/entry_64.S >> index f347f50..853717f 100644 >> --- a/arch/powerpc/kernel/entry_64.S >> +++ b/arch/powerpc/kernel/entry_64.S >> @@ -1225,6 +1225,9 @@ _GLOBAL(ftrace_caller) >> >> /* Calculate ip from nip-4 into r3 for call below */ >> subi r3, r7, MCOUNT_INSN_SIZE >> +#ifdef CONFIG_LIVEPATCH >> + mr r14,r3 /* remember old NIP */ >> +#endif >> >> /* Put the original return address in r4 as parent_ip */ >> mr r4, r0 >> @@ -1247,6 +1250,9 @@ ftrace_call: >> /* Load ctr with the possibly modified NIP */ >> ld r3, _NIP(r1) >> mtctr r3 >> +#ifdef CONFIG_LIVEPATCH >> + cmpd r14,r3 /* has NIP been altered? */ >> +#endif >> >> /* Restore gprs */ >> REST_8GPRS(0,r1) >> @@ -1264,6 +1270,27 @@ ftrace_call: >> ld r0, LRSAVE(r1) >> mtlr r0 >> >> +#ifdef CONFIG_LIVEPATCH >> + beq+ 4f /* likely(old_NIP == new_NIP) */ >> + >> + /* 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. >> + */ > That comment could use rewording. I'm not sure what the last sentence is trying > to say. And normal comment formatting would be good. Yep, from the existing patches will do >> + stdu r1,-32(r1) /* open new mini stack frame */ >> + std r0,24(r1) /* save TOC now, unconditionally. */ > The toc isn't in r0 in my version, use can just save r2 directly. Absolutely! will do, thanks for catching this >> + bl 5f >> +5: mflr r12 >> + addi r12,r12,(KLP_return_helper+4-.)@l > It's a pity we need to do this. Just a few instructions ago we had the kernel > toc in r2, which would allow us to just do this normally. I guess we'll go with > this for now, but I think we can do better in the medium term. > >> + std r12,LRSAVE(r1) > That's the callee's LRSAVE slot, not ours, I think we can just drop that line. > >> + mtlr r12 >> + mfctr r12 /* allow for TOC calculation in newfunc */ > A comment here on the content of ctr would be helpful I think. Agreed >> + bctr >> +4: >> +#endif >> + >> #ifdef CONFIG_FUNCTION_GRAPH_TRACER >> stdu r1, -112(r1) >> .globl ftrace_graph_call >> @@ -1279,6 +1306,25 @@ _GLOBAL(ftrace_graph_stub) >> #endif /* CC_USING_MPROFILE_KERNEL */ >> _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. >> +*/ > Can you fix the formatting on that. > > Also it's wrong, we always return via KLP_return_helper() if we're livepatching > AFAICS. I've not tested livepatching + ftrace together.. good experiment to try > >> +KLP_return_helper: > Can we call it klp_return_helper please. Yep >> + ld r2,24(r1) /* restore TOC (saved by ftrace_caller) */ >> + addi r1, r1, 32 /* destroy mini stack frame */ >> + ld r0,LRSAVE(r1) /* get the real return address */ >> + mtlr r0 >> + blr >> +#endif >> + >> + >> #else >> _GLOBAL_TOC(_mcount) >> /* Taken from output of objdump from lib64/glibc */ > > cheers > -- To unsubscribe from this list: send the line "unsubscribe live-patching" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
