* Oleg Nesterov <o...@redhat.com> [2012-09-14 19:15:57]:
> If handle_swbp()->add_utask() fails but UPROBE_SKIP_SSTEP is set,
> cleanup_ret: path do not restart the insn, this is wrong. Remove
> this check and add the additional label for can_skip_sstep() = T
> case.
>
> Note also that UPROBE_SKIP_SSTEP can be false positive, we simply
> can not trust it unless arch_uprobe_skip_sstep() was already called.
>
> Also, move another UPROBE_SKIP_SSTEP check before can_skip_sstep()
> into this helper, this looks more clean and understandable.
>
> Note: probably we should rename "skip" to "emulate" and I think
yes we can rename can_skip_step to can_emulate_insn and
arch_uprobe_skip_step() to arch_uprobe_emulate_insn
Similarly UPROBE_SKIP_SSTEP can be renamed as UPROBE_EMULATE_INSN
> that "clear UPROBE_SKIP_SSTEP" should be moved to arch_can_skip.
>
Currently struct uprobe is not exposed to arch specific code as
suggested by Ingo. Adding a flag in arch_uprobe just for this and
expecting all archs to define one is probably an overhead.
Hence I am not sure moving the clear flag to arch is a good idea.
> Signed-off-by: Oleg Nesterov <o...@redhat.com>
Acked-by: Srikar Dronamraju <sri...@linux.vnet.ibm.com>
> ---
> kernel/events/uprobes.c | 31 +++++++++++++++----------------
> 1 files changed, 15 insertions(+), 16 deletions(-)
>
> diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
> index 9893cba..403d2e0 100644
> --- a/kernel/events/uprobes.c
> +++ b/kernel/events/uprobes.c
> @@ -1389,10 +1389,11 @@ bool uprobe_deny_signal(void)
> */
> static bool can_skip_sstep(struct uprobe *uprobe, struct pt_regs *regs)
> {
> - if (arch_uprobe_skip_sstep(&uprobe->arch, regs))
> - return true;
> -
> - uprobe->flags &= ~UPROBE_SKIP_SSTEP;
> + if (uprobe->flags & UPROBE_SKIP_SSTEP) {
> + if (arch_uprobe_skip_sstep(&uprobe->arch, regs))
> + return true;
> + uprobe->flags &= ~UPROBE_SKIP_SSTEP;
> + }
> return false;
> }
>
> @@ -1494,12 +1495,12 @@ static void handle_swbp(struct pt_regs *regs)
> utask = add_utask();
> /* Cannot allocate; re-execute the instruction. */
> if (!utask)
> - goto cleanup_ret;
> + goto restart;
> }
>
> handler_chain(uprobe, regs);
> - if (uprobe->flags & UPROBE_SKIP_SSTEP && can_skip_sstep(uprobe, regs))
> - goto cleanup_ret;
> + if (can_skip_sstep(uprobe, regs))
> + goto out;
>
> if (!pre_ssout(uprobe, regs, bp_vaddr)) {
> arch_uprobe_enable_step(&uprobe->arch);
> @@ -1508,15 +1509,13 @@ static void handle_swbp(struct pt_regs *regs)
> return;
> }
>
> -cleanup_ret:
> - if (!(uprobe->flags & UPROBE_SKIP_SSTEP))
> -
> - /*
> - * cannot singlestep; cannot skip instruction;
> - * re-execute the instruction.
> - */
> - instruction_pointer_set(regs, bp_vaddr);
> -
> +restart:
> + /*
> + * cannot singlestep; cannot skip instruction;
> + * re-execute the instruction.
> + */
> + instruction_pointer_set(regs, bp_vaddr);
> +out:
> put_uprobe(uprobe);
> }
>
> --
> 1.5.5.1
>
--
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/
