From: Andrii Nakryiko <andrii.nakryiko@gmail.com>
To: Alan Maguire <alan.maguire@oracle.com>
Cc: "Andrii Nakryiko" <andrii@kernel.org>,
"Daniel Borkmann" <daniel@iogearbox.net>,
"Alexei Starovoitov" <ast@kernel.org>,
"Martin Lau" <kafai@fb.com>, "Song Liu" <songliubraving@fb.com>,
"Yonghong Song" <yhs@fb.com>,
"john fastabend" <john.fastabend@gmail.com>,
"KP Singh" <kpsingh@kernel.org>,
"Toke Høiland-Jørgensen" <toke@redhat.com>,
"Yucong Sun" <sunyucong@gmail.com>,
Networking <netdev@vger.kernel.org>, bpf <bpf@vger.kernel.org>
Subject: Re: [PATCH v5 bpf-next 0/5] libbpf: name-based u[ret]probe attach
Date: Sun, 3 Apr 2022 18:14:43 -0700 [thread overview]
Message-ID: <CAEf4BzZdV60ZeNt1YfS8qoB69pggSe+=gjnDZ1tZy00L4Quazw@mail.gmail.com> (raw)
In-Reply-To: <1648654000-21758-1-git-send-email-alan.maguire@oracle.com>
On Wed, Mar 30, 2022 at 8:27 AM Alan Maguire <alan.maguire@oracle.com> wrote:
>
> This patch series focuses on supporting name-based attach - similar
> to that supported for kprobes - for uprobe BPF programs.
>
> 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.
> Patch 1 supports expansion of the binary_path argument used for
> bpf_program__attach_uprobe_opts(), allowing it to determine paths
> for programs and shared objects automatically, allowing for
> specification of "libc.so.6" rather than the full path
> "/usr/lib64/libc.so.6".
>
> Patch 2 adds the "func_name" option to allow uprobe attach by
> name; the mechanics are described there.
>
> Having name-based support allows us to support auto-attach for
> uprobes; patch 3 adds auto-attach support while attempting
> to handle backwards-compatibility issues that arise. The format
> supported is
>
> u[ret]probe/binary_path:[raw_offset|function[+offset]]
>
> For example, to attach to libc malloc:
>
> SEC("uprobe//usr/lib64/libc.so.6:malloc")
>
> ..or, making use of the path computation mechanisms introduced in patch 1
>
> SEC("uprobe/libc.so.6:malloc")
>
> Finally patch 4 add tests to the attach_probe selftests covering
> attach by name, with patch 5 covering skeleton auto-attach.
>
> Changes since v4 [1]:
> - replaced strtok_r() usage with copying segments from static char *; avoids
> unneeded string allocation (Andrii, patch 1)
> - switched to using access() instead of stat() when checking path-resolved
> binary (Andrii, patch 1)
> - removed computation of .plt offset for instrumenting shared library calls
> within binaries. Firstly it proved too brittle, and secondly it was somewhat
> unintuitive in that this form of instrumentation did not support function+offset
> as the "local function in binary" and "shared library function in shared library"
> cases did. We can still instrument library calls, just need to do it in the
> library .so (patch 2)
ah, that's too bad, it seemed like a nice and clever idea. What was
brittle? Curious to learn for the future.
The fact that function+offset isn't supported int this "mode" seems
totally fine to me, we can provide a nice descriptive error in this
case anyways.
Anyways, all the added functionality makes sense and we can further
improve on it if necessary and possible. I've found a few small issues
with your patches and fixed some of them while applying, please do a
follow up for the rest. Thanks, great work and great addition to
libbpf!
> - added binary path logging in cases where it was missing (Andrii, patch 2)
> - avoid strlen() calcuation in checking name match (Andrii, patch 2)
> - reword comments for func_name option (Andrii, patch 2)
> - tightened SEC() name validation to support "u[ret]probe" and fail on other
> permutations that do not support auto-attach (i.e. have u[ret]probe/binary_path:func
> format (Andrii, patch 3)
> - fixed selftests to fail independently rather than skip remainder on failure
> (Andrii, patches 4,5)
> Changes since v3 [2]:
> - reworked variable naming to fit better with libbpf conventions
> (Andrii, patch 2)
> - use quoted binary path in log messages (Andrii, patch 2)
> - added path determination mechanisms using LD_LIBRARY_PATH/PATH and
> standard locations (patch 1, Andrii)
> - changed section lookup to be type+name (if name is specified) to
> simplify use cases (patch 2, Andrii)
> - fixed .plt lookup scheme to match symbol table entries with .plt
> index via the .rela.plt table; also fix the incorrect assumption
> that the code in the .plt that does library linking is the same
> size as .plt entries (it just happens to be on x86_64)
> - aligned with pluggable section support such that uprobe SEC() names
> that do not conform to auto-attach format do not cause skeleton load
> failure (patch 3, Andrii)
> - no longer need to look up absolute path to libraries used by test_progs
> since we have mechanism to determine path automatically
> - replaced CHECK()s with ASSERT*()s for attach_probe test (Andrii, patch 4)
> - added auto-attach selftests also (Andrii, patch 5)
> Changes since RFC [3]:
> - used "long" for addresses instead of ssize_t (Andrii, patch 1).
> - used gelf_ interfaces to avoid assumptions about 64-bit
> binaries (Andrii, patch 1)
> - clarified string matching in symbol table lookups
> (Andrii, patch 1)
> - added support for specification of shared object functions
> in a non-shared object binary. This approach instruments
> the Procedure Linking Table (PLT) - malloc@PLT.
> - changed logic in symbol search to check dynamic symbol table
> first, then fall back to symbol table (Andrii, patch 1).
> - modified auto-attach string to require "/" separator prior
> to path prefix i.e. uprobe//path/to/binary (Andrii, patch 2)
> - modified auto-attach string to use ':' separator (Andrii,
> patch 2)
> - modified auto-attach to support raw offset (Andrii, patch 2)
> - modified skeleton attach to interpret -ESRCH errors as
> a non-fatal "unable to auto-attach" (Andrii suggested
> -EOPNOTSUPP but my concern was it might collide with other
> instances where that value is returned and reflects a
> failure to attach a to-be-expected attachment rather than
> skip a program that does not present an auto-attachable
> section name. Admittedly -EOPNOTSUPP seems a more natural
> value here).
> - moved library path retrieval code to trace_helpers (Andrii,
> patch 3)
>
> [1] https://lore.kernel.org/bpf/1647000658-16149-1-git-send-email-alan.maguire@oracle.com/
> [2] https://lore.kernel.org/bpf/1643645554-28723-1-git-send-email-alan.maguire@oracle.com/
> [3] https://lore.kernel.org/bpf/1642678950-19584-1-git-send-email-alan.maguire@oracle.com/
>
> Alan Maguire (5):
> libbpf: bpf_program__attach_uprobe_opts() should determine paths for
> programs/libraries where possible
> libbpf: support function name-based attach uprobes
> libbpf: add auto-attach for uprobes based on section name
> selftests/bpf: add tests for u[ret]probe attach by name
> selftests/bpf: add tests for uprobe auto-attach via skeleton
>
> tools/lib/bpf/libbpf.c | 330 ++++++++++++++++++++-
> tools/lib/bpf/libbpf.h | 10 +-
> .../selftests/bpf/prog_tests/attach_probe.c | 85 +++++-
> .../selftests/bpf/prog_tests/uprobe_autoattach.c | 38 +++
> .../selftests/bpf/progs/test_attach_probe.c | 41 ++-
> .../selftests/bpf/progs/test_uprobe_autoattach.c | 52 ++++
> 6 files changed, 535 insertions(+), 21 deletions(-)
> create mode 100644 tools/testing/selftests/bpf/prog_tests/uprobe_autoattach.c
> create mode 100644 tools/testing/selftests/bpf/progs/test_uprobe_autoattach.c
>
> --
> 1.8.3.1
>
next prev parent reply other threads:[~2022-04-04 1:15 UTC|newest]
Thread overview: 18+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-03-30 15:26 [PATCH v5 bpf-next 0/5] libbpf: name-based u[ret]probe attach Alan Maguire
2022-03-30 15:26 ` [PATCH v5 bpf-next 1/5] libbpf: bpf_program__attach_uprobe_opts() should determine paths for programs/libraries where possible Alan Maguire
2022-04-04 1:14 ` Andrii Nakryiko
2022-03-30 15:26 ` [PATCH v5 bpf-next 2/5] libbpf: support function name-based attach uprobes Alan Maguire
2022-04-04 1:14 ` Andrii Nakryiko
2022-03-30 15:26 ` [PATCH v5 bpf-next 3/5] libbpf: add auto-attach for uprobes based on section name Alan Maguire
2022-04-04 1:14 ` Andrii Nakryiko
2022-04-04 4:46 ` Andrii Nakryiko
2022-04-04 4:49 ` Andrii Nakryiko
2022-04-04 9:36 ` Ilya Leoshkevich
2022-04-04 21:43 ` Alan Maguire
2022-03-30 15:26 ` [PATCH v5 bpf-next 4/5] selftests/bpf: add tests for u[ret]probe attach by name Alan Maguire
2022-03-30 15:26 ` [PATCH v5 bpf-next 5/5] selftests/bpf: add tests for uprobe auto-attach via skeleton Alan Maguire
2022-04-04 1:15 ` Andrii Nakryiko
2022-04-04 1:14 ` Andrii Nakryiko [this message]
2022-04-05 10:27 ` [PATCH v5 bpf-next 0/5] libbpf: name-based u[ret]probe attach Alan Maguire
2022-04-05 23:47 ` Andrii Nakryiko
2022-04-04 1:20 ` patchwork-bot+netdevbpf
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CAEf4BzZdV60ZeNt1YfS8qoB69pggSe+=gjnDZ1tZy00L4Quazw@mail.gmail.com' \
--to=andrii.nakryiko@gmail.com \
--cc=alan.maguire@oracle.com \
--cc=andrii@kernel.org \
--cc=ast@kernel.org \
--cc=bpf@vger.kernel.org \
--cc=daniel@iogearbox.net \
--cc=john.fastabend@gmail.com \
--cc=kafai@fb.com \
--cc=kpsingh@kernel.org \
--cc=netdev@vger.kernel.org \
--cc=songliubraving@fb.com \
--cc=sunyucong@gmail.com \
--cc=toke@redhat.com \
--cc=yhs@fb.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).