On Tue, Jul 11, 2023 at 1:28 AM Jiri Olsa <olsajiri@xxxxxxxxx> wrote:
>
> 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@xxxxxxxxx> 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@xxxxxxxxxx> 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@xxxxxxxxxx>
> > > > > Signed-off-by: Jiri Olsa <jolsa@xxxxxxxxxx>
> > > > > ---
> > > > > 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
ok, sounds great, seems like we have a pretty logical behavior for
both uprobe and uretprobe.
>
> 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
>
Yep, I don't think kernel can easily know where the function even
begins. But that's ok. We can clarify helper's behavior in UAPI
comments: uprobe/uretprobe always work, but return not function IP,
but the traced location. Thanks!
> 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);
> > > > >
> > > >
> > > > [...]
