@@ -218,7 +218,7 @@ static int klp_resolve_symbols(Elf64_Shdr *sechdrs, const char *strtab,relas=(Elf_Rela*)relasec->sh_addr;/* For each rela in this klp relocation section */for(i=0;i<relasec->sh_size/sizeof(Elf_Rela);i++){-sym=(Elf64_Sym*)sechdrs[symndx].sh_addr+ELF_R_SYM(relas[i].r_info);+sym=(Elf_Sym*)sechdrs[symndx].sh_addr+ELF_R_SYM(relas[i].r_info);if(sym->st_shndx!=SHN_LIVEPATCH){pr_err("symbol %s is not marked as a livepatch symbol\n",strtab+sym->st_name);
Livepatching a loaded module involves applying relocations through
apply_relocate_add(), which attempts to write to read-only memory when
CONFIG_STRICT_MODULE_RWX=y.
R_PPC_ADDR16_LO, R_PPC_ADDR16_HI, R_PPC_ADDR16_HA and R_PPC_REL24 are
the types generated by the kpatch-build userspace tool or klp-convert
kernel tree observed applying a relocation to a post-init module.
Use patch_instruction() to patch those relocations.
Commit 8734b41b3efe ("powerpc/module_64: Fix livepatching for
RO modules") did similar change in module_64.
Signed-off-by: Christophe Leroy <redacted>
Cc: Russell Currey <redacted>
---
arch/powerpc/kernel/module_32.c | 44 ++++++++++++++++++++++-----------
1 file changed, 30 insertions(+), 14 deletions(-)
@@ -18,6 +18,7 @@#include<linux/bug.h>#include<linux/sort.h>#include<asm/setup.h>+#include<asm/code-patching.h>/* Count how many different relocations (different symbol, differentaddend)*/
@@ -174,15 +175,25 @@ static uint32_t do_plt_call(void *location,entry++;}-entry->jump[0]=PPC_RAW_LIS(_R12,PPC_HA(val));-entry->jump[1]=PPC_RAW_ADDI(_R12,_R12,PPC_LO(val));-entry->jump[2]=PPC_RAW_MTCTR(_R12);-entry->jump[3]=PPC_RAW_BCTR();+if(patch_instruction(&entry->jump[0],ppc_inst(PPC_RAW_LIS(_R12,PPC_HA(val)))))+return0;+if(patch_instruction(&entry->jump[1],ppc_inst(PPC_RAW_ADDI(_R12,_R12,PPC_LO(val)))))+return0;+if(patch_instruction(&entry->jump[2],ppc_inst(PPC_RAW_MTCTR(_R12))))+return0;+if(patch_instruction(&entry->jump[3],ppc_inst(PPC_RAW_BCTR())))+return0;pr_debug("Initialized plt for 0x%x at %p\n",val,entry);return(uint32_t)entry;}+staticintpatch_location_16(uint32_t*loc,u16value)+{+loc=PTR_ALIGN_DOWN(loc,sizeof(u32));+returnpatch_instruction(loc,ppc_inst((*loc&0xffff0000)|value));+}+intapply_relocate_add(Elf32_Shdr*sechdrs,constchar*strtab,unsignedintsymindex,
@@ -216,37 +227,42 @@ int apply_relocate_add(Elf32_Shdr *sechdrs,caseR_PPC_ADDR16_LO:/* Low half of the symbol */-*(uint16_t*)location=value;+if(patch_location_16(location,PPC_LO(value)))+return-EFAULT;break;caseR_PPC_ADDR16_HI:/* Higher half of the symbol */-*(uint16_t*)location=(value>>16);+if(patch_location_16(location,PPC_HI(value)))+return-EFAULT;break;caseR_PPC_ADDR16_HA:-/* Sign-adjusted lower 16 bits: PPC ELF ABI says:-(((x>>16)+((x&0x8000)?1:0)))&0xFFFF.-Thisisthesame,onlysane.-*/-*(uint16_t*)location=(value+0x8000)>>16;+if(patch_location_16(location,PPC_HA(value)))+return-EFAULT;break;caseR_PPC_REL24:if((int)(value-(uint32_t)location)<-0x02000000-||(int)(value-(uint32_t)location)>=0x02000000)+||(int)(value-(uint32_t)location)>=0x02000000){value=do_plt_call(location,value,sechdrs,module);+if(!value)+return-EFAULT;+}/* Only replace bits 2 through 26 */pr_debug("REL24 value = %08X. location = %08X\n",value,(uint32_t)location);pr_debug("Location before: %08X.\n",*(uint32_t*)location);-*(uint32_t*)location-=(*(uint32_t*)location&~0x03fffffc)+value=(*(uint32_t*)location&~0x03fffffc)|((value-(uint32_t)location)&0x03fffffc);++if(patch_instruction(location,ppc_inst(value)))+return-EFAULT;+pr_debug("Location after: %08X.\n",*(uint32_t*)location);pr_debug("ie. jump to %08X+%08X = %08X\n",
PPC64 needs some special logic to properly set up the TOC.
See commit 85baa095497f ("powerpc/livepatch: Add live patching support
on ppc64le") for details.
PPC32 doesn't have TOC so it doesn't need that logic, so adding
LIVEPATCH support is straight forward.
Add CONFIG_LIVEPATCH_64 and move livepatch stack logic into that item.
Livepatch sample modules all work.
Signed-off-by: Christophe Leroy <redacted>
---
arch/powerpc/Kconfig | 6 +++++-
arch/powerpc/include/asm/livepatch.h | 8 +++++---
arch/powerpc/include/asm/thread_info.h | 2 +-
arch/powerpc/kernel/asm-offsets.c | 2 +-
4 files changed, 12 insertions(+), 6 deletions(-)
PPC32 mcount() caller already saves LR on stack,
no need to save it again.
Signed-off-by: Christophe Leroy <redacted>
---
arch/powerpc/kernel/trace/ftrace_32.S | 3 ---
1 file changed, 3 deletions(-)
return_to_handler() was copied from PPC64. For PPC32 it
just needs to save r3 and r4, and doesn't require any nop
after the bl.
Signed-off-by: Christophe Leroy <redacted>
---
arch/powerpc/kernel/trace/ftrace_32.S | 16 ++++++----------
1 file changed, 6 insertions(+), 10 deletions(-)
In order to implement CONFIG_DYNAMIC_FTRACE_WITH_ARGS, change ftrace_caller()
to handle LIVEPATCH the same way as frace_caller_regs().
Signed-off-by: Christophe Leroy <redacted>
---
.../powerpc/kernel/trace/ftrace_64_mprofile.S | 25 ++++++++++++++-----
1 file changed, 19 insertions(+), 6 deletions(-)
Implement CONFIG_DYNAMIC_FTRACE_WITH_ARGS. It accelerates the call
of livepatching.
Also note that powerpc being the last one to convert to
CONFIG_DYNAMIC_FTRACE_WITH_ARGS, it will now be possible to remove
klp_arch_set_pc() on all architectures.
Signed-off-by: Christophe Leroy <redacted>
---
arch/powerpc/Kconfig | 1 +
arch/powerpc/include/asm/ftrace.h | 17 +++++++++++++++++
arch/powerpc/include/asm/livepatch.h | 4 +---
3 files changed, 19 insertions(+), 3 deletions(-)
ftrace_enable_ftrace_graph_caller() and
ftrace_disable_ftrace_graph_caller() have common code.
They will have even more common code after following patch.
Refactor into a single ftrace_modify_ftrace_graph_caller() function.
Signed-off-by: Christophe Leroy <redacted>
---
arch/powerpc/kernel/trace/ftrace.c | 21 +++++++++------------
1 file changed, 9 insertions(+), 12 deletions(-)
Modify function graph tracer to be handled directly by the standard
ftrace caller.
This is made possible as powerpc now supports
CONFIG_DYNAMIC_FTRACE_WITH_ARGS.
This change simplifies the call of function graph ftrace.
Signed-off-by: Christophe Leroy <redacted>
---
arch/powerpc/include/asm/ftrace.h | 6 ++
arch/powerpc/kernel/trace/ftrace.c | 11 ++++
arch/powerpc/kernel/trace/ftrace_32.S | 53 +--------------
.../powerpc/kernel/trace/ftrace_64_mprofile.S | 64 +------------------
4 files changed, 20 insertions(+), 114 deletions(-)
PPC64 mprofile versions and PPC32 are very similar.
Modify PPC64 version so that if can be reused for PPC32.
Signed-off-by: Christophe Leroy <redacted>
---
.../powerpc/kernel/trace/ftrace_64_mprofile.S | 73 +++++++++++++------
1 file changed, 51 insertions(+), 22 deletions(-)
@@ -63,10 +69,11 @@ _GLOBAL(ftrace_regs_caller)/*Getthe_mcount()callsiteoutofLR*/mflrr7/*Saveitaspt_regs->nip*/-stdr7,_NIP(r1)+PPC_STLr7,_NIP(r1)/*SavethereadLRinpt_regs->link*/-stdr0,_LINK(r1)+PPC_STLr0,_LINK(r1)+#ifdef CONFIG_PPC64/*Savecallee's TOC in the ABI compliant location */stdr2,24(r1)ldr2,PACATOC(r13)/*getkernelTOCinr2*/
@@ -1,152 +0,0 @@-/* SPDX-License-Identifier: GPL-2.0-or-later */-/*- * Split from entry_32.S- */--#include <linux/magic.h>-#include <asm/reg.h>-#include <asm/ppc_asm.h>-#include <asm/asm-offsets.h>-#include <asm/ftrace.h>-#include <asm/export.h>-#include <asm/ptrace.h>--_GLOBAL(mcount)-_GLOBAL(_mcount)- /*- * It is required that _mcount on PPC32 must preserve the- * link register. But we have r12 to play with. We use r12- * to push the return address back to the caller of mcount- * into the ctr register, restore the link register and- * then jump back using the ctr register.- */- mflr r12- mtctr r12- mtlr r0- bctr-EXPORT_SYMBOL(_mcount)--_GLOBAL(ftrace_caller)- stwu r1, -INT_FRAME_SIZE(r1)-- SAVE_GPRS(3, 10, r1)-- addi r8, r1, INT_FRAME_SIZE- stw r8, GPR1(r1)-- mflr r3- stw r3, _NIP(r1)- subi r3, r3, MCOUNT_INSN_SIZE-- stw r0, _LINK(r1)- mr r4, r0-- lis r5,function_trace_op@ha- lwz r5,function_trace_op@l(r5)-- addi r6, r1, STACK_FRAME_OVERHEAD-.globl ftrace_call-ftrace_call:- bl ftrace_stub- nop-- lwz r3, _NIP(r1)- mtctr r3-- REST_GPRS(3, 10, r1)-- lwz r0, _LINK(r1)- mtlr r0-- addi r1, r1, INT_FRAME_SIZE- /* old link register ends up in ctr reg */- bctr---_GLOBAL(ftrace_stub)- blr--_GLOBAL(ftrace_regs_caller)- /* Create our stack frame + pt_regs */- stwu r1,-INT_FRAME_SIZE(r1)-- /* Save all gprs to pt_regs */- stw r0, GPR0(r1)- stmw r2, GPR2(r1)-- /* Save previous stack pointer (r1) */- addi r8, r1, INT_FRAME_SIZE- stw r8, GPR1(r1)-- /* Load special regs for save below */- mfmsr r8- mfctr r9- mfxer r10- mfcr r11-- /* Get the _mcount() call site out of LR */- mflr r7- /* Save it as pt_regs->nip */- stw r7, _NIP(r1)- /* Save the read LR in pt_regs->link */- stw r0, _LINK(r1)-- lis r3,function_trace_op@ha- lwz r5,function_trace_op@l(r3)-- /* Calculate ip from nip-4 into r3 for call below */- subi r3, r7, MCOUNT_INSN_SIZE-- /* Put the original return address in r4 as parent_ip */- mr r4, r0-- /* Save special regs */- stw r8, _MSR(r1)- stw r9, _CTR(r1)- stw r10, _XER(r1)- stw r11, _CCR(r1)-- /* Load &pt_regs in r6 for call below */- addi r6, r1, STACK_FRAME_OVERHEAD-- /* ftrace_call(r3, r4, r5, r6) */-.globl ftrace_regs_call-ftrace_regs_call:- bl ftrace_stub- nop-- /* Load ctr with the possibly modified NIP */- lwz r3, _NIP(r1)- mtctr r3-- /* Restore gprs */- lmw r2, GPR2(r1)-- /* Restore possibly modified LR */- lwz r0, _LINK(r1)- mtlr r0-- /* Pop our stack frame */- addi r1, r1, INT_FRAME_SIZE- /* old link register ends up in ctr reg */- bctr--#ifdef CONFIG_FUNCTION_GRAPH_TRACER-_GLOBAL(return_to_handler)- /* need to save return values */- stwu r1, -16(r1)- stw r3, 8(r1)- stw r4, 12(r1)-- bl ftrace_return_to_handler-- /* return value has real return address */- mtlr r3-- lwz r3, 8(r1)- lwz r4, 12(r1)- addi r1, r1, 16-- /* Jump back to real return address */- blr-#endif /* CONFIG_FUNCTION_GRAPH_TRACER */
diff --git a/arch/powerpc/kernel/trace/ftrace_64.S b/arch/powerpc/kernel/trace/ftrace_low.Ssimilarity index 85%rename from arch/powerpc/kernel/trace/ftrace_64.Srename to arch/powerpc/kernel/trace/ftrace_low.Sindex 25e5b9e47c06..0bddf1fa6636 100644--- a/arch/powerpc/kernel/trace/ftrace_64.S+++ b/arch/powerpc/kernel/trace/ftrace_low.S
diff --git a/arch/powerpc/kernel/trace/ftrace_64_mprofile.S b/arch/powerpc/kernel/trace/ftrace_mprofile.Ssimilarity index 100%rename from arch/powerpc/kernel/trace/ftrace_64_mprofile.Srename to arch/powerpc/kernel/trace/ftrace_mprofile.S
--
2.33.1
From: Miroslav Benes <mbenes@suse.cz> Date: 2021-12-22 14:00:50
On Mon, 20 Dec 2021, Christophe Leroy wrote:
PPC64 needs some special logic to properly set up the TOC.
See commit 85baa095497f ("powerpc/livepatch: Add live patching support
on ppc64le") for details.
PPC32 doesn't have TOC so it doesn't need that logic, so adding
LIVEPATCH support is straight forward.
Add CONFIG_LIVEPATCH_64 and move livepatch stack logic into that item.
Livepatch sample modules all work.
From: Miroslav Benes <mbenes@suse.cz> Date: 2021-12-22 14:19:24
On Mon, 20 Dec 2021, Christophe Leroy wrote:
Implement CONFIG_DYNAMIC_FTRACE_WITH_ARGS. It accelerates the call
of livepatching.
Also note that powerpc being the last one to convert to
CONFIG_DYNAMIC_FTRACE_WITH_ARGS, it will now be possible to remove
klp_arch_set_pc() on all architectures.
Correct. We could replace it ftrace_instruction_pointer_set() and that is
it. In fact, livepatch.h in both arch/x86/include/asm/ and
arch/s390/include/asm/ could be removed with that.
On the other hand, there is arm64 live patching support being worked on
and I am not sure what their plans about DYNAMIC_FTRACE_WITH_ARGS are. The
above would make it a prerequisite.
Adding CCs... you can find the whole thread at
https://lore.kernel.org/all/cover.1640017960.git.christophe.leroy@csgroup.eu/
Miroslav
@@ -218,7 +218,7 @@ static int klp_resolve_symbols(Elf64_Shdr *sechdrs, const char *strtab,relas=(Elf_Rela*)relasec->sh_addr;/* For each rela in this klp relocation section */for(i=0;i<relasec->sh_size/sizeof(Elf_Rela);i++){-sym=(Elf64_Sym*)sechdrs[symndx].sh_addr+ELF_R_SYM(relas[i].r_info);+sym=(Elf_Sym*)sechdrs[symndx].sh_addr+ELF_R_SYM(relas[i].r_info);if(sym->st_shndx!=SHN_LIVEPATCH){pr_err("symbol %s is not marked as a livepatch symbol\n",strtab+sym->st_name);
--
2.33.1
Thanks for finding and fixing, lgtm.
Acked-by: Joe Lawrence <joe.lawrence@redhat.com>
-- Joe
From: Joe Lawrence <joe.lawrence@redhat.com> Date: 2022-01-04 19:44:38
On Mon, Dec 20, 2021 at 04:38:09PM +0000, Christophe Leroy wrote:
quoted hunk
Livepatching a loaded module involves applying relocations through
apply_relocate_add(), which attempts to write to read-only memory when
CONFIG_STRICT_MODULE_RWX=y.
R_PPC_ADDR16_LO, R_PPC_ADDR16_HI, R_PPC_ADDR16_HA and R_PPC_REL24 are
the types generated by the kpatch-build userspace tool or klp-convert
kernel tree observed applying a relocation to a post-init module.
Use patch_instruction() to patch those relocations.
Commit 8734b41b3efe ("powerpc/module_64: Fix livepatching for
RO modules") did similar change in module_64.
Signed-off-by: Christophe Leroy <redacted>
Cc: Russell Currey <redacted>
---
arch/powerpc/kernel/module_32.c | 44 ++++++++++++++++++++++-----------
1 file changed, 30 insertions(+), 14 deletions(-)
@@ -18,6 +18,7 @@#include<linux/bug.h>#include<linux/sort.h>#include<asm/setup.h>+#include<asm/code-patching.h>/* Count how many different relocations (different symbol, differentaddend)*/
@@ -174,15 +175,25 @@ static uint32_t do_plt_call(void *location,entry++;}-entry->jump[0]=PPC_RAW_LIS(_R12,PPC_HA(val));-entry->jump[1]=PPC_RAW_ADDI(_R12,_R12,PPC_LO(val));-entry->jump[2]=PPC_RAW_MTCTR(_R12);-entry->jump[3]=PPC_RAW_BCTR();+if(patch_instruction(&entry->jump[0],ppc_inst(PPC_RAW_LIS(_R12,PPC_HA(val)))))+return0;+if(patch_instruction(&entry->jump[1],ppc_inst(PPC_RAW_ADDI(_R12,_R12,PPC_LO(val)))))+return0;+if(patch_instruction(&entry->jump[2],ppc_inst(PPC_RAW_MTCTR(_R12))))+return0;+if(patch_instruction(&entry->jump[3],ppc_inst(PPC_RAW_BCTR())))+return0;pr_debug("Initialized plt for 0x%x at %p\n",val,entry);return(uint32_t)entry;}+staticintpatch_location_16(uint32_t*loc,u16value)+{+loc=PTR_ALIGN_DOWN(loc,sizeof(u32));+returnpatch_instruction(loc,ppc_inst((*loc&0xffff0000)|value));+}+intapply_relocate_add(Elf32_Shdr*sechdrs,constchar*strtab,unsignedintsymindex,
@@ -216,37 +227,42 @@ int apply_relocate_add(Elf32_Shdr *sechdrs,caseR_PPC_ADDR16_LO:/* Low half of the symbol */-*(uint16_t*)location=value;+if(patch_location_16(location,PPC_LO(value)))+return-EFAULT;break;caseR_PPC_ADDR16_HI:/* Higher half of the symbol */-*(uint16_t*)location=(value>>16);+if(patch_location_16(location,PPC_HI(value)))+return-EFAULT;break;caseR_PPC_ADDR16_HA:-/* Sign-adjusted lower 16 bits: PPC ELF ABI says:-(((x>>16)+((x&0x8000)?1:0)))&0xFFFF.-Thisisthesame,onlysane.-*/-*(uint16_t*)location=(value+0x8000)>>16;+if(patch_location_16(location,PPC_HA(value)))+return-EFAULT;break;caseR_PPC_REL24:if((int)(value-(uint32_t)location)<-0x02000000-||(int)(value-(uint32_t)location)>=0x02000000)+||(int)(value-(uint32_t)location)>=0x02000000){value=do_plt_call(location,value,sechdrs,module);+if(!value)+return-EFAULT;+}/* Only replace bits 2 through 26 */pr_debug("REL24 value = %08X. location = %08X\n",value,(uint32_t)location);pr_debug("Location before: %08X.\n",*(uint32_t*)location);-*(uint32_t*)location-=(*(uint32_t*)location&~0x03fffffc)+value=(*(uint32_t*)location&~0x03fffffc)|((value-(uint32_t)location)&0x03fffffc);++if(patch_instruction(location,ppc_inst(value)))+return-EFAULT;+pr_debug("Location after: %08X.\n",*(uint32_t*)location);pr_debug("ie. jump to %08X+%08X = %08X\n",
--
2.33.1
IIRC, offlist we hacked up klp-convert to create the klp-relocations for
a 32-bit target and then you hit the selftest late relocation crash, so
I assume that part is happy after this fix. :) Thanks again for the
testing.
For the livepatching implications,
Acked-by: Joe Lawrence <joe.lawrence@redhat.com>
-- Joe
From: Naveen N. Rao <hidden> Date: 2022-02-14 15:20:46
Hi Christophe,
Thanks for your work enabling DYNAMIC_FTRACE_WITH_ARGS on powerpc. Sorry
for the late review on this series, but I have a few comments below.
Christophe Leroy wrote:
In order to implement CONFIG_DYNAMIC_FTRACE_WITH_ARGS, change ftrace_caller()
to handle LIVEPATCH the same way as frace_caller_regs().
Signed-off-by: Christophe Leroy <redacted>
---
.../powerpc/kernel/trace/ftrace_64_mprofile.S | 25 ++++++++++++++-----
1 file changed, 19 insertions(+), 6 deletions(-)
I think we also need to save r1 into pt_regs so that the stack pointer
is available in the callbacks.
Other than that, a few minor nits below...
Please add a blank line here, to match the formatting for the rest of
this file.
/* Calculate ip from nip-4 into r3 for call below */
subi r3, r7, MCOUNT_INSN_SIZE
/* Put the original return address in r4 as parent_ip */
+ std r0, _LINK(r1)
mr r4, r0
- /* Set pt_regs to NULL */
- li r6, 0
+ /* Load &pt_regs in r6 for call below */
+ addi r6, r1 ,STACK_FRAME_OVERHEAD
From: Naveen N. Rao <hidden> Date: 2022-02-14 15:26:19
Christophe Leroy wrote:
quoted hunk
Implement CONFIG_DYNAMIC_FTRACE_WITH_ARGS. It accelerates the call
of livepatching.
Also note that powerpc being the last one to convert to
CONFIG_DYNAMIC_FTRACE_WITH_ARGS, it will now be possible to remove
klp_arch_set_pc() on all architectures.
Signed-off-by: Christophe Leroy <redacted>
---
arch/powerpc/Kconfig | 1 +
arch/powerpc/include/asm/ftrace.h | 17 +++++++++++++++++
arch/powerpc/include/asm/livepatch.h | 4 +---
3 files changed, 19 insertions(+), 3 deletions(-)
@@ -22,6 +22,23 @@ static inline unsigned long ftrace_call_adjust(unsigned long addr)structdyn_arch_ftrace{structmodule*mod;};++#ifdef CONFIG_DYNAMIC_FTRACE_WITH_ARGS+structftrace_regs{+structpt_regsregs;+};++static__always_inlinestructpt_regs*arch_ftrace_get_regs(structftrace_regs*fregs)+{+return&fregs->regs;+}
I think this is wrong. We need to differentiate between ftrace_caller()
and ftrace_regs_caller() here, and only return pt_regs if coming in
through ftrace_regs_caller() (i.e., FL_SAVE_REGS is set).
Should we use that helper here? regs_set_return_ip() also updates some
other state related to taking interrupts and I don't think it makes
sense for use with ftrace.
- Naveen
From: Naveen N. Rao <hidden> Date: 2022-02-14 17:25:26
Christophe Leroy wrote:
quoted hunk
Modify function graph tracer to be handled directly by the standard
ftrace caller.
This is made possible as powerpc now supports
CONFIG_DYNAMIC_FTRACE_WITH_ARGS.
This change simplifies the call of function graph ftrace.
Signed-off-by: Christophe Leroy <redacted>
---
arch/powerpc/include/asm/ftrace.h | 6 ++
arch/powerpc/kernel/trace/ftrace.c | 11 ++++
arch/powerpc/kernel/trace/ftrace_32.S | 53 +--------------
.../powerpc/kernel/trace/ftrace_64_mprofile.S | 64 +------------------
4 files changed, 20 insertions(+), 114 deletions(-)
@@ -917,6 +917,9 @@ static int ftrace_modify_ftrace_graph_caller(bool enable)unsignedlongstub=(unsignedlong)(&ftrace_graph_stub);ppc_inst_told,new;+if(IS_ENABLED(CONFIG_DYNAMIC_FTRACE_WITH_ARGS))+return0;+old=ftrace_call_replace(ip,enable?stub:addr,0);new=ftrace_call_replace(ip,enable?addr:stub,0);
@@ -955,6 +958,14 @@ unsigned long prepare_ftrace_return(unsigned long parent, unsigned long ip,out:returnparent;}
For x86, commit 0c0593b45c9b4e ("x86/ftrace: Make function graph use
ftrace directly") also adds recursion check before the call to
function_graph_enter() in prepare_ftrace_return(). Do we need that on
powerpc as well?
- Naveen
From: Naveen N. Rao <hidden> Date: 2022-02-14 17:52:34
Christophe Leroy wrote:
PPC64 mprofile versions and PPC32 are very similar.
Modify PPC64 version so that if can be reused for PPC32.
Signed-off-by: Christophe Leroy <redacted>
---
.../powerpc/kernel/trace/ftrace_64_mprofile.S | 73 +++++++++++++------
1 file changed, 51 insertions(+), 22 deletions(-)
While I agree that ppc32 and -mprofile-kernel ftrace code are very
similar, I think this patch adds way too many #ifdefs. IMHO, this
makes the resultant code quite difficult to follow.
- Naveen
@@ -63,10 +69,11 @@ _GLOBAL(ftrace_regs_caller)/*Getthe_mcount()callsiteoutofLR*/mflrr7/*Saveitaspt_regs->nip*/-stdr7,_NIP(r1)+PPC_STLr7,_NIP(r1)/*SavethereadLRinpt_regs->link*/-stdr0,_LINK(r1)+PPC_STLr0,_LINK(r1)+#ifdef CONFIG_PPC64/*Savecallee's TOC in the ABI compliant location */stdr2,24(r1)ldr2,PACATOC(r13)/*getkernelTOCinr2*/
From: Steven Rostedt <rostedt@goodmis.org> Date: 2022-02-14 21:34:38
On Mon, 14 Feb 2022 22:54:23 +0530
"Naveen N. Rao" [off-list ref] wrote:
For x86, commit 0c0593b45c9b4e ("x86/ftrace: Make function graph use
ftrace directly") also adds recursion check before the call to
function_graph_enter() in prepare_ftrace_return(). Do we need that on
powerpc as well?
Yes. The function_graph_enter() does not provide any recursion protection,
so if it were to call something that gets function graph traced, it will
crash the machine.
-- Steve
Implement CONFIG_DYNAMIC_FTRACE_WITH_ARGS. It accelerates the call
of livepatching.
Also note that powerpc being the last one to convert to
CONFIG_DYNAMIC_FTRACE_WITH_ARGS, it will now be possible to remove
klp_arch_set_pc() on all architectures.
Signed-off-by: Christophe Leroy <redacted>
---
arch/powerpc/Kconfig | 1 +
arch/powerpc/include/asm/ftrace.h | 17 +++++++++++++++++
arch/powerpc/include/asm/livepatch.h | 4 +---
3 files changed, 19 insertions(+), 3 deletions(-)
I think this is wrong. We need to differentiate between ftrace_caller()
and ftrace_regs_caller() here, and only return pt_regs if coming in
through ftrace_regs_caller() (i.e., FL_SAVE_REGS is set).
Not sure I follow you.
This is based on 5740a7c71ab6 ("s390/ftrace: add
HAVE_DYNAMIC_FTRACE_WITH_ARGS support")
It's all the point of HAVE_DYNAMIC_FTRACE_WITH_ARGS, have the regs also
with ftrace_caller().
Sure you only have the params, but that's the same on s390, so what did
I miss ?
Should we use that helper here? regs_set_return_ip() also updates some
other state related to taking interrupts and I don't think it makes
sense for use with ftrace.
Today we have:
static inline void klp_arch_set_pc(struct ftrace_regs *fregs, unsigned
long ip)
{
struct pt_regs *regs = ftrace_get_regs(fregs);
regs_set_return_ip(regs, ip);
}
Which like x86 and s390 becomes:
static inline void klp_arch_set_pc(struct ftrace_regs *fregs, unsigned
long ip)
{
ftrace_instruction_pointer_set(fregs, ip);
}
That's the reason why I've been using regs_set_return_ip(). Do you think
it was wrong to use regs_set_return_ip() in klp_arch_set_pc() ?
That was added by 59dc5bfca0cb ("powerpc/64s: avoid reloading (H)SRR
registers if they are still valid")
Christophe
PPC64 mprofile versions and PPC32 are very similar.
Modify PPC64 version so that if can be reused for PPC32.
Signed-off-by: Christophe Leroy <redacted>
---
.../powerpc/kernel/trace/ftrace_64_mprofile.S | 73 +++++++++++++------
1 file changed, 51 insertions(+), 22 deletions(-)
While I agree that ppc32 and -mprofile-kernel ftrace code are very
similar, I think this patch adds way too many #ifdefs. IMHO, this
makes the resultant code quite difficult to follow.
Ok, I can introduce some GAS macros for a few of them in a followup patch.
Christophe
From: Michael Ellerman <mpe@ellerman.id.au> Date: 2022-02-15 11:05:16
Christophe Leroy [off-list ref] writes:
Le 14/02/2022 à 16:25, Naveen N. Rao a écrit :
quoted
Christophe Leroy wrote:
quoted
Implement CONFIG_DYNAMIC_FTRACE_WITH_ARGS. It accelerates the call
of livepatching.
Also note that powerpc being the last one to convert to
CONFIG_DYNAMIC_FTRACE_WITH_ARGS, it will now be possible to remove
klp_arch_set_pc() on all architectures.
Signed-off-by: Christophe Leroy <redacted>
---
arch/powerpc/Kconfig | 1 +
arch/powerpc/include/asm/ftrace.h | 17 +++++++++++++++++
arch/powerpc/include/asm/livepatch.h | 4 +---
3 files changed, 19 insertions(+), 3 deletions(-)
I think this is wrong. We need to differentiate between ftrace_caller()
and ftrace_regs_caller() here, and only return pt_regs if coming in
through ftrace_regs_caller() (i.e., FL_SAVE_REGS is set).
Not sure I follow you.
This is based on 5740a7c71ab6 ("s390/ftrace: add
HAVE_DYNAMIC_FTRACE_WITH_ARGS support")
It's all the point of HAVE_DYNAMIC_FTRACE_WITH_ARGS, have the regs also
with ftrace_caller().
Sure you only have the params, but that's the same on s390, so what did
I miss ?
I already have this series in next, I can pull it out, but I'd rather
not.
I'll leave it in for now, hopefully you two can agree overnight my time
whether this is a big problem or something we can fix with a fixup
patch.
Should we use that helper here? regs_set_return_ip() also updates some
other state related to taking interrupts and I don't think it makes
sense for use with ftrace.
Today we have:
static inline void klp_arch_set_pc(struct ftrace_regs *fregs, unsigned
long ip)
{
struct pt_regs *regs = ftrace_get_regs(fregs);
regs_set_return_ip(regs, ip);
}
Which like x86 and s390 becomes:
static inline void klp_arch_set_pc(struct ftrace_regs *fregs, unsigned
long ip)
{
ftrace_instruction_pointer_set(fregs, ip);
}
That's the reason why I've been using regs_set_return_ip(). Do you think
it was wrong to use regs_set_return_ip() in klp_arch_set_pc() ?
That was added by 59dc5bfca0cb ("powerpc/64s: avoid reloading (H)SRR
registers if they are still valid")
It's not wrong, but I think it's unnecessary. We need to use
regs_set_return_ip() if we're changing the regs->ip of an interrupt
frame, so that the interrupt return code will reload it.
But AIUI in this case we're not doing that, we're changing the regs->ip
of a pt_regs provided by ftrace, which shouldn't ever be an interrupt
frame.
So it's not a bug to use regs_set_return_ip(), but it is unncessary and
means we'll reload the interrupt state unnecessarily on the next
interrupt return.
cheers
From: Naveen N. Rao <hidden> Date: 2022-02-15 13:37:38
Michael Ellerman wrote:
Christophe Leroy [off-list ref] writes:
quoted
Le 14/02/2022 à 16:25, Naveen N. Rao a écrit :
quoted
Christophe Leroy wrote:
quoted
Implement CONFIG_DYNAMIC_FTRACE_WITH_ARGS. It accelerates the call
of livepatching.
Also note that powerpc being the last one to convert to
CONFIG_DYNAMIC_FTRACE_WITH_ARGS, it will now be possible to remove
klp_arch_set_pc() on all architectures.
Signed-off-by: Christophe Leroy <redacted>
---
arch/powerpc/Kconfig | 1 +
arch/powerpc/include/asm/ftrace.h | 17 +++++++++++++++++
arch/powerpc/include/asm/livepatch.h | 4 +---
3 files changed, 19 insertions(+), 3 deletions(-)
I think this is wrong. We need to differentiate between ftrace_caller()
and ftrace_regs_caller() here, and only return pt_regs if coming in
through ftrace_regs_caller() (i.e., FL_SAVE_REGS is set).
Not sure I follow you.
This is based on 5740a7c71ab6 ("s390/ftrace: add
HAVE_DYNAMIC_FTRACE_WITH_ARGS support")
It's all the point of HAVE_DYNAMIC_FTRACE_WITH_ARGS, have the regs also
with ftrace_caller().
Sure you only have the params, but that's the same on s390, so what did
I miss ?
It looks like s390 is special since it apparently saves all registers
even for ftrace_caller:
https://lore.kernel.org/all/YbipdU5X4HNDWIni@osiris/
As I understand it, the reason ftrace_get_regs() was introduced was to
be able to only return the pt_regs, if _all_ registers were saved into
it, which we don't do when coming in through ftrace_caller(). See the
x86 implementation (commit 02a474ca266a47 ("ftrace/x86: Allow for
arguments to be passed in to ftrace_regs by default"), which returns
pt_regs conditionally.
I already have this series in next, I can pull it out, but I'd rather
not.
Yeah, I'm sorry about the late review on this one.
I'll leave it in for now, hopefully you two can agree overnight my time
whether this is a big problem or something we can fix with a fixup
patch.
I think changes to this particular patch can be added as an incremental
patch. If anything, pt_regs won't have all valid registers, but no one
should depend on it without also setting FL_SAVE_REGS anyway.
I was concerned about patch 8 though, where we are missing saving r1
into pt_regs. That gets used in patch 11, and will be used during
unwinding when the function_graph tracer is active. But, this should
still just result in us being unable to unwind the stack, so I think
that can also be an incremental patch.
Thanks,
Naveen
Implement CONFIG_DYNAMIC_FTRACE_WITH_ARGS. It accelerates the call
of livepatching.
Also note that powerpc being the last one to convert to
CONFIG_DYNAMIC_FTRACE_WITH_ARGS, it will now be possible to remove
klp_arch_set_pc() on all architectures.
Signed-off-by: Christophe Leroy <redacted>
---
arch/powerpc/Kconfig | 1 +
arch/powerpc/include/asm/ftrace.h | 17 +++++++++++++++++
arch/powerpc/include/asm/livepatch.h | 4 +---
3 files changed, 19 insertions(+), 3 deletions(-)
I think this is wrong. We need to differentiate between
ftrace_caller() and ftrace_regs_caller() here, and only return
pt_regs if coming in through ftrace_regs_caller() (i.e.,
FL_SAVE_REGS is set).
Not sure I follow you.
This is based on 5740a7c71ab6 ("s390/ftrace: add
HAVE_DYNAMIC_FTRACE_WITH_ARGS support")
It's all the point of HAVE_DYNAMIC_FTRACE_WITH_ARGS, have the regs
also with ftrace_caller().
Sure you only have the params, but that's the same on s390, so what
did I miss ?
It is not what I understand from their code, see
https://elixir.bootlin.com/linux/v5.17-rc3/source/arch/s390/kernel/mcount.S#L37
They have a common macro called with argument 'allregs' which is set to
0 for ftrace_caller() and 1 for ftrace_regs_caller().
When allregs == 1, the macro seems to save more.
But ok, I can do like x86, but I need a trick to know whether
FL_SAVE_REGS is set or not, like they do with fregs->regs.cs
Any idea what the condition can be for powerpc ?
Thanks
Christophe
From: Steven Rostedt <rostedt@goodmis.org> Date: 2022-02-15 14:38:58
On Tue, 15 Feb 2022 19:06:48 +0530
"Naveen N. Rao" [off-list ref] wrote:
As I understand it, the reason ftrace_get_regs() was introduced was to
be able to only return the pt_regs, if _all_ registers were saved into
it, which we don't do when coming in through ftrace_caller(). See the
x86 implementation (commit 02a474ca266a47 ("ftrace/x86: Allow for
arguments to be passed in to ftrace_regs by default"), which returns
pt_regs conditionally.
I can give you the history of ftrace_caller and ftrace_regs_caller.
ftrace_caller saved just enough as was denoted for gcc mcount trampolines.
The new fentry which happens at the start of the function, whereas mcount
happens after the stack frame is set up, may change the rules on some
architectures.
As for ftrace_regs_caller, that was created for kprobes. As the majority of
kprobes were added at the start of the function, it made sense to hook into
ftrace as the ftrace trampoline call is much faster than taking a
breakpoint interrupt. But to keep compatibility with breakpoint
interrupts, we needed to fill in all the registers, and make it act just
like a breakpoint interrupt.
I've been wanting to record function parameters, and because the ftrace
trampoline must at a minimum save the function parameters before calling
the ftrace callbacks, all the information for those parameters were being
saved but were never exposed to the ftrace callbacks. I created the the
DYNAMIC_FTRACE_WITH_ARGS to expose them. I first just used pt_regs with
just the parameters filled in, but that was criticized as it could be
confusing where the non filled in pt_regs might be used and thinking they
are legitimate. So I created ftrace_regs that would give you just the
function arguments (if DYNAMIC_FTRACE_WITH_ARGS is defined), or it will
give you a full pt_regs, if the caller came from the ftrace_regs_caller. If
not, it will give you a NULL pointer.
The first user to use the args was live kernel patching, as they only need
that and the return pointer.
-- Steve
+ S390 people
Le 15/02/2022 à 15:28, Christophe Leroy a écrit :
Le 15/02/2022 à 14:36, Naveen N. Rao a écrit :
quoted
Michael Ellerman wrote:
quoted
Christophe Leroy [off-list ref] writes:
quoted
Le 14/02/2022 à 16:25, Naveen N. Rao a écrit :
quoted
Christophe Leroy wrote:
quoted
Implement CONFIG_DYNAMIC_FTRACE_WITH_ARGS. It accelerates the call
of livepatching.
Also note that powerpc being the last one to convert to
CONFIG_DYNAMIC_FTRACE_WITH_ARGS, it will now be possible to remove
klp_arch_set_pc() on all architectures.
Signed-off-by: Christophe Leroy <redacted>
---
arch/powerpc/Kconfig | 1 +
arch/powerpc/include/asm/ftrace.h | 17 +++++++++++++++++
arch/powerpc/include/asm/livepatch.h | 4 +---
3 files changed, 19 insertions(+), 3 deletions(-)
I think this is wrong. We need to differentiate between
ftrace_caller() and ftrace_regs_caller() here, and only return
pt_regs if coming in through ftrace_regs_caller() (i.e.,
FL_SAVE_REGS is set).
Not sure I follow you.
This is based on 5740a7c71ab6 ("s390/ftrace: add
HAVE_DYNAMIC_FTRACE_WITH_ARGS support")
It's all the point of HAVE_DYNAMIC_FTRACE_WITH_ARGS, have the regs
also with ftrace_caller().
Sure you only have the params, but that's the same on s390, so what
did I miss ?
It is not what I understand from their code, see
https://elixir.bootlin.com/linux/v5.17-rc3/source/arch/s390/kernel/mcount.S#L37
They have a common macro called with argument 'allregs' which is set to
0 for ftrace_caller() and 1 for ftrace_regs_caller().
When allregs == 1, the macro seems to save more.
But ok, I can do like x86, but I need a trick to know whether
FL_SAVE_REGS is set or not, like they do with fregs->regs.cs
Any idea what the condition can be for powerpc ?
Finally, it looks like this change is done via commit 894979689d3a
("s390/ftrace: provide separate ftrace_caller/ftrace_regs_caller
implementations") four hours the same day after the implementation of
arch_ftrace_get_regs()
They may have forgotten to change arch_ftrace_get_regs() which was added
in commit 5740a7c71ab6 ("s390/ftrace: add HAVE_DYNAMIC_FTRACE_WITH_ARGS
support") with the assumption that ftrace_caller and ftrace_regs_caller
where identical.
Christophe
From: Naveen N. Rao <hidden> Date: 2022-02-15 16:26:44
Christophe Leroy wrote:
+ S390 people
Le 15/02/2022 à 15:28, Christophe Leroy a écrit :
quoted
Le 15/02/2022 à 14:36, Naveen N. Rao a écrit :
quoted
Michael Ellerman wrote:
quoted
Christophe Leroy [off-list ref] writes:
quoted
Le 14/02/2022 à 16:25, Naveen N. Rao a écrit :
quoted
Christophe Leroy wrote:
quoted
Implement CONFIG_DYNAMIC_FTRACE_WITH_ARGS. It accelerates the call
of livepatching.
Also note that powerpc being the last one to convert to
CONFIG_DYNAMIC_FTRACE_WITH_ARGS, it will now be possible to remove
klp_arch_set_pc() on all architectures.
Signed-off-by: Christophe Leroy <redacted>
---
arch/powerpc/Kconfig | 1 +
arch/powerpc/include/asm/ftrace.h | 17 +++++++++++++++++
arch/powerpc/include/asm/livepatch.h | 4 +---
3 files changed, 19 insertions(+), 3 deletions(-)
I think this is wrong. We need to differentiate between
ftrace_caller() and ftrace_regs_caller() here, and only return
pt_regs if coming in through ftrace_regs_caller() (i.e.,
FL_SAVE_REGS is set).
Not sure I follow you.
This is based on 5740a7c71ab6 ("s390/ftrace: add
HAVE_DYNAMIC_FTRACE_WITH_ARGS support")
It's all the point of HAVE_DYNAMIC_FTRACE_WITH_ARGS, have the regs
also with ftrace_caller().
Sure you only have the params, but that's the same on s390, so what
did I miss ?
It is not what I understand from their code, see
https://elixir.bootlin.com/linux/v5.17-rc3/source/arch/s390/kernel/mcount.S#L37
They have a common macro called with argument 'allregs' which is set to
0 for ftrace_caller() and 1 for ftrace_regs_caller().
When allregs == 1, the macro seems to save more.
But ok, I can do like x86, but I need a trick to know whether
FL_SAVE_REGS is set or not, like they do with fregs->regs.cs
Any idea what the condition can be for powerpc ?
We'll need to explicitly zero-out something in pt_regs in
ftrace_caller(). We can probably use regs->msr since we don't expect it
to be zero when saved from ftrace_regs_caller().
quoted
Finally, it looks like this change is done via commit 894979689d3a
("s390/ftrace: provide separate ftrace_caller/ftrace_regs_caller
implementations") four hours the same day after the implementation of
arch_ftrace_get_regs()
They may have forgotten to change arch_ftrace_get_regs() which was added
in commit 5740a7c71ab6 ("s390/ftrace: add HAVE_DYNAMIC_FTRACE_WITH_ARGS
support") with the assumption that ftrace_caller and ftrace_regs_caller
where identical.
From: Naveen N. Rao <hidden> Date: 2022-02-15 16:27:40
Steven Rostedt wrote:
On Tue, 15 Feb 2022 19:06:48 +0530
"Naveen N. Rao" [off-list ref] wrote:
quoted
As I understand it, the reason ftrace_get_regs() was introduced was to
be able to only return the pt_regs, if _all_ registers were saved into
it, which we don't do when coming in through ftrace_caller(). See the
x86 implementation (commit 02a474ca266a47 ("ftrace/x86: Allow for
arguments to be passed in to ftrace_regs by default"), which returns
pt_regs conditionally.
I can give you the history of ftrace_caller and ftrace_regs_caller.
ftrace_caller saved just enough as was denoted for gcc mcount trampolines.
The new fentry which happens at the start of the function, whereas mcount
happens after the stack frame is set up, may change the rules on some
architectures.
As for ftrace_regs_caller, that was created for kprobes. As the majority of
kprobes were added at the start of the function, it made sense to hook into
ftrace as the ftrace trampoline call is much faster than taking a
breakpoint interrupt. But to keep compatibility with breakpoint
interrupts, we needed to fill in all the registers, and make it act just
like a breakpoint interrupt.
I've been wanting to record function parameters, and because the ftrace
trampoline must at a minimum save the function parameters before calling
the ftrace callbacks, all the information for those parameters were being
saved but were never exposed to the ftrace callbacks. I created the the
DYNAMIC_FTRACE_WITH_ARGS to expose them. I first just used pt_regs with
just the parameters filled in, but that was criticized as it could be
confusing where the non filled in pt_regs might be used and thinking they
are legitimate. So I created ftrace_regs that would give you just the
function arguments (if DYNAMIC_FTRACE_WITH_ARGS is defined), or it will
give you a full pt_regs, if the caller came from the ftrace_regs_caller. If
not, it will give you a NULL pointer.
The first user to use the args was live kernel patching, as they only need
that and the return pointer.
From: Michael Ellerman <hidden> Date: 2022-02-16 12:30:29
On Mon, 20 Dec 2021 16:37:58 +0000, Christophe Leroy wrote:
This series implements livepatch on PPC32 and implements
CONFIG_DYNAMIC_FTRACE_WITH_ARGS to simplify ftracing.
v2:
- Fix problem with strict modules RWX
- Convert powerpc to CONFIG_DYNAMIC_FTRACE_WITH_ARGS
- Convert function graph tracing to C
- Refactor PPC32 versus PPC64
[...]
On Tue, Feb 15, 2022 at 09:55:52PM +0530, Naveen N. Rao wrote:
quoted
quoted
quoted
quoted
quoted
quoted
I think this is wrong. We need to differentiate
between ftrace_caller() and ftrace_regs_caller()
here, and only return pt_regs if coming in through
ftrace_regs_caller() (i.e., FL_SAVE_REGS is set).
Not sure I follow you.
This is based on 5740a7c71ab6 ("s390/ftrace: add
HAVE_DYNAMIC_FTRACE_WITH_ARGS support")
It's all the point of HAVE_DYNAMIC_FTRACE_WITH_ARGS,
have the regs also with ftrace_caller().
Sure you only have the params, but that's the same on
s390, so what did I miss ?
It is not what I understand from their code, see https://elixir.bootlin.com/linux/v5.17-rc3/source/arch/s390/kernel/mcount.S#L37
They have a common macro called with argument 'allregs' which is set
to 0 for ftrace_caller() and 1 for ftrace_regs_caller().
When allregs == 1, the macro seems to save more.
But ok, I can do like x86, but I need a trick to know whether
FL_SAVE_REGS is set or not, like they do with fregs->regs.cs
Any idea what the condition can be for powerpc ?
We'll need to explicitly zero-out something in pt_regs in ftrace_caller().
We can probably use regs->msr since we don't expect it to be zero when saved
from ftrace_regs_caller().
quoted
Finally, it looks like this change is done via commit 894979689d3a
("s390/ftrace: provide separate ftrace_caller/ftrace_regs_caller
implementations") four hours the same day after the implementation of
arch_ftrace_get_regs()
They may have forgotten to change arch_ftrace_get_regs() which was added
in commit 5740a7c71ab6 ("s390/ftrace: add HAVE_DYNAMIC_FTRACE_WITH_ARGS
support") with the assumption that ftrace_caller and ftrace_regs_caller
where identical.
Indeed, good find!
Thank you for bringing this up!
So, the in both variants s390 provides nearly identical data. The only
difference is that for FL_SAVE_REGS the program status word mask is
missing; therefore it is not possible to figure out the condition code
or if interrupts were enabled/disabled.
Vasily, Sven, I think we have two options here:
- don't provide sane psw mask contents at all and say (again) that
ptregs contents are identical
- provide (finally) a full psw mask contents using epsw, and indicate
validity with a flags bit in pt_regs
I would vote for the second option, even though epsw is slow. But this
is about the third or fourth time this came up in different
contexts. So I'd guess we should go for the slow but complete
solution. Opinions?