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

Andrew Morton
2008-10-22T19:10:33Z

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?