Date:   Wed, 19 Jan 2022 10:49:36 -0800
From:   Andrii Nakryiko <andrii.nakryiko@...il.com>
To:     Alan Maguire <alan.maguire@...cle.com>
Cc:     Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        Andrii Nakryiko <andrii@...nel.org>, Martin Lau <kafai@...com>,
        Song Liu <songliubraving@...com>, Yonghong Song <yhs@...com>,
        john fastabend <john.fastabend@...il.com>,
        KP Singh <kpsingh@...nel.org>, Jiri Olsa <jolsa@...nel.org>,
        Yucong Sun <sunyucong@...il.com>,
        Networking <netdev@...r.kernel.org>, bpf <bpf@...r.kernel.org>
Subject: Re: [RFC bpf-next 0/4] libbpf: userspace attach by name

On Wed, Jan 19, 2022 at 6:04 AM Alan Maguire <alan.maguire@...cle.com> wrote:
>
> On Fri, 14 Jan 2022, Andrii Nakryiko wrote:
>
> > > The one piece that seems to be missing from my perspective - and this may
> > > be in more recent versions - is uprobe function attachment by name. Most of
> > > the work is  already done in libusdt so it's reasonably doable I think - at a
> > > minimum  it would require an equivalent to the find_elf_func_offset()
> > > function in my  patch 1. Now the name of the library libusdt suggests its
> > > focus is on USDT of course, but I think having userspace function attach
> > > by name too would be great. Is that part of your plans for this work?
> >
> > True, uprobes don't supprot attaching by function name, which is quite
> > annoying. It's certainly not a focus for libusdt (or whatever it will
> > end up being called when open-sources). But if it's not much code and
> > complexity we should probably just add that to libbpf directly for
> > uprobes.
> >
>
> I've been looking at this, and I've got the following cases working:
>
> - local symbols in a binary. This involves symbol table lookup and
>   relative offset calcuation.
> - shared object symbols in a shared object.  In this case, the symbol
>   table values suffice, no adjustment needed.
>
> The former works using the program headers (instead of /proc/pid/maps for
> offset computation), so can be run for all processes, lifting the
> limitation in the RFC which only supported name lookup for a specific
> process. Around a hundred lines for this makes it worthwhile I think.
>
> There is one more case, which is a shared library function in a binary -
> where I specify "malloc" as the function and /usr/bin/foo as the binary
> path.  In this case, for dynamic symbols we can't just look up the symbol
> table in the binary, since the associated values are 0.  Ideally it would
> be nice if the user could just specify "malloc" and not need to use libc
> as the binary path argument, but getting this working is proving to be
> trickier. I've tried making use of PLT section information but no luck
> yet (the idea being we try to use the trampoline address of malloc@@PLT
> instead, but I'm still trying to figure out how to extract that).
>
> So I'm wondering if we just fail lookup for that case, assuming the user
> will specify the shared library path if they want to trace a shared library
> function. What do you think? Thanks!

I think it all makes sense (but let's see the code as well ;) ). For
the latter, can you please double-check what sort of functionality BCC
provides? Also make sure that you support specifying absolute address
instead of function name as well (func+0x123 probably as well, just
like for kprobes?).

The annoying bit is libbpf's convention to use '/' as a separator in
SEC() definitions. I think bpftrace/dtrace's ':' makes more sense, but
it seems to disruptive to switch it now. Because of this, specifying
absolute path to the binary would look weird:

SEC("uprobe//usr/bin/bash/readline")

or something like that would consistent with current convention, but
super weird.

Did you run into this issue during your experiments?

I can see two improvements, more and less radical (short of switching
from / to : completely):

1. less radical is to use "custom" format for uprobe after the "uprobe/" part:

SEC("uprobe//usr/bin/bash:readline")

2. a bit more radical (but probably better long term) is to support
'/' and ':' interchangeably (but only one of them in any given SEC()
definition).  For existing definitions, we can say that both forms are
supported now:

SEC("kprobe/some_func") and SEC("kprobe:some_func")

For uprobe I'd probably combine #1 and #2 and say that these two forms
are supported:

SEC("uprobe//usr/bin/bash:readline") (so function separator is always ':')

and

SEC("uprobe:/usr/bin/bash:readline") (nicer and more consistent).


Thoughts?

BTW, as much as I like consistency, the proposal to switch to ':'
exclusively in libbpf 1.0 is a no-go, IMO, it's too much of a
disruption for tons of users.


>
> Alan

Powered by blists - more mailing lists