On Tue, Jun 20, 2023 at 1:37 AM Jiri Olsa <jolsa@xxxxxxxxxx> wrote:
>
> Adding bpf_program__attach_uprobe_multi_opts function that
> allows to attach multiple uprobes with uprobe_multi link.
>
> The user can specify uprobes with direct arguments:
>
> binary_path/func_pattern
>
> or with struct bpf_uprobe_multi_opts opts argument fields:
>
> const char *path;
> const char **syms;
> const unsigned long *offsets;
> const unsigned long *ref_ctr_offsets;
>
> User can specify 3 mutually exclusive set of incputs:
typo: inputs
>
> 1) use only binary_path/func_pattern aruments
typo: arguments
>
> 2) use only opts argument with allowed combinations of:
> path/offsets/ref_ctr_offsets/cookies/cnt
>
> 3) use binary_path with allowed combinations of:
> syms/offsets/ref_ctr_offsets/cookies/cnt
>
why do we need this duplication of binary_path and opts.path? same for
pid and opts.pid?
> Any other usage results in error.
>
> Signed-off-by: Jiri Olsa <jolsa@xxxxxxxxxx>
> ---
> tools/lib/bpf/libbpf.c | 131 +++++++++++++++++++++++++++++++++++++++
> tools/lib/bpf/libbpf.h | 31 +++++++++
> tools/lib/bpf/libbpf.map | 1 +
> 3 files changed, 163 insertions(+)
>
> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> index 3e5c88caf5d5..d972cea4c658 100644
> --- a/tools/lib/bpf/libbpf.c
> +++ b/tools/lib/bpf/libbpf.c
> @@ -11402,6 +11402,137 @@ static int resolve_full_path(const char *file, char *result, size_t result_sz)
> return -ENOENT;
> }
>
> +struct bpf_link *
> +bpf_program__attach_uprobe_multi_opts(const struct bpf_program *prog,
let's call it just bpf_program__attach_uprobe_multi()
we should have done that with bpf_program__attach_kprobe_multi_opts()
as well. Generally, if the only variant of API is the one with opts,
then there is no point in adding "_opts" suffix to the API name, IMO
> + pid_t pid,
> + const char *binary_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;
> + const char **resolved_symbols = NULL;
> + int err = 0, link_fd, prog_fd;
> + struct bpf_link *link = NULL;
> + char errmsg[STRERR_BUFSIZE];
> + const char *path, **syms;
> + char full_path[PATH_MAX];
> + const __u64 *cookies;
> + bool has_pattern;
> + bool retprobe;
> + size_t cnt;
> +
> + if (!OPTS_VALID(opts, bpf_uprobe_multi_opts))
> + return libbpf_err_ptr(-EINVAL);
> +
> + path = OPTS_GET(opts, path, NULL);
> + 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 3 mutually exclusive set of incputs:
> + *
> + * 1) use only binary_path/func_pattern aruments
same typos
> + *
> + * 2) use only opts argument with allowed combinations of:
> + * path/offsets/ref_ctr_offsets/cookies/cnt
> + *
> + * 3) use binary_path with allowed combinations of:
> + * syms/offsets/ref_ctr_offsets/cookies/cnt
> + *
> + * Any other usage results in error.
> + */
> +
Looking at this part (sorry, I already trimmed reply before realizing
I want to comment on this):
+ err = elf_find_pattern_func_offset(binary_path, func_pattern,
+ &resolved_symbols,
&resolved_offsets,
+ &cnt);
+ if (err < 0)
+ return libbpf_err_ptr(err);
+ offsets = resolved_offsets;
+ } else if (syms) {
+ err = elf_find_multi_func_offset(binary_path, cnt,
syms, &resolved_offsets);
Few thoughts.
First, we are not using resolved_symbols returned from
elf_find_pattern_func_offset(), do we? If not, let's drop it.
Second, I'm not a big fan of chosen naming. Maybe something like
elf_resolve_{pattern,syms}_offsets?
[...]
> +error:
> + free(resolved_offsets);
> + free(resolved_symbols);
> + 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..b6ff7d69a1d7 100644
> --- a/tools/lib/bpf/libbpf.h
> +++ b/tools/lib/bpf/libbpf.h
> @@ -529,6 +529,37 @@ 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;
> + /* path to attach */
> + const char *path;
how is this different from binary_path? not clear why we need to paths
> + /* array of function symbols to attach */
> + const char **syms;
> + /* array of function addresses to attach */
> + const unsigned long *offsets;
> + /* array of refctr offsets to attach */
> + const unsigned long *ref_ctr_offsets;
> + /* array of user-provided values fetchable through bpf_get_attach_cookie */
> + const __u64 *cookies;
> + /* number of elements in syms/addrs/cookies arrays */
> + size_t cnt;
> + /* create return uprobes */
> + bool retprobe;
> + /* pid filter */
> + int pid;
another duplicate, we already pass pid argument
> + size_t :0;
> +};
> +
> +#define bpf_uprobe_multi_opts__last_field pid
> +
> +LIBBPF_API struct bpf_link *
> +bpf_program__attach_uprobe_multi_opts(const struct bpf_program *prog,
> + pid_t pid,
> + const char *binary_path,
> + const char *func_pattern,
> + const struct bpf_uprobe_multi_opts *opts);
> +
> 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..81558ef1bc38 100644
> --- a/tools/lib/bpf/libbpf.map
> +++ b/tools/lib/bpf/libbpf.map
> @@ -390,6 +390,7 @@ LIBBPF_1.2.0 {
> bpf_link_get_info_by_fd;
> bpf_map_get_info_by_fd;
> bpf_prog_get_info_by_fd;
> + bpf_program__attach_uprobe_multi_opts;
should be in 1.3.0?
> } LIBBPF_1.1.0;
>
> LIBBPF_1.3.0 {
> --
> 2.41.0
>
