lkml.org 
[lkml]   [2011]   [Aug]   [11]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH 5/5][RFC] kprobes: Use ftrace hooks when probing ftrace nops
    Hi Steven,

    As I suggested in another reply, I'm now attracted by the idea
    of using ftrace in kprobe-tracer, because of simplicity.
    Anyway, here is my review.

    (2011/08/11 1:22), Steven Rostedt wrote:
    > From: Steven Rostedt <srostedt@redhat.com>
    >
    > Currently, if a probe is set at an ftrace nop, the probe will fail.
    >
    > When -mfentry is used by ftrace (in the near future), the nop will be
    > at the beginning of the function. This will prevent kprobes from hooking
    > to the most common place that kprobes are attached.
    >
    > Now that ftrace supports individual users as well as pt_regs passing,
    > kprobes can use the ftrace infrastructure when a probe is placed on
    > a ftrace nop.

    My one general concern is the timing of enabling ftrace-based probe.
    Breakpoint-based kprobe (including optimized kprobe) is enabled
    right after registering. Users might expect that.
    And AFAIK, dynamic ftraces handler will be enabled (activated)
    after a while, because it has to wait for an NMI, doesn't it?

    And theoretically, this ftrace-based probe can't support jprobe,
    because it changes IP address. Nowadays, this may becomes minor
    problem (because someone who wants to trace function args can
    use kprobe-tracer), but still exist.


    > Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
    > ---
    > include/linux/kprobes.h | 6 ++
    > kernel/kprobes.c | 163 ++++++++++++++++++++++++++++++++++++++++++++---
    > 2 files changed, 160 insertions(+), 9 deletions(-)
    >
    > diff --git a/include/linux/kprobes.h b/include/linux/kprobes.h
    > index dd7c12e..5742071 100644
    > --- a/include/linux/kprobes.h
    > +++ b/include/linux/kprobes.h
    > @@ -37,6 +37,7 @@
    > #include <linux/spinlock.h>
    > #include <linux/rcupdate.h>
    > #include <linux/mutex.h>
    > +#include <linux/ftrace.h>
    >
    > #ifdef CONFIG_KPROBES
    > #include <asm/kprobes.h>
    > @@ -112,6 +113,11 @@ struct kprobe {
    > /* copy of the original instruction */
    > struct arch_specific_insn ainsn;
    >
    > +#ifdef CONFIG_FUNCTION_TRACER
    > + /* If it is possible to use ftrace to probe */
    > + struct ftrace_ops fops;
    > +#endif
    > +
    > /*
    > * Indicates various status flags.
    > * Protected by kprobe_mutex after this kprobe is registered.
    > diff --git a/kernel/kprobes.c b/kernel/kprobes.c
    > index e6c25eb..2160768 100644
    > --- a/kernel/kprobes.c
    > +++ b/kernel/kprobes.c
    > @@ -102,6 +102,31 @@ static struct kprobe_blackpoint kprobe_blacklist[] = {
    > {NULL} /* Terminator */
    > };
    >
    > +#ifdef CONFIG_FUNCTION_TRACER
    > +/* kprobe stub function for use when probe is not connected to ftrace */
    > +static void
    > +kprobe_ftrace_stub(unsigned long ip, unsigned long pip,
    > + struct ftrace_ops *op, struct pt_regs *pt_regs)
    > +{
    > +}
    > +
    > +static bool ftrace_kprobe(struct kprobe *p)
    > +{
    > + return p->fops.func && p->fops.func != kprobe_ftrace_stub;
    > +}
    > +
    > +static void init_non_ftrace_kprobe(struct kprobe *p)
    > +{
    > + p->fops.func = kprobe_ftrace_stub;
    > +}
    > +
    > +#else
    > +static bool ftrace_kprobe(struct kprobe *p)
    > +{
    > + return false;
    > +}
    > +#endif
    > +
    > #ifdef __ARCH_WANT_KPROBES_INSN_SLOT
    > /*
    > * kprobe->ainsn.insn points to the copy of the instruction to be
    > @@ -396,6 +421,9 @@ static __kprobes void free_aggr_kprobe(struct kprobe *p)
    > {
    > struct optimized_kprobe *op;
    >
    > + if (ftrace_kprobe(p))
    > + return;
    > +
    > op = container_of(p, struct optimized_kprobe, kp);
    > arch_remove_optimized_kprobe(op);
    > arch_remove_kprobe(p);
    > @@ -759,6 +787,9 @@ static __kprobes void try_to_optimize_kprobe(struct kprobe *p)
    > struct kprobe *ap;
    > struct optimized_kprobe *op;
    >
    > + if (ftrace_kprobe(p))
    > + return;
    > +
    > ap = alloc_aggr_kprobe(p);
    > if (!ap)
    > return;
    > @@ -849,6 +880,10 @@ static void __kprobes __arm_kprobe(struct kprobe *p)
    > {
    > struct kprobe *_p;
    >
    > + /* Only arm non-ftrace probes */
    > + if (ftrace_kprobe(p))
    > + return;
    > +
    > /* Check collision with other optimized kprobes */
    > _p = get_optimized_kprobe((unsigned long)p->addr);
    > if (unlikely(_p))
    > @@ -864,6 +899,10 @@ static void __kprobes __disarm_kprobe(struct kprobe *p,
    bool reopt)
    > {
    > struct kprobe *_p;
    >
    > + /* Only disarm non-ftrace probes */
    > + if (ftrace_kprobe(p))
    > + return;
    > +
    > unoptimize_kprobe(p, false); /* Try to unoptimize */
    >
    > if (!kprobe_queued(p)) {
    > @@ -878,13 +917,26 @@ static void __kprobes __disarm_kprobe(struct kprobe *p,
    bool reopt)
    >
    > #else /* !CONFIG_OPTPROBES */
    >
    > +static void __kprobes __arm_kprobe(struct kprobe *p)
    > +{
    > + /* Only arm non-ftrace probes */
    > + if (!ftrace_kprobe(p))
    > + arch_arm_kprobe(p);
    > +}
    > +
    > +/* Remove the breakpoint of a probe. Must be called with text_mutex locked */
    > +static void __kprobes __disarm_kprobe(struct kprobe *p, bool reopt)
    > +{
    > + /* Only disarm non-ftrace probes */
    > + if (!ftrace_kprobe(p))
    > + arch_disarm_kprobe(p);
    > +}

    If it ignores disabling/enabling, kprobe_ftrace_callback must
    check kprobe_disabled(p) and skip it.

    > +
    > #define optimize_kprobe(p) do {} while (0)
    > #define unoptimize_kprobe(p, f) do {} while (0)
    > #define kill_optimized_kprobe(p) do {} while (0)
    > #define prepare_optimized_kprobe(p) do {} while (0)
    > #define try_to_optimize_kprobe(p) do {} while (0)
    > -#define __arm_kprobe(p) arch_arm_kprobe(p)
    > -#define __disarm_kprobe(p, o) arch_disarm_kprobe(p)
    > #define kprobe_disarmed(p) kprobe_disabled(p)
    > #define wait_for_kprobe_optimizer() do {} while (0)
    >
    > @@ -897,7 +949,9 @@ static void reuse_unused_kprobe(struct kprobe *ap)
    >
    > static __kprobes void free_aggr_kprobe(struct kprobe *p)
    > {
    > - arch_remove_kprobe(p);
    > +
    > + if (!ftrace_kprobe(p))
    > + arch_remove_kprobe(p);
    > kfree(p);
    > }
    >
    > @@ -910,6 +964,12 @@ static __kprobes struct kprobe *alloc_aggr_kprobe(struct
    kprobe *p)
    > /* Arm a kprobe with text_mutex */
    > static void __kprobes arm_kprobe(struct kprobe *kp)
    > {
    > + /* ftrace probes can skip arch calls */
    > + if (ftrace_kprobe(kp)) {
    > + register_ftrace_function(&kp->fops);
    > + return;
    > + }
    > +
    > /*
    > * Here, since __arm_kprobe() doesn't use stop_machine(),
    > * this doesn't cause deadlock on text_mutex. So, we don't
    > @@ -924,6 +984,12 @@ static void __kprobes arm_kprobe(struct kprobe *kp)
    > static void __kprobes disarm_kprobe(struct kprobe *kp)
    > {
    > /* Ditto */
    > +
    > + if (ftrace_kprobe(kp)) {
    > + unregister_ftrace_function(&kp->fops);
    > + return;
    > + }
    > +
    > mutex_lock(&text_mutex);
    > __disarm_kprobe(kp, true);
    > mutex_unlock(&text_mutex);
    > @@ -1313,6 +1379,56 @@ static inline int check_kprobe_rereg(struct kprobe *p)
    > return ret;
    > }
    >
    > +#ifdef CONFIG_FUNCTION_TRACER
    > +static notrace void
    > +kprobe_ftrace_callback(unsigned long ip, unsigned long parent_ip,
    > + struct ftrace_ops *op, struct pt_regs *pt_regs)
    > +{
    > + struct kprobe *p = container_of(op, struct kprobe, fops);
    > +

    Here, we need to set up kprobe_ctlblk and some of pt_regs members,
    ip, cs and orig_ax as optimized_callback()@arch/x86/kernel/kprobes.c
    does.

    > + /* A nop has been trapped, just run both handlers back to back */
    > + if (p->pre_handler)
    > + p->pre_handler(p, pt_regs);

    And increment regs->ip here for NOP.

    > + if (p->post_handler)
    > + p->post_handler(p, pt_regs, 0);
    > +}

    Anyway, above operations strongly depends on arch, so
    kprobe_ftrace_callback should be moved to arch/*/kprobes.c.
    And I think most of the code can be shared with optimized code.

    > +
    > +static int use_ftrace_hook(struct kprobe *p)
    > +{
    > + char str[KSYM_SYMBOL_LEN];
    > +
    > + /* see if this is an ftrace function */
    > + if (!ftrace_text_reserved(p->addr, p->addr)) {
    > + /* make sure fops->func is nop */
    > + init_non_ftrace_kprobe(p);
    > + return 0;
    > + }
    > +
    > + /* If arch does not support pt_regs passing, bail */
    > + if (!ARCH_SUPPORTS_FTRACE_SAVE_REGS)
    > + return -EINVAL;

    Hmm, I think this should be checked at build time...

    > +
    > + /* Use ftrace hook instead */
    > +
    > + memset(&p->fops, 0, sizeof(p->fops));
    > +
    > + /* Find the function this is connected with this addr */
    > + kallsyms_lookup((unsigned long)p->addr, NULL, NULL, NULL, str);
    > +
    > + p->fops.flags = FTRACE_OPS_FL_SAVE_REGS;
    > + p->fops.func = kprobe_ftrace_callback;
    > +
    > + ftrace_set_filter(&p->fops, str, strlen(str), 1);

    Hmm, IMHO, ftrace_set_filter should not be called here, because
    there can be other kprobes are already registered on the same
    address. In that case, it is natural that we use an aggr_kprobe
    for handling several kprobes on same address. Or, kprobe hash table
    will have several different probes on same address.

    > +
    > + return 0;
    > +}
    > +#else
    > +static int use_ftrace_hook(struct kprobe *p)
    > +{
    > + return 0;
    > +}
    > +#endif
    > +
    > int __kprobes register_kprobe(struct kprobe *p)
    > {
    > int ret = 0;
    > @@ -1329,11 +1445,14 @@ int __kprobes register_kprobe(struct kprobe *p)
    > if (ret)
    > return ret;
    >
    > + ret = use_ftrace_hook(p);
    > + if (ret)
    > + return ret;
    > +
    > jump_label_lock();
    > preempt_disable();
    > if (!kernel_text_address((unsigned long) p->addr) ||
    > in_kprobes_functions((unsigned long) p->addr) ||
    > - ftrace_text_reserved(p->addr, p->addr) ||
    > jump_label_text_reserved(p->addr, p->addr))
    > goto fail_with_jump_label;
    >
    > @@ -1384,15 +1503,17 @@ int __kprobes register_kprobe(struct kprobe *p)
    > goto out;
    > }
    >
    > - ret = arch_prepare_kprobe(p);
    > - if (ret)
    > - goto out;
    > + if (!ftrace_kprobe(p)) {
    > + ret = arch_prepare_kprobe(p);
    > + if (ret)
    > + goto out;
    > + }
    >
    > INIT_HLIST_NODE(&p->hlist);
    > hlist_add_head_rcu(&p->hlist,
    > &kprobe_table[hash_ptr(p->addr, KPROBE_HASH_BITS)]);
    >
    > - if (!kprobes_all_disarmed && !kprobe_disabled(p))
    > + if (!ftrace_kprobe(p) && !kprobes_all_disarmed && !kprobe_disabled(p))
    > __arm_kprobe(p);
    >
    > /* Try to optimize kprobe */
    > @@ -1400,6 +1521,12 @@ int __kprobes register_kprobe(struct kprobe *p)
    >
    > out:
    > mutex_unlock(&text_mutex);
    > +
    > + /* ftrace kprobes must be set outside of text_mutex */
    > + if (!ret && ftrace_kprobe(p) &&
    > + !kprobes_all_disarmed && !kprobe_disabled(p))
    > + arm_kprobe(p);
    > +
    > put_online_cpus();
    > jump_label_unlock();
    > mutex_unlock(&kprobe_mutex);

    After this, we must handle some fails which can happen when probing
    on a module.


    > @@ -2134,6 +2261,14 @@ static void __kprobes arm_all_kprobes(void)
    > }
    > mutex_unlock(&text_mutex);
    >
    > + /* ftrace kprobes are enabled outside of text_mutex */
    > + for (i = 0; i < KPROBE_TABLE_SIZE; i++) {
    > + head = &kprobe_table[i];
    > + hlist_for_each_entry_rcu(p, node, head, hlist)
    > + if (ftrace_kprobe(p) && !kprobe_disabled(p))
    > + arm_kprobe(p);
    > + }
    > +
    > kprobes_all_disarmed = false;
    > printk(KERN_INFO "Kprobes globally enabled\n");
    >
    > @@ -2164,11 +2299,21 @@ static void __kprobes disarm_all_kprobes(void)
    > for (i = 0; i < KPROBE_TABLE_SIZE; i++) {
    > head = &kprobe_table[i];
    > hlist_for_each_entry_rcu(p, node, head, hlist) {
    > - if (!arch_trampoline_kprobe(p) && !kprobe_disabled(p))
    > + if (!ftrace_kprobe(p) &&
    > + !arch_trampoline_kprobe(p) && !kprobe_disabled(p))
    > __disarm_kprobe(p, false);
    > }
    > }
    > mutex_unlock(&text_mutex);
    > +
    > + /* ftrace kprobes are disabled outside of text_mutex */
    > + for (i = 0; i < KPROBE_TABLE_SIZE; i++) {
    > + head = &kprobe_table[i];
    > + hlist_for_each_entry_rcu(p, node, head, hlist) {
    > + if (ftrace_kprobe(p) && !kprobe_disabled(p))
    > + disarm_kprobe(p);
    > + }
    > + }
    > mutex_unlock(&kprobe_mutex);
    >
    > /* Wait for disarming all kprobes by optimizer */

    Thank you,

    --
    Masami HIRAMATSU
    Software Platform Research Dept. Linux Technology Center
    Hitachi, Ltd., Yokohama Research Laboratory
    E-mail: masami.hiramatsu.pt@hitachi.com


    \
     
     \ /
      Last update: 2011-08-11 09:43    [from the cache]
    ©2003-2014 Jasper Spaans. Advertise on this site