| Message ID | 20081116212515.728019601@goodmis.org (mailing list archive) |
|---|---|
| State | Accepted, archived |
| Commit | e4486fe316895e87672a563c4f36393218f84ff1 |
| Headers | show |
Steven Rostedt writes: > Impact: use cleaner probe_kernel API over assembly > > Using probe_kernel_read/write interface is a much cleaner approach > than the current assembly version. Possibly naive question: how is it possible for the accesses to the instructions to fault, given that we are called through kstop_machine (according to the comment) and nothing else should be happening? Paul.
On Mon, 17 Nov 2008, Paul Mackerras wrote: > Steven Rostedt writes: > > > Impact: use cleaner probe_kernel API over assembly > > > > Using probe_kernel_read/write interface is a much cleaner approach > > than the current assembly version. > > Possibly naive question: how is it possible for the accesses to the > instructions to fault, given that we are called through kstop_machine > (according to the comment) and nothing else should be happening? Actually, only start up and shutdown of the tracer goes through kstop_machine. With the new code, it can happen before SMP starts (in init/main.c) or on the module itself, before the module loads. But that's not why we do the probe_kernel_* calls. The reason for probe_kernel_ is because ftrace is very intrusive. Ingo and I have been making ftrace very paranoid about anything it does. It does not trust itself to be correct. If anything were to fail, it will warn and shut itself down peacefully. So far, I have not seen these warnings, but this is part of the robustness features that have been added since 2.6.27. -- Steve
diff --git a/arch/powerpc/kernel/ftrace.c b/arch/powerpc/kernel/ftrace.c index aa5d0b2..3852919 100644 --- a/arch/powerpc/kernel/ftrace.c +++ b/arch/powerpc/kernel/ftrace.c @@ -9,6 +9,7 @@ #include <linux/spinlock.h> #include <linux/hardirq.h> +#include <linux/uaccess.h> #include <linux/ftrace.h> #include <linux/percpu.h> #include <linux/init.h> @@ -72,45 +73,33 @@ static int ftrace_modify_code(unsigned long ip, unsigned char *old_code, unsigned char *new_code) { - unsigned replaced; - unsigned old = *(unsigned *)old_code; - unsigned new = *(unsigned *)new_code; - int faulted = 0; + unsigned char replaced[MCOUNT_INSN_SIZE]; /* * Note: Due to modules and __init, code can * disappear and change, we need to protect against faulting - * as well as code changing. + * as well as code changing. We do this by using the + * probe_kernel_* functions. * * No real locking needed, this code is run through - * kstop_machine. + * kstop_machine, or before SMP starts. */ - asm volatile ( - "1: lwz %1, 0(%2)\n" - " cmpw %1, %5\n" - " bne 2f\n" - " stwu %3, 0(%2)\n" - "2:\n" - ".section .fixup, \"ax\"\n" - "3: li %0, 1\n" - " b 2b\n" - ".previous\n" - ".section __ex_table,\"a\"\n" - _ASM_ALIGN "\n" - _ASM_PTR "1b, 3b\n" - ".previous" - : "=r"(faulted), "=r"(replaced) - : "r"(ip), "r"(new), - "0"(faulted), "r"(old) - : "memory"); - - if (replaced != old && replaced != new) - faulted = 2; - - if (!faulted) - flush_icache_range(ip, ip + 8); - - return faulted; + + /* read the text we want to modify */ + if (probe_kernel_read(replaced, (void *)ip, MCOUNT_INSN_SIZE)) + return -EFAULT; + + /* Make sure it is what we expect it to be */ + if (memcmp(replaced, old_code, MCOUNT_INSN_SIZE) != 0) + return -EINVAL; + + /* replace the text with the new text */ + if (probe_kernel_write((void *)ip, new_code, MCOUNT_INSN_SIZE)) + return -EPERM; + + flush_icache_range(ip, ip + 8); + + return 0; } static int test_24bit_addr(unsigned long ip, unsigned long addr)
Impact: use cleaner probe_kernel API over assembly Using probe_kernel_read/write interface is a much cleaner approach than the current assembly version. Signed-off-by: Steven Rostedt <srostedt@redhat.com> --- arch/powerpc/kernel/ftrace.c | 53 ++++++++++++++++------------------------- 1 files changed, 21 insertions(+), 32 deletions(-)