Re: [PATCH v6 6/8] x86: modify ftrace function using the new int3-based framework

[Steven Rostedt <rostedt@goodmis.org>]

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;
> +}
> +