| Message ID | cover.1635423081.git.christophe.leroy@csgroup.eu (mailing list archive) |
|---|---|
| Headers | show |
| Series | Implement livepatch on PPC32 | expand |
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
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
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
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
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 <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
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
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
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
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
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(¤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 <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(¤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 <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
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
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
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
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?
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
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