Re: [RFC][PATCH] PPC64: return address probes

[Jim Keniston <jkenisto at us dot ibm dot com>]

On Fri, 2005-05-20 at 05:15, Ananth N Mavinakayanahalli wrote:
> Hi,
> 
> Here is the return address probes prototype for ppc64. This patch
> applies on 2.6.12-rc4-mm2 + Rusty's arch_break_inst.patch and Hien's
> kprobe_flush.patch.
...
> Comments welcome!
> 
> Ananth
> 
> ______________________________________________________________________
> +
> +void arch_kprobe_flush_task(struct task_struct *tk)
> +{
> +	struct kretprobe_instance *ri;
> +
> +	while ((ri = get_rp_inst_tsk(tk)) != NULL) {
> +		*(ri->arch_retaddr.linkreg_ptr) = (unsigned long)ri->ret_addr;
> +		recycle_rp_inst(ri);
> +	}
> +}
> +
> +void arch_prepare_kretprobe(struct kretprobe *rp, struct pt_regs *regs)
> +{
> +	struct kretprobe_instance *ri;
> +	static void *orig_ret_addr;
> +
> +	/* Save the return address when this hits the first time */
> +	if (regs->link != (unsigned long)kretprobe_trampoline)
> +		orig_ret_addr = (void *)regs->link;
> +
> +	if ((ri = get_free_rp_inst(rp)) != NULL) {
> +		ri->rp = rp;
> +		ri->stack_addr = (void *)regs->gpr[1];
> +		ri->ret_addr = orig_ret_addr;
> +		ri->arch_retaddr.linkreg_ptr =
> +			(kprobe_opcode_t *)&regs->link;

This looks wrong to me.  It seems to me that &regs->link is in the
middle of the pt_regs struct, which will disappear as soon as we return
from the trap.  To be of use to arch_kprobe_flush_task(),
ri->arch_retaddr.linkreg_ptr should point to the place in the
yet-to-be-constructed stack frame of the probed function where the link
register will be saved.

If this location is always predictable, and does indeed turn out to be
equal to &regs->link, then you should add a comment explaining the
coincidence.  I was under the impression that it's not predictable from
the information available to kprobes.  If that's the case, then I guess
that arch_kprobe_flush_task() can't do the restore-the-return-address
trick.  This would affect only return-probed functions that are active
when arch_kprobe_flush_task() is called, and actually return.  I think
that this is a fairly small class of functions; flush_old_exec() would
be one.

> +		add_rp_inst(ri);
> +		/* Replace original return address with trampoline address */
> +		regs->link = (unsigned long)kretprobe_trampoline;
> +	} else {
> +		rp->nmissed++;
> +	}
> +}
> +

Jim