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

[Oleg Nesterov <oleg@redhat.com>]

On 05/08, Srikar Dronamraju wrote:
>
> > 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.
>
> The above weak function should work with ppc.

I don't think so. Even if I know nothing about !x86.

> Infact I see only 2 arch
> that define CONFIG_STACK_GROWSUP

Ah, please forget about GROWSUP, this is not the problem.

> We even seem to use this assumption when kprobe_tracer/uprobe_tracer
> fetch arguments from stack. See fetch_kernel_stack_address() /
> fetch_user_stack_address() and get_user_stack_nth().

But this all is completely different.

No. I don't think arch_uretprobe_is_alive() above can work for powerpc,
at least the same way.

The problem is, when the function is called, the ret-addr is not pushed
on stack. If it was, then arch_uretprobe_hijack_return_addr() on powerpc
is just wrong. But I guess it is correct ;)

x86 is "simple". We know that the probed function should do "ret" and the
ret-addr lives on stack. This means that "regs->sp <= sp" is correct, it
can't be false-negative. Simply because if regs->sp > sp then *sp can be
never used by "ret". And everything above regs->sp can be overwritten by
a signal handler. powerpc/etc differs, they use the link register.

Just for example. Lets look at prepare_uretprobe(). Suppose it adds the
new return_instance to ->return_instances list. Note that on 86
arch_uretprobe_is_alive(&new_ri->auret) is obviously (and correctly) true.
Is it also true on powerpc? I am not sure, I think it is not. Yes, this
doesn't really matter in prepare_uretprobe(), but this will matter if
the new ret-addr won't be saved on stack when we hit the next bp.

So. Lets do this per-arch. Try to do, actually. I am not even sure these
new hooks can actually help powerpc/etc. If not, we will have to switch
to "plan B".

If x86 can share the same code with (say) powerpc, we can always cleanup
this later, this is trivial. Right now I'd like to ensure that if the
same or similar logic can work on powerpc, it only needs to touch the
code in arch/powerpc.

Oleg.