* [RFC bpf-next 1/3] libbpf: support function name-based attach for uprobes
2022-01-20 11:42 [RFC bpf-next 0/3] libbpf: name-based u[ret]probe attach Alan Maguire
@ 2022-01-20 11:42 ` Alan Maguire
2022-01-21 19:24 ` Andrii Nakryiko
2022-01-20 11:42 ` [RFC bpf-next 2/3] libbpf: add auto-attach for uprobes based on section name Alan Maguire
` (3 subsequent siblings)
4 siblings, 1 reply; 15+ messages in thread
From: Alan Maguire @ 2022-01-20 11:42 UTC (permalink / raw)
To: ast, daniel, andrii
Cc: kafai, songliubraving, yhs, john.fastabend, kpsingh, jolsa,
sunyucong, netdev, bpf, Alan Maguire
kprobe attach is name-based, using lookups of kallsyms to translate
a function name to an address. Currently uprobe attach is done
via an offset value as described in [1]. Extend uprobe opts
for attach to include a function name which can then be converted
into a uprobe-friendly offset. The calcualation is done in two
steps:
- first, determine the symbol address using libelf; this gives us
the offset as reported by objdump; then, in the case of local
functions
- subtract the base address associated with the object, retrieved
from ELF program headers.
The resultant value is then added to the func_offset value passed
in to specify the uprobe attach address. So specifying a func_offset
of 0 along with a function name "printf" will attach to printf entry.
The modes of operation supported are to attach to a local function
in a binary - function "foo1" in /usr/bin/foo - or to attach to
a library function in a shared object - function "malloc" in
/usr/lib64/libc.so.6. Because the symbol table values of shared
object functions in a binary will be 0, we cannot attach to a
shared object function in a binary ("malloc" in /usr/bin/foo).
[1] https://www.kernel.org/doc/html/latest/trace/uprobetracer.html
Signed-off-by: Alan Maguire <alan.maguire@oracle.com>
---
tools/lib/bpf/libbpf.c | 199 +++++++++++++++++++++++++++++++++++++++++++++++++
tools/lib/bpf/libbpf.h | 10 ++-
2 files changed, 208 insertions(+), 1 deletion(-)
diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
index fdb3536..6479aae 100644
--- a/tools/lib/bpf/libbpf.c
+++ b/tools/lib/bpf/libbpf.c
@@ -10183,6 +10183,191 @@ static int perf_event_uprobe_open_legacy(const char *probe_name, bool retprobe,
return pfd;
}
+/* uprobes deal in relative offsets; subtract the base address associated with
+ * the mapped binary. See Documentation/trace/uprobetracer.rst for more
+ * details.
+ */
+static ssize_t find_elf_relative_offset(Elf *elf, ssize_t addr)
+{
+ size_t n;
+ int i;
+
+ if (elf_getphdrnum(elf, &n)) {
+ pr_warn("elf: failed to find program headers: %s\n",
+ elf_errmsg(-1));
+ return -ENOENT;
+ }
+
+ for (i = 0; i < n; i++) {
+ int seg_start, seg_end, seg_offset;
+ GElf_Phdr phdr;
+
+ if (!gelf_getphdr(elf, i, &phdr)) {
+ pr_warn("elf: failed to get program header %d: %s\n",
+ i, elf_errmsg(-1));
+ return -ENOENT;
+ }
+ if (phdr.p_type != PT_LOAD || !(phdr.p_flags & PF_X))
+ continue;
+
+ seg_start = phdr.p_vaddr;
+ seg_end = seg_start + phdr.p_memsz;
+ seg_offset = phdr.p_offset;
+ if (addr >= seg_start && addr < seg_end)
+ return (ssize_t)addr - seg_start + seg_offset;
+ }
+ pr_warn("elf: failed to find prog header containing 0x%lx\n", addr);
+ return -ENOENT;
+}
+
+/* Return next ELF section of sh_type after scn, or first of that type
+ * if scn is NULL.
+ */
+static Elf_Scn *find_elfscn(Elf *elf, int sh_type, Elf_Scn *scn)
+{
+ Elf64_Shdr *sh;
+
+ while ((scn = elf_nextscn(elf, scn)) != NULL) {
+ sh = elf64_getshdr(scn);
+ if (sh && sh->sh_type == sh_type)
+ break;
+ }
+ return scn;
+}
+
+/* Find offset of function name in object specified by path. "name" matches
+ * symbol name or name@@LIB for library functions.
+ */
+static ssize_t find_elf_func_offset(const char *binary_path, const char *name)
+{
+ size_t si, strtabidx, nr_syms;
+ bool dynamic, is_shared_lib;
+ char errmsg[STRERR_BUFSIZE];
+ Elf_Data *symbols = NULL;
+ int lastbind = -1, fd;
+ ssize_t ret = -ENOENT;
+ Elf_Scn *scn = NULL;
+ const char *sname;
+ Elf64_Shdr *sh;
+ GElf_Ehdr ehdr;
+ Elf *elf;
+
+ if (!binary_path) {
+ pr_warn("name-based attach requires binary_path\n");
+ return -EINVAL;
+ }
+ if (elf_version(EV_CURRENT) == EV_NONE) {
+ pr_warn("elf: failed to init libelf for %s\n", binary_path);
+ return -LIBBPF_ERRNO__LIBELF;
+ }
+ fd = open(binary_path, O_RDONLY | O_CLOEXEC);
+ if (fd < 0) {
+ ret = -errno;
+ pr_warn("failed to open %s: %s\n", binary_path,
+ libbpf_strerror_r(ret, errmsg, sizeof(errmsg)));
+ return ret;
+ }
+ elf = elf_begin(fd, ELF_C_READ_MMAP, NULL);
+ if (!elf) {
+ pr_warn("elf: could not read elf from %s: %s\n",
+ binary_path, elf_errmsg(-1));
+ close(fd);
+ return -LIBBPF_ERRNO__FORMAT;
+ }
+ if (!gelf_getehdr(elf, &ehdr)) {
+ pr_warn("elf: failed to get ehdr from %s: %s\n",
+ binary_path, elf_errmsg(-1));
+ ret = -LIBBPF_ERRNO__FORMAT;
+ goto out;
+ }
+ is_shared_lib = ehdr.e_type == ET_DYN;
+ dynamic = is_shared_lib;
+retry:
+ scn = find_elfscn(elf, dynamic ? SHT_DYNSYM : SHT_SYMTAB, NULL);
+ if (!scn) {
+ pr_warn("elf: failed to find symbol table ELF section in %s\n",
+ binary_path);
+ ret = -ENOENT;
+ goto out;
+ }
+
+ sh = elf64_getshdr(scn);
+ strtabidx = sh->sh_link;
+ symbols = elf_getdata(scn, 0);
+ if (!symbols) {
+ pr_warn("elf: failed to get symtab section in %s: %s\n",
+ binary_path, elf_errmsg(-1));
+ ret = -LIBBPF_ERRNO__FORMAT;
+ goto out;
+ }
+
+ lastbind = -1;
+ nr_syms = symbols->d_size / sizeof(Elf64_Sym);
+ for (si = 0; si < nr_syms; si++) {
+ Elf64_Sym *sym = (Elf64_Sym *)symbols->d_buf + si;
+ size_t matchlen;
+ int currbind;
+
+ if (ELF64_ST_TYPE(sym->st_info) != STT_FUNC)
+ continue;
+
+ sname = elf_strptr(elf, strtabidx, sym->st_name);
+ if (!sname) {
+ pr_warn("elf: failed to get sym name string in %s\n",
+ binary_path);
+ ret = -EIO;
+ goto out;
+ }
+ currbind = ELF64_ST_BIND(sym->st_info);
+
+ /* If matching on func@@LIB, match on everything prior to
+ * the '@@'; otherwise match on full string.
+ */
+ matchlen = strstr(sname, "@@") ? strstr(sname, "@@") - sname :
+ strlen(sname);
+
+ if (strlen(name) == matchlen &&
+ strncmp(sname, name, matchlen) == 0) {
+ if (ret >= 0 && lastbind != -1) {
+ /* handle multiple matches */
+ if (lastbind != STB_WEAK && currbind != STB_WEAK) {
+ /* Only accept one non-weak bind. */
+ pr_warn("elf: additional match for '%s': %s\n",
+ sname, name);
+ ret = -LIBBPF_ERRNO__FORMAT;
+ goto out;
+ } else if (currbind == STB_WEAK) {
+ /* already have a non-weak bind, and
+ * this is a weak bind, so ignore.
+ */
+ continue;
+ }
+ }
+ ret = sym->st_value;
+ lastbind = currbind;
+ }
+ }
+ if (ret == 0) {
+ if (!dynamic) {
+ dynamic = true;
+ goto retry;
+ }
+ pr_warn("elf: '%s' is 0 in symbol table; try using shared library path instead of '%s'\n",
+ name, binary_path);
+ ret = -ENOENT;
+ }
+ if (ret > 0) {
+ pr_debug("elf: symbol table match for '%s': 0x%lx\n",
+ name, ret);
+ if (!is_shared_lib)
+ ret = find_elf_relative_offset(elf, ret);
+ }
+out:
+ elf_end(elf);
+ close(fd);
+ return ret;
+}
+
LIBBPF_API struct bpf_link *
bpf_program__attach_uprobe_opts(const struct bpf_program *prog, pid_t pid,
const char *binary_path, size_t func_offset,
@@ -10194,6 +10379,7 @@ static int perf_event_uprobe_open_legacy(const char *probe_name, bool retprobe,
size_t ref_ctr_off;
int pfd, err;
bool retprobe, legacy;
+ const char *func_name;
if (!OPTS_VALID(opts, bpf_uprobe_opts))
return libbpf_err_ptr(-EINVAL);
@@ -10202,6 +10388,19 @@ static int perf_event_uprobe_open_legacy(const char *probe_name, bool retprobe,
ref_ctr_off = OPTS_GET(opts, ref_ctr_offset, 0);
pe_opts.bpf_cookie = OPTS_GET(opts, bpf_cookie, 0);
+ func_name = OPTS_GET(opts, func_name, NULL);
+ if (func_name) {
+ ssize_t sym_off;
+
+ sym_off = find_elf_func_offset(binary_path, func_name);
+ if (sym_off < 0) {
+ pr_debug("could not find sym offset for %s in %s\n",
+ func_name, binary_path);
+ return libbpf_err_ptr(sym_off);
+ }
+ func_offset += (size_t)sym_off;
+ }
+
legacy = determine_uprobe_perf_type() < 0;
if (!legacy) {
pfd = perf_event_open_probe(true /* uprobe */, retprobe, binary_path,
diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h
index 9728551..4675586 100644
--- a/tools/lib/bpf/libbpf.h
+++ b/tools/lib/bpf/libbpf.h
@@ -431,9 +431,17 @@ struct bpf_uprobe_opts {
__u64 bpf_cookie;
/* uprobe is return probe, invoked at function return time */
bool retprobe;
+ /* name of function name or function@@LIBRARY. Partial matches
+ * work for library name, such as printf, printf@@GLIBC.
+ * To specify function entry, func_offset argument should be 0 and
+ * func_name should specify function to trace. To trace an offset
+ * within the function, specify func_name and use func_offset
+ * argument to specify argument _within_ the function.
+ */
+ const char *func_name;
size_t :0;
};
-#define bpf_uprobe_opts__last_field retprobe
+#define bpf_uprobe_opts__last_field func_name
/**
* @brief **bpf_program__attach_uprobe()** attaches a BPF program
--
1.8.3.1
^ permalink raw reply related [flat|nested] 15+ messages in thread* Re: [RFC bpf-next 1/3] libbpf: support function name-based attach for uprobes
2022-01-20 11:42 ` [RFC bpf-next 1/3] libbpf: support function name-based attach for uprobes Alan Maguire
@ 2022-01-21 19:24 ` Andrii Nakryiko
0 siblings, 0 replies; 15+ messages in thread
From: Andrii Nakryiko @ 2022-01-21 19:24 UTC (permalink / raw)
To: Alan Maguire
Cc: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko, Martin Lau,
Song Liu, Yonghong Song, john fastabend, KP Singh, Jiri Olsa,
Yucong Sun, Networking, bpf
On Thu, Jan 20, 2022 at 3:43 AM Alan Maguire <alan.maguire@oracle.com> wrote:
>
> kprobe attach is name-based, using lookups of kallsyms to translate
> a function name to an address. Currently uprobe attach is done
> via an offset value as described in [1]. Extend uprobe opts
> for attach to include a function name which can then be converted
> into a uprobe-friendly offset. The calcualation is done in two
> steps:
>
> - first, determine the symbol address using libelf; this gives us
> the offset as reported by objdump; then, in the case of local
> functions
> - subtract the base address associated with the object, retrieved
> from ELF program headers.
>
> The resultant value is then added to the func_offset value passed
> in to specify the uprobe attach address. So specifying a func_offset
> of 0 along with a function name "printf" will attach to printf entry.
>
> The modes of operation supported are to attach to a local function
> in a binary - function "foo1" in /usr/bin/foo - or to attach to
> a library function in a shared object - function "malloc" in
> /usr/lib64/libc.so.6. Because the symbol table values of shared
> object functions in a binary will be 0, we cannot attach to a
> shared object function in a binary ("malloc" in /usr/bin/foo).
>
> [1] https://www.kernel.org/doc/html/latest/trace/uprobetracer.html
>
> Signed-off-by: Alan Maguire <alan.maguire@oracle.com>
> ---
> tools/lib/bpf/libbpf.c | 199 +++++++++++++++++++++++++++++++++++++++++++++++++
> tools/lib/bpf/libbpf.h | 10 ++-
> 2 files changed, 208 insertions(+), 1 deletion(-)
>
> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> index fdb3536..6479aae 100644
> --- a/tools/lib/bpf/libbpf.c
> +++ b/tools/lib/bpf/libbpf.c
> @@ -10183,6 +10183,191 @@ static int perf_event_uprobe_open_legacy(const char *probe_name, bool retprobe,
> return pfd;
> }
>
> +/* uprobes deal in relative offsets; subtract the base address associated with
> + * the mapped binary. See Documentation/trace/uprobetracer.rst for more
> + * details.
> + */
> +static ssize_t find_elf_relative_offset(Elf *elf, ssize_t addr)
> +{
> + size_t n;
> + int i;
> +
> + if (elf_getphdrnum(elf, &n)) {
> + pr_warn("elf: failed to find program headers: %s\n",
> + elf_errmsg(-1));
> + return -ENOENT;
> + }
> +
> + for (i = 0; i < n; i++) {
> + int seg_start, seg_end, seg_offset;
> + GElf_Phdr phdr;
> +
> + if (!gelf_getphdr(elf, i, &phdr)) {
> + pr_warn("elf: failed to get program header %d: %s\n",
> + i, elf_errmsg(-1));
> + return -ENOENT;
> + }
> + if (phdr.p_type != PT_LOAD || !(phdr.p_flags & PF_X))
double space after ||
> + continue;
> +
> + seg_start = phdr.p_vaddr;
> + seg_end = seg_start + phdr.p_memsz;
> + seg_offset = phdr.p_offset;
> + if (addr >= seg_start && addr < seg_end)
> + return (ssize_t)addr - seg_start + seg_offset;
> + }
> + pr_warn("elf: failed to find prog header containing 0x%lx\n", addr);
%lx will be wrong for ssize_t on some arches, leading to compilation
warnings. But also why addr is signed ssize_t? size_t or long for
simplicity, I guess.
> + return -ENOENT;
> +}
> +
> +/* Return next ELF section of sh_type after scn, or first of that type
> + * if scn is NULL.
> + */
> +static Elf_Scn *find_elfscn(Elf *elf, int sh_type, Elf_Scn *scn)
elf_find_next_scn_by_type() would be less ambiguous name, IMO (and
sort of following naming convention of other elf_ helpers in libbpf.c)
> +{
> + Elf64_Shdr *sh;
> +
> + while ((scn = elf_nextscn(elf, scn)) != NULL) {
> + sh = elf64_getshdr(scn);
assumptions about 64-bit environment. 64-bit ELF assumption is correct
for BPF ELF binaries, but not for attaching to host binaries (which
could be 32-bit if libbpf is built for 32-bit arch).
> + if (sh && sh->sh_type == sh_type)
> + break;
> + }
> + return scn;
> +}
> +
> +/* Find offset of function name in object specified by path. "name" matches
> + * symbol name or name@@LIB for library functions.
> + */
> +static ssize_t find_elf_func_offset(const char *binary_path, const char *name)
> +{
> + size_t si, strtabidx, nr_syms;
> + bool dynamic, is_shared_lib;
> + char errmsg[STRERR_BUFSIZE];
> + Elf_Data *symbols = NULL;
> + int lastbind = -1, fd;
> + ssize_t ret = -ENOENT;
> + Elf_Scn *scn = NULL;
> + const char *sname;
> + Elf64_Shdr *sh;
> + GElf_Ehdr ehdr;
> + Elf *elf;
> +
> + if (!binary_path) {
probably better to do this check and exit eary in
bpf_program__attach_uprobe_opts()?
> + pr_warn("name-based attach requires binary_path\n");
> + return -EINVAL;
> + }
> + if (elf_version(EV_CURRENT) == EV_NONE) {
> + pr_warn("elf: failed to init libelf for %s\n", binary_path);
> + return -LIBBPF_ERRNO__LIBELF;
> + }
we already did this when opening BPF ELF, no need to do it again, we
wouldn't get all the way here otherwise
> + fd = open(binary_path, O_RDONLY | O_CLOEXEC);
> + if (fd < 0) {
> + ret = -errno;
> + pr_warn("failed to open %s: %s\n", binary_path,
> + libbpf_strerror_r(ret, errmsg, sizeof(errmsg)));
> + return ret;
> + }
> + elf = elf_begin(fd, ELF_C_READ_MMAP, NULL);
> + if (!elf) {
> + pr_warn("elf: could not read elf from %s: %s\n",
> + binary_path, elf_errmsg(-1));
> + close(fd);
> + return -LIBBPF_ERRNO__FORMAT;
> + }
> + if (!gelf_getehdr(elf, &ehdr)) {
> + pr_warn("elf: failed to get ehdr from %s: %s\n",
> + binary_path, elf_errmsg(-1));
try to keep single lines if they are under 100 characters, unnecessary
line wrapping hurts readability
> + ret = -LIBBPF_ERRNO__FORMAT;
> + goto out;
> + }
> + is_shared_lib = ehdr.e_type == ET_DYN;
> + dynamic = is_shared_lib;
why both is_shared_lib and dynamic? same value, same meaning
> +retry:
> + scn = find_elfscn(elf, dynamic ? SHT_DYNSYM : SHT_SYMTAB, NULL);
If I understand correctly, DYNSYM is a subset of SYMTAB, so if you'd
like to attach to non-exported function in shared lib, you still need
to use SYMTAB. So let's use SYMTAB, if it is available, otherwise fall
back to DYNSYM?
> + if (!scn) {
> + pr_warn("elf: failed to find symbol table ELF section in %s\n",
> + binary_path);
> + ret = -ENOENT;
> + goto out;
> + }
> +
> + sh = elf64_getshdr(scn);
again, bitness assumptions, you have to stick to gelf APIs for this :(
> + strtabidx = sh->sh_link;
> + symbols = elf_getdata(scn, 0);
> + if (!symbols) {
> + pr_warn("elf: failed to get symtab section in %s: %s\n",
> + binary_path, elf_errmsg(-1));
> + ret = -LIBBPF_ERRNO__FORMAT;
> + goto out;
> + }
> +
> + lastbind = -1;
last_bind, cur_bind, match_len please stick to naming conventions used
more or less consistently in libbpf
> + nr_syms = symbols->d_size / sizeof(Elf64_Sym);
> + for (si = 0; si < nr_syms; si++) {
> + Elf64_Sym *sym = (Elf64_Sym *)symbols->d_buf + si;
> + size_t matchlen;
> + int currbind;
> +
> + if (ELF64_ST_TYPE(sym->st_info) != STT_FUNC)
> + continue;
> +
> + sname = elf_strptr(elf, strtabidx, sym->st_name);
> + if (!sname) {
> + pr_warn("elf: failed to get sym name string in %s\n",
> + binary_path);
> + ret = -EIO;
> + goto out;
> + }
> + currbind = ELF64_ST_BIND(sym->st_info);
> +
> + /* If matching on func@@LIB, match on everything prior to
> + * the '@@'; otherwise match on full string.
> + */
> + matchlen = strstr(sname, "@@") ? strstr(sname, "@@") - sname :
> + strlen(sname);
remember strstr() result and reuse
> +
> + if (strlen(name) == matchlen &&
strlen(name) == matchlen is equivalent to non-NULL result of strstr(),
again, remember and use that one instead of unnecessary string
operations
but also isn't strncmp() alone enough?
> + strncmp(sname, name, matchlen) == 0) {
invert if condition and continue, reduce nesting
> + if (ret >= 0 && lastbind != -1) {
> + /* handle multiple matches */
> + if (lastbind != STB_WEAK && currbind != STB_WEAK) {
> + /* Only accept one non-weak bind. */
> + pr_warn("elf: additional match for '%s': %s\n",
additional -> ambiguous?
> + sname, name);
> + ret = -LIBBPF_ERRNO__FORMAT;
> + goto out;
> + } else if (currbind == STB_WEAK) {
> + /* already have a non-weak bind, and
> + * this is a weak bind, so ignore.
> + */
> + continue;
> + }
> + }
> + ret = sym->st_value;
> + lastbind = currbind;
> + }
> + }
> + if (ret == 0) {
> + if (!dynamic) {
> + dynamic = true;
> + goto retry;
> + }
hm.. trying to understand this piece... I can understand trying DYNSYM
first and falling back to SYMTAB (for performance reasons, probably).
But the other way, not entirely clear. Can you explain and leave a
comment?
> + pr_warn("elf: '%s' is 0 in symbol table; try using shared library path instead of '%s'\n",
> + name, binary_path);
> + ret = -ENOENT;
> + }
> + if (ret > 0) {
> + pr_debug("elf: symbol table match for '%s': 0x%lx\n",
> + name, ret);
> + if (!is_shared_lib)
> + ret = find_elf_relative_offset(elf, ret);
> + }
> +out:
> + elf_end(elf);
> + close(fd);
> + return ret;
> +}
> +
> LIBBPF_API struct bpf_link *
> bpf_program__attach_uprobe_opts(const struct bpf_program *prog, pid_t pid,
> const char *binary_path, size_t func_offset,
> @@ -10194,6 +10379,7 @@ static int perf_event_uprobe_open_legacy(const char *probe_name, bool retprobe,
> size_t ref_ctr_off;
> int pfd, err;
> bool retprobe, legacy;
> + const char *func_name;
>
> if (!OPTS_VALID(opts, bpf_uprobe_opts))
> return libbpf_err_ptr(-EINVAL);
> @@ -10202,6 +10388,19 @@ static int perf_event_uprobe_open_legacy(const char *probe_name, bool retprobe,
> ref_ctr_off = OPTS_GET(opts, ref_ctr_offset, 0);
> pe_opts.bpf_cookie = OPTS_GET(opts, bpf_cookie, 0);
>
> + func_name = OPTS_GET(opts, func_name, NULL);
> + if (func_name) {
> + ssize_t sym_off;
> +
> + sym_off = find_elf_func_offset(binary_path, func_name);
> + if (sym_off < 0) {
> + pr_debug("could not find sym offset for %s in %s\n",
> + func_name, binary_path);
pr_warn? maybe also prefix with "elf: " like other similar messages
(we also use "failed to" language most consistently, I think)
> + return libbpf_err_ptr(sym_off);
> + }
> + func_offset += (size_t)sym_off;
> + }
> +
> legacy = determine_uprobe_perf_type() < 0;
> if (!legacy) {
> pfd = perf_event_open_probe(true /* uprobe */, retprobe, binary_path,
> diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h
> index 9728551..4675586 100644
> --- a/tools/lib/bpf/libbpf.h
> +++ b/tools/lib/bpf/libbpf.h
> @@ -431,9 +431,17 @@ struct bpf_uprobe_opts {
> __u64 bpf_cookie;
> /* uprobe is return probe, invoked at function return time */
> bool retprobe;
> + /* name of function name or function@@LIBRARY. Partial matches
just to clarify the @@LIB handling. If we were using SYMTAB
everywhere, wouldn't exact match still work for shared library symbol
search?
> + * work for library name, such as printf, printf@@GLIBC.
> + * To specify function entry, func_offset argument should be 0 and
> + * func_name should specify function to trace. To trace an offset
> + * within the function, specify func_name and use func_offset
> + * argument to specify argument _within_ the function.
> + */
> + const char *func_name;
> size_t :0;
> };
> -#define bpf_uprobe_opts__last_field retprobe
> +#define bpf_uprobe_opts__last_field func_name
>
> /**
> * @brief **bpf_program__attach_uprobe()** attaches a BPF program
> --
> 1.8.3.1
>
^ permalink raw reply [flat|nested] 15+ messages in thread
* [RFC bpf-next 2/3] libbpf: add auto-attach for uprobes based on section name
2022-01-20 11:42 [RFC bpf-next 0/3] libbpf: name-based u[ret]probe attach Alan Maguire
2022-01-20 11:42 ` [RFC bpf-next 1/3] libbpf: support function name-based attach for uprobes Alan Maguire
@ 2022-01-20 11:42 ` Alan Maguire
2022-01-21 19:33 ` Andrii Nakryiko
2022-01-20 11:42 ` [RFC bpf-next 3/3] selftests/bpf: add tests for u[ret]probe attach by name Alan Maguire
` (2 subsequent siblings)
4 siblings, 1 reply; 15+ messages in thread
From: Alan Maguire @ 2022-01-20 11:42 UTC (permalink / raw)
To: ast, daniel, andrii
Cc: kafai, songliubraving, yhs, john.fastabend, kpsingh, jolsa,
sunyucong, netdev, bpf, Alan Maguire
Now that u[ret]probes can use name-based specification, it makes
sense to add support for auto-attach based on SEC() definition.
The format proposed is
SEC("u[ret]probe[/]/path/to/prog/function[+offset]")
For example, to trace malloc() in libc:
SEC("uprobe/usr/lib64/libc.so.6/malloc")
Auto-attach is done for all tasks (pid -1).
Future work will look at generalizing section specification to
substitute ':' for '/'.
Signed-off-by: Alan Maguire <alan.maguire@oracle.com>
---
tools/lib/bpf/libbpf.c | 60 ++++++++++++++++++++++++++++++++++++++++++++++++--
1 file changed, 58 insertions(+), 2 deletions(-)
diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
index 6479aae..1c8c8f0 100644
--- a/tools/lib/bpf/libbpf.c
+++ b/tools/lib/bpf/libbpf.c
@@ -8565,6 +8565,7 @@ int bpf_program__set_log_buf(struct bpf_program *prog, char *log_buf, size_t log
}
static struct bpf_link *attach_kprobe(const struct bpf_program *prog, long cookie);
+static struct bpf_link *attach_uprobe(const struct bpf_program *prog, long cookie);
static struct bpf_link *attach_tp(const struct bpf_program *prog, long cookie);
static struct bpf_link *attach_raw_tp(const struct bpf_program *prog, long cookie);
static struct bpf_link *attach_trace(const struct bpf_program *prog, long cookie);
@@ -8576,9 +8577,9 @@ int bpf_program__set_log_buf(struct bpf_program *prog, char *log_buf, size_t log
SEC_DEF("sk_reuseport/migrate", SK_REUSEPORT, BPF_SK_REUSEPORT_SELECT_OR_MIGRATE, SEC_ATTACHABLE | SEC_SLOPPY_PFX),
SEC_DEF("sk_reuseport", SK_REUSEPORT, BPF_SK_REUSEPORT_SELECT, SEC_ATTACHABLE | SEC_SLOPPY_PFX),
SEC_DEF("kprobe/", KPROBE, 0, SEC_NONE, attach_kprobe),
- SEC_DEF("uprobe/", KPROBE, 0, SEC_NONE),
+ SEC_DEF("uprobe/", KPROBE, 0, SEC_NONE, attach_uprobe),
SEC_DEF("kretprobe/", KPROBE, 0, SEC_NONE, attach_kprobe),
- SEC_DEF("uretprobe/", KPROBE, 0, SEC_NONE),
+ SEC_DEF("uretprobe/", KPROBE, 0, SEC_NONE, attach_uprobe),
SEC_DEF("tc", SCHED_CLS, 0, SEC_NONE),
SEC_DEF("classifier", SCHED_CLS, 0, SEC_NONE | SEC_SLOPPY_PFX),
SEC_DEF("action", SCHED_ACT, 0, SEC_NONE | SEC_SLOPPY_PFX),
@@ -10454,6 +10455,61 @@ static ssize_t find_elf_func_offset(const char *binary_path, const char *name)
}
+/* Format of u[ret]probe section definition supporting auto-attach:
+ * u[ret]probe[/]/path/to/prog/function[+offset]
+ *
+ * Many uprobe programs do not avail of auto-attach, so we need to handle the
+ * case where the format is uprobe/myfunc by returning NULL rather than an
+ * error.
+ */
+static struct bpf_link *attach_uprobe(const struct bpf_program *prog, long cookie)
+{
+ DECLARE_LIBBPF_OPTS(bpf_uprobe_opts, opts);
+ char *func_name, binary_path[512];
+ char *func, *probe_name;
+ struct bpf_link *link;
+ size_t offset = 0;
+ int n, err;
+
+ opts.retprobe = str_has_pfx(prog->sec_name, "uretprobe/");
+ if (opts.retprobe)
+ probe_name = prog->sec_name + sizeof("uretprobe/") - 1;
+ else
+ probe_name = prog->sec_name + sizeof("uprobe/") - 1;
+
+ /* First char in binary_path is a '/'; this allows us to support
+ * uprobe/path/2/prog and uprobe//path/2/prog, while also
+ * distinguishing between old-style uprobe/something definitions.
+ */
+ snprintf(binary_path, sizeof(binary_path) - 1, "/%s", probe_name);
+ /* last '/' should be prior to function+offset */
+ func_name = strrchr(binary_path + 1, '/');
+ if (!func_name) {
+ pr_debug("section '%s' is old-style u[ret]probe/function, cannot auto-attach\n",
+ prog->sec_name);
+ return NULL;
+ }
+ func_name[0] = '\0';
+ func_name++;
+ n = sscanf(func_name, "%m[a-zA-Z0-9_.]+%li", &func, &offset);
+ if (n < 1) {
+ err = -EINVAL;
+ pr_warn("uprobe name is invalid: %s\n", func_name);
+ return libbpf_err_ptr(err);
+ }
+ if (opts.retprobe && offset != 0) {
+ free(func);
+ err = -EINVAL;
+ pr_warn("uretprobes do not support offset specification\n");
+ return libbpf_err_ptr(err);
+ }
+ opts.func_name = func;
+
+ link = bpf_program__attach_uprobe_opts(prog, -1, binary_path, offset, &opts);
+ free(func);
+ return link;
+}
+
struct bpf_link *bpf_program__attach_uprobe(const struct bpf_program *prog,
bool retprobe, pid_t pid,
const char *binary_path,
--
1.8.3.1
^ permalink raw reply related [flat|nested] 15+ messages in thread* Re: [RFC bpf-next 2/3] libbpf: add auto-attach for uprobes based on section name
2022-01-20 11:42 ` [RFC bpf-next 2/3] libbpf: add auto-attach for uprobes based on section name Alan Maguire
@ 2022-01-21 19:33 ` Andrii Nakryiko
0 siblings, 0 replies; 15+ messages in thread
From: Andrii Nakryiko @ 2022-01-21 19:33 UTC (permalink / raw)
To: Alan Maguire
Cc: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko, Martin Lau,
Song Liu, Yonghong Song, john fastabend, KP Singh, Jiri Olsa,
Yucong Sun, Networking, bpf
On Thu, Jan 20, 2022 at 3:43 AM Alan Maguire <alan.maguire@oracle.com> wrote:
>
> Now that u[ret]probes can use name-based specification, it makes
> sense to add support for auto-attach based on SEC() definition.
> The format proposed is
>
> SEC("u[ret]probe[/]/path/to/prog/function[+offset]")
>
> For example, to trace malloc() in libc:
>
> SEC("uprobe/usr/lib64/libc.so.6/malloc")
>
> Auto-attach is done for all tasks (pid -1).
>
> Future work will look at generalizing section specification to
> substitute ':' for '/'.
>
> Signed-off-by: Alan Maguire <alan.maguire@oracle.com>
> ---
> tools/lib/bpf/libbpf.c | 60 ++++++++++++++++++++++++++++++++++++++++++++++++--
> 1 file changed, 58 insertions(+), 2 deletions(-)
>
> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> index 6479aae..1c8c8f0 100644
> --- a/tools/lib/bpf/libbpf.c
> +++ b/tools/lib/bpf/libbpf.c
> @@ -8565,6 +8565,7 @@ int bpf_program__set_log_buf(struct bpf_program *prog, char *log_buf, size_t log
> }
>
> static struct bpf_link *attach_kprobe(const struct bpf_program *prog, long cookie);
> +static struct bpf_link *attach_uprobe(const struct bpf_program *prog, long cookie);
> static struct bpf_link *attach_tp(const struct bpf_program *prog, long cookie);
> static struct bpf_link *attach_raw_tp(const struct bpf_program *prog, long cookie);
> static struct bpf_link *attach_trace(const struct bpf_program *prog, long cookie);
> @@ -8576,9 +8577,9 @@ int bpf_program__set_log_buf(struct bpf_program *prog, char *log_buf, size_t log
> SEC_DEF("sk_reuseport/migrate", SK_REUSEPORT, BPF_SK_REUSEPORT_SELECT_OR_MIGRATE, SEC_ATTACHABLE | SEC_SLOPPY_PFX),
> SEC_DEF("sk_reuseport", SK_REUSEPORT, BPF_SK_REUSEPORT_SELECT, SEC_ATTACHABLE | SEC_SLOPPY_PFX),
> SEC_DEF("kprobe/", KPROBE, 0, SEC_NONE, attach_kprobe),
> - SEC_DEF("uprobe/", KPROBE, 0, SEC_NONE),
> + SEC_DEF("uprobe/", KPROBE, 0, SEC_NONE, attach_uprobe),
> SEC_DEF("kretprobe/", KPROBE, 0, SEC_NONE, attach_kprobe),
> - SEC_DEF("uretprobe/", KPROBE, 0, SEC_NONE),
> + SEC_DEF("uretprobe/", KPROBE, 0, SEC_NONE, attach_uprobe),
> SEC_DEF("tc", SCHED_CLS, 0, SEC_NONE),
> SEC_DEF("classifier", SCHED_CLS, 0, SEC_NONE | SEC_SLOPPY_PFX),
> SEC_DEF("action", SCHED_ACT, 0, SEC_NONE | SEC_SLOPPY_PFX),
> @@ -10454,6 +10455,61 @@ static ssize_t find_elf_func_offset(const char *binary_path, const char *name)
>
> }
>
> +/* Format of u[ret]probe section definition supporting auto-attach:
> + * u[ret]probe[/]/path/to/prog/function[+offset]
> + *
> + * Many uprobe programs do not avail of auto-attach, so we need to handle the
> + * case where the format is uprobe/myfunc by returning NULL rather than an
> + * error.
> + */
> +static struct bpf_link *attach_uprobe(const struct bpf_program *prog, long cookie)
> +{
> + DECLARE_LIBBPF_OPTS(bpf_uprobe_opts, opts);
> + char *func_name, binary_path[512];
> + char *func, *probe_name;
> + struct bpf_link *link;
> + size_t offset = 0;
> + int n, err;
> +
> + opts.retprobe = str_has_pfx(prog->sec_name, "uretprobe/");
> + if (opts.retprobe)
> + probe_name = prog->sec_name + sizeof("uretprobe/") - 1;
> + else
> + probe_name = prog->sec_name + sizeof("uprobe/") - 1;
> +
> + /* First char in binary_path is a '/'; this allows us to support
> + * uprobe/path/2/prog and uprobe//path/2/prog, while also
> + * distinguishing between old-style uprobe/something definitions.
> + */
see my comments on cover letter, I don't think this is the right thing to do
> + snprintf(binary_path, sizeof(binary_path) - 1, "/%s", probe_name);
> + /* last '/' should be prior to function+offset */
> + func_name = strrchr(binary_path + 1, '/');
> + if (!func_name) {
> + pr_debug("section '%s' is old-style u[ret]probe/function, cannot auto-attach\n",
> + prog->sec_name);
> + return NULL;
this is actually a regression, we need to adapt
bpf_object__attach_skeleton() to accommodate SEC_DEF()s with attach
functions that might decide not to auto-attach (but not treat it as an
error). Maybe -EOPNOTSUPP should be treated specially?
> + }
> + func_name[0] = '\0';
> + func_name++;
> + n = sscanf(func_name, "%m[a-zA-Z0-9_.]+%li", &func, &offset);
> + if (n < 1) {
> + err = -EINVAL;
> + pr_warn("uprobe name is invalid: %s\n", func_name);
> + return libbpf_err_ptr(err);
> + }
> + if (opts.retprobe && offset != 0) {
> + free(func);
> + err = -EINVAL;
> + pr_warn("uretprobes do not support offset specification\n");
> + return libbpf_err_ptr(err);
> + }
> + opts.func_name = func;
> +
> + link = bpf_program__attach_uprobe_opts(prog, -1, binary_path, offset, &opts);
> + free(func);
> + return link;
> +}
> +
> struct bpf_link *bpf_program__attach_uprobe(const struct bpf_program *prog,
> bool retprobe, pid_t pid,
> const char *binary_path,
> --
> 1.8.3.1
>
^ permalink raw reply [flat|nested] 15+ messages in thread
* [RFC bpf-next 3/3] selftests/bpf: add tests for u[ret]probe attach by name
2022-01-20 11:42 [RFC bpf-next 0/3] libbpf: name-based u[ret]probe attach Alan Maguire
2022-01-20 11:42 ` [RFC bpf-next 1/3] libbpf: support function name-based attach for uprobes Alan Maguire
2022-01-20 11:42 ` [RFC bpf-next 2/3] libbpf: add auto-attach for uprobes based on section name Alan Maguire
@ 2022-01-20 11:42 ` Alan Maguire
2022-01-21 19:40 ` Andrii Nakryiko
2022-01-21 1:44 ` [RFC bpf-next 0/3] libbpf: name-based u[ret]probe attach Alexei Starovoitov
2022-01-21 18:31 ` Andrii Nakryiko
4 siblings, 1 reply; 15+ messages in thread
From: Alan Maguire @ 2022-01-20 11:42 UTC (permalink / raw)
To: ast, daniel, andrii
Cc: kafai, songliubraving, yhs, john.fastabend, kpsingh, jolsa,
sunyucong, netdev, bpf, Alan Maguire
add tests that verify attaching by name for local functions in
a program and library functions in a shared object succeed for
uprobe and uretprobes using new "func_name" option for
bpf_program__attach_uprobe_opts(). Also verify auto-attach
works where uprobe, path to binary and function name are
specified.
Signed-off-by: Alan Maguire <alan.maguire@oracle.com>
---
.../selftests/bpf/prog_tests/attach_probe.c | 114 ++++++++++++++++++---
.../selftests/bpf/progs/test_attach_probe.c | 33 ++++++
2 files changed, 130 insertions(+), 17 deletions(-)
diff --git a/tools/testing/selftests/bpf/prog_tests/attach_probe.c b/tools/testing/selftests/bpf/prog_tests/attach_probe.c
index d0bd51e..1bfb09e 100644
--- a/tools/testing/selftests/bpf/prog_tests/attach_probe.c
+++ b/tools/testing/selftests/bpf/prog_tests/attach_probe.c
@@ -10,16 +10,24 @@ static void method(void) {
return ;
}
+/* attach point for byname uprobe */
+static void method2(void)
+{
+}
+
void test_attach_probe(void)
{
DECLARE_LIBBPF_OPTS(bpf_uprobe_opts, uprobe_opts);
int duration = 0;
struct bpf_link *kprobe_link, *kretprobe_link;
- struct bpf_link *uprobe_link, *uretprobe_link;
+ struct bpf_link *uprobe_link[3], *uretprobe_link[3];
struct test_attach_probe* skel;
size_t uprobe_offset;
ssize_t base_addr, ref_ctr_offset;
+ char libc_path[256];
bool legacy;
+ char *mem;
+ FILE *f;
/* Check if new-style kprobe/uprobe API is supported.
* Kernels that support new FD-based kprobe and uprobe BPF attachment
@@ -69,14 +77,14 @@ void test_attach_probe(void)
uprobe_opts.retprobe = false;
uprobe_opts.ref_ctr_offset = legacy ? 0 : ref_ctr_offset;
- uprobe_link = bpf_program__attach_uprobe_opts(skel->progs.handle_uprobe,
- 0 /* self pid */,
- "/proc/self/exe",
- uprobe_offset,
- &uprobe_opts);
- if (!ASSERT_OK_PTR(uprobe_link, "attach_uprobe"))
+ uprobe_link[0] = bpf_program__attach_uprobe_opts(skel->progs.handle_uprobe,
+ 0 /* self pid */,
+ "/proc/self/exe",
+ uprobe_offset,
+ &uprobe_opts);
+ if (!ASSERT_OK_PTR(uprobe_link[0], "attach_uprobe"))
goto cleanup;
- skel->links.handle_uprobe = uprobe_link;
+ skel->links.handle_uprobe = uprobe_link[0];
if (!legacy)
ASSERT_GT(uprobe_ref_ctr, 0, "uprobe_ref_ctr_after");
@@ -84,17 +92,79 @@ void test_attach_probe(void)
/* if uprobe uses ref_ctr, uretprobe has to use ref_ctr as well */
uprobe_opts.retprobe = true;
uprobe_opts.ref_ctr_offset = legacy ? 0 : ref_ctr_offset;
- uretprobe_link = bpf_program__attach_uprobe_opts(skel->progs.handle_uretprobe,
- -1 /* any pid */,
+ uretprobe_link[0] = bpf_program__attach_uprobe_opts(skel->progs.handle_uretprobe,
+ -1 /* any pid */,
+ "/proc/self/exe",
+ uprobe_offset, &uprobe_opts);
+ if (!ASSERT_OK_PTR(uretprobe_link[0], "attach_uretprobe"))
+ goto cleanup;
+ skel->links.handle_uretprobe = uretprobe_link[0];
+
+ uprobe_opts.func_name = "method2";
+ uprobe_opts.retprobe = false;
+ uprobe_opts.ref_ctr_offset = 0;
+ uprobe_link[1] = bpf_program__attach_uprobe_opts(skel->progs.handle_uprobe_byname,
+ 0 /* this pid */,
"/proc/self/exe",
- uprobe_offset, &uprobe_opts);
- if (!ASSERT_OK_PTR(uretprobe_link, "attach_uretprobe"))
+ 0, &uprobe_opts);
+ if (!ASSERT_OK_PTR(uprobe_link[1], "attach_uprobe_byname"))
+ goto cleanup;
+ skel->links.handle_uprobe_byname = uprobe_link[1];
+
+ /* verify auto-attach works */
+ uretprobe_link[1] = bpf_program__attach(skel->progs.handle_uretprobe_byname);
+ if (!ASSERT_OK_PTR(uretprobe_link[1], "attach_uretprobe_byname"))
+ goto cleanup;
+ skel->links.handle_uretprobe_byname = uretprobe_link[1];
+
+ /* test attach by name for a library function, using the library
+ * as the binary argument. To do this, find path to libc used
+ * by test_progs via /proc/self/maps.
+ */
+ f = fopen("/proc/self/maps", "r");
+ if (!ASSERT_OK_PTR(f, "read /proc/self/maps"))
+ goto cleanup;
+ while (fscanf(f, "%*s %*s %*s %*s %*s %[^\n]", libc_path) == 1) {
+ if (strstr(libc_path, "libc-"))
+ break;
+ }
+ fclose(f);
+ if (!ASSERT_NEQ(strstr(libc_path, "libc-"), NULL, "find libc path in /proc/self/maps"))
+ goto cleanup;
+
+ uprobe_opts.func_name = "malloc";
+ uprobe_opts.retprobe = false;
+ uprobe_link[2] = bpf_program__attach_uprobe_opts(skel->progs.handle_uprobe_byname2,
+ 0 /* this pid */,
+ libc_path,
+ 0, &uprobe_opts);
+ if (!ASSERT_OK_PTR(uprobe_link[2], "attach_uprobe_byname2"))
goto cleanup;
- skel->links.handle_uretprobe = uretprobe_link;
+ skel->links.handle_uprobe_byname2 = uprobe_link[2];
- /* trigger & validate kprobe && kretprobe */
+ uprobe_opts.func_name = "free";
+ uprobe_opts.retprobe = true;
+ uretprobe_link[2] = bpf_program__attach_uprobe_opts(skel->progs.handle_uretprobe_byname2,
+ -1 /* any pid */,
+ libc_path,
+ 0, &uprobe_opts);
+ if (!ASSERT_OK_PTR(uretprobe_link[2], "attach_uretprobe_byname2"))
+ goto cleanup;
+ skel->links.handle_uretprobe_byname2 = uretprobe_link[2];
+
+ /* trigger & validate kprobe && kretprobe && uretprobe by name */
usleep(1);
+ /* trigger & validate shared library u[ret]probes attached by name */
+ mem = malloc(1);
+ free(mem);
+
+ /* trigger & validate uprobe & uretprobe */
+ method();
+
+ /* trigger & validate uprobe attached by name */
+ method2();
+
if (CHECK(skel->bss->kprobe_res != 1, "check_kprobe_res",
"wrong kprobe res: %d\n", skel->bss->kprobe_res))
goto cleanup;
@@ -102,9 +172,6 @@ void test_attach_probe(void)
"wrong kretprobe res: %d\n", skel->bss->kretprobe_res))
goto cleanup;
- /* trigger & validate uprobe & uretprobe */
- method();
-
if (CHECK(skel->bss->uprobe_res != 3, "check_uprobe_res",
"wrong uprobe res: %d\n", skel->bss->uprobe_res))
goto cleanup;
@@ -112,6 +179,19 @@ void test_attach_probe(void)
"wrong uretprobe res: %d\n", skel->bss->uretprobe_res))
goto cleanup;
+ if (CHECK(skel->bss->uprobe_byname_res != 5, "check_uprobe_byname_res",
+ "wrong uprobe byname res: %d\n", skel->bss->uprobe_byname_res))
+ goto cleanup;
+ if (CHECK(skel->bss->uretprobe_byname_res != 6, "check_uretprobe_byname_res",
+ "wrong uretprobe byname res: %d\n", skel->bss->uretprobe_byname_res))
+ goto cleanup;
+ if (CHECK(skel->bss->uprobe_byname2_res != 7, "check_uprobe_byname2_res",
+ "wrong uprobe byname2 res: %d\n", skel->bss->uprobe_byname2_res))
+ goto cleanup;
+ if (CHECK(skel->bss->uretprobe_byname2_res != 8, "check_uretprobe_byname2_res",
+ "wrong uretprobe byname2 res: %d\n", skel->bss->uretprobe_byname2_res))
+ goto cleanup;
+
cleanup:
test_attach_probe__destroy(skel);
ASSERT_EQ(uprobe_ref_ctr, 0, "uprobe_ref_ctr_cleanup");
diff --git a/tools/testing/selftests/bpf/progs/test_attach_probe.c b/tools/testing/selftests/bpf/progs/test_attach_probe.c
index 8056a4c..c176c89 100644
--- a/tools/testing/selftests/bpf/progs/test_attach_probe.c
+++ b/tools/testing/selftests/bpf/progs/test_attach_probe.c
@@ -10,6 +10,10 @@
int kretprobe_res = 0;
int uprobe_res = 0;
int uretprobe_res = 0;
+int uprobe_byname_res = 0;
+int uretprobe_byname_res = 0;
+int uprobe_byname2_res = 0;
+int uretprobe_byname2_res = 0;
SEC("kprobe/sys_nanosleep")
int handle_kprobe(struct pt_regs *ctx)
@@ -39,4 +43,33 @@ int handle_uretprobe(struct pt_regs *ctx)
return 0;
}
+SEC("uprobe/trigger_func_byname")
+int handle_uprobe_byname(struct pt_regs *ctx)
+{
+ uprobe_byname_res = 5;
+ return 0;
+}
+
+/* use auto-attach format for section definition. */
+SEC("uretprobe/proc/self/exe/method2")
+int handle_uretprobe_byname(struct pt_regs *ctx)
+{
+ uretprobe_byname_res = 6;
+ return 0;
+}
+
+SEC("uprobe/trigger_func_byname2")
+int handle_uprobe_byname2(struct pt_regs *ctx)
+{
+ uprobe_byname2_res = 7;
+ return 0;
+}
+
+SEC("uretprobe/trigger_func_byname2")
+int handle_uretprobe_byname2(struct pt_regs *ctx)
+{
+ uretprobe_byname2_res = 8;
+ return 0;
+}
+
char _license[] SEC("license") = "GPL";
--
1.8.3.1
^ permalink raw reply related [flat|nested] 15+ messages in thread* Re: [RFC bpf-next 3/3] selftests/bpf: add tests for u[ret]probe attach by name
2022-01-20 11:42 ` [RFC bpf-next 3/3] selftests/bpf: add tests for u[ret]probe attach by name Alan Maguire
@ 2022-01-21 19:40 ` Andrii Nakryiko
0 siblings, 0 replies; 15+ messages in thread
From: Andrii Nakryiko @ 2022-01-21 19:40 UTC (permalink / raw)
To: Alan Maguire
Cc: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko, Martin Lau,
Song Liu, Yonghong Song, john fastabend, KP Singh, Jiri Olsa,
Yucong Sun, Networking, bpf
On Thu, Jan 20, 2022 at 3:43 AM Alan Maguire <alan.maguire@oracle.com> wrote:
>
> add tests that verify attaching by name for local functions in
> a program and library functions in a shared object succeed for
> uprobe and uretprobes using new "func_name" option for
> bpf_program__attach_uprobe_opts(). Also verify auto-attach
> works where uprobe, path to binary and function name are
> specified.
>
> Signed-off-by: Alan Maguire <alan.maguire@oracle.com>
> ---
> .../selftests/bpf/prog_tests/attach_probe.c | 114 ++++++++++++++++++---
> .../selftests/bpf/progs/test_attach_probe.c | 33 ++++++
> 2 files changed, 130 insertions(+), 17 deletions(-)
>
> diff --git a/tools/testing/selftests/bpf/prog_tests/attach_probe.c b/tools/testing/selftests/bpf/prog_tests/attach_probe.c
> index d0bd51e..1bfb09e 100644
> --- a/tools/testing/selftests/bpf/prog_tests/attach_probe.c
> +++ b/tools/testing/selftests/bpf/prog_tests/attach_probe.c
> @@ -10,16 +10,24 @@ static void method(void) {
> return ;
> }
>
> +/* attach point for byname uprobe */
> +static void method2(void)
> +{
> +}
> +
> void test_attach_probe(void)
> {
> DECLARE_LIBBPF_OPTS(bpf_uprobe_opts, uprobe_opts);
> int duration = 0;
> struct bpf_link *kprobe_link, *kretprobe_link;
> - struct bpf_link *uprobe_link, *uretprobe_link;
> + struct bpf_link *uprobe_link[3], *uretprobe_link[3];
why do you need an array, uprobe_link and uretprobe_link are just
temporary variables for shorter code, those links are assigned into
skel->links section and taken care of by skeleton destroy method
> struct test_attach_probe* skel;
> size_t uprobe_offset;
> ssize_t base_addr, ref_ctr_offset;
> + char libc_path[256];
> bool legacy;
> + char *mem;
> + FILE *f;
>
> /* Check if new-style kprobe/uprobe API is supported.
> * Kernels that support new FD-based kprobe and uprobe BPF attachment
> @@ -69,14 +77,14 @@ void test_attach_probe(void)
>
> uprobe_opts.retprobe = false;
> uprobe_opts.ref_ctr_offset = legacy ? 0 : ref_ctr_offset;
> - uprobe_link = bpf_program__attach_uprobe_opts(skel->progs.handle_uprobe,
> - 0 /* self pid */,
> - "/proc/self/exe",
> - uprobe_offset,
> - &uprobe_opts);
> - if (!ASSERT_OK_PTR(uprobe_link, "attach_uprobe"))
> + uprobe_link[0] = bpf_program__attach_uprobe_opts(skel->progs.handle_uprobe,
> + 0 /* self pid */,
> + "/proc/self/exe",
> + uprobe_offset,
> + &uprobe_opts);
> + if (!ASSERT_OK_PTR(uprobe_link[0], "attach_uprobe"))
> goto cleanup;
> - skel->links.handle_uprobe = uprobe_link;
> + skel->links.handle_uprobe = uprobe_link[0];
>
> if (!legacy)
> ASSERT_GT(uprobe_ref_ctr, 0, "uprobe_ref_ctr_after");
> @@ -84,17 +92,79 @@ void test_attach_probe(void)
> /* if uprobe uses ref_ctr, uretprobe has to use ref_ctr as well */
> uprobe_opts.retprobe = true;
> uprobe_opts.ref_ctr_offset = legacy ? 0 : ref_ctr_offset;
> - uretprobe_link = bpf_program__attach_uprobe_opts(skel->progs.handle_uretprobe,
> - -1 /* any pid */,
> + uretprobe_link[0] = bpf_program__attach_uprobe_opts(skel->progs.handle_uretprobe,
> + -1 /* any pid */,
> + "/proc/self/exe",
> + uprobe_offset, &uprobe_opts);
> + if (!ASSERT_OK_PTR(uretprobe_link[0], "attach_uretprobe"))
> + goto cleanup;
> + skel->links.handle_uretprobe = uretprobe_link[0];
> +
> + uprobe_opts.func_name = "method2";
> + uprobe_opts.retprobe = false;
> + uprobe_opts.ref_ctr_offset = 0;
> + uprobe_link[1] = bpf_program__attach_uprobe_opts(skel->progs.handle_uprobe_byname,
> + 0 /* this pid */,
> "/proc/self/exe",
> - uprobe_offset, &uprobe_opts);
> - if (!ASSERT_OK_PTR(uretprobe_link, "attach_uretprobe"))
> + 0, &uprobe_opts);
> + if (!ASSERT_OK_PTR(uprobe_link[1], "attach_uprobe_byname"))
> + goto cleanup;
> + skel->links.handle_uprobe_byname = uprobe_link[1];
> +
> + /* verify auto-attach works */
> + uretprobe_link[1] = bpf_program__attach(skel->progs.handle_uretprobe_byname);
> + if (!ASSERT_OK_PTR(uretprobe_link[1], "attach_uretprobe_byname"))
> + goto cleanup;
> + skel->links.handle_uretprobe_byname = uretprobe_link[1];
> +
> + /* test attach by name for a library function, using the library
> + * as the binary argument. To do this, find path to libc used
> + * by test_progs via /proc/self/maps.
> + */
> + f = fopen("/proc/self/maps", "r");
> + if (!ASSERT_OK_PTR(f, "read /proc/self/maps"))
> + goto cleanup;
> + while (fscanf(f, "%*s %*s %*s %*s %*s %[^\n]", libc_path) == 1) {
> + if (strstr(libc_path, "libc-"))
> + break;
> + }
> + fclose(f);
> + if (!ASSERT_NEQ(strstr(libc_path, "libc-"), NULL, "find libc path in /proc/self/maps"))
> + goto cleanup;
maybe let's extract this into a small helper, we'll eventually
probably use it for other tests (and it just makes it a bit more
obvious what's going on)
> +
> + uprobe_opts.func_name = "malloc";
> + uprobe_opts.retprobe = false;
> + uprobe_link[2] = bpf_program__attach_uprobe_opts(skel->progs.handle_uprobe_byname2,
> + 0 /* this pid */,
> + libc_path,
> + 0, &uprobe_opts);
> + if (!ASSERT_OK_PTR(uprobe_link[2], "attach_uprobe_byname2"))
> goto cleanup;
> - skel->links.handle_uretprobe = uretprobe_link;
> + skel->links.handle_uprobe_byname2 = uprobe_link[2];
>
> - /* trigger & validate kprobe && kretprobe */
> + uprobe_opts.func_name = "free";
> + uprobe_opts.retprobe = true;
> + uretprobe_link[2] = bpf_program__attach_uprobe_opts(skel->progs.handle_uretprobe_byname2,
> + -1 /* any pid */,
> + libc_path,
> + 0, &uprobe_opts);
> + if (!ASSERT_OK_PTR(uretprobe_link[2], "attach_uretprobe_byname2"))
> + goto cleanup;
> + skel->links.handle_uretprobe_byname2 = uretprobe_link[2];
> +
> + /* trigger & validate kprobe && kretprobe && uretprobe by name */
> usleep(1);
>
> + /* trigger & validate shared library u[ret]probes attached by name */
> + mem = malloc(1);
> + free(mem);
> +
> + /* trigger & validate uprobe & uretprobe */
> + method();
> +
> + /* trigger & validate uprobe attached by name */
> + method2();
> +
> if (CHECK(skel->bss->kprobe_res != 1, "check_kprobe_res",
> "wrong kprobe res: %d\n", skel->bss->kprobe_res))
> goto cleanup;
> @@ -102,9 +172,6 @@ void test_attach_probe(void)
> "wrong kretprobe res: %d\n", skel->bss->kretprobe_res))
> goto cleanup;
>
> - /* trigger & validate uprobe & uretprobe */
> - method();
> -
> if (CHECK(skel->bss->uprobe_res != 3, "check_uprobe_res",
> "wrong uprobe res: %d\n", skel->bss->uprobe_res))
> goto cleanup;
> @@ -112,6 +179,19 @@ void test_attach_probe(void)
> "wrong uretprobe res: %d\n", skel->bss->uretprobe_res))
> goto cleanup;
>
> + if (CHECK(skel->bss->uprobe_byname_res != 5, "check_uprobe_byname_res",
> + "wrong uprobe byname res: %d\n", skel->bss->uprobe_byname_res))
> + goto cleanup;
> + if (CHECK(skel->bss->uretprobe_byname_res != 6, "check_uretprobe_byname_res",
> + "wrong uretprobe byname res: %d\n", skel->bss->uretprobe_byname_res))
> + goto cleanup;
> + if (CHECK(skel->bss->uprobe_byname2_res != 7, "check_uprobe_byname2_res",
> + "wrong uprobe byname2 res: %d\n", skel->bss->uprobe_byname2_res))
> + goto cleanup;
> + if (CHECK(skel->bss->uretprobe_byname2_res != 8, "check_uretprobe_byname2_res",
> + "wrong uretprobe byname2 res: %d\n", skel->bss->uretprobe_byname2_res))
> + goto cleanup;
use ASSERT_xxx() instead of CHECK() for new tests
> +
> cleanup:
> test_attach_probe__destroy(skel);
> ASSERT_EQ(uprobe_ref_ctr, 0, "uprobe_ref_ctr_cleanup");
> diff --git a/tools/testing/selftests/bpf/progs/test_attach_probe.c b/tools/testing/selftests/bpf/progs/test_attach_probe.c
> index 8056a4c..c176c89 100644
> --- a/tools/testing/selftests/bpf/progs/test_attach_probe.c
> +++ b/tools/testing/selftests/bpf/progs/test_attach_probe.c
> @@ -10,6 +10,10 @@
> int kretprobe_res = 0;
> int uprobe_res = 0;
> int uretprobe_res = 0;
> +int uprobe_byname_res = 0;
> +int uretprobe_byname_res = 0;
> +int uprobe_byname2_res = 0;
> +int uretprobe_byname2_res = 0;
>
> SEC("kprobe/sys_nanosleep")
> int handle_kprobe(struct pt_regs *ctx)
> @@ -39,4 +43,33 @@ int handle_uretprobe(struct pt_regs *ctx)
> return 0;
> }
>
> +SEC("uprobe/trigger_func_byname")
this should be just SEC("uprobe") ?
> +int handle_uprobe_byname(struct pt_regs *ctx)
> +{
> + uprobe_byname_res = 5;
> + return 0;
> +}
> +
> +/* use auto-attach format for section definition. */
> +SEC("uretprobe/proc/self/exe/method2")
> +int handle_uretprobe_byname(struct pt_regs *ctx)
> +{
> + uretprobe_byname_res = 6;
> + return 0;
> +}
> +
> +SEC("uprobe/trigger_func_byname2")
> +int handle_uprobe_byname2(struct pt_regs *ctx)
> +{
> + uprobe_byname2_res = 7;
> + return 0;
> +}
> +
> +SEC("uretprobe/trigger_func_byname2")
same here and above, SEC("uprobe") if you don't specify binary + func.
We should tighten this up for other SEC_DEF()s as well, but that's
separate change from yours.
> +int handle_uretprobe_byname2(struct pt_regs *ctx)
> +{
> + uretprobe_byname2_res = 8;
> + return 0;
> +}
> +
> char _license[] SEC("license") = "GPL";
> --
> 1.8.3.1
>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [RFC bpf-next 0/3] libbpf: name-based u[ret]probe attach
2022-01-20 11:42 [RFC bpf-next 0/3] libbpf: name-based u[ret]probe attach Alan Maguire
` (2 preceding siblings ...)
2022-01-20 11:42 ` [RFC bpf-next 3/3] selftests/bpf: add tests for u[ret]probe attach by name Alan Maguire
@ 2022-01-21 1:44 ` Alexei Starovoitov
2022-01-21 18:15 ` Andrii Nakryiko
2022-01-21 18:31 ` Andrii Nakryiko
4 siblings, 1 reply; 15+ messages in thread
From: Alexei Starovoitov @ 2022-01-21 1:44 UTC (permalink / raw)
To: Alan Maguire
Cc: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
Martin KaFai Lau, Song Liu, Yonghong Song, John Fastabend,
KP Singh, Jiri Olsa, sunyucong, Network Development, bpf
On Thu, Jan 20, 2022 at 3:43 AM Alan Maguire <alan.maguire@oracle.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].
I think the pid discussion here and in the patches only causes
confusion. I think it's best to remove pid from the api.
uprobes are attached to an inode. They're not attached to a pid
or a process. Any existing process or future process started
from that inode (executable file) will have that uprobe triggering.
The kernel can do pid filtering through predicate mechanism,
but bpf uprobe doesn't do any filtering. iirc.
Similarly "attach to all processes" doesn't sound right either.
It's attached to all current and future processes.
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [RFC bpf-next 0/3] libbpf: name-based u[ret]probe attach
2022-01-21 1:44 ` [RFC bpf-next 0/3] libbpf: name-based u[ret]probe attach Alexei Starovoitov
@ 2022-01-21 18:15 ` Andrii Nakryiko
2022-01-21 18:20 ` Alexei Starovoitov
2022-01-24 14:13 ` Alan Maguire
0 siblings, 2 replies; 15+ messages in thread
From: Andrii Nakryiko @ 2022-01-21 18:15 UTC (permalink / raw)
To: Alexei Starovoitov
Cc: Alan Maguire, Alexei Starovoitov, Daniel Borkmann,
Andrii Nakryiko, Martin KaFai Lau, Song Liu, Yonghong Song,
John Fastabend, KP Singh, Jiri Olsa, Yucong Sun,
Network Development, bpf
On Thu, Jan 20, 2022 at 5:44 PM Alexei Starovoitov
<alexei.starovoitov@gmail.com> wrote:
>
> On Thu, Jan 20, 2022 at 3:43 AM Alan Maguire <alan.maguire@oracle.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].
>
> I think the pid discussion here and in the patches only causes
> confusion. I think it's best to remove pid from the api.
It's already part of the uprobe API in libbpf
(bpf_program__attach_uprobe), but nothing really changes there.
API-wise Alan just added an optional func_name option. I think it
makes sense overall.
For auto-attach it has to be all PIDs, of course.
> uprobes are attached to an inode. They're not attached to a pid
> or a process. Any existing process or future process started
> from that inode (executable file) will have that uprobe triggering.
> The kernel can do pid filtering through predicate mechanism,
> but bpf uprobe doesn't do any filtering. iirc.
> Similarly "attach to all processes" doesn't sound right either.
> It's attached to all current and future processes.
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [RFC bpf-next 0/3] libbpf: name-based u[ret]probe attach
2022-01-21 18:15 ` Andrii Nakryiko
@ 2022-01-21 18:20 ` Alexei Starovoitov
2022-01-21 18:27 ` Andrii Nakryiko
2022-01-24 14:13 ` Alan Maguire
1 sibling, 1 reply; 15+ messages in thread
From: Alexei Starovoitov @ 2022-01-21 18:20 UTC (permalink / raw)
To: Andrii Nakryiko
Cc: Alan Maguire, Alexei Starovoitov, Daniel Borkmann,
Andrii Nakryiko, Martin KaFai Lau, Song Liu, Yonghong Song,
John Fastabend, KP Singh, Jiri Olsa, Yucong Sun,
Network Development, bpf
On Fri, Jan 21, 2022 at 10:15 AM Andrii Nakryiko
<andrii.nakryiko@gmail.com> wrote:
>
> On Thu, Jan 20, 2022 at 5:44 PM Alexei Starovoitov
> <alexei.starovoitov@gmail.com> wrote:
> >
> > On Thu, Jan 20, 2022 at 3:43 AM Alan Maguire <alan.maguire@oracle.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].
> >
> > I think the pid discussion here and in the patches only causes
> > confusion. I think it's best to remove pid from the api.
>
> It's already part of the uprobe API in libbpf
> (bpf_program__attach_uprobe), but nothing really changes there.
> API-wise Alan just added an optional func_name option. I think it
> makes sense overall.
Technically it can be deprecated.
So "it's already part of api" isn't really an excuse to keep
confusing the users.
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [RFC bpf-next 0/3] libbpf: name-based u[ret]probe attach
2022-01-21 18:20 ` Alexei Starovoitov
@ 2022-01-21 18:27 ` Andrii Nakryiko
0 siblings, 0 replies; 15+ messages in thread
From: Andrii Nakryiko @ 2022-01-21 18:27 UTC (permalink / raw)
To: Alexei Starovoitov
Cc: Alan Maguire, Alexei Starovoitov, Daniel Borkmann,
Andrii Nakryiko, Martin KaFai Lau, Song Liu, Yonghong Song,
John Fastabend, KP Singh, Jiri Olsa, Yucong Sun,
Network Development, bpf
On Fri, Jan 21, 2022 at 10:20 AM Alexei Starovoitov
<alexei.starovoitov@gmail.com> wrote:
>
> On Fri, Jan 21, 2022 at 10:15 AM Andrii Nakryiko
> <andrii.nakryiko@gmail.com> wrote:
> >
> > On Thu, Jan 20, 2022 at 5:44 PM Alexei Starovoitov
> > <alexei.starovoitov@gmail.com> wrote:
> > >
> > > On Thu, Jan 20, 2022 at 3:43 AM Alan Maguire <alan.maguire@oracle.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].
> > >
> > > I think the pid discussion here and in the patches only causes
> > > confusion. I think it's best to remove pid from the api.
> >
> > It's already part of the uprobe API in libbpf
> > (bpf_program__attach_uprobe), but nothing really changes there.
> > API-wise Alan just added an optional func_name option. I think it
> > makes sense overall.
>
> Technically it can be deprecated.
> So "it's already part of api" isn't really an excuse to keep
> confusing the users.
... but I don't find it confusing and no one really ever complained?..
So it doesn't seem like we need to remove this.
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [RFC bpf-next 0/3] libbpf: name-based u[ret]probe attach
2022-01-21 18:15 ` Andrii Nakryiko
2022-01-21 18:20 ` Alexei Starovoitov
@ 2022-01-24 14:13 ` Alan Maguire
2022-01-24 22:47 ` Andrii Nakryiko
1 sibling, 1 reply; 15+ messages in thread
From: Alan Maguire @ 2022-01-24 14:13 UTC (permalink / raw)
To: Andrii Nakryiko
Cc: Alexei Starovoitov, Alan Maguire, Alexei Starovoitov,
Daniel Borkmann, Andrii Nakryiko, Martin KaFai Lau, Song Liu,
Yonghong Song, John Fastabend, KP Singh, Jiri Olsa, Yucong Sun,
Network Development, bpf
On Fri, 21 Jan 2022, Andrii Nakryiko wrote:
> On Thu, Jan 20, 2022 at 5:44 PM Alexei Starovoitov
> <alexei.starovoitov@gmail.com> wrote:
> >
> > On Thu, Jan 20, 2022 at 3:43 AM Alan Maguire <alan.maguire@oracle.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].
> >
> > I think the pid discussion here and in the patches only causes
> > confusion. I think it's best to remove pid from the api.
>
> It's already part of the uprobe API in libbpf
> (bpf_program__attach_uprobe), but nothing really changes there.
> API-wise Alan just added an optional func_name option. I think it
> makes sense overall.
>
> For auto-attach it has to be all PIDs, of course.
>
Makes sense.
> > uprobes are attached to an inode. They're not attached to a pid
> > or a process. Any existing process or future process started
> > from that inode (executable file) will have that uprobe triggering.
> > The kernel can do pid filtering through predicate mechanism,
> > but bpf uprobe doesn't do any filtering. iirc.
I _think_ there is filtering in uprobe_perf_func() - it calls
uprobe_perf_filter() prior to calling __uprobe_perf_func(), and
the latter runs the BPF program. Maybe I'm missing something here
though? However I think we need to give the user ways to minimize
the cost of breakpoint placement where possible. See below...
> > Similarly "attach to all processes" doesn't sound right either.
> > It's attached to all current and future processes.
>
True, will fix for the next version.
I think for users it'd be good to clarify what the overheads are. If I
want to see malloc()s in a particular process, say I specify the libc
path along with the process ID I'm interested in. This adds the
breakpoint to libc and will - as far as I understand it - trigger
breakpoints system-wide which are then filtered out by uprobe perf handling
for the specific process specified. That's pretty expensive
performance-wise, so we should probably try and give users options to
limit the cost in cases where they don't want to incur system-wide
overheads. I've been investigating adding support for instrumenting shared
library calls _within_ programs by placing the breakpoint on the procedure
linking table call associated with the function. As this is local to the
program, it will only have an effect on malloc()s in that specific
program. So the next version will have a solution which allows us to
trace malloc() in /usr/bin/foo as well as in libc. Thanks!
Alan
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [RFC bpf-next 0/3] libbpf: name-based u[ret]probe attach
2022-01-24 14:13 ` Alan Maguire
@ 2022-01-24 22:47 ` Andrii Nakryiko
2022-01-27 22:54 ` Alan Maguire
0 siblings, 1 reply; 15+ messages in thread
From: Andrii Nakryiko @ 2022-01-24 22:47 UTC (permalink / raw)
To: Alan Maguire
Cc: Alexei Starovoitov, Alexei Starovoitov, Daniel Borkmann,
Andrii Nakryiko, Martin KaFai Lau, Song Liu, Yonghong Song,
John Fastabend, KP Singh, Jiri Olsa, Yucong Sun,
Network Development, bpf
On Mon, Jan 24, 2022 at 6:14 AM Alan Maguire <alan.maguire@oracle.com> wrote:
>
> On Fri, 21 Jan 2022, Andrii Nakryiko wrote:
>
> > On Thu, Jan 20, 2022 at 5:44 PM Alexei Starovoitov
> > <alexei.starovoitov@gmail.com> wrote:
> > >
> > > On Thu, Jan 20, 2022 at 3:43 AM Alan Maguire <alan.maguire@oracle.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].
> > >
> > > I think the pid discussion here and in the patches only causes
> > > confusion. I think it's best to remove pid from the api.
> >
> > It's already part of the uprobe API in libbpf
> > (bpf_program__attach_uprobe), but nothing really changes there.
> > API-wise Alan just added an optional func_name option. I think it
> > makes sense overall.
> >
> > For auto-attach it has to be all PIDs, of course.
> >
>
> Makes sense.
>
> > > uprobes are attached to an inode. They're not attached to a pid
> > > or a process. Any existing process or future process started
> > > from that inode (executable file) will have that uprobe triggering.
> > > The kernel can do pid filtering through predicate mechanism,
> > > but bpf uprobe doesn't do any filtering. iirc.
>
> I _think_ there is filtering in uprobe_perf_func() - it calls
> uprobe_perf_filter() prior to calling __uprobe_perf_func(), and
> the latter runs the BPF program. Maybe I'm missing something here
> though? However I think we need to give the user ways to minimize
> the cost of breakpoint placement where possible. See below...
>
> > > Similarly "attach to all processes" doesn't sound right either.
> > > It's attached to all current and future processes.
> >
>
> True, will fix for the next version.
>
> I think for users it'd be good to clarify what the overheads are. If I
> want to see malloc()s in a particular process, say I specify the libc
> path along with the process ID I'm interested in. This adds the
> breakpoint to libc and will - as far as I understand it - trigger
> breakpoints system-wide which are then filtered out by uprobe perf handling
> for the specific process specified. That's pretty expensive
> performance-wise, so we should probably try and give users options to
> limit the cost in cases where they don't want to incur system-wide
> overheads. I've been investigating adding support for instrumenting shared
> library calls _within_ programs by placing the breakpoint on the procedure
> linking table call associated with the function. As this is local to the
You mean to patch PLT stubs ([0])? One concern with that is (besides
making sure that pt_regs still have exactly the same semantics and
stuff) that uprobes are much faster when patching nop instructions (if
the library was compiled with nop "preambles". Do you know if @plt
entries can be compiled with nops as well? The difference in
performance is more than 2x from my non-scientific testing recently.
So it can be a pretty big difference.
[0] https://www.technovelty.org/linux/plt-and-got-the-key-to-code-sharing-and-dynamic-libraries.html
> program, it will only have an effect on malloc()s in that specific
> program. So the next version will have a solution which allows us to
> trace malloc() in /usr/bin/foo as well as in libc. Thanks!
>
> Alan
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [RFC bpf-next 0/3] libbpf: name-based u[ret]probe attach
2022-01-24 22:47 ` Andrii Nakryiko
@ 2022-01-27 22:54 ` Alan Maguire
0 siblings, 0 replies; 15+ messages in thread
From: Alan Maguire @ 2022-01-27 22:54 UTC (permalink / raw)
To: Andrii Nakryiko
Cc: Alan Maguire, Alexei Starovoitov, Alexei Starovoitov,
Daniel Borkmann, Andrii Nakryiko, Martin KaFai Lau, Song Liu,
Yonghong Song, John Fastabend, KP Singh, Jiri Olsa, Yucong Sun,
Network Development, bpf
On Mon, 24 Jan 2022, Andrii Nakryiko wrote:
> On Mon, Jan 24, 2022 at 6:14 AM Alan Maguire <alan.maguire@oracle.com> wrote:
> >
> > I think for users it'd be good to clarify what the overheads are. If I
> > want to see malloc()s in a particular process, say I specify the libc
> > path along with the process ID I'm interested in. This adds the
> > breakpoint to libc and will - as far as I understand it - trigger
> > breakpoints system-wide which are then filtered out by uprobe perf handling
> > for the specific process specified. That's pretty expensive
> > performance-wise, so we should probably try and give users options to
> > limit the cost in cases where they don't want to incur system-wide
> > overheads. I've been investigating adding support for instrumenting shared
> > library calls _within_ programs by placing the breakpoint on the procedure
> > linking table call associated with the function. As this is local to the
>
> You mean to patch PLT stubs ([0])?
Yep, the .plt table, as shown by "objdump -D -j .plt <program>"
Disassembly of section .plt:
000000000040d020 <.plt>:
40d020: ff 35 e2 5f 4b 00 pushq 0x4b5fe2(%rip) #
8c3008 <
_GLOBAL_OFFSET_TABLE_+0x8>
40d026: ff 25 e4 5f 4b 00 jmpq *0x4b5fe4(%rip) #
8c3010
<_GLOBAL_OFFSET_TABLE_+0x10>
40d02c: 0f 1f 40 00 nopl 0x0(%rax)
000000000040d030 <inet_ntop@plt>:
40d030: ff 25 e2 5f 4b 00 jmpq *0x4b5fe2(%rip) #
8c3018
<inet_ntop@GLIBC_2.2.5>
40d036: 68 00 00 00 00 pushq $0x0
40d03b: e9 e0 ff ff ff jmpq 40d020 <.plt>
In the case of inet_ntop() the address would be 40d030 - which we
then do the relative address calcuation on, giving the address to
be uprobe'd as 0xd030.
> One concern with that is (besides
> making sure that pt_regs still have exactly the same semantics and
> stuff) that uprobes are much faster when patching nop instructions (if
> the library was compiled with nop "preambles". Do you know if @plt
> entries can be compiled with nops as well?
I haven't found any way to do that unfortunately.
> The difference in
> performance is more than 2x from my non-scientific testing recently.
> So it can be a pretty big difference.
>
Interesting! There may be a cleaner way to achieve the goal of
tracing shared library calls in the local binary, but I'm not seeing
an alternate approach yet unfortunately. To me the key thing is
to ensure we have an alternative to globally tracing in libc. I'll send
out the v2 addressing the things you found in the RFC shortly (and that
uses the .plt instrumentation approach). Thanks!
Alan
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [RFC bpf-next 0/3] libbpf: name-based u[ret]probe attach
2022-01-20 11:42 [RFC bpf-next 0/3] libbpf: name-based u[ret]probe attach Alan Maguire
` (3 preceding siblings ...)
2022-01-21 1:44 ` [RFC bpf-next 0/3] libbpf: name-based u[ret]probe attach Alexei Starovoitov
@ 2022-01-21 18:31 ` Andrii Nakryiko
4 siblings, 0 replies; 15+ messages in thread
From: Andrii Nakryiko @ 2022-01-21 18:31 UTC (permalink / raw)
To: Alan Maguire
Cc: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko, Martin Lau,
Song Liu, Yonghong Song, john fastabend, KP Singh, Jiri Olsa,
Yucong Sun, Networking, bpf
On Thu, Jan 20, 2022 at 3:43 AM Alan Maguire <alan.maguire@oracle.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
>
^ permalink raw reply [flat|nested] 15+ messages in thread