Date:   Fri, 21 Jan 2022 10:31:00 -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/3] libbpf: name-based u[ret]probe attach

On Thu, Jan 20, 2022 at 3:43 AM Alan Maguire <alan.maguire@...cle.com> wrote:
>
> This patch series is a refinement of the RFC patchset [1], focusing
> on support for attach by name for uprobes and uretprobes.  Still
> marked RFC as there are unresolved questions.
>
> Currently attach for such probes is done by determining the offset
> manually, so the aim is to try and mimic the simplicity of kprobe
> attach, making use of uprobe opts to specify a name string.
>
> uprobe attach is done by specifying a binary path, a pid (where
> 0 means "this process" and -1 means "all processes") and an
> offset.  Here a 'func_name' option is added to 'struct uprobe_opts'
> and that name is searched for in symbol tables.  If the binary
> is a program, relative offset calcuation must be done to the
> symbol address as described in [2].
>
> Having a name allows us to support auto-attach via SEC()
> specification, for example
>
> SEC("uprobe/usr/lib64/libc.so.6/malloc")
>
> Unresolved questions:
>
>  - the current scheme uses
>
> u[ret]probe[/]/path/2/binary/function[+offset]

that / after uprobe is not optional. This should be parsed as
"uprobe/<path-to-binary>/<func_name>[+<offset>]", in general. If
<path-to-binary> doesn't have leading '/' it will be just treated as a
relative path. Otherwise it's going to be ambiguous. So with your
example SEC("uprobe/usr/lib64/libc.so.6/malloc") you are specifying
"usr/lib64/libc.so.6", relative path, which is wrong. It has to be
SEC("uprobe//usr/lib64/libc.so.6/malloc"), however ugly that might
look.

>
>    ...as SEC() format for auto-attach, for example
>
> SEC("uprobe/usr/lib64/libc.so.6/malloc")
>
>    It would be cleaner to delimit binary and function with ':'
>    as is done by bcc.  One simple way to achieve that would be
>    to support section string pre-processing, where instances of
>    ':' are replaced by a '/'; this would get us to supporting
>    a similar probe specification as bcc without the backward
>    compatibility headaches.  I can't think of any valid
>    cases where SEC() definitions have a ':' that we would
>    replace with '/' in error, but I might be missing something.

I think at least for separating path and function name using ':' is
much better. I'd go with

SEC("uprobe//usr/lib64/libc.so.6:malloc")

for your example

>
>  - the current scheme doesn't support a raw offset address, since
>    it felt un-portable to encourage that, but can add this support
>    if needed.

I think for consistency with kprobe it's good to support it. And there
are local experimentation situations where this could be useful. So
let's add (sscanf() is pretty great at parsing this anyways)

>
>  - The auto-attach behaviour is to attach to all processes.
>    It would be good to have a way to specify the attach process
>    target. A few possibilities that would be compatible with
>    BPF skeleton support are to use the open opts (feels kind of
>    wrong conceptually since it's an attach-time attribute) or
>    to support opts with attach pid field in "struct bpf_prog_skeleton".
>    Latter would even allow a skeleton to attach to multiple
>    different processes with prog-level granularity (perhaps a union
>    of the various attach opts or similar?). There may be other
>    ways to achieve this.

Let's keep it simple and for auto-attach it's always -1 (all PIDs). If
that's not satisfactory, user shouldn't use auto-attach. Skeleton's
auto-attach (or bpf_program__attach()) is a convenience feature, not a
mandatory step.

>
> Changes since RFC [1]:
>  - focused on uprobe entry/return, omitting USDT attach (Andrii)
>  - use ELF program headers in calculating relative offsets, as this
>    works for the case where we do not specify a process.  The
>    previous approach relied on /proc/pid/maps so would not work
>    for the "all processes" case (where pid is -1).
>  - add support for auto-attach (patch 2)
>  - fix selftests to use a real library function.  I didn't notice
>    selftests override the usleep(3) definition, so as a result of
>    this, the libc function wasn't being called, so usleep() should
>    not be used to test shared library attach.  Also switch to
>    using libc path as the binary argument for these cases, as
>    specifying a shared library function name for a program is
>    not supported.  Tests now instrument malloc/free.
>  - added selftest that verifies auto-attach.
>
> [1] https://lore.kernel.org/bpf/1642004329-23514-1-git-send-email-alan.maguire@oracle.com/
> [2] https://www.kernel.org/doc/html/latest/trace/uprobetracer.html
>
> Alan Maguire (3):
>   libbpf: support function name-based attach for uprobes
>   libbpf: add auto-attach for uprobes based on section name
>   selftests/bpf: add tests for u[ret]probe attach by name
>
>  tools/lib/bpf/libbpf.c                             | 259 ++++++++++++++++++++-
>  tools/lib/bpf/libbpf.h                             |  10 +-
>  .../selftests/bpf/prog_tests/attach_probe.c        | 114 +++++++--
>  .../selftests/bpf/progs/test_attach_probe.c        |  33 +++
>  4 files changed, 396 insertions(+), 20 deletions(-)
>
> --
> 1.8.3.1
>

Powered by blists - more mailing lists