Re: [PATCHv3 bpf-next 05/26] bpf: Add bpf_get_func_ip helper support for uprobe link

[Jiri Olsa <olsajiri@gmail.com>]

On Mon, Jul 10, 2023 at 10:55:36AM -0700, Andrii Nakryiko wrote:
> On Mon, Jul 10, 2023 at 12:24 AM Jiri Olsa <olsajiri@gmail.com> wrote:
> >
> > On Thu, Jul 06, 2023 at 03:29:08PM -0700, Andrii Nakryiko wrote:
> > > On Fri, Jun 30, 2023 at 1:34 AM Jiri Olsa <jolsa@kernel.org> wrote:
> > > >
> > > > Adding support for bpf_get_func_ip helper being called from
> > > > ebpf program attached by uprobe_multi link.
> > > >
> > > > It returns the ip of the uprobe.
> > > >
> > > > Acked-by: Andrii Nakryiko <andrii@kernel.org>
> > > > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> > > > ---
> > > >  kernel/trace/bpf_trace.c | 33 ++++++++++++++++++++++++++++++---
> > > >  1 file changed, 30 insertions(+), 3 deletions(-)
> > > >
> > >
> > > A slight aside related to bpf_get_func_ip() support in
> > > uprobe/uretprobe. We just had a conversation with Alastair and Jordan
> > > (cc'ed) about bpftrace and using bpf_get_func_ip() there with
> > > uretprobes, and it seems like it doesn't work.
> > >
> > > Is that intentional or we just missed that bpf_get_func_ip() doesn't
> > > work with uprobes/uretprobes? Do you think it would be hard to add
> > > support for them for bpf_get_func_ip()? It's a very useful helper,
> > > would be nice to have it working in all cases where it has meaningful
> > > behavior (and I think it does for uprobe and uretprobe).
> >
> > I'm not sure we discussed at the time we added this helper,
> > but I see same problem as we had with kprobes where we need
> > to know if the probe is on the start of the symbol
> >
> > we use KPROBE_FLAG_ON_FUNC_ENTRY flag for that in kprobe's
> > get_func_ip helper version:
> >
> >         BPF_CALL_1(bpf_get_func_ip_kprobe, struct pt_regs *, regs)
> >         {
> >                 struct kprobe *kp = kprobe_running();
> >
> >                 if (!kp || !(kp->flags & KPROBE_FLAG_ON_FUNC_ENTRY))
> >                         return 0;
> >
> >                 return get_entry_ip((uintptr_t)kp->addr);
> >         }
> >
> > I don't think we can have same flag for uprobe, we don't have
> > the binary symbol data as we have for kernel
> 
> what if we just return whatever was the IP where uprobe/uretprobe
> interrupt instruction was installed at? I haven't tried what does
> regs->ip contain for u*ret*probe, though, if it already has the IP
> where we installed uprobe itself, it might be ok as is. But still,

the entry uprobe gets the IP of the probed instruction, but the uretprobe
gets IP of the next instruction after probed func retun - like 4c57f0 in
the example below when uprobe is placed on trigger_func_get_func_ip

	  4c57eb:       e8 4b fe ff ff          call   4c563b <trigger_func_get_func_ip>
	  4c57f0:       eb 01                   jmp    4c57f3 <test_uprobe+0x1b1>

> feels like it would be nice to have bpf_get_func_ip() working for
> uprobe/uretprobe (even if semantics might differ for uprobes not at
> the entry of the function).

but it looks like we actually have the original uprobe address stored in
current->utask->vaddr before bpf program is executed (in uretprobe_dispatcher)
so we should be able to get that address from there in uretprobe's get_func_ip
helper function, I'll check

I don't mind the semantic change for uprobe's get_func_ip, because I think
it's unlikely we will be able to change it in future to return the real
function entry

jirka

> 
> >
> > I guess the app needs to store regs->ip and match it against symbol
> > addresses in user space
> >
> > jirka
> >
> >
> > >
> > > Thanks!
> > >
> > > > diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
> > > > index 4ef51fd0497f..f5a41c1604b8 100644
> > > > --- a/kernel/trace/bpf_trace.c
> > > > +++ b/kernel/trace/bpf_trace.c
> > > > @@ -88,6 +88,7 @@ static u64 bpf_kprobe_multi_cookie(struct bpf_run_ctx *ctx);
> > > >  static u64 bpf_kprobe_multi_entry_ip(struct bpf_run_ctx *ctx);
> > > >
> > >
> > > [...]