Re: [RFC] [PATCH 2.6.37-rc5-tip 7/20] 7: uprobes: store/restoreoriginal instruction.

From: Peter Zijlstra
Date: Tue Jan 25 2011 - 07:15:41 EST


On Thu, 2010-12-16 at 15:28 +0530, Srikar Dronamraju wrote:
> On the first probe insertion, copy the original instruction and opcode.
> If multiple vmas map the same text area corresponding to an inode, we
> only need to copy the instruction just once.
> The copied instruction is further copied to a designated slot on probe
> hit. Its also used at the time of probe removal to restore the original
> instruction.
> opcode is used to analyze the instruction and determine the fixups.
> Determining fixups at probe hit time would result in doing the same
> operation on every probe hit. Hence Instruction analysis using the
> opcode is done at probe insertion time.
>
> Signed-off-by: Srikar Dronamraju <srikar@xxxxxxxxxxxxxxxxxx>
> ---
> arch/Kconfig | 1 +
> kernel/uprobes.c | 61 ++++++++++++++++++++++++++++++++++++++++++++++++++----
> 2 files changed, 57 insertions(+), 5 deletions(-)
>
> diff --git a/arch/Kconfig b/arch/Kconfig
> index 6e8f26e..bba8108 100644
> --- a/arch/Kconfig
> +++ b/arch/Kconfig
> @@ -65,6 +65,7 @@ config UPROBES
> bool "User-space probes (EXPERIMENTAL)"
> depends on ARCH_SUPPORTS_UPROBES
> depends on MMU
> + select MM_OWNER
> help
> Uprobes enables kernel subsystems to establish probepoints
> in user applications and execute handler functions when
> diff --git a/kernel/uprobes.c b/kernel/uprobes.c
> index 8a5da38..858ddb1 100644
> --- a/kernel/uprobes.c
> +++ b/kernel/uprobes.c
> @@ -448,21 +448,72 @@ static int del_consumer(struct uprobe *uprobe,
> return ret;
> }
>
> +static int copy_insn(struct task_struct *tsk, unsigned long vaddr,
> + struct uprobe *uprobe)
> +{
> + int len;
> +
> + len = uprobes_read_vm(tsk, (void __user *)vaddr, uprobe->insn,
> + MAX_UINSN_BYTES);
> + if (len < uprobe_opcode_sz) {
> + print_insert_fail(tsk, vaddr,
> + "error reading original instruction");
> + return -EINVAL;
> + }
> + memcpy(&uprobe->opcode, uprobe->insn, uprobe_opcode_sz);
> + if (is_bkpt_insn(uprobe)) {
> + print_insert_fail(tsk, vaddr,
> + "breakpoint instruction already exists");
> + return -EEXIST;
> + }
> + if (analyze_insn(tsk, uprobe)) {
> + print_insert_fail(tsk, vaddr,
> + "instruction type cannot be probed");
> + return -EINVAL;
> + }
> + uprobe->copy = 1;
> + return 0;
> +}

Since you actually have the inode, you could read it from the
page-cache. Also, why do you have the whole opcode/insn thing, that
looks like its data duplication.

> static int install_uprobe(struct mm_struct *mm, struct uprobe *uprobe)
> {
> - int ret = 0;
> + struct task_struct *tsk;
> + int ret = -EINVAL;
>
> - /*TODO: install breakpoint */
> - if (!ret)
> + get_task_struct(mm->owner);
> + tsk = mm->owner;
> + if (!tsk)
> + return ret;
> +
> + if (!uprobe->copy) {
> + ret = copy_insn(tsk, mm->uprobes_vaddr, uprobe);
> + if (ret)
> + goto put_return;
> + }

So you do know that uprobes_vaddr can point to some random piece of
memory by now, right? :-)

> + ret = set_bkpt(tsk, mm->uprobes_vaddr);
> + if (ret < 0)
> + print_insert_fail(tsk, mm->uprobes_vaddr,
> + "failed to insert bkpt instruction");
> + else
> atomic_inc(&mm->uprobes_count);
> +
> +put_return:
> + put_task_struct(tsk);
> return ret;
> }
>
> static int remove_uprobe(struct mm_struct *mm, struct uprobe *uprobe)
> {
> - int ret = 0;
> + struct task_struct *tsk;
> + int ret;
> +
> + get_task_struct(mm->owner);
> + tsk = mm->owner;
> + if (!tsk)
> + return -EINVAL;
>
> - /*TODO: remove breakpoint */
> + ret = set_orig_insn(tsk, mm->uprobes_vaddr, true, uprobe);
> if (!ret)
> atomic_dec(&mm->uprobes_count);

Same here, there is no guarantee vaddr is even still mapped.


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/