Re: [PATCH 07/10] uprobes/x86: Introduce arch_uretprobe_is_alive()

[Oleg Nesterov <oleg@redhat.com>]

On 05/07, Srikar Dronamraju wrote:
>
> >
> > diff --git a/arch/x86/include/asm/uprobes.h b/arch/x86/include/asm/uprobes.h
> > index f011fd0..60777f3 100644
> > --- a/arch/x86/include/asm/uprobes.h
> > +++ b/arch/x86/include/asm/uprobes.h
> > @@ -57,6 +57,7 @@ struct arch_uprobe {
> >  };
> >
> >  struct arch_uretprobe {
> > +	unsigned long	sp;
>
> While this looks good, I was wondering if you did think of having the sp
> in the return_instance structure itself. I mean can we use
> user_stack_pointer() to populate the ri->sp?

Yes, yes, I considered this option. And note that we can cleanup the
(a bit ugly) arch_uretprobe_hijack_return_addr() if we export
"struct return_instance" and pass it to arch_ helper.

> In which case the weak function itself should suffice for most archs.
>
> Something like this.
> prepare_uretprobe() we can have
> ri->sp = user_stack_pointer(regs)

Yes, and we can do this without changing arch_uretprobe_hijack_return_addr()
interface (which imo should be changed anyway, but this is off-topic).

> and handle_trampoline() would call something like
>
> 	arch_uretprobe_is_alive(next->sp, regs);
>
> bool __weak arch_uretprobe_is_alive(unsigned long sp, struct pt_regs *regs)
> {
> 	return user_stack_pointer(regs) <= sp;
> }

The problem is, I simply do not know if this is right on !x86.

And. I wanted to ensure that if (say) arch/ppc needs something else to
save/check in hijack/alive, then this architecture can just add the new
members in arch_uretprobe and change the arch_ helpers.

> Am I missing something?

I do not know. Lets wait for the comments from arch/ maintainers?

Oleg.