On Fri, Jun 30, 2023 at 1:36 AM Jiri Olsa <jolsa@xxxxxxxxxx> wrote:
>
> Adding bpf_program__attach_uprobe_multi function that
> allows to attach multiple uprobes with uprobe_multi link.
>
> The user can specify uprobes with direct arguments:
>
> binary_path/func_pattern/pid
>
> or with struct bpf_uprobe_multi_opts opts argument fields:
>
> const char **syms;
> const unsigned long *offsets;
> const unsigned long *ref_ctr_offsets;
> const __u64 *cookies;
>
> User can specify 2 mutually exclusive set of inputs:
>
> 1) use only path/func_pattern/pid arguments
>
> 2) use path/pid with allowed combinations of:
> syms/offsets/ref_ctr_offsets/cookies/cnt
>
> - syms and offsets are mutually exclusive
> - ref_ctr_offsets and cookies are optional
>
> Any other usage results in error.
>
> Signed-off-by: Jiri Olsa <jolsa@xxxxxxxxxx>
> ---
> tools/lib/bpf/libbpf.c | 122 +++++++++++++++++++++++++++++++++++++++
> tools/lib/bpf/libbpf.h | 27 +++++++++
> tools/lib/bpf/libbpf.map | 1 +
> 3 files changed, 150 insertions(+)
>
> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> index f33ef7cb1adc..b942f248038e 100644
> --- a/tools/lib/bpf/libbpf.c
> +++ b/tools/lib/bpf/libbpf.c
> @@ -10954,6 +10954,128 @@ static int resolve_full_path(const char *file, char *result, size_t result_sz)
> return -ENOENT;
> }
>
> +struct bpf_link *
> +bpf_program__attach_uprobe_multi(const struct bpf_program *prog,
> + pid_t pid,
> + const char *path,
> + const char *func_pattern,
> + const struct bpf_uprobe_multi_opts *opts)
> +{
> + const unsigned long *ref_ctr_offsets = NULL, *offsets = NULL;
> + LIBBPF_OPTS(bpf_link_create_opts, lopts);
> + unsigned long *resolved_offsets = NULL;
> + int err = 0, link_fd, prog_fd;
> + struct bpf_link *link = NULL;
> + char errmsg[STRERR_BUFSIZE];
> + char full_path[PATH_MAX];
> + const __u64 *cookies;
> + const char **syms;
> + bool has_pattern;
> + bool retprobe;
> + size_t cnt;
> +
> + if (!OPTS_VALID(opts, bpf_uprobe_multi_opts))
> + return libbpf_err_ptr(-EINVAL);
> +
> + syms = OPTS_GET(opts, syms, NULL);
> + offsets = OPTS_GET(opts, offsets, NULL);
> + ref_ctr_offsets = OPTS_GET(opts, ref_ctr_offsets, NULL);
> + cookies = OPTS_GET(opts, cookies, NULL);
> + cnt = OPTS_GET(opts, cnt, 0);
> +
> + /*
> + * User can specify 2 mutually exclusive set of inputs:
> + *
> + * 1) use only path/func_pattern/pid arguments
> + *
> + * 2) use path/pid with allowed combinations of:
> + * syms/offsets/ref_ctr_offsets/cookies/cnt
> + *
> + * - syms and offsets are mutually exclusive
> + * - ref_ctr_offsets and cookies are optional
> + *
> + * Any other usage results in error.
> + */
> +
> + if (!path && !func_pattern && !cnt)
weird, I'd expect separate if (!path) return error (already bad,
regardless of func_pattern or cnt)
then if (!func_pattern && cnt == 0) return error
> + return libbpf_err_ptr(-EINVAL);
> + if (func_pattern && !path)
> + return libbpf_err_ptr(-EINVAL);
> +
> + has_pattern = path && func_pattern;
this and above check must be some leftovers from previous version.
path should always be present. and so you don't need has_pattern
variable, just use "func_pattern" check
> +
> + if (has_pattern) {
> + if (syms || offsets || ref_ctr_offsets || cookies || cnt)
> + return libbpf_err_ptr(-EINVAL);
> + } else {
> + if (!cnt)
> + return libbpf_err_ptr(-EINVAL);
> + if (!!syms == !!offsets)
> + return libbpf_err_ptr(-EINVAL);
> + }
> +
> + if (has_pattern) {
> + if (!strchr(path, '/')) {
> + err = resolve_full_path(path, full_path, sizeof(full_path));
> + if (err) {
> + pr_warn("prog '%s': failed to resolve full path for '%s': %d\n",
> + prog->name, path, err);
> + return libbpf_err_ptr(err);
> + }
> + path = full_path;
> + }
> +
> + err = elf_resolve_pattern_offsets(path, func_pattern,
> + &resolved_offsets, &cnt);
> + if (err < 0)
> + return libbpf_err_ptr(err);
> + offsets = resolved_offsets;
> + } else if (syms) {
> + err = elf_resolve_syms_offsets(path, cnt, syms, &resolved_offsets);
> + if (err < 0)
> + return libbpf_err_ptr(err);
> + offsets = resolved_offsets;
you can extract this common error checking and `offsets =
resolved_offsets;` to after if, it's common for both branches
> + }
> +
> + retprobe = OPTS_GET(opts, retprobe, false);
> +
> + lopts.uprobe_multi.path = path;
> + lopts.uprobe_multi.offsets = offsets;
> + lopts.uprobe_multi.ref_ctr_offsets = ref_ctr_offsets;
> + lopts.uprobe_multi.cookies = cookies;
> + lopts.uprobe_multi.cnt = cnt;
> + lopts.uprobe_multi.flags = retprobe ? BPF_F_UPROBE_MULTI_RETURN : 0;
retprobe is another unnecessary var, just inline check here to keep it simpler
> +
> + if (pid == 0)
> + pid = getpid();
> + if (pid > 0)
> + lopts.uprobe_multi.pid = pid;
> +
> + link = calloc(1, sizeof(*link));
> + if (!link) {
> + err = -ENOMEM;
> + goto error;
> + }
> + link->detach = &bpf_link__detach_fd;
> +
> + prog_fd = bpf_program__fd(prog);
> + link_fd = bpf_link_create(prog_fd, 0, BPF_TRACE_UPROBE_MULTI, &lopts);
> + if (link_fd < 0) {
> + err = -errno;
> + pr_warn("prog '%s': failed to attach: %s\n",
"failed to attach multi-uprobe"? We probably should have added "failed
to attach multi-kprobe" in bpf_program__attach_kprobe_multi_opts as
well?
> + prog->name, libbpf_strerror_r(err, errmsg, sizeof(errmsg)));
> + goto error;
> + }
> + link->fd = link_fd;
> + free(resolved_offsets);
> + return link;
> +
> +error:
> + free(resolved_offsets);
> + free(link);
> + return libbpf_err_ptr(err);
> +}
> +
> 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,
> diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h
> index 754da73c643b..7c218f610210 100644
> --- a/tools/lib/bpf/libbpf.h
> +++ b/tools/lib/bpf/libbpf.h
> @@ -529,6 +529,33 @@ bpf_program__attach_kprobe_multi_opts(const struct bpf_program *prog,
> const char *pattern,
> const struct bpf_kprobe_multi_opts *opts);
>
> +struct bpf_uprobe_multi_opts {
> + /* size of this struct, for forward/backward compatibility */
> + size_t sz;
> + /* array of function symbols to attach */
attach to?
> + const char **syms;
> + /* array of function addresses to attach */
attach to?
> + const unsigned long *offsets;
> + /* array of refctr offsets to attach */
we don't really attach to ref counters, so maybe "optional, array of
associated ref counter offsets" or something along those lines ?
> + const unsigned long *ref_ctr_offsets;
> + /* array of user-provided values fetchable through bpf_get_attach_cookie */
"array of associated BPF cookies"? we can't keep explaining what BPF
cookie is in every possible API :)
> + const __u64 *cookies;
> + /* number of elements in syms/addrs/cookies arrays */
> + size_t cnt;
> + /* create return uprobes */
> + bool retprobe;
> + size_t :0;
> +};
> +
> +#define bpf_uprobe_multi_opts__last_field retprobe
> +
> +LIBBPF_API struct bpf_link *
> +bpf_program__attach_uprobe_multi(const struct bpf_program *prog,
> + pid_t pid,
> + const char *binary_path,
> + const char *func_pattern,
> + const struct bpf_uprobe_multi_opts *opts);
> +
ok, let's be good citizens and add documentation for this new API.
Those comments about valid combinations belong here as well. Please
take a look at existing doccomments for the format and conventions.
Thanks!
> struct bpf_ksyscall_opts {
> /* size of this struct, for forward/backward compatibility */
> size_t sz;
> diff --git a/tools/lib/bpf/libbpf.map b/tools/lib/bpf/libbpf.map
> index 7521a2fb7626..d8d11ea6c35e 100644
> --- a/tools/lib/bpf/libbpf.map
> +++ b/tools/lib/bpf/libbpf.map
> @@ -395,4 +395,5 @@ LIBBPF_1.2.0 {
> LIBBPF_1.3.0 {
> global:
> bpf_obj_pin_opts;
> + bpf_program__attach_uprobe_multi;
> } LIBBPF_1.2.0;
> --
> 2.41.0
>
