[RFC][PATCH] PPC64: return address probes

[Ananth N Mavinakayanahalli]

Jim Keniston wrote:
> On Fri, 2005-05-20 at 05:15, Ananth N Mavinakayanahalli wrote:

Hi Jim,

Thanks for the comments...

>>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.

Yes, this is incorrect - I'll fix that (come to think of it, I am not
sure how I could assume that :-). However, I don't see an easy way to
determine where the location for restoring the value. Also, you may
never take a stack frame unless there is another function call from
within the return address probed function.

> 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.

As I see in the code, do_exit() calls exit_thread() but doesn't "return" 
and as you mention, I see that flush_old_exec() is the only function
calls flush_tread() and does a return.

What do you suggest would be a good way to handle this? Do we put a
caveat that users can't have return probes on flush_old_exec()? Any
other ideas?

Ananth