(2014/04/01 4:43), Oleg Nesterov wrote:
> UPROBE_COPY_INSN, UPROBE_SKIP_SSTEP, and uprobe->flags must die. This
> patch kills UPROBE_SKIP_SSTEP. I never understood why it was added;
> not only it doesn't help, it harms.
>
> It can only help to avoid arch_uprobe_skip_sstep() if it was already
> called before and failed. But this is ugly, if we want to know whether
> we can emulate this instruction or not we should do this analysis in
> arch_uprobe_analyze_insn(), not when we hit this probe for the first
> time.
>
> And in fact this logic is simply wrong. arch_uprobe_skip_sstep() can
> fail or not depending on the task/register state, if this insn can be
> emulated but, say, put_user() fails we need to xol it this time, but
> this doesn't mean we shouldn't try to emulate it when this or another
> thread hist this bp next time.
>
> And this is the actual reason for this change. We need to emulate the
> "call" insn, but push(return-address) can obviously fail.
>
> Per-arch notes:
>
> x86: __skip_sstep() can only emulate "rep;nop". With this
> change it will be called every time and most probably
> for no reason.
>
> This will be fixed by the next changes. We need to
> change this suboptimal code anyway.
>
> arm: Should not be affected. It has its own "bool simulate"
> flag checked in arch_uprobe_skip_sstep().
>
> ppc: Looks like, it can emulate almost everything. Does it
> actually needs to record the fact that emulate_step()
> failed? Hopefully not. But if yes, it can add the ppc-
> specific flag into arch_uprobe.
>
> TODO: rename arch_uprobe_skip_sstep() to arch_uprobe_emulate_insn(),
>
> Signed-off-by: Oleg Nesterov <o...@redhat.com>
Looks reasonable to me.
Reviewed-by: Masami Hiramatsu <masami.hiramatsu...@hitachi.com>
Thank you,
> ---
> kernel/events/uprobes.c | 23 ++---------------------
> 1 files changed, 2 insertions(+), 21 deletions(-)
>
> diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
> index 307d87c..7a3e14e 100644
> --- a/kernel/events/uprobes.c
> +++ b/kernel/events/uprobes.c
> @@ -60,8 +60,6 @@ static struct percpu_rw_semaphore dup_mmap_sem;
>
> /* Have a copy of original instruction */
> #define UPROBE_COPY_INSN 0
> -/* Can skip singlestep */
> -#define UPROBE_SKIP_SSTEP 1
>
> struct uprobe {
> struct rb_node rb_node; /* node in the rb tree */
> @@ -491,12 +489,9 @@ static struct uprobe *alloc_uprobe(struct inode *inode,
> loff_t offset)
> uprobe->offset = offset;
> init_rwsem(&uprobe->register_rwsem);
> init_rwsem(&uprobe->consumer_rwsem);
> - /* For now assume that the instruction need not be single-stepped */
> - __set_bit(UPROBE_SKIP_SSTEP, &uprobe->flags);
>
> /* add to uprobes_tree, sorted on inode:offset */
> cur_uprobe = insert_uprobe(uprobe);
> -
> /* a uprobe exists for this inode:offset combination */
> if (cur_uprobe) {
> kfree(uprobe);
> @@ -1628,20 +1623,6 @@ bool uprobe_deny_signal(void)
> return true;
> }
>
> -/*
> - * Avoid singlestepping the original instruction if the original instruction
> - * is a NOP or can be emulated.
> - */
> -static bool can_skip_sstep(struct uprobe *uprobe, struct pt_regs *regs)
> -{
> - if (test_bit(UPROBE_SKIP_SSTEP, &uprobe->flags)) {
> - if (arch_uprobe_skip_sstep(&uprobe->arch, regs))
> - return true;
> - clear_bit(UPROBE_SKIP_SSTEP, &uprobe->flags);
> - }
> - return false;
> -}
> -
> static void mmf_recalc_uprobes(struct mm_struct *mm)
> {
> struct vm_area_struct *vma;
> @@ -1859,13 +1840,13 @@ static void handle_swbp(struct pt_regs *regs)
> goto out;
>
> handler_chain(uprobe, regs);
> - if (can_skip_sstep(uprobe, regs))
> + if (arch_uprobe_skip_sstep(&uprobe->arch, regs))
> goto out;
>
> if (!pre_ssout(uprobe, regs, bp_vaddr))
> return;
>
> - /* can_skip_sstep() succeeded, or restart if can't singlestep */
> + /* arch_uprobe_skip_sstep() succeeded, or restart if can't singlestep */
> out:
> put_uprobe(uprobe);
> }
>
--
Masami HIRAMATSU
IT Management Research Dept. Linux Technology Center
Hitachi, Ltd., Yokohama Research Laboratory
E-mail: masami.hiramatsu...@hitachi.com
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
