Messages in this thread Patch in this message | | | Date | Tue, 14 Jan 2014 20:04:45 -0500 | From | Steven Rostedt <> | Subject | Re: [PATCH v6 6/8] x86: modify ftrace function using the new int3-based framework |
| |
On Tue, 10 Dec 2013 16:42:18 +0100 Petr Mladek <pmladek@suse.cz> wrote:
> The commit fd4363fff3d9 (x86: Introduce int3 (breakpoint)-based instruction > patching) uses the same technique that has been used in ftrace since 08d636b > ("ftrace/x86: Have arch x86_64 use breakpoints instead of stop machine") > > It would be great to use the new generic implementation and clean up the x86 > ftrace code a bit. > > Let's start with ftrace_modify_code. It does basically the same as text_poke_bp. > In addition, it checks whether the replaced code is the expected one to avoid > problems with modules. The checking code can be split from > ftrace_modify_code_direct. > > Note that we do not longer need to set modifying_ftrace_code in > ftrace_update_ftrace_func. The int3 interrupt will be handled by > poke_int3_handler. > > Signed-off-by: Petr Mladek <pmladek@suse.cz> > --- > arch/x86/kernel/ftrace.c | 120 +++++++++++++++++++---------------------------- > 1 file changed, 47 insertions(+), 73 deletions(-) > > diff --git a/arch/x86/kernel/ftrace.c b/arch/x86/kernel/ftrace.c > index d4bdd253fea7..5ade40e4a175 100644 > --- a/arch/x86/kernel/ftrace.c > +++ b/arch/x86/kernel/ftrace.c > @@ -52,6 +52,33 @@ union ftrace_code_union { > } __attribute__((packed)); > }; > > +/* > + * Note: Due to modules and __init, code can > + * disappear and change, we need to protect against faulting > + * as well as code changing. We do this by using the > + * probe_kernel_* functions.
Actually the above comment is no longer accurate. I know that you just copied it from my code, and this is my fault. But instead of keeping an inaccurate comment around, lets fix it.
/* * Note: Modifying kernel code is extremely critical to get right. * We must try to detect any error, and we should only change code * with code that we expect to be changed. Always test that the old * text is what we expect it to be before we modify it, and fail * if it is not. */
> + */ > +static int > +ftrace_check_code(unsigned long ip, unsigned const char *expected) > +{ > + unsigned char actual[MCOUNT_INSN_SIZE]; > + > + if (probe_kernel_read(actual, (void *)ip, MCOUNT_INSN_SIZE)) > + return -EFAULT; > + > + if (memcmp(actual, expected, MCOUNT_INSN_SIZE) != 0) { > + WARN_ONCE(1, > + "ftrace check failed: %x %x%x%x%x vs. %x %x%x%x%x\n", > + actual[0], > + actual[1], actual[2], actual[3], actual[4], > + expected[0], > + expected[1], expected[2], expected[3], expected[4]); > + return -EINVAL;
Hmm, I guess I'm fine with warning here like this, but it is also covered in kernel/ftrace.c too, with the return code o -EINVAL.
But duplicate warnings may not be that bad.
> + } > + > + return 0; > +} > + > static int ftrace_calc_offset(long ip, long addr) > { > return (int)(addr - ip); > @@ -103,28 +130,12 @@ static int > ftrace_modify_code_direct(unsigned long ip, unsigned const char *old_code, > unsigned const char *new_code) > { > - 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. We do this by using the > - * probe_kernel_* functions. > - * > - * No real locking needed, this code is run through > - * kstop_machine, or before SMP starts. > - */ > + int ret; > > - /* 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; > + ret = ftrace_check_code(ip, old_code);
Wait? If this fails, why not return with ret? Otherwise, we just returned with success!
-- Steve
> > /* replace the text with the new text */ > - if (do_ftrace_mod_code(ip, new_code)) > + if (!ret && do_ftrace_mod_code(ip, new_code)) > return -EPERM; > > sync_core(); > @@ -132,6 +143,22 @@ ftrace_modify_code_direct(unsigned long ip, unsigned const char *old_code, > return 0; > } > > +static int > +ftrace_modify_code(unsigned long ip, unsigned const char *old_code, > + unsigned const char *new_code) > +{ > + int ret; > + > + ret = ftrace_check_code(ip, old_code); > + > + /* replace the text with the new text */ > + if (!ret) > + ret = text_poke_bp((void *)ip, new_code, MCOUNT_INSN_SIZE, > + (void *)ip + MCOUNT_INSN_SIZE); > + > + return ret; > +} > +
| |