lkml.org 
[lkml]   [2014]   [Jan]   [22]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH v6 7/8] x86: patch all traced function calls using the int3-based framework
From
Date
On Wed, 2014-01-15 at 10:47 -0500, Steven Rostedt wrote:
> On Tue, 10 Dec 2013 16:42:19 +0100
> Petr Mladek <pmladek@suse.cz> wrote:
>
> >
> > diff --git a/arch/x86/kernel/alternative.c b/arch/x86/kernel/alternative.c
> > index 03abf9abf681..e5c02af3a8cc 100644
> > --- a/arch/x86/kernel/alternative.c
> > +++ b/arch/x86/kernel/alternative.c
> > @@ -613,7 +613,7 @@ void __kprobes text_poke_or_die(void *addr, const void *opcode, size_t len)
> > * responsible for setting the patched code read-write. This is more effective
> > * only when you patch many addresses at the same time.
> > */
> > -static int text_poke_part(void *addr, const void *opcode, size_t len)
> > +static int notrace text_poke_part(void *addr, const void *opcode, size_t len)
>
> I can understand why you want to not trace any of these, but again, I
> hate using notrace unless it is absolutely necessary. That's what the
> set_ftrace_notrace is for. If you don't want to trace it, simply tell
> the kernel not to.

I removed "notrace" from all locations, except for "poke_int3_handler".
Then I tried to switch between 7 tracers, enable and disable tracing,
and repeated this 100 times. It slowed down from:

real 3m25.837s 3m29.280s 3m27.408s
user 0m0.004s 0m0.004s 0m0.000s
sys 0m3.532s 0m3.460s 0m3.416s

to

real 5m23.294s 5m24.944s 5m28.056s
user 0m0.004s 0m0.004s 0m0.004s
sys 0m3.076s 0m3.180s 0m3.396s

It means a change by 60%. As you know indeed, the reason is that the
functions used during patching are called via the int3 handler once the
breakpoint is added.

I thought about patching these functions separately, e.g. using separate
list or extra filters with two round patching. But I think that it is
not worth the extra complexity.

I agree that it might be worth to keep the functions traceable. So, if
the slowness is acceptable for you, I am fine with it as well.

> > {
> > int ret;
> >
> > @@ -662,7 +662,7 @@ static void *bp_int3_handler, *bp_int3_addr;
> > static size_t bp_int3_len;
> > static void *(*bp_int3_is_handled)(const unsigned long ip);
> >
> > -int poke_int3_handler(struct pt_regs *regs)
> > +int notrace poke_int3_handler(struct pt_regs *regs)
> > {
> > /* bp_patching_in_progress */
> > smp_rmb();
[...]
> > @@ -192,44 +162,10 @@ int ftrace_make_call(struct dyn_ftrace *rec, unsigned long addr)
> > old = ftrace_nop_replace();
> > new = ftrace_call_replace(ip, addr);
> >
> > - /* Should only be called when module is loaded */
> > - return ftrace_modify_code_direct(rec->ip, old, new);
> > + return ftrace_modify_code(ip, old, new);
> > }
> >
> > /*
> > - * The modifying_ftrace_code is used to tell the breakpoint
> > - * handler to call ftrace_int3_handler(). If it fails to
> > - * call this handler for a breakpoint added by ftrace, then
> > - * the kernel may crash.
> > - *
> > - * As atomic_writes on x86 do not need a barrier, we do not
> > - * need to add smp_mb()s for this to work. It is also considered
> > - * that we can not read the modifying_ftrace_code before
> > - * executing the breakpoint. That would be quite remarkable if
> > - * it could do that. Here's the flow that is required:
> > - *
> > - * CPU-0 CPU-1
> > - *
> > - * atomic_inc(mfc);
> > - * write int3s
> > - * <trap-int3> // implicit (r)mb
> > - * if (atomic_read(mfc))
> > - * call ftrace_int3_handler()
> > - *
> > - * Then when we are finished:
> > - *
> > - * atomic_dec(mfc);
> > - *
> > - * If we hit a breakpoint that was not set by ftrace, it does not
> > - * matter if ftrace_int3_handler() is called or not. It will
> > - * simply be ignored. But it is crucial that a ftrace nop/caller
> > - * breakpoint is handled. No other user should ever place a
> > - * breakpoint on an ftrace nop/caller location. It must only
> > - * be done by this code.
> > - */
>
> The above is a nice comment. It should be copied over to the
> alternative.c file for the explanation of bp_patching_in_progress.

Well, text_poke is using barriers instead of the atomic variable. I
think that the barriers have two functions in text_poke stuff:

1) make sure that "bp_patching_in_progress" is enabled before the
breakpoint is added; it means that "poke_int3_handler" is opened to
handle the address before the related trap happens.

2) make sure that the variables, e.g. bp_int3_handler, bp_int3_add, have
correct values before they are accessed in "poke_int3_handler".

I think that the atomic variable has only the 1st function. It does not
solve synchronization of the other variables. I thin that it was not a
problem in ftrace because it patched almost static list of addresses
but it might be problem in the generic text_poke_bp.

> > -atomic_t modifying_ftrace_code __read_mostly;
> > -
> > -/*
> > * Should never be called:
> > * As it is only called by __ftrace_replace_code() which is called by
> > * ftrace_replace_code() that x86 overrides, and by ftrace_update_code()


Best Regards,
Petr




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