lkml.org 
[lkml]   [2014]   [Jan]   [14]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
Date
From
SubjectRe: [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;
> +}
> +


\
 
 \ /
  Last update: 2014-01-15 02:21    [W:0.164 / U:0.180 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site