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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
This is heavily copied from PPC64. Not much to say about it.
Livepatch sample modules all work.
Signed-off-by: Christophe Leroy <[email protected]>
---
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
On Thu, 28 Oct 2021 14:24:00 +0200
Christophe Leroy <[email protected]> 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
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
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 <[email protected]>
Makes sense. I haven't tested it but it looks correct ;-)
Acked-by: Petr Mladek <[email protected]>
Best Regards,
Petr
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 <[email protected]>
> ---
> 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
Christophe Leroy <[email protected]> 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
Le 24/11/2021 à 23:34, Michael Ellerman a écrit :
> Christophe Leroy <[email protected]> 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
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
Le 28/10/2021 à 15:35, Steven Rostedt a écrit :
> On Thu, 28 Oct 2021 14:24:00 +0200
> Christophe Leroy <[email protected]> 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
On Mon, 13 Dec 2021 14:39:15 +0000
Christophe Leroy <[email protected]> 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
Le 13/12/2021 à 18:15, Steven Rostedt a écrit :
> On Mon, 13 Dec 2021 14:39:15 +0000
> Christophe Leroy <[email protected]> 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
On Mon, 13 Dec 2021 17:30:48 +0000
Christophe Leroy <[email protected]> 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
Le 13/12/2021 à 18:33, Steven Rostedt a écrit :
> On Mon, 13 Dec 2021 17:30:48 +0000
> Christophe Leroy <[email protected]> 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
On Mon, 13 Dec 2021 17:50:52 +0000
Christophe Leroy <[email protected]> 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(¤t->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
Le 13/12/2021 à 19:54, Steven Rostedt a écrit :
> On Mon, 13 Dec 2021 17:50:52 +0000
> Christophe Leroy <[email protected]> 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(¤t->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
On Mon, 13 Dec 2021 19:33:47 +0000
Christophe Leroy <[email protected]> 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
Le 13/12/2021 à 20:46, Steven Rostedt a écrit :
> On Mon, 13 Dec 2021 19:33:47 +0000
> Christophe Leroy <[email protected]> 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
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 <[email protected]> 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
On Tue, 14 Dec 2021 08:35:14 +0100
Christophe Leroy <[email protected]> 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
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 <[email protected]> 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?
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 <[email protected]> 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
Le 14/12/2021 à 15:01, Steven Rostedt a écrit :
> On Tue, 14 Dec 2021 08:35:14 +0100
> Christophe Leroy <[email protected]> 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