Re: [PATCH 04/11] ftrace: comment arch ftrace code
From: Andrew Morton
Date: Wed Oct 22 2008 - 15:10:34 EST
On Wed, 22 Oct 2008 14:43:17 -0400
Steven Rostedt <rostedt@xxxxxxxxxxx> wrote:
> Add comments to explain what is happening in the x86 arch ftrace code.
> Signed-off-by: Steven Rostedt <srostedt@xxxxxxxxxx>
> 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,
> 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,
> return -1;
__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().
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?
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/