lkml.org 
[lkml]   [2010]   [Mar]   [23]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    SubjectRe: [PATCH v1 7/10] Uprobes Implementation
    From
    Date
    On Sat, 2010-03-20 at 19:56 +0530, Srikar Dronamraju wrote:
    > +struct uprobe {
    > + /*
    > + * The pid of the probed process. Currently, this can be the
    > + * thread ID (task->pid) of any active thread in the process.
    > + */
    > + pid_t pid;
    > +
    > + /* Location of the probepoint */
    > + unsigned long vaddr;
    > +
    > + /* Handler to run when the probepoint is hit */
    > + void (*handler)(struct uprobe*, struct pt_regs*);
    > +
    > + /* true if handler runs in interrupt context*/
    > + bool handler_in_interrupt;
    > +};

    I would still prefer to see something like:

    vma:offset, instead of tid:vaddr

    You want to probe a symbol in a DSO, filtering per-task comes after that
    if desired.

    Also, like we discussed in person, I think we can do away with the
    handler_in_interrupt thing by letting the handler have an error return
    value and doing something like:

    do_int3:

    uprobe = find_probe_point(addr);

    pagefault_disable();
    err = uprobe->handler(uprobe, regs);
    pagefault_enable();

    if (err == -EFAULT) {
    /* set TIF flag and call the handler again from
    task context */
    }

    This should allow the handler to optimistically access memory from the
    trap handler, but in case it does need to fault pages in we'll call it
    from task context.

    > diff --git a/include/linux/sched.h b/include/linux/sched.h
    > index dad7f66..2d2433a 100644
    > --- a/include/linux/sched.h
    > +++ b/include/linux/sched.h
    > @@ -1506,6 +1506,9 @@ struct task_struct {
    > unsigned long memsw_bytes; /* uncharged mem+swap usage */
    > } memcg_batch;
    > #endif
    > +#ifdef CONFIG_UPROBES
    > + struct uprobe_task *utask;
    > +#endif
    > };
    >
    > /* Future-safe accessor for struct task_struct's cpus_allowed. */
    > diff --git a/include/linux/tracehook.h b/include/linux/tracehook.h
    > index 10db010..9a91d1e 100644
    > --- a/include/linux/tracehook.h
    > +++ b/include/linux/tracehook.h
    > @@ -49,6 +49,7 @@
    > #include <linux/sched.h>
    > #include <linux/ptrace.h>
    > #include <linux/security.h>
    > +#include <linux/uprobes.h>
    > struct linux_binprm;
    >
    > /**
    > @@ -204,6 +205,11 @@ static inline void tracehook_report_exec(struct linux_binfmt *fmt,
    > if (!ptrace_event(PT_TRACE_EXEC, PTRACE_EVENT_EXEC, 0) &&
    > unlikely(task_ptrace(current) & PT_PTRACED))
    > send_sig(SIGTRAP, current, 0);
    > +
    > +#ifdef CONFIG_UPROBES
    > + if (unlikely(current->utask))
    > + uprobe_free_utask();
    > +#endif
    > }
    >
    > /**
    > @@ -219,6 +225,10 @@ static inline void tracehook_report_exec(struct linux_binfmt *fmt,
    > static inline void tracehook_report_exit(long *exit_code)
    > {
    > ptrace_event(PT_TRACE_EXIT, PTRACE_EVENT_EXIT, *exit_code);
    > +#ifdef CONFIG_UPROBES
    > + if (unlikely(current->utask))
    > + uprobe_free_utask();
    > +#endif
    > }
    >
    > /**
    > @@ -293,6 +303,10 @@ static inline void tracehook_report_clone(struct pt_regs *regs,
    > sigaddset(&child->pending.signal, SIGSTOP);
    > set_tsk_thread_flag(child, TIF_SIGPENDING);
    > }
    > +#ifdef CONFIG_UPROBES
    > + if (unlikely(current->utask))
    > + uprobe_handle_clone(clone_flags, child);
    > +#endif
    > }
    >
    > /**
    > @@ -593,6 +607,10 @@ static inline void set_notify_resume(struct task_struct *task)
    > */
    > static inline void tracehook_notify_resume(struct pt_regs *regs)
    > {
    > +#ifdef CONFIG_UPROBES
    > + if (current->utask && current->utask->active_ppt)
    > + uprobe_notify_resume(regs);
    > +#endif
    > }
    > #endif /* TIF_NOTIFY_RESUME */


    Everybody else simply places callbacks in kernel/fork.c and
    kernel/exit.c, but as it is I don't think you want per-task state like
    this.

    One thing I would like to see is a slot per task, that has a number of
    advantages over the current patch-set in that it doesn't have one page
    limit in number of probe sites, nor do you need to insert vmas into each
    and every address space that happens to have your DSO mapped.

    Also, I would simply kill the user_bkpt stuff and merge it into uprobes,
    we don't have a kernel_bkpt thing either, only kprobes.



    \
     
     \ /
      Last update: 2010-03-23 12:05    [W:0.026 / U:32.632 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site