[PATCH v1 0/5] Implement livepatch on PPC32

[PATCH v1 0/5] Implement livepatch on PPC32

[Christophe Leroy]

This series implements livepatch on PPC32.

This is largely copied from what's done on PPC64.

Christophe Leroy (5):
  livepatch: Fix build failure on 32 bits processors
  powerpc/ftrace: No need to read LR from stack in _mcount()
  powerpc/ftrace: Add module_trampoline_target() for PPC32
  powerpc/ftrace: Activate HAVE_DYNAMIC_FTRACE_WITH_REGS on PPC32
  powerpc/ftrace: Add support for livepatch to PPC32

 arch/powerpc/Kconfig                  |   2 +-
 arch/powerpc/include/asm/livepatch.h  |   4 +-
 arch/powerpc/kernel/module_32.c       |  33 +++++
 arch/powerpc/kernel/trace/ftrace.c    |  53 +++-----
 arch/powerpc/kernel/trace/ftrace_32.S | 187 ++++++++++++++++++++++++--
 kernel/livepatch/core.c               |   4 +-
 6 files changed, 230 insertions(+), 53 deletions(-)

-- 
2.31.1

[PATCH v1 1/5] livepatch: Fix build failure on 32 bits processors

[Christophe Leroy]

Trying to build livepatch on powerpc/32 results in:

	kernel/livepatch/core.c: In function 'klp_resolve_symbols':
	kernel/livepatch/core.c:221:23: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast]
	  221 |                 sym = (Elf64_Sym *)sechdrs[symndx].sh_addr + ELF_R_SYM(relas[i].r_info);
	      |                       ^
	kernel/livepatch/core.c:221:21: error: assignment to 'Elf32_Sym *' {aka 'struct elf32_sym *'} from incompatible pointer type 'Elf64_Sym *' {aka 'struct elf64_sym *'} [-Werror=incompatible-pointer-types]
	  221 |                 sym = (Elf64_Sym *)sechdrs[symndx].sh_addr + ELF_R_SYM(relas[i].r_info);
	      |                     ^
	kernel/livepatch/core.c: In function 'klp_apply_section_relocs':
	kernel/livepatch/core.c:312:35: error: passing argument 1 of 'klp_resolve_symbols' from incompatible pointer type [-Werror=incompatible-pointer-types]
	  312 |         ret = klp_resolve_symbols(sechdrs, strtab, symndx, sec, sec_objname);
	      |                                   ^~~~~~~
	      |                                   |
	      |                                   Elf32_Shdr * {aka struct elf32_shdr *}
	kernel/livepatch/core.c:193:44: note: expected 'Elf64_Shdr *' {aka 'struct elf64_shdr *'} but argument is of type 'Elf32_Shdr *' {aka 'struct elf32_shdr *'}
	  193 | static int klp_resolve_symbols(Elf64_Shdr *sechdrs, const char *strtab,
	      |                                ~~~~~~~~~~~~^~~~~~~

Fix it by using the right types instead of forcing 64 bits types.

Fixes: 7c8e2bdd5f0d ("livepatch: Apply vmlinux-specific KLP relocations early")
Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
---
 kernel/livepatch/core.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/kernel/livepatch/core.c b/kernel/livepatch/core.c
index 335d988bd811..c0789383807b 100644
--- a/kernel/livepatch/core.c
+++ b/kernel/livepatch/core.c
@@ -190,7 +190,7 @@ static int klp_find_object_symbol(const char *objname, const char *name,
 	return -EINVAL;
 }
 
-static int klp_resolve_symbols(Elf64_Shdr *sechdrs, const char *strtab,
+static int klp_resolve_symbols(Elf_Shdr *sechdrs, const char *strtab,
 			       unsigned int symndx, Elf_Shdr *relasec,
 			       const char *sec_objname)
 {
@@ -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.31.1

[PATCH v1 2/5] powerpc/ftrace: No need to read LR from stack in _mcount()

[Christophe Leroy]

All functions calling _mcount do it exactly the same way, with the
following sequence of instructions:

	c07de788:       7c 08 02 a6     mflr    r0
	c07de78c:       90 01 00 04     stw     r0,4(r1)
	c07de790:       4b 84 13 65     bl      c001faf4 <_mcount>

Allthough LR is pushed on stack, it is still in r0 while entering
_mcount().

Function arguments are in r3-r10, so r11 and r12 are still available
at that point.

Do like PPC64 and use r12 to move LR into CTR, so that r0 is preserved
and doesn't need to be restored from the stack.

While at it, bring back the EXPORT_SYMBOL at the end of _mcount.

Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
---
 arch/powerpc/kernel/trace/ftrace_32.S | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/arch/powerpc/kernel/trace/ftrace_32.S b/arch/powerpc/kernel/trace/ftrace_32.S
index e023ae59c429..c7d57124cc59 100644
--- a/arch/powerpc/kernel/trace/ftrace_32.S
+++ b/arch/powerpc/kernel/trace/ftrace_32.S
@@ -14,16 +14,16 @@ _GLOBAL(mcount)
 _GLOBAL(_mcount)
 	/*
 	 * It is required that _mcount on PPC32 must preserve the
-	 * link register. But we have r0 to play with. We use r0
+	 * 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	r0
-	mtctr	r0
-	lwz	r0, 4(r1)
+	mflr	r12
+	mtctr	r12
 	mtlr	r0
 	bctr
+EXPORT_SYMBOL(_mcount)
 
 _GLOBAL(ftrace_caller)
 	MCOUNT_SAVE_FRAME
@@ -43,7 +43,6 @@ _GLOBAL(ftrace_graph_stub)
 	/* old link register ends up in ctr reg */
 	bctr
 
-EXPORT_SYMBOL(_mcount)
 
 _GLOBAL(ftrace_stub)
 	blr
-- 
2.31.1

[PATCH v1 3/5] powerpc/ftrace: Add module_trampoline_target() for PPC32

[Christophe Leroy]

module_trampoline_target() is used by __ftrace_modify_call().

Implement it for PPC32 so that CONFIG_DYNAMIC_FTRACE_WITH_REGS
can be activated on PPC32 as well.

Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
---
 arch/powerpc/kernel/module_32.c    | 25 ++++++++++++++++++++
 arch/powerpc/kernel/trace/ftrace.c | 37 ++++--------------------------
 2 files changed, 29 insertions(+), 33 deletions(-)

diff --git a/arch/powerpc/kernel/module_32.c b/arch/powerpc/kernel/module_32.c
index f417afc08d33..5dedd76346b2 100644
--- a/arch/powerpc/kernel/module_32.c
+++ b/arch/powerpc/kernel/module_32.c
@@ -273,6 +273,31 @@ int apply_relocate_add(Elf32_Shdr *sechdrs,
 }
 
 #ifdef CONFIG_DYNAMIC_FTRACE
+int module_trampoline_target(struct module *mod, unsigned long addr,
+			     unsigned long *target)
+{
+	unsigned int jmp[4];
+
+	/* Find where the trampoline jumps to */
+	if (copy_from_kernel_nofault(jmp, (void *)addr, sizeof(jmp)))
+		return -EFAULT;
+
+	/* verify that this is what we expect it to be */
+	if ((jmp[0] & 0xffff0000) != PPC_RAW_LIS(_R12, 0) ||
+	    (jmp[1] & 0xffff0000) != PPC_RAW_ADDI(_R12, _R12, 0) ||
+	    jmp[2] != PPC_RAW_MTCTR(_R12) ||
+	    jmp[3] != PPC_RAW_BCTR())
+		return -EINVAL;
+
+	addr = (jmp[1] & 0xffff) | ((jmp[0] & 0xffff) << 16);
+	if (addr & 0x8000)
+		addr -= 0x10000;
+
+	*target = addr;
+
+	return 0;
+}
+
 int module_finalize_ftrace(struct module *module, const Elf_Shdr *sechdrs)
 {
 	module->arch.tramp = do_plt_call(module->core_layout.base,
diff --git a/arch/powerpc/kernel/trace/ftrace.c b/arch/powerpc/kernel/trace/ftrace.c
index d89c5df4f206..c1d54c18e912 100644
--- a/arch/powerpc/kernel/trace/ftrace.c
+++ b/arch/powerpc/kernel/trace/ftrace.c
@@ -222,9 +222,8 @@ __ftrace_make_nop(struct module *mod,
 		  struct dyn_ftrace *rec, unsigned long addr)
 {
 	struct ppc_inst op;
-	unsigned int jmp[4];
 	unsigned long ip = rec->ip;
-	unsigned long tramp;
+	unsigned long tramp, ptr;
 
 	if (copy_from_kernel_nofault(&op, (void *)ip, MCOUNT_INSN_SIZE))
 		return -EFAULT;
@@ -238,41 +237,13 @@ __ftrace_make_nop(struct module *mod,
 	/* lets find where the pointer goes */
 	tramp = find_bl_target(ip, op);
 
-	/*
-	 * On PPC32 the trampoline looks like:
-	 *  0x3d, 0x80, 0x00, 0x00  lis r12,sym@ha
-	 *  0x39, 0x8c, 0x00, 0x00  addi r12,r12,sym@l
-	 *  0x7d, 0x89, 0x03, 0xa6  mtctr r12
-	 *  0x4e, 0x80, 0x04, 0x20  bctr
-	 */
-
-	pr_devel("ip:%lx jumps to %lx", ip, tramp);
-
 	/* Find where the trampoline jumps to */
-	if (copy_from_kernel_nofault(jmp, (void *)tramp, sizeof(jmp))) {
-		pr_err("Failed to read %lx\n", tramp);
+	if (module_trampoline_target(mod, tramp, &ptr)) {
+		pr_err("Failed to get trampoline target\n");
 		return -EFAULT;
 	}
 
-	pr_devel(" %08x %08x ", jmp[0], jmp[1]);
-
-	/* verify that this is what we expect it to be */
-	if (((jmp[0] & 0xffff0000) != 0x3d800000) ||
-	    ((jmp[1] & 0xffff0000) != 0x398c0000) ||
-	    (jmp[2] != 0x7d8903a6) ||
-	    (jmp[3] != 0x4e800420)) {
-		pr_err("Not a trampoline\n");
-		return -EINVAL;
-	}
-
-	tramp = (jmp[1] & 0xffff) |
-		((jmp[0] & 0xffff) << 16);
-	if (tramp & 0x8000)
-		tramp -= 0x10000;
-
-	pr_devel(" %lx ", tramp);
-
-	if (tramp != addr) {
+	if (ptr != addr) {
 		pr_err("Trampoline location %08lx does not match addr\n",
 		       tramp);
 		return -EINVAL;
-- 
2.31.1

[PATCH v1 4/5] powerpc/ftrace: Activate HAVE_DYNAMIC_FTRACE_WITH_REGS on PPC32

[Christophe Leroy]

Unlike PPC64, PPC32 doesn't require any special compiler option
to get _mcount() call not clobbering registers.

Provide ftrace_regs_caller() and ftrace_regs_call() and activate
HAVE_DYNAMIC_FTRACE_WITH_REGS.

That's heavily copied from ftrace_64_mprofile.S

For the time being leave livepatching aside, it will come with
following patch.

Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
---
 arch/powerpc/Kconfig                  |   4 +-
 arch/powerpc/kernel/module_32.c       |   8 ++
 arch/powerpc/kernel/trace/ftrace.c    |  16 +++-
 arch/powerpc/kernel/trace/ftrace_32.S | 109 ++++++++++++++++++++++++--
 4 files changed, 125 insertions(+), 12 deletions(-)

diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
index e43e17987b92..f66eb1984b00 100644
--- a/arch/powerpc/Kconfig
+++ b/arch/powerpc/Kconfig
@@ -206,7 +206,7 @@ config PPC
 	select HAVE_DEBUG_KMEMLEAK
 	select HAVE_DEBUG_STACKOVERFLOW
 	select HAVE_DYNAMIC_FTRACE
-	select HAVE_DYNAMIC_FTRACE_WITH_REGS	if MPROFILE_KERNEL
+	select HAVE_DYNAMIC_FTRACE_WITH_REGS	if MPROFILE_KERNEL || PPC32
 	select HAVE_EBPF_JIT
 	select HAVE_EFFICIENT_UNALIGNED_ACCESS	if !(CPU_LITTLE_ENDIAN && POWER7_CPU)
 	select HAVE_FAST_GUP
@@ -230,7 +230,7 @@ config PPC
 	select HAVE_KPROBES_ON_FTRACE
 	select HAVE_KRETPROBES
 	select HAVE_LD_DEAD_CODE_DATA_ELIMINATION
-	select HAVE_LIVEPATCH			if HAVE_DYNAMIC_FTRACE_WITH_REGS
+	select HAVE_LIVEPATCH			if HAVE_DYNAMIC_FTRACE_WITH_REGS && PPC64
 	select HAVE_MOD_ARCH_SPECIFIC
 	select HAVE_NMI				if PERF_EVENTS || (PPC64 && PPC_BOOK3S)
 	select HAVE_OPTPROBES
diff --git a/arch/powerpc/kernel/module_32.c b/arch/powerpc/kernel/module_32.c
index 5dedd76346b2..a491ad481d85 100644
--- a/arch/powerpc/kernel/module_32.c
+++ b/arch/powerpc/kernel/module_32.c
@@ -306,6 +306,14 @@ int module_finalize_ftrace(struct module *module, const Elf_Shdr *sechdrs)
 	if (!module->arch.tramp)
 		return -ENOENT;
 
+#ifdef CONFIG_DYNAMIC_FTRACE_WITH_REGS
+	module->arch.tramp_regs = do_plt_call(module->core_layout.base,
+					      (unsigned long)ftrace_regs_caller,
+					      sechdrs, module);
+	if (!module->arch.tramp_regs)
+		return -ENOENT;
+#endif
+
 	return 0;
 }
 #endif
diff --git a/arch/powerpc/kernel/trace/ftrace.c b/arch/powerpc/kernel/trace/ftrace.c
index c1d54c18e912..faa0fa29ac20 100644
--- a/arch/powerpc/kernel/trace/ftrace.c
+++ b/arch/powerpc/kernel/trace/ftrace.c
@@ -561,6 +561,8 @@ __ftrace_make_call(struct dyn_ftrace *rec, unsigned long addr)
 	int err;
 	struct ppc_inst op;
 	u32 *ip = (u32 *)rec->ip;
+	struct module *mod = rec->arch.mod;
+	unsigned long tramp;
 
 	/* read where this goes */
 	if (copy_inst_from_kernel_nofault(&op, ip))
@@ -573,13 +575,23 @@ __ftrace_make_call(struct dyn_ftrace *rec, unsigned long addr)
 	}
 
 	/* If we never set up a trampoline to ftrace_caller, then bail */
-	if (!rec->arch.mod->arch.tramp) {
+#ifdef CONFIG_DYNAMIC_FTRACE_WITH_REGS
+	if (!mod->arch.tramp || !mod->arch.tramp_regs) {
+#else
+	if (!mod->arch.tramp) {
+#endif
 		pr_err("No ftrace trampoline\n");
 		return -EINVAL;
 	}
 
+#ifdef CONFIG_DYNAMIC_FTRACE_WITH_REGS
+	if (rec->flags & FTRACE_FL_REGS)
+		tramp = mod->arch.tramp_regs;
+	else
+#endif
+		tramp = mod->arch.tramp;
 	/* create the branch to the trampoline */
-	err = create_branch(&op, ip, rec->arch.mod->arch.tramp, BRANCH_SET_LINK);
+	err = create_branch(&op, ip, tramp, BRANCH_SET_LINK);
 	if (err) {
 		pr_err("REL24 out of range!\n");
 		return -EINVAL;
diff --git a/arch/powerpc/kernel/trace/ftrace_32.S b/arch/powerpc/kernel/trace/ftrace_32.S
index c7d57124cc59..0a02c0cb12d9 100644
--- a/arch/powerpc/kernel/trace/ftrace_32.S
+++ b/arch/powerpc/kernel/trace/ftrace_32.S
@@ -9,6 +9,7 @@
 #include <asm/asm-offsets.h>
 #include <asm/ftrace.h>
 #include <asm/export.h>
+#include <asm/ptrace.h>
 
 _GLOBAL(mcount)
 _GLOBAL(_mcount)
@@ -29,17 +30,21 @@ _GLOBAL(ftrace_caller)
 	MCOUNT_SAVE_FRAME
 	/* r3 ends up with link register */
 	subi	r3, r3, MCOUNT_INSN_SIZE
+	lis	r5,function_trace_op@ha
+	lwz	r5,function_trace_op@l(r5)
+	li	r6, 0
 .globl ftrace_call
 ftrace_call:
 	bl	ftrace_stub
 	nop
+	MCOUNT_RESTORE_FRAME
+ftrace_caller_common:
 #ifdef CONFIG_FUNCTION_GRAPH_TRACER
 .globl ftrace_graph_call
 ftrace_graph_call:
 	b	ftrace_graph_stub
 _GLOBAL(ftrace_graph_stub)
 #endif
-	MCOUNT_RESTORE_FRAME
 	/* old link register ends up in ctr reg */
 	bctr
 
@@ -47,16 +52,92 @@ _GLOBAL(ftrace_graph_stub)
 _GLOBAL(ftrace_stub)
 	blr
 
+_GLOBAL(ftrace_regs_caller)
+	/* Save the original return address in A's stack frame */
+	stw	r0,LRSAVE(r1)
+
+	/* 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
+
+	b	ftrace_caller_common
+
 #ifdef CONFIG_FUNCTION_GRAPH_TRACER
 _GLOBAL(ftrace_graph_caller)
+	stwu	r1,-48(r1)
+	stw	r3, 12(r1)
+	stw	r4, 16(r1)
+	stw	r5, 20(r1)
+	stw	r6, 24(r1)
+	stw	r7, 28(r1)
+	stw	r8, 32(r1)
+	stw	r9, 36(r1)
+	stw	r10,40(r1)
+
 	addi	r5, r1, 48
-	/* load r4 with local address */
-	lwz	r4, 44(r1)
+	mfctr	r4		/* ftrace_caller has moved local addr here */
+	stw	r4, 44(r1)
+	mflr	r3		/* ftrace_caller has restored LR from stack */
 	subi	r4, r4, MCOUNT_INSN_SIZE
 
-	/* Grab the LR out of the caller stack frame */
-	lwz	r3,52(r1)
-
 	bl	prepare_ftrace_return
 	nop
 
@@ -65,9 +146,21 @@ _GLOBAL(ftrace_graph_caller)
          * Change the LR in the callers stack frame to this.
          */
 	stw	r3,52(r1)
+	mtlr	r3
+	lwz	r0,44(r1)
+	mtctr	r0
+
+	lwz	r3, 12(r1)
+	lwz	r4, 16(r1)
+	lwz	r5, 20(r1)
+	lwz	r6, 24(r1)
+	lwz	r7, 28(r1)
+	lwz	r8, 32(r1)
+	lwz	r9, 36(r1)
+	lwz	r10,40(r1)
+
+	addi	r1, r1, 48
 
-	MCOUNT_RESTORE_FRAME
-	/* old link register ends up in ctr reg */
 	bctr
 
 _GLOBAL(return_to_handler)
-- 
2.31.1

[PATCH v1 5/5] powerpc/ftrace: Add support for livepatch to PPC32

[Christophe Leroy]

This is heavily copied from PPC64. Not much to say about it.

Livepatch sample modules all work.

Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
---
 arch/powerpc/Kconfig                  |  2 +-
 arch/powerpc/include/asm/livepatch.h  |  4 +-
 arch/powerpc/kernel/trace/ftrace_32.S | 69 +++++++++++++++++++++++++++
 3 files changed, 72 insertions(+), 3 deletions(-)

diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
index f66eb1984b00..eceee3b814b9 100644
--- a/arch/powerpc/Kconfig
+++ b/arch/powerpc/Kconfig
@@ -230,7 +230,7 @@ config PPC
 	select HAVE_KPROBES_ON_FTRACE
 	select HAVE_KRETPROBES
 	select HAVE_LD_DEAD_CODE_DATA_ELIMINATION
-	select HAVE_LIVEPATCH			if HAVE_DYNAMIC_FTRACE_WITH_REGS && PPC64
+	select HAVE_LIVEPATCH			if HAVE_DYNAMIC_FTRACE_WITH_REGS
 	select HAVE_MOD_ARCH_SPECIFIC
 	select HAVE_NMI				if PERF_EVENTS || (PPC64 && PPC_BOOK3S)
 	select HAVE_OPTPROBES
diff --git a/arch/powerpc/include/asm/livepatch.h b/arch/powerpc/include/asm/livepatch.h
index 4fe018cc207b..daf24d837241 100644
--- a/arch/powerpc/include/asm/livepatch.h
+++ b/arch/powerpc/include/asm/livepatch.h
@@ -23,8 +23,8 @@ static inline void klp_arch_set_pc(struct ftrace_regs *fregs, unsigned long ip)
 static inline unsigned long klp_get_ftrace_location(unsigned long faddr)
 {
 	/*
-	 * Live patch works only with -mprofile-kernel on PPC. In this case,
-	 * the ftrace location is always within the first 16 bytes.
+	 * Live patch works on PPC32 and only with -mprofile-kernel on PPC64. In
+	 * both cases, the ftrace location is always within the first 16 bytes.
 	 */
 	return ftrace_location_range(faddr, faddr + 16);
 }
diff --git a/arch/powerpc/kernel/trace/ftrace_32.S b/arch/powerpc/kernel/trace/ftrace_32.S
index 0a02c0cb12d9..2545d6bb9f02 100644
--- a/arch/powerpc/kernel/trace/ftrace_32.S
+++ b/arch/powerpc/kernel/trace/ftrace_32.S
@@ -10,6 +10,7 @@
 #include <asm/ftrace.h>
 #include <asm/export.h>
 #include <asm/ptrace.h>
+#include <asm/bug.h>
 
 _GLOBAL(mcount)
 _GLOBAL(_mcount)
@@ -83,6 +84,9 @@ _GLOBAL(ftrace_regs_caller)
 	lis	r3,function_trace_op@ha
 	lwz	r5,function_trace_op@l(r3)
 
+#ifdef CONFIG_LIVEPATCH
+	mr	r14,r7		/* remember old NIP */
+#endif
 	/* Calculate ip from nip-4 into r3 for call below */
 	subi    r3, r7, MCOUNT_INSN_SIZE
 
@@ -107,6 +111,9 @@ ftrace_regs_call:
 	/* Load ctr with the possibly modified NIP */
 	lwz	r3, _NIP(r1)
 	mtctr	r3
+#ifdef CONFIG_LIVEPATCH
+	cmpw	r14, r3		/* has NIP been altered? */
+#endif
 
 	/* Restore gprs */
 	lmw	r2, GPR2(r1)
@@ -118,8 +125,70 @@ ftrace_regs_call:
 	/* Pop our stack frame */
 	addi r1, r1, INT_FRAME_SIZE
 
+#ifdef CONFIG_LIVEPATCH
+        /* Based on the cmpw above, if the NIP was altered handle livepatch */
+	bne-	livepatch_handler
+#endif
 	b	ftrace_caller_common
 
+#ifdef CONFIG_LIVEPATCH
+	/*
+	 * This function runs in the mcount context, between two functions. As
+	 * such it can only clobber registers which are volatile and used in
+	 * function linkage.
+	 *
+	 * We get here when a function A, calls another function B, but B has
+	 * been live patched with a new function C.
+	 *
+	 * On entry:
+	 *  - we have no stack frame and can not allocate one
+	 *  - LR points back to the original caller (in A)
+	 *  - CTR holds the new NIP in C
+	 *  - r0, r11 & r12 are free
+	 */
+livepatch_handler:
+	/* Allocate 2 x 8 bytes */
+	lwz	r11, TI_livepatch_sp+THREAD(r2)
+	addi	r11, r11, 16
+	stw	r11, TI_livepatch_sp+THREAD(r2)
+
+	/* Save real LR on livepatch stack */
+	mflr	r12
+	stw	r12, -16(r11)
+
+	/* Store stack end marker */
+	lis     r12, STACK_END_MAGIC@h
+	ori     r12, r12, STACK_END_MAGIC@l
+	stw	r12, -4(r11)
+
+	/* Branch to ctr */
+	bctrl
+
+	/*
+	 * Now we are returning from the patched function to the original
+	 * caller A. We are free to use r11 and r12.
+	 */
+
+	lwz	r11, TI_livepatch_sp+THREAD(r2)
+
+	/* Check stack marker hasn't been trashed */
+	lwz	r12, -4(r11)
+	subis	r12, r12, STACK_END_MAGIC@h
+1:	twnei	r12, STACK_END_MAGIC@l
+	EMIT_BUG_ENTRY 1b, __FILE__, __LINE__ - 1, 0
+
+	/* Restore LR from livepatch stack */
+	lwz	r12, -16(r11)
+	mtlr	r12
+
+	/* Pop livepatch stack frame */
+	subi	r11, r11, 16
+	stw	r11, TI_livepatch_sp+THREAD(r2)
+
+	/* Return to original caller of live patched function */
+	blr
+#endif /* CONFIG_LIVEPATCH */
+
 #ifdef CONFIG_FUNCTION_GRAPH_TRACER
 _GLOBAL(ftrace_graph_caller)
 	stwu	r1,-48(r1)
-- 
2.31.1

Re: [PATCH v1 0/5] Implement livepatch on PPC32

[Steven Rostedt]

On Thu, 28 Oct 2021 14:24:00 +0200
Christophe Leroy <christophe.leroy@csgroup.eu> wrote:

> This series implements livepatch on PPC32.
> 
> This is largely copied from what's done on PPC64.
> 
> Christophe Leroy (5):
>   livepatch: Fix build failure on 32 bits processors
>   powerpc/ftrace: No need to read LR from stack in _mcount()
>   powerpc/ftrace: Add module_trampoline_target() for PPC32
>   powerpc/ftrace: Activate HAVE_DYNAMIC_FTRACE_WITH_REGS on PPC32
>   powerpc/ftrace: Add support for livepatch to PPC32
> 
>  arch/powerpc/Kconfig                  |   2 +-
>  arch/powerpc/include/asm/livepatch.h  |   4 +-
>  arch/powerpc/kernel/module_32.c       |  33 +++++
>  arch/powerpc/kernel/trace/ftrace.c    |  53 +++-----
>  arch/powerpc/kernel/trace/ftrace_32.S | 187 ++++++++++++++++++++++++--
>  kernel/livepatch/core.c               |   4 +-
>  6 files changed, 230 insertions(+), 53 deletions(-)
> 

This is great that you are doing this, but I wonder if it would even be
easier, and more efficient, if you could implement
HAVE_DYNAMIC_FTRACE_WITH_ARGS?

Then you don't need to save all regs for live kernel patching. And I am
also working on function tracing with arguments with this too.

That is, to call a generic ftrace callback, you need to save all the args
that are stored in registers to prevent the callback from clobbering them.
As live kernel patching only needs to have the arguments of the functions,
you save time from having to save the other regs as well.

The callbacks now have "struct ftrace_regs" instead of pt_regs, because it
will allow non ftrace_regs_caller functions to access the arguments if it
is supported.

Look at how x86_64 implements this. It should be possible to do this for
all other archs as well.

Also note, by doing this, we can then get rid of the ftrace_graph_caller,
and have function graph tracer be a function tracing callback, as it will
allow ftrace_graph_caller to have access to the stack and the return as
well.

If you need any more help or information to do this, I'd be happy to assist
you.

Note, you can implement this first, (I looked over the patches and they
seem fine) and then update both ppc64 and ppc32 to implement
DYNAMIC_FTRACE_WITH_ARGS.

Cheers,

-- Steve

Re: [PATCH v1 0/5] Implement livepatch on PPC32

[Miroslav Benes]

Hi,

On Thu, 28 Oct 2021, Christophe Leroy wrote:

> This series implements livepatch on PPC32.
> 
> This is largely copied from what's done on PPC64.
> 
> Christophe Leroy (5):
>   livepatch: Fix build failure on 32 bits processors
>   powerpc/ftrace: No need to read LR from stack in _mcount()
>   powerpc/ftrace: Add module_trampoline_target() for PPC32
>   powerpc/ftrace: Activate HAVE_DYNAMIC_FTRACE_WITH_REGS on PPC32
>   powerpc/ftrace: Add support for livepatch to PPC32
> 
>  arch/powerpc/Kconfig                  |   2 +-
>  arch/powerpc/include/asm/livepatch.h  |   4 +-
>  arch/powerpc/kernel/module_32.c       |  33 +++++
>  arch/powerpc/kernel/trace/ftrace.c    |  53 +++-----
>  arch/powerpc/kernel/trace/ftrace_32.S | 187 ++++++++++++++++++++++++--
>  kernel/livepatch/core.c               |   4 +-
>  6 files changed, 230 insertions(+), 53 deletions(-)

thanks for the patch set!

I wondered whether the reliability of stack traces also applies to PPC32. 
This was obviously resolved by accdd093f260 ("powerpc: Activate 
HAVE_RELIABLE_STACKTRACE for all").

Did the patch set pass the selftests in 
tools/testing/selftests/livepatch/ ?

Regards

Miroslav

Re: [PATCH v1 1/5] livepatch: Fix build failure on 32 bits processors

[Petr Mladek]

On Thu 2021-10-28 14:24:01, Christophe Leroy wrote:
> Trying to build livepatch on powerpc/32 results in:
> 
> 	kernel/livepatch/core.c: In function 'klp_resolve_symbols':
> 	kernel/livepatch/core.c:221:23: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast]
> 	  221 |                 sym = (Elf64_Sym *)sechdrs[symndx].sh_addr + ELF_R_SYM(relas[i].r_info);
> 	      |                       ^
> 	kernel/livepatch/core.c:221:21: error: assignment to 'Elf32_Sym *' {aka 'struct elf32_sym *'} from incompatible pointer type 'Elf64_Sym *' {aka 'struct elf64_sym *'} [-Werror=incompatible-pointer-types]
> 	  221 |                 sym = (Elf64_Sym *)sechdrs[symndx].sh_addr + ELF_R_SYM(relas[i].r_info);
> 	      |                     ^
> 	kernel/livepatch/core.c: In function 'klp_apply_section_relocs':
> 	kernel/livepatch/core.c:312:35: error: passing argument 1 of 'klp_resolve_symbols' from incompatible pointer type [-Werror=incompatible-pointer-types]
> 	  312 |         ret = klp_resolve_symbols(sechdrs, strtab, symndx, sec, sec_objname);
> 	      |                                   ^~~~~~~
> 	      |                                   |
> 	      |                                   Elf32_Shdr * {aka struct elf32_shdr *}
> 	kernel/livepatch/core.c:193:44: note: expected 'Elf64_Shdr *' {aka 'struct elf64_shdr *'} but argument is of type 'Elf32_Shdr *' {aka 'struct elf32_shdr *'}
> 	  193 | static int klp_resolve_symbols(Elf64_Shdr *sechdrs, const char *strtab,
> 	      |                                ~~~~~~~~~~~~^~~~~~~
> 
> Fix it by using the right types instead of forcing 64 bits types.
> 
> Fixes: 7c8e2bdd5f0d ("livepatch: Apply vmlinux-specific KLP relocations early")
> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>

Makes sense. I haven't tested it but it looks correct ;-)

Acked-by: Petr Mladek <pmladek@suse.com>

Best Regards,
Petr

Re: [PATCH v1 5/5] powerpc/ftrace: Add support for livepatch to PPC32

[Petr Mladek]

On Thu 2021-10-28 14:24:05, Christophe Leroy wrote:
> This is heavily copied from PPC64. Not much to say about it.
> 
> Livepatch sample modules all work.
> 
> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
> ---
> diff --git a/arch/powerpc/include/asm/livepatch.h b/arch/powerpc/include/asm/livepatch.h
> index 4fe018cc207b..daf24d837241 100644
> --- a/arch/powerpc/include/asm/livepatch.h
> +++ b/arch/powerpc/include/asm/livepatch.h
> @@ -23,8 +23,8 @@ static inline void klp_arch_set_pc(struct ftrace_regs *fregs, unsigned long ip)
>  static inline unsigned long klp_get_ftrace_location(unsigned long faddr)
>  {
>  	/*
> -	 * Live patch works only with -mprofile-kernel on PPC. In this case,
> -	 * the ftrace location is always within the first 16 bytes.
> +	 * Live patch works on PPC32 and only with -mprofile-kernel on PPC64. In
> +	 * both cases, the ftrace location is always within the first 16 bytes.

Nit: I had some problems to parse it. I wonder if the following is
better:

	 * Live patch works on PPC32 out of box and on PPC64 only with
	 * -mprofile-kernel. In both cases, the ftrace location is always
	 * within the first 16 bytes.


>  	 */
>  	return ftrace_location_range(faddr, faddr + 16);
>  }

Best Regards,
Petr

Re: [PATCH v1 0/5] Implement livepatch on PPC32

[Michael Ellerman]

Christophe Leroy <christophe.leroy@csgroup.eu> writes:
> This series implements livepatch on PPC32.
>
> This is largely copied from what's done on PPC64.
>
> Christophe Leroy (5):
>   livepatch: Fix build failure on 32 bits processors
>   powerpc/ftrace: No need to read LR from stack in _mcount()
>   powerpc/ftrace: Add module_trampoline_target() for PPC32
>   powerpc/ftrace: Activate HAVE_DYNAMIC_FTRACE_WITH_REGS on PPC32
>   powerpc/ftrace: Add support for livepatch to PPC32

I think we know patch 5 will need a respin because of the STRICT RWX vs
livepatching issue (https://github.com/linuxppc/issues/issues/375).

So should I take patches 2,3,4 for now?

cheers

Re: [PATCH v1 0/5] Implement livepatch on PPC32

[Christophe Leroy]

Le 24/11/2021 à 23:34, Michael Ellerman a écrit :
> Christophe Leroy <christophe.leroy@csgroup.eu> writes:
>> This series implements livepatch on PPC32.
>>
>> This is largely copied from what's done on PPC64.
>>
>> Christophe Leroy (5):
>>    livepatch: Fix build failure on 32 bits processors
>>    powerpc/ftrace: No need to read LR from stack in _mcount()
>>    powerpc/ftrace: Add module_trampoline_target() for PPC32
>>    powerpc/ftrace: Activate HAVE_DYNAMIC_FTRACE_WITH_REGS on PPC32
>>    powerpc/ftrace: Add support for livepatch to PPC32
> 
> I think we know patch 5 will need a respin because of the STRICT RWX vs
> livepatching issue (https://github.com/linuxppc/issues/issues/375).
> 
> So should I take patches 2,3,4 for now?
> 

Yes you can take them now I think.

Thanks
Christophe

Re: [PATCH v1 0/5] Implement livepatch on PPC32

[Michael Ellerman]

On Thu, 28 Oct 2021 14:24:00 +0200, Christophe Leroy wrote:
> This series implements livepatch on PPC32.
> 
> This is largely copied from what's done on PPC64.
> 
> Christophe Leroy (5):
>   livepatch: Fix build failure on 32 bits processors
>   powerpc/ftrace: No need to read LR from stack in _mcount()
>   powerpc/ftrace: Add module_trampoline_target() for PPC32
>   powerpc/ftrace: Activate HAVE_DYNAMIC_FTRACE_WITH_REGS on PPC32
>   powerpc/ftrace: Add support for livepatch to PPC32
> 
> [...]

Patches 2-4 applied to powerpc/next.

[2/5] powerpc/ftrace: No need to read LR from stack in _mcount()
      https://git.kernel.org/powerpc/c/88670fdb26800228606c078ba4a018e9522a75a8
[3/5] powerpc/ftrace: Add module_trampoline_target() for PPC32
      https://git.kernel.org/powerpc/c/c93d4f6ecf4b0699d0f2088f7bd9cd09af45d65a
[4/5] powerpc/ftrace: Activate HAVE_DYNAMIC_FTRACE_WITH_REGS on PPC32
      https://git.kernel.org/powerpc/c/7dfbfb87c243cf08bc2b9cc23699ac207b726458

cheers

Re: [PATCH v1 0/5] Implement livepatch on PPC32

[Christophe Leroy]

Le 28/10/2021 à 15:35, Steven Rostedt a écrit :
> On Thu, 28 Oct 2021 14:24:00 +0200
> Christophe Leroy <christophe.leroy@csgroup.eu> wrote:
> 
>> This series implements livepatch on PPC32.
>>
>> This is largely copied from what's done on PPC64.
>>
>> Christophe Leroy (5):
>>    livepatch: Fix build failure on 32 bits processors
>>    powerpc/ftrace: No need to read LR from stack in _mcount()
>>    powerpc/ftrace: Add module_trampoline_target() for PPC32
>>    powerpc/ftrace: Activate HAVE_DYNAMIC_FTRACE_WITH_REGS on PPC32
>>    powerpc/ftrace: Add support for livepatch to PPC32
>>
>>   arch/powerpc/Kconfig                  |   2 +-
>>   arch/powerpc/include/asm/livepatch.h  |   4 +-
>>   arch/powerpc/kernel/module_32.c       |  33 +++++
>>   arch/powerpc/kernel/trace/ftrace.c    |  53 +++-----
>>   arch/powerpc/kernel/trace/ftrace_32.S | 187 ++++++++++++++++++++++++--
>>   kernel/livepatch/core.c               |   4 +-
>>   6 files changed, 230 insertions(+), 53 deletions(-)
>>
> 
> This is great that you are doing this, but I wonder if it would even be
> easier, and more efficient, if you could implement
> HAVE_DYNAMIC_FTRACE_WITH_ARGS?
> 
> Then you don't need to save all regs for live kernel patching. And I am
> also working on function tracing with arguments with this too.
> 
> That is, to call a generic ftrace callback, you need to save all the args
> that are stored in registers to prevent the callback from clobbering them.
> As live kernel patching only needs to have the arguments of the functions,
> you save time from having to save the other regs as well.
> 
> The callbacks now have "struct ftrace_regs" instead of pt_regs, because it
> will allow non ftrace_regs_caller functions to access the arguments if it
> is supported.
> 
> Look at how x86_64 implements this. It should be possible to do this for
> all other archs as well.
> 
> Also note, by doing this, we can then get rid of the ftrace_graph_caller,
> and have function graph tracer be a function tracing callback, as it will
> allow ftrace_graph_caller to have access to the stack and the return as
> well.
> 
> If you need any more help or information to do this, I'd be happy to assist
> you.
> 
> Note, you can implement this first, (I looked over the patches and they
> seem fine) and then update both ppc64 and ppc32 to implement
> DYNAMIC_FTRACE_WITH_ARGS.
> 

I tried to activate DYNAMIC_FTRACE_WITH_ARGS on PPC32.

I copied into powerpc the changes from 5740a7c71ab6 ("s390/ftrace: add 
HAVE_DYNAMIC_FTRACE_WITH_ARGS support")

Ftrace selftests tell "Testing tracer function_graph: FAILED!".

Is there anything else to do ?

Thanks for your help
Christophe

Re: [PATCH v1 0/5] Implement livepatch on PPC32

[Steven Rostedt]

On Mon, 13 Dec 2021 14:39:15 +0000
Christophe Leroy <christophe.leroy@csgroup.eu> wrote:

> > Note, you can implement this first, (I looked over the patches and they
> > seem fine) and then update both ppc64 and ppc32 to implement
> > DYNAMIC_FTRACE_WITH_ARGS.
> >   
> 
> I tried to activate DYNAMIC_FTRACE_WITH_ARGS on PPC32.
> 
> I copied into powerpc the changes from 5740a7c71ab6 ("s390/ftrace: add 
> HAVE_DYNAMIC_FTRACE_WITH_ARGS support")
> 
> Ftrace selftests tell "Testing tracer function_graph: FAILED!".
> 
> Is there anything else to do ?

Yes. Because BPF is now hooking into the function callbacks, it causes
issues with function graph tracer. So what we did was to have function
graph tracing to now use the function tracer callback as well (this allows
both the BPF direct trampolines to work with function graph tracer).

As it requires DYNAMIC_FTRACE_WITH_ARGS, and x86 was the only one to
support that for now, I decided to make all the archs change function graph
tracing when they implement DYNAMIC_FTRACE_WITH_ARGS too. (It is becoming a
pain to have too many variants of function tracing between the archs).

The change that did this for x86 was:

0c0593b45c9b4 ("x86/ftrace: Make function graph use ftrace directly")

This actually simplifies the function graph tracer, as you no longer need
it's own entry trampoline (still need the trampoline for the return of the
function).

What you need to do is:

In your arch/*/include/asm/ftrace.h add:

struct ftrace_ops;

#define ftrace_graph_func ftrace_graph_func
void ftrace_graph_func(unsigned long ip, unsigned long parent_ip,
                      struct ftrace_ops *op, struct ftrace_regs *fregs);


Where ftrace_graph_func() is now what is called for the function graph
tracer, directly from the ftrace callbacks (no longer a secondary
trampoline).

Define the ftrace_graph_func() to be something like:

void ftrace_graph_func(unsigned long ip, unsigned long parent_ip,
                      struct ftrace_ops *op, struct ftrace_regs *fregs)
{
       struct pt_regs *regs = &fregs->regs;
       unsigned long *stack = (unsigned long *)kernel_stack_pointer(regs);

       prepare_ftrace_return(ip, (unsigned long *)stack, 0);
}

This is called by the function tracer code. But because with
DYNAMIC_FTRACE_WITH_ARGS, we have access to the argument register, we should
also have access to the link register and the stack. Then you can use that
to modify the stack and or link register to jump to the the return
trampoline.

This should all work with powerpc (both 64 and 32) but if it does not, let
me know. I'm happy to help out.

-- Steve

Re: [PATCH v1 0/5] Implement livepatch on PPC32

[Christophe Leroy]

Le 13/12/2021 à 18:15, Steven Rostedt a écrit :
> On Mon, 13 Dec 2021 14:39:15 +0000
> Christophe Leroy <christophe.leroy@csgroup.eu> wrote:
> 
>>> Note, you can implement this first, (I looked over the patches and they
>>> seem fine) and then update both ppc64 and ppc32 to implement
>>> DYNAMIC_FTRACE_WITH_ARGS.
>>>    
>>
>> I tried to activate DYNAMIC_FTRACE_WITH_ARGS on PPC32.
>>
>> I copied into powerpc the changes from 5740a7c71ab6 ("s390/ftrace: add
>> HAVE_DYNAMIC_FTRACE_WITH_ARGS support")
>>
>> Ftrace selftests tell "Testing tracer function_graph: FAILED!".
>>
>> Is there anything else to do ?
> 
> Yes. Because BPF is now hooking into the function callbacks, it causes
> issues with function graph tracer. So what we did was to have function
> graph tracing to now use the function tracer callback as well (this allows
> both the BPF direct trampolines to work with function graph tracer).
> 
> As it requires DYNAMIC_FTRACE_WITH_ARGS, and x86 was the only one to
> support that for now, I decided to make all the archs change function graph
> tracing when they implement DYNAMIC_FTRACE_WITH_ARGS too. (It is becoming a
> pain to have too many variants of function tracing between the archs).
> 
> The change that did this for x86 was:
> 
> 0c0593b45c9b4 ("x86/ftrace: Make function graph use ftrace directly")
> 
> This actually simplifies the function graph tracer, as you no longer need
> it's own entry trampoline (still need the trampoline for the return of the
> function).
> 
> What you need to do is:
> 
> In your arch/*/include/asm/ftrace.h add:
> 
> struct ftrace_ops;
> 
> #define ftrace_graph_func ftrace_graph_func
> void ftrace_graph_func(unsigned long ip, unsigned long parent_ip,
>                        struct ftrace_ops *op, struct ftrace_regs *fregs);
> 
> 
> Where ftrace_graph_func() is now what is called for the function graph
> tracer, directly from the ftrace callbacks (no longer a secondary
> trampoline).
> 
> Define the ftrace_graph_func() to be something like:
> 
> void ftrace_graph_func(unsigned long ip, unsigned long parent_ip,
>                        struct ftrace_ops *op, struct ftrace_regs *fregs)
> {
>         struct pt_regs *regs = &fregs->regs;
>         unsigned long *stack = (unsigned long *)kernel_stack_pointer(regs);
> 
>         prepare_ftrace_return(ip, (unsigned long *)stack, 0);
> }
> 
> This is called by the function tracer code. But because with
> DYNAMIC_FTRACE_WITH_ARGS, we have access to the argument register, we should
> also have access to the link register and the stack. Then you can use that
> to modify the stack and or link register to jump to the the return
> trampoline.
> 
> This should all work with powerpc (both 64 and 32) but if it does not, let
> me know. I'm happy to help out.
> 

Thanks, I will try that.

I can't find ftrace_graph_func() in s390. Does it mean that s390 doesn't 
have a working function tracer anymore ?

I see your commit 0c0593b45c9b4 ("x86/ftrace: Make function graph use 
ftrace directly") is dated 8 Oct 2021 while 5740a7c71ab6 ("s390/ftrace: 
add HAVE_DYNAMIC_FTRACE_WITH_ARGS support") is 4 Oct 2021.

Christophe

Re: [PATCH v1 0/5] Implement livepatch on PPC32

[Steven Rostedt]

On Mon, 13 Dec 2021 17:30:48 +0000
Christophe Leroy <christophe.leroy@csgroup.eu> wrote:

> Thanks, I will try that.
> 
> I can't find ftrace_graph_func() in s390. Does it mean that s390 doesn't 
> have a working function tracer anymore ?
> 
> I see your commit 0c0593b45c9b4 ("x86/ftrace: Make function graph use 
> ftrace directly") is dated 8 Oct 2021 while 5740a7c71ab6 ("s390/ftrace: 
> add HAVE_DYNAMIC_FTRACE_WITH_ARGS support") is 4 Oct 2021.

Hmm, maybe not. I can't test it.

This needs to be fixed if that's the case.

Thanks for bringing it up!

-- Steve

Re: [PATCH v1 0/5] Implement livepatch on PPC32

[Christophe Leroy]

Le 13/12/2021 à 18:33, Steven Rostedt a écrit :
> On Mon, 13 Dec 2021 17:30:48 +0000
> Christophe Leroy <christophe.leroy@csgroup.eu> wrote:
> 
>> Thanks, I will try that.
>>
>> I can't find ftrace_graph_func() in s390. Does it mean that s390 doesn't
>> have a working function tracer anymore ?
>>
>> I see your commit 0c0593b45c9b4 ("x86/ftrace: Make function graph use
>> ftrace directly") is dated 8 Oct 2021 while 5740a7c71ab6 ("s390/ftrace:
>> add HAVE_DYNAMIC_FTRACE_WITH_ARGS support") is 4 Oct 2021.
> 
> Hmm, maybe not. I can't test it.
> 
> This needs to be fixed if that's the case.
> 
> Thanks for bringing it up!
> 

On PPC32, I did your suggested changes, I get an Oops:

[    8.038441] Testing tracer function_graph:
[    8.064147] Kernel attempted to read user page (4) - exploit attempt? 
(uid: 0)
[    8.075296] Kernel attempted to read user page (4) - exploit attempt? 
(uid: 0)
[    8.082424] BUG: Kernel NULL pointer dereference on read at 0x00000004
[    8.088864] Faulting instruction address: 0xc001468c
[    8.093778] Oops: Kernel access of bad area, sig: 11 [#1]
[    8.099105] BE PAGE_SIZE=16K PREEMPT CMPC885
[    8.103329] Modules linked in:
[    8.106340] CPU: 0 PID: 1 Comm: swapper Not tainted 
5.16.0-rc3-s3k-dev-02295-g0bd6d618bcd8-dirty #732
[    8.115461] NIP:  c001468c LR: c00c8414 CTR: c0014674
[    8.120448] REGS: c902ba00 TRAP: 0300   Not tainted 
(5.16.0-rc3-s3k-dev-02295-g0bd6d618bcd8-dirty)
[    8.129398] MSR:  00001032 <ME,IR,DR,RI>  CR: 88022252  XER: 20000000
[    8.135853] DAR: 00000004 DSISR: c0000000
[    8.135853] GPR00: c00c8414 c902bac0 c2140000 c0015260 c0003ac4 
c122db78 00000000 00000300
[    8.135853] GPR08: c2140000 c0014674 c0015260 00000000 2802b252 
00000000 c0004f38 00000000
[    8.135853] GPR16: 00000000 00000000 00000000 00000000 00000000 
00000010 c1037d1c c12d0000
[    8.135853] GPR24: c121c440 c12b5380 c12b0000 c0003ac4 c0015260 
00000000 00000200 c0015260
[    8.174493] NIP [c001468c] ftrace_graph_func+0x18/0x74
[    8.179572] LR [c00c8414] arch_ftrace_ops_list_func+0x118/0x230
[    8.185430] Call Trace:
[    8.187837] [c902bac0] [c12b5380] ftrace_list_end+0x0/0x50 (unreliable)
[    8.194379] [c902bad0] [c00c8414] arch_ftrace_ops_list_func+0x118/0x230
[    8.200920] [c902bb20] [c001475c] ftrace_call+0x4/0x44
[    8.205997] [c902bb50] [c0003ac4] DataTLBError_virt+0x114/0x118
[    8.211848] --- interrupt: 300 at ftrace_graph_func+0x18/0x74
[    8.217527] NIP:  c001468c LR: c00c8414 CTR: c0014674
[    8.222516] REGS: c902bb60 TRAP: 0300   Not tainted 
(5.16.0-rc3-s3k-dev-02295-g0bd6d618bcd8-dirty)
[    8.231466] MSR:  00001032 <ME,IR,DR,RI>  CR: 82002842  XER: 20000000
[    8.237920] DAR: 00000004 DSISR: c0000000
[    8.237920] GPR00: c00c8414 c902bc20 c2140000 c001573c c001624c 
c122db78 00000000 00000100
[    8.237920] GPR08: c2140000 c0014674 c001573c 00000000 22004842 
00000000 c0004f38 00000000
[    8.237920] GPR16: 00000000 00000000 00000000 00000000 00000000 
00000010 c1037d1c c12d0000
[    8.237920] GPR24: c121c440 c12b5380 c12b0000 c001624c c001573c 
00000000 00000100 c001573c
[    8.276561] NIP [c001468c] ftrace_graph_func+0x18/0x74
[    8.281639] LR [c00c8414] arch_ftrace_ops_list_func+0x118/0x230
[    8.287491] --- interrupt: 300
[    8.290508] [c902bc20] [00000001] 0x1 (unreliable)
[    8.295242] [c902bc30] [c00c8414] arch_ftrace_ops_list_func+0x118/0x230
[    8.301782] [c902bc80] [c001475c] ftrace_call+0x4/0x44
[    8.306860] [c902bcb0] [c001624c] map_kernel_page+0xc8/0x12c
[    8.312454] [c902bd00] [c0019cb0] patch_instruction+0xbc/0x278
[    8.318221] [c902bd30] [c0013964] ftrace_modify_code+0x38/0xc4
[    8.323986] [c902bd70] [c00c2c0c] ftrace_replace_code+0x78/0xec
[    8.329838] [c902bd90] [c00c2e30] ftrace_modify_all_code+0xd0/0x148
[    8.336035] [c902bdb0] [c00c2f38] ftrace_run_update_code+0x28/0x88
[    8.342145] [c902bdc0] [c00c75dc] ftrace_startup+0x118/0x1e0
[    8.347739] [c902bde0] [c00e8310] register_ftrace_graph+0x334/0x3c0
[    8.353935] [c902be20] [c100ccf4] 
trace_selftest_startup_function_graph+0x64/0x164
[    8.361422] [c902be50] [c00debc0] run_tracer_selftest+0x120/0x1b4
[    8.367447] [c902be70] [c100c74c] register_tracer+0x14c/0x218
[    8.373126] [c902be90] [c0004a30] do_one_initcall+0x44/0x1e8
[    8.378720] [c902bef0] [c10011f4] kernel_init_freeable+0x1a8/0x250
[    8.384831] [c902bf20] [c0004f68] kernel_init+0x30/0x150
[    8.390081] [c902bf30] [c001322c] ret_from_kernel_thread+0x5c/0x64
[    8.396193] Instruction dump:
[    8.399115] 83c10018 83e1001c 3863489c 7c0803a6 38210020 4e800020 
9421fff0 7c0802a6
[    8.407031] 93e1000c 90010014 93c10008 7c7f1b78 <83c60004> 480d343d 
2c030000 40820038
[    8.415154] ---[ end trace 717d695f81a0970d ]---

I also tried with the additional change below, but still the same:

int ftrace_enable_ftrace_graph_caller(void)
{
	return 0;
}

int ftrace_disable_ftrace_graph_caller(void)
{
	return 0;
}

Anything else to do ?

Full change below:

diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
index cdac2115eb00..e2b1792b2aae 100644
--- a/arch/powerpc/Kconfig
+++ b/arch/powerpc/Kconfig
@@ -210,6 +210,7 @@ config PPC
  	select HAVE_DEBUG_KMEMLEAK
  	select HAVE_DEBUG_STACKOVERFLOW
  	select HAVE_DYNAMIC_FTRACE
+	select HAVE_DYNAMIC_FTRACE_WITH_ARGS	if MPROFILE_KERNEL || PPC32
  	select HAVE_DYNAMIC_FTRACE_WITH_REGS	if MPROFILE_KERNEL || PPC32
  	select HAVE_EBPF_JIT
  	select HAVE_EFFICIENT_UNALIGNED_ACCESS	if !(CPU_LITTLE_ENDIAN && 
POWER7_CPU)
diff --git a/arch/powerpc/include/asm/ftrace.h 
b/arch/powerpc/include/asm/ftrace.h
index debe8c4f7062..68f503294342 100644
--- a/arch/powerpc/include/asm/ftrace.h
+++ b/arch/powerpc/include/asm/ftrace.h
@@ -59,9 +59,24 @@ static inline unsigned long 
ftrace_call_adjust(unsigned long addr)
  struct dyn_arch_ftrace {
  	struct module *mod;
  };
+
+struct ftrace_regs {
+	struct pt_regs regs;
+};
+
+static __always_inline struct pt_regs *arch_ftrace_get_regs(struct 
ftrace_regs *fregs)
+{
+	return &fregs->regs;
+}
+
+struct ftrace_ops;
+
+#define ftrace_graph_func ftrace_graph_func
+void ftrace_graph_func(unsigned long ip, unsigned long parent_ip,
+		       struct ftrace_ops *op, struct ftrace_regs *fregs);
  #endif /* __ASSEMBLY__ */

-#ifdef CONFIG_DYNAMIC_FTRACE_WITH_REGS
+#if defined(CONFIG_DYNAMIC_FTRACE_WITH_REGS) || 
defined(CONFIG_DYNAMIC_FTRACE_WITH_ARGS)
  #define ARCH_SUPPORTS_FTRACE_OPS 1
  #endif
  #endif /* CONFIG_FUNCTION_TRACER */
diff --git a/arch/powerpc/kernel/trace/ftrace.c 
b/arch/powerpc/kernel/trace/ftrace.c
index 80b6285769f2..7662c88c4c0c 100644
--- a/arch/powerpc/kernel/trace/ftrace.c
+++ b/arch/powerpc/kernel/trace/ftrace.c
@@ -912,28 +912,12 @@ extern void ftrace_graph_stub(void);

  int ftrace_enable_ftrace_graph_caller(void)
  {
-	unsigned long ip = (unsigned long)(&ftrace_graph_call);
-	unsigned long addr = (unsigned long)(&ftrace_graph_caller);
-	unsigned long stub = (unsigned long)(&ftrace_graph_stub);
-	ppc_inst_t old, new;
-
-	old = ftrace_call_replace(ip, stub, 0);
-	new = ftrace_call_replace(ip, addr, 0);
-
-	return ftrace_modify_code(ip, old, new);
+	return 0;
  }

  int ftrace_disable_ftrace_graph_caller(void)
  {
-	unsigned long ip = (unsigned long)(&ftrace_graph_call);
-	unsigned long addr = (unsigned long)(&ftrace_graph_caller);
-	unsigned long stub = (unsigned long)(&ftrace_graph_stub);
-	ppc_inst_t old, new;
-
-	old = ftrace_call_replace(ip, addr, 0);
-	new = ftrace_call_replace(ip, stub, 0);
-
-	return ftrace_modify_code(ip, old, new);
+	return 0;
  }

  /*
@@ -958,6 +942,12 @@ unsigned long prepare_ftrace_return(unsigned long 
parent, unsigned long ip,
  out:
  	return parent;
  }
+
+void ftrace_graph_func(unsigned long ip, unsigned long parent_ip,
+		       struct ftrace_ops *op, struct ftrace_regs *fregs)
+{
+	prepare_ftrace_return(ip, kernel_stack_pointer(&fregs->regs), 0);
+}
  #endif /* CONFIG_FUNCTION_GRAPH_TRACER */

  #ifdef PPC64_ELF_ABI_v1


Christophe

Re: [PATCH v1 0/5] Implement livepatch on PPC32

[Steven Rostedt]

On Mon, 13 Dec 2021 17:50:52 +0000
Christophe Leroy <christophe.leroy@csgroup.eu> wrote:

> @@ -958,6 +942,12 @@ unsigned long prepare_ftrace_return(unsigned long 
> parent, unsigned long ip,
>   out:
>   	return parent;
>   }
> +
> +void ftrace_graph_func(unsigned long ip, unsigned long parent_ip,
> +		       struct ftrace_ops *op, struct ftrace_regs *fregs)
> +{
> +	prepare_ftrace_return(ip, kernel_stack_pointer(&fregs->regs), 0);
> +}

I have for powerpc prepare_ftrace_return as:


unsigned long prepare_ftrace_return(unsigned long parent, unsigned long ip,
                                                unsigned long sp)
{
        unsigned long return_hooker;

        if (unlikely(ftrace_graph_is_dead()))
                goto out;

        if (unlikely(atomic_read(&current->tracing_graph_pause)))
                goto out;

        return_hooker = ppc_function_entry(return_to_handler);

        if (!function_graph_enter(parent, ip, 0, (unsigned long *)sp))
                parent = return_hooker;
out:
        return parent;
}

Which means you'll need different parameters to it than what x86 has, which
has the prototype of:

void prepare_ftrace_return(unsigned long ip, unsigned long *parent,
			   unsigned long frame_pointer)

and it does not use the frame_pointer for this case, which is why it is
zero.

For powerpc though, it uses the stack pointer, so you parameters are
incorrect. Looks like it should be:

	prepare_ftrace_return(parent_ip, ip, kernel_stack_pointer(&fregs->regs));

And that will likely not be enough. I'll need to update the ctr register,
as that is where the return address is saved. So you'll probably need it to be:

void ftrace_graph_func(unsigned long ip, unsigned long parent_ip,
		       struct ftrace_ops *op, struct ftrace_regs *fregs)
{
	unsigned long parent;

	parent = prepare_ftrace_return(parent_ip, ip, kernel_stack_pointer(&fregs->regs));
	fregs->regs.ctr = parent;
}



-- Steve

Re: [PATCH v1 0/5] Implement livepatch on PPC32

[Christophe Leroy]

Le 13/12/2021 à 19:54, Steven Rostedt a écrit :
> On Mon, 13 Dec 2021 17:50:52 +0000
> Christophe Leroy <christophe.leroy@csgroup.eu> wrote:
> 
>> @@ -958,6 +942,12 @@ unsigned long prepare_ftrace_return(unsigned long
>> parent, unsigned long ip,
>>    out:
>>    	return parent;
>>    }
>> +
>> +void ftrace_graph_func(unsigned long ip, unsigned long parent_ip,
>> +		       struct ftrace_ops *op, struct ftrace_regs *fregs)
>> +{
>> +	prepare_ftrace_return(ip, kernel_stack_pointer(&fregs->regs), 0);
>> +}
> 
> I have for powerpc prepare_ftrace_return as:
> 
> 
> unsigned long prepare_ftrace_return(unsigned long parent, unsigned long ip,
>                                                  unsigned long sp)
> {
>          unsigned long return_hooker;
> 
>          if (unlikely(ftrace_graph_is_dead()))
>                  goto out;
> 
>          if (unlikely(atomic_read(&current->tracing_graph_pause)))
>                  goto out;
> 
>          return_hooker = ppc_function_entry(return_to_handler);
> 
>          if (!function_graph_enter(parent, ip, 0, (unsigned long *)sp))
>                  parent = return_hooker;
> out:
>          return parent;
> }
> 
> Which means you'll need different parameters to it than what x86 has, which
> has the prototype of:
> 
> void prepare_ftrace_return(unsigned long ip, unsigned long *parent,
> 			   unsigned long frame_pointer)
> 
> and it does not use the frame_pointer for this case, which is why it is
> zero.
> 
> For powerpc though, it uses the stack pointer, so you parameters are
> incorrect. Looks like it should be:
> 
> 	prepare_ftrace_return(parent_ip, ip, kernel_stack_pointer(&fregs->regs));
> 
> And that will likely not be enough. I'll need to update the ctr register,
> as that is where the return address is saved. So you'll probably need it to be:
> 
> void ftrace_graph_func(unsigned long ip, unsigned long parent_ip,
> 		       struct ftrace_ops *op, struct ftrace_regs *fregs)
> {
> 	unsigned long parent;
> 
> 	parent = prepare_ftrace_return(parent_ip, ip, kernel_stack_pointer(&fregs->regs));
> 	fregs->regs.ctr = parent;
> }
> 

STill the same Oops, below
I will look more closely tomorrow.

[    8.018219] Testing tracer function_graph:
[    8.043884] Kernel attempted to read user page (4) - exploit attempt? 
(uid: 0)
[    8.055074] Kernel attempted to read user page (4) - exploit attempt? 
(uid: 0)
[    8.062204] BUG: Kernel NULL pointer dereference on read at 0x00000004
[    8.068643] Faulting instruction address: 0xc0014694
[    8.073556] Oops: Kernel access of bad area, sig: 11 [#1]
[    8.078884] BE PAGE_SIZE=16K PREEMPT CMPC885
[    8.083109] Modules linked in:
[    8.086120] CPU: 0 PID: 1 Comm: swapper Not tainted 
5.16.0-rc3-s3k-dev-02295-g0bd6d618bcd8-dirty #733
[    8.095240] NIP:  c0014694 LR: c00c8434 CTR: c0014674
[    8.100227] REGS: c902b9e0 TRAP: 0300   Not tainted 
(5.16.0-rc3-s3k-dev-02295-g0bd6d618bcd8-dirty)
[    8.109178] MSR:  00001032 <ME,IR,DR,RI>  CR: 88022242  XER: 20000000
[    8.115632] DAR: 00000004 DSISR: c0000000
[    8.115632] GPR00: c00c8434 c902baa0 c2140000 c0015278 c0003ac4 
c122db78 00000000 00000300
[    8.115632] GPR08: c2140000 c0014674 c0015278 00000000 2802b242 
00000000 c0004f38 00000000
[    8.115632] GPR16: 00000000 00000000 00000000 00000000 00000000 
00000010 c1037d1c c12d0000
[    8.115632] GPR24: c121c440 c12b5380 c12b0000 c0003ac4 c0015278 
00000000 00000000 c122db78
[    8.154272] NIP [c0014694] ftrace_graph_func+0x20/0x8c
[    8.159351] LR [c00c8434] arch_ftrace_ops_list_func+0x118/0x230
[    8.165208] Call Trace:
[    8.167616] [c902baa0] [c006c048] vprintk_emit+0x188/0x2a4 (unreliable)
[    8.174158] [c902bac0] [c00c8434] arch_ftrace_ops_list_func+0x118/0x230
[    8.180699] [c902bb10] [c0014774] ftrace_call+0x4/0x44
[    8.185776] [c902bb40] [c0003ac4] DataTLBError_virt+0x114/0x118
[    8.191627] --- interrupt: 300 at ftrace_graph_func+0x20/0x8c
[    8.197306] NIP:  c0014694 LR: c00c8434 CTR: c0014674
[    8.202296] REGS: c902bb50 TRAP: 0300   Not tainted 
(5.16.0-rc3-s3k-dev-02295-g0bd6d618bcd8-dirty)
[    8.211245] MSR:  00001032 <ME,IR,DR,RI>  CR: 82002842  XER: 20000000
[    8.217699] DAR: 00000004 DSISR: c0000000
[    8.217699] GPR00: c00c8434 c902bc10 c2140000 c0015754 c0016264 
c122db78 00000000 00000100
[    8.217699] GPR08: c2140000 c0014674 c0015754 00000000 22004842 
00000000 c0004f38 00000000
[    8.217699] GPR16: 00000000 00000000 00000000 00000000 00000000 
00000010 c1037d1c c12d0000
[    8.217699] GPR24: c121c440 c12b5380 c12b0000 c0016264 c0015754 
00000000 00000000 c122db78
[    8.256340] NIP [c0014694] ftrace_graph_func+0x20/0x8c
[    8.261418] LR [c00c8434] arch_ftrace_ops_list_func+0x118/0x230
[    8.267270] --- interrupt: 300
[    8.270288] [c902bc10] [c00adb98] 
clockevents_program_event+0x108/0x254 (unreliable)
[    8.277947] [c902bc30] [c00c8434] arch_ftrace_ops_list_func+0x118/0x230
[    8.284488] [c902bc80] [c0014774] ftrace_call+0x4/0x44
[    8.289565] [c902bcb0] [c0016264] map_kernel_page+0xc8/0x12c
[    8.295159] [c902bd00] [c0019cc8] patch_instruction+0xbc/0x278
[    8.300926] [c902bd30] [c0013964] ftrace_modify_code+0x38/0xc4
[    8.306691] [c902bd70] [c00c2c2c] ftrace_replace_code+0x78/0xec
[    8.312543] [c902bd90] [c00c2e50] ftrace_modify_all_code+0xd0/0x148
[    8.318740] [c902bdb0] [c00c2f58] ftrace_run_update_code+0x28/0x88
[    8.324850] [c902bdc0] [c00c75fc] ftrace_startup+0x118/0x1e0
[    8.330443] [c902bde0] [c00e8330] register_ftrace_graph+0x334/0x3c0
[    8.336640] [c902be20] [c100ccf4] 
trace_selftest_startup_function_graph+0x64/0x164
[    8.344127] [c902be50] [c00debe0] run_tracer_selftest+0x120/0x1b4
[    8.350152] [c902be70] [c100c74c] register_tracer+0x14c/0x218
[    8.355832] [c902be90] [c0004a30] do_one_initcall+0x44/0x1e8
[    8.361425] [c902bef0] [c10011f4] kernel_init_freeable+0x1a8/0x250
[    8.367536] [c902bf20] [c0004f68] kernel_init+0x30/0x150
[    8.372785] [c902bf30] [c001322c] ret_from_kernel_thread+0x5c/0x64
[    8.378898] Instruction dump:
[    8.381821] 386348b4 7c0803a6 38210020 4e800020 9421ffe0 7c0802a6 
93a10014 93c10018
[    8.389737] 93e1001c 90010024 93810010 7cde3378 <83860004> 7c7d1b78 
7c9f2378 480d344d
[    8.397859] ---[ end trace 93333951fba49ac1 ]---


Thanks
Christophe

Re: [PATCH v1 0/5] Implement livepatch on PPC32

[Steven Rostedt]

On Mon, 13 Dec 2021 19:33:47 +0000
Christophe Leroy <christophe.leroy@csgroup.eu> wrote:

> STill the same Oops, below

Unfortunately, I don't have a PPC machine (32 nor 64 bit) to help debug
this.


> I will look more closely tomorrow.

OK, thanks.

-- Steve

Re: [PATCH v1 0/5] Implement livepatch on PPC32

[Christophe Leroy]

Le 13/12/2021 à 20:46, Steven Rostedt a écrit :
> On Mon, 13 Dec 2021 19:33:47 +0000
> Christophe Leroy <christophe.leroy@csgroup.eu> wrote:
> 
>> STill the same Oops, below
> 
> Unfortunately, I don't have a PPC machine (32 nor 64 bit) to help debug
> this.
> 
> 
>> I will look more closely tomorrow.
> 
> OK, thanks.
> 

The Oops was due to ftrace_caller() setting the regs argument to NULL.

After fixing that, I'm back into a situation where I get "Testing tracer 
function_graph: FAILED!"

Will continue investigating.

Christophe

Re: [PATCH v1 0/5] Implement livepatch on PPC32

[Christophe Leroy]

Le 14/12/2021 à 07:09, Christophe Leroy a écrit :
> 
> 
> Le 13/12/2021 à 20:46, Steven Rostedt a écrit :
>> On Mon, 13 Dec 2021 19:33:47 +0000
>> Christophe Leroy <christophe.leroy@csgroup.eu> wrote:
>>
>>> STill the same Oops, below
>>
>> Unfortunately, I don't have a PPC machine (32 nor 64 bit) to help debug
>> this.
>>
>>
>>> I will look more closely tomorrow.
>>
>> OK, thanks.
>>
> 
> The Oops was due to ftrace_caller() setting the regs argument to NULL.
> 
> After fixing that, I'm back into a situation where I get "Testing tracer 
> function_graph: FAILED!"
> 
> Will continue investigating.
> 

trace_selftest_startup_function_graph() calls register_ftrace_direct() 
which returns -ENOSUPP because powerpc doesn't select 
CONFIG_DYNAMIC_FTRACE_WITH_DIRECT_CALLS.

Should TEST_DIRECT_TRAMP depend on CONFIG_DYNAMIC_FTRACE_WITH_DIRECT_CALLS ?

Christophe

Re: [PATCH v1 0/5] Implement livepatch on PPC32

[Steven Rostedt]

On Tue, 14 Dec 2021 08:35:14 +0100
Christophe Leroy <christophe.leroy@csgroup.eu> wrote:

> > Will continue investigating.
> >   
> 
> trace_selftest_startup_function_graph() calls register_ftrace_direct() 
> which returns -ENOSUPP because powerpc doesn't select 
> CONFIG_DYNAMIC_FTRACE_WITH_DIRECT_CALLS.
> 
> Should TEST_DIRECT_TRAMP depend on CONFIG_DYNAMIC_FTRACE_WITH_DIRECT_CALLS ?

Yes, that should be:

#if defined(CONFIG_DYNAMIC_FTRACE) && \
    defined(CONFIG_HAVE_DYNAMIC_FTRACE_WITH_DIRECT_CALLS)
#define TEST_DIRECT_TRAMP
noinline __noclone static void trace_direct_tramp(void) { }
#endif


And make it test it with or without the args.

Thanks for finding this.

-- Steve

Re: [PATCH v1 0/5] Implement livepatch on PPC32

[Heiko Carstens]

On Mon, Dec 13, 2021 at 05:50:52PM +0000, Christophe Leroy wrote:
> Le 13/12/2021 à 18:33, Steven Rostedt a écrit :
> > On Mon, 13 Dec 2021 17:30:48 +0000
> > Christophe Leroy <christophe.leroy@csgroup.eu> wrote:
> > 
> >> Thanks, I will try that.
> >>
> >> I can't find ftrace_graph_func() in s390. Does it mean that s390 doesn't
> >> have a working function tracer anymore ?
> >>
> >> I see your commit 0c0593b45c9b4 ("x86/ftrace: Make function graph use
> >> ftrace directly") is dated 8 Oct 2021 while 5740a7c71ab6 ("s390/ftrace:
> >> add HAVE_DYNAMIC_FTRACE_WITH_ARGS support") is 4 Oct 2021.
> > 
> > Hmm, maybe not. I can't test it.
> > 
> > This needs to be fixed if that's the case.
> > 
> > Thanks for bringing it up!

It still works, we run the full ftrace/kprobes selftests from the
kernel every day on multiple machines with several kernels (besides
other Linus' tree, but also linux-next). That said, I wanted to change
s390's code follow what x86 is currently doing anyway.

One thing to note: commit 5740a7c71ab6 ("s390/ftrace: add
HAVE_DYNAMIC_FTRACE_WITH_ARGS support") looks only that simple because
ftrace_caller _and_ ftrace_regs_caller used to save all register
contents into the pt_regs structure, which never was a requirement,
but implicitly fulfills the HAVE_DYNAMIC_FTRACE_WITH_ARGS
requirements.
Not sure if powerpc passes enough register contents via pt_regs for
HAVE_DYNAMIC_FTRACE_WITH_ARGS though. Might be something to check?

Re: [PATCH v1 0/5] Implement livepatch on PPC32

[Christophe Leroy]

Le 14/12/2021 à 15:25, Heiko Carstens a écrit :
> On Mon, Dec 13, 2021 at 05:50:52PM +0000, Christophe Leroy wrote:
>> Le 13/12/2021 à 18:33, Steven Rostedt a écrit :
>>> On Mon, 13 Dec 2021 17:30:48 +0000
>>> Christophe Leroy <christophe.leroy@csgroup.eu> wrote:
>>>
>>>> Thanks, I will try that.
>>>>
>>>> I can't find ftrace_graph_func() in s390. Does it mean that s390 doesn't
>>>> have a working function tracer anymore ?
>>>>
>>>> I see your commit 0c0593b45c9b4 ("x86/ftrace: Make function graph use
>>>> ftrace directly") is dated 8 Oct 2021 while 5740a7c71ab6 ("s390/ftrace:
>>>> add HAVE_DYNAMIC_FTRACE_WITH_ARGS support") is 4 Oct 2021.
>>>
>>> Hmm, maybe not. I can't test it.
>>>
>>> This needs to be fixed if that's the case.
>>>
>>> Thanks for bringing it up!
> 
> It still works, we run the full ftrace/kprobes selftests from the
> kernel every day on multiple machines with several kernels (besides
> other Linus' tree, but also linux-next). That said, I wanted to change
> s390's code follow what x86 is currently doing anyway.
> 
> One thing to note: commit 5740a7c71ab6 ("s390/ftrace: add
> HAVE_DYNAMIC_FTRACE_WITH_ARGS support") looks only that simple because
> ftrace_caller _and_ ftrace_regs_caller used to save all register
> contents into the pt_regs structure, which never was a requirement,
> but implicitly fulfills the HAVE_DYNAMIC_FTRACE_WITH_ARGS
> requirements.
> Not sure if powerpc passes enough register contents via pt_regs for
> HAVE_DYNAMIC_FTRACE_WITH_ARGS though. Might be something to check?
> 

In fact there is no need to rework the function graph logic. It still 
works as is with HAVE_DYNAMIC_FTRACE_WITH_ARGS.

The problem was that the sefltests were failing with 
CONFIG_DYNAMIC_FTRACE_WITH_DIRECT_CALLS not being selected on powerpc.

As s390 selects CONFIG_DYNAMIC_FTRACE_WITH_DIRECT_CALLS, there is no 
problem.

Thanks
Christophe

Re: [PATCH v1 0/5] Implement livepatch on PPC32

[Christophe Leroy]

Le 14/12/2021 à 15:01, Steven Rostedt a écrit :
> On Tue, 14 Dec 2021 08:35:14 +0100
> Christophe Leroy <christophe.leroy@csgroup.eu> wrote:
> 
>>> Will continue investigating.
>>>    
>>
>> trace_selftest_startup_function_graph() calls register_ftrace_direct()
>> which returns -ENOSUPP because powerpc doesn't select
>> CONFIG_DYNAMIC_FTRACE_WITH_DIRECT_CALLS.
>>
>> Should TEST_DIRECT_TRAMP depend on CONFIG_DYNAMIC_FTRACE_WITH_DIRECT_CALLS ?
> 
> Yes, that should be:
> 
> #if defined(CONFIG_DYNAMIC_FTRACE) && \
>      defined(CONFIG_HAVE_DYNAMIC_FTRACE_WITH_DIRECT_CALLS)
> #define TEST_DIRECT_TRAMP
> noinline __noclone static void trace_direct_tramp(void) { }
> #endif
> 
> 
> And make it test it with or without the args.
> 

Shouldn't it just be:

#ifdef CONFIG_DYNAMIC_FTRACE_WITH_DIRECT_CALLS

Because

register_ftrace_direct() depends on that symbol, so if you have 
CONFIG_DYNAMIC_FTRACE && CONFIG_HAVE_DYNAMIC_FTRACE_WITH_DIRECT_CALLS 
but not DYNAMIC_FTRACE_WITH_REGS then 
CONFIG_DYNAMIC_FTRACE_WITH_DIRECT_CALLS is unset and 
register_ftrace_direct() returns -ENOTSUPP

Christophe