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

[Peter Zijlstra <peterz@infradead.org>]

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@linux.vnet.ibm.com>
> ---
>  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.