Re: [PATCH 04/11] ftrace: comment arch ftrace code

[Andrew Morton <akpm@linux-foundation.org>]

On Wed, 22 Oct 2008 14:43:17 -0400
Steven Rostedt <rostedt@goodmis.org> wrote:

> Add comments to explain what is happening in the x86 arch ftrace code.
> 
> Signed-off-by: Steven Rostedt <srostedt@redhat.com>
> ---
>  arch/x86/kernel/ftrace.c |    7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> Index: linux-compile.git/arch/x86/kernel/ftrace.c
> ===================================================================
> --- linux-compile.git.orig/arch/x86/kernel/ftrace.c	2008-10-22 13:15:36.000000000 -0400
> +++ linux-compile.git/arch/x86/kernel/ftrace.c	2008-10-22 13:16:35.000000000 -0400
> @@ -66,18 +66,23 @@ ftrace_modify_code(unsigned long ip, uns
>  	/*
>  	 * 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
> +	 *  __copy_*_user functions.
>  	 *
>  	 * No real locking needed, this code is run through
>  	 * kstop_machine, or before SMP starts.
>  	 */
> +
> +	/* read the text we want to modify */
>  	if (__copy_from_user_inatomic(replaced, (char __user *)ip,
>  				      MCOUNT_INSN_SIZE))
>  		return -1;
>  
> +	/* Make sure it is what we expect it to be */
>  	if (memcmp(replaced, old_code, MCOUNT_INSN_SIZE) != 0)
>  		return -1;
>  
> +	/* replace the text with the new text */
>  	if (__copy_to_user_inatomic((char __user *)ip, new_code,
>  				    MCOUNT_INSN_SIZE))
>  		return -1;
> 

I dunno.

__copy_to_user_inatomic() is for "copying memory from userspace while
in an atomic context".

But what you're doing here is "modifying some kernel text which might
generate a fault".  It seems somewhat interface-abusive to use a
userspace access function for that just because it happens right now to
do the right thing.

I'd suggest that for clarity and for future-safety, you create some new
interface function which does that thing.  Right now it can be a simple
wrapper around __copy_from_user_inatomic().

<looks>

oh, someone added one - probe_kernel_write().  Why not use that?

<wonders why he doesn't know what's going on any more>


Also, I hope that the above code is called from within a
pagefault_disable()d region?  Or are relying upon some magical
side-effect of something which happens to do the same thing as
pagefault_disable()?  IOW: by what means does the above code ensure
that do_page_fault() will see in_atomic()==true?